diff mbox series

[2/2,v2] wifi: mwifiex: cleanup TX error handling

Message ID 20230728084407.101930-2-dmantipov@yandex.ru
State New
Headers show
Series [1/5,v2] wifi: mwifiex: fix memory leak in mwifiex_histogram_read() | expand

Commit Message

Dmitry Antipov July 28, 2023, 8:43 a.m. UTC 
Since 'mwifiex_process_tx()' is the only place from where both
'mwifiex_process_sta_txpd()' and 'mwifiex_process_uap_txpd()'
are called, these functions may be converted to 'void' after
moving skb layout check to the caller, which may be simplified
as well. Also adjust somewhat obfuscating error messages and
add 'mwifiex_interface_name()' to make them a bit more useful.

Suggested-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: some redesign in attempt to integrate Brian's feedback
---
 .../net/wireless/marvell/mwifiex/11n_aggr.c   |  4 +-
 drivers/net/wireless/marvell/mwifiex/main.h   | 18 ++++++-
 drivers/net/wireless/marvell/mwifiex/sta_tx.c | 26 ++-------
 drivers/net/wireless/marvell/mwifiex/txrx.c   | 53 ++++++++++---------
 .../net/wireless/marvell/mwifiex/uap_txrx.c   | 15 +-----
 drivers/net/wireless/marvell/mwifiex/wmm.c    |  3 +-
 6 files changed, 55 insertions(+), 64 deletions(-)

Comments

Brian Norris Aug. 1, 2023, 5:55 p.m. UTC | #1 
On Fri, Jul 28, 2023 at 11:43:43AM +0300, Dmitry Antipov wrote:
> Since 'mwifiex_process_tx()' is the only place from where both
> 'mwifiex_process_sta_txpd()' and 'mwifiex_process_uap_txpd()'
> are called, these functions may be converted to 'void' after
> moving skb layout check to the caller, which may be simplified
> as well. Also adjust somewhat obfuscating error messages and
> add 'mwifiex_interface_name()' to make them a bit more useful.

Do you actually run this driver on anything, or are you just compile
testing / running static analysis? Because IIUC, all these messages are
perfectly clear when using the mwifiex_dbg() macro, which usually ends
up in dev_info(), and includes things like "mwifiex_pcie",
"mwifiex_sdio", and "mwifiex_usb" already. So the
mwifiex_interface_name() stuff just seems superfluous. I'd suggest
removing that.

At the same time, it seems like you're working hard on trying *not* to
get your stuff merged in a timely manner, as you shift the goalposts
every time you refactor your series. Specifically, you're now violating
basic guidelines like these:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_messages

"If you find yourself listing out a number of changes in the commit
message as a bulleted list or similar, consider splitting up the patch
into discrete changes that each do one thing. Similarly, if one of the
additional considerations is refactoring, try to shift that into a
separate patch."

This started out as "avoid BUG_ON()", which is a great goal on its own.
But now you haven't even mentioned that in your laundry list of
refactors. This seems like you have at least two patch candidates here:
refactoring the error handling, and dropping the BUG_ON(). (If the
refactoring becomes extremely trivial, maybe this is OK as 1 patch. But
in its current form, it's not.)

Before you resubmit, please read the patch submission guidelines again.
I'd also suggest sticking this (as multiple patches) at the end of the
series, so the other patches (which have been reviewed/ack'd multiple
times) can land cleanly.

Brian

> Suggested-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v2: some redesign in attempt to integrate Brian's feedback
[...]
Antipov, Dmitriy Aug. 2, 2023, 11:45 a.m. UTC | #2 
On Tue, 2023-08-01 at 10:55 -0700, Brian Norris wrote:

> Do you actually run this driver on anything, or are you just compile
> testing / running static analysis?

Short answer: compile testing and static analysis.

Long answer: this is not just me, and an overall picture is far more
interesting. ISPRAS (https://www.ispras.ru/en) manages a sophisticated
static analysis tool. Running over Linux kernel sources, this tool
produces a lot (I don't know exactly, but probably tens of thousands)
warnings about possibly unsafe and/or incorrect code. These warnings
should be investigated and, after filtering out irrelevant, false
positive etc. cases, some of them may (and should) be fixed. This work
is distributed among more than 30 companies, has its own (not too tight
but still) schedule, and it's not always easy to incorporate it into
the regular workflows of a kernel communities.

As for the the rest of your comments, I'll try to address them in an
incoming v3. And of course great thanks for your reviews.

Dmitry
diff mbox series

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/11n_aggr.c b/drivers/net/wireless/marvell/mwifiex/11n_aggr.c
index 34b4b34276d6..4de2ff688cc3 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n_aggr.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n_aggr.c
@@ -273,8 +273,8 @@  mwifiex_11n_aggregate_pkt(struct mwifiex_private *priv,
 		mwifiex_dbg(adapter, ERROR, "data: -EBUSY is returned\n");
 		break;
 	case -1:
-		mwifiex_dbg(adapter, ERROR, "%s: host_to_card failed: %#x\n",
-			    __func__, ret);
+		mwifiex_dbg(adapter, ERROR, "%s: %s interface error\n",
+			    __func__, mwifiex_interface_name(adapter));
 		adapter->dbg.num_tx_host_to_card_failure++;
 		mwifiex_write_data_complete(adapter, skb_aggr, 1, ret);
 		return 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index b95886e1413e..24b07256e822 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1155,8 +1155,8 @@  int mwifiex_process_uap_event(struct mwifiex_private *);
 void mwifiex_delete_all_station_list(struct mwifiex_private *priv);
 void mwifiex_wmm_del_peer_ra_list(struct mwifiex_private *priv,
 				  const u8 *ra_addr);
-void *mwifiex_process_sta_txpd(struct mwifiex_private *, struct sk_buff *skb);
-void *mwifiex_process_uap_txpd(struct mwifiex_private *, struct sk_buff *skb);
+void mwifiex_process_sta_txpd(struct mwifiex_private *priv, struct sk_buff *skb);
+void mwifiex_process_uap_txpd(struct mwifiex_private *priv, struct sk_buff *skb);
 int mwifiex_sta_init_cmd(struct mwifiex_private *, u8 first_sta, bool init);
 int mwifiex_cmd_802_11_scan(struct host_cmd_ds_command *cmd,
 			    struct mwifiex_scan_cmd_config *scan_cfg);
@@ -1471,6 +1471,20 @@  static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
 	}
 }
 
+static inline char *mwifiex_interface_name(struct mwifiex_adapter *adapter)
+{
+	switch (adapter->iface_type) {
+	case MWIFIEX_SDIO:
+		return "SDIO";
+	case MWIFIEX_PCIE:
+		return "PCIE";
+	case MWIFIEX_USB:
+		return "USB";
+	default:
+		return "<unknown>";
+	}
+}
+
 int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
 			     u32 func_init_shutdown);
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
index 13c0e67ededf..918a6f444ae4 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
@@ -29,8 +29,8 @@ 
  *      - Priority specific Tx control
  *      - Flags
  */
-void *mwifiex_process_sta_txpd(struct mwifiex_private *priv,
-				struct sk_buff *skb)
+void mwifiex_process_sta_txpd(struct mwifiex_private *priv,
+			      struct sk_buff *skb)
 {
 	struct mwifiex_adapter *adapter = priv->adapter;
 	struct txpd *local_tx_pd;
@@ -39,15 +39,6 @@  void *mwifiex_process_sta_txpd(struct mwifiex_private *priv,
 	u16 pkt_type, pkt_offset;
 	int hroom = adapter->intf_hdr_len;
 
-	if (!skb->len) {
-		mwifiex_dbg(adapter, ERROR,
-			    "Tx: bad packet length: %d\n", skb->len);
-		tx_info->status_code = -1;
-		return skb->data;
-	}
-
-	BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
-
 	pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
 
 	pad = ((uintptr_t)skb->data - (sizeof(*local_tx_pd) + hroom)) &
@@ -109,8 +100,6 @@  void *mwifiex_process_sta_txpd(struct mwifiex_private *priv,
 	if (!local_tx_pd->tx_control)
 		/* TxCtrl set by user or default */
 		local_tx_pd->tx_control = cpu_to_le32(priv->pkt_tx_ctrl);
-
-	return skb->data;
 }
 
 /*
@@ -176,17 +165,10 @@  int mwifiex_send_null_packet(struct mwifiex_private *priv, u8 flags)
 	}
 	switch (ret) {
 	case -EBUSY:
-		dev_kfree_skb_any(skb);
-		mwifiex_dbg(adapter, ERROR,
-			    "%s: host_to_card failed: ret=%d\n",
-			    __func__, ret);
-		adapter->dbg.num_tx_host_to_card_failure++;
-		break;
 	case -1:
 		dev_kfree_skb_any(skb);
-		mwifiex_dbg(adapter, ERROR,
-			    "%s: host_to_card failed: ret=%d\n",
-			    __func__, ret);
+		mwifiex_dbg(adapter, ERROR, "%s: %s interface error\n",
+			    __func__, mwifiex_interface_name(adapter));
 		adapter->dbg.num_tx_host_to_card_failure++;
 		break;
 	case 0:
diff --git a/drivers/net/wireless/marvell/mwifiex/txrx.c b/drivers/net/wireless/marvell/mwifiex/txrx.c
index 54c204608dab..3e176502ced3 100644
--- a/drivers/net/wireless/marvell/mwifiex/txrx.c
+++ b/drivers/net/wireless/marvell/mwifiex/txrx.c
@@ -72,13 +72,18 @@  EXPORT_SYMBOL_GPL(mwifiex_handle_rx_packet);
 int mwifiex_process_tx(struct mwifiex_private *priv, struct sk_buff *skb,
 		       struct mwifiex_tx_param *tx_param)
 {
-	int hroom, ret = -1;
+	int hroom, ret;
 	struct mwifiex_adapter *adapter = priv->adapter;
-	u8 *head_ptr;
 	struct txpd *local_tx_pd = NULL;
 	struct mwifiex_sta_node *dest_node;
 	struct ethhdr *hdr = (void *)skb->data;
 
+	if (unlikely(!skb->len ||
+		     skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	hroom = adapter->intf_hdr_len;
 
 	if (priv->bss_role == MWIFIEX_BSS_ROLE_UAP) {
@@ -87,34 +92,32 @@  int mwifiex_process_tx(struct mwifiex_private *priv, struct sk_buff *skb,
 			dest_node->stats.tx_bytes += skb->len;
 			dest_node->stats.tx_packets++;
 		}
-
-		head_ptr = mwifiex_process_uap_txpd(priv, skb);
+		mwifiex_process_uap_txpd(priv, skb);
 	} else {
-		head_ptr = mwifiex_process_sta_txpd(priv, skb);
+		mwifiex_process_sta_txpd(priv, skb);
 	}
 
-	if ((adapter->data_sent || adapter->tx_lock_flag) && head_ptr) {
+	if (adapter->data_sent || adapter->tx_lock_flag) {
 		skb_queue_tail(&adapter->tx_data_q, skb);
 		atomic_inc(&adapter->tx_queued);
 		return 0;
 	}
 
-	if (head_ptr) {
-		if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA)
-			local_tx_pd = (struct txpd *)(head_ptr + hroom);
-		if (adapter->iface_type == MWIFIEX_USB) {
-			ret = adapter->if_ops.host_to_card(adapter,
-							   priv->usb_port,
-							   skb, tx_param);
-		} else {
-			ret = adapter->if_ops.host_to_card(adapter,
-							   MWIFIEX_TYPE_DATA,
-							   skb, tx_param);
-		}
+	if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA)
+		local_tx_pd = (struct txpd *)(skb->data + hroom);
+	if (adapter->iface_type == MWIFIEX_USB) {
+		ret = adapter->if_ops.host_to_card(adapter,
+						   priv->usb_port,
+						   skb, tx_param);
+	} else {
+		ret = adapter->if_ops.host_to_card(adapter,
+						   MWIFIEX_TYPE_DATA,
+						   skb, tx_param);
 	}
+
 	mwifiex_dbg_dump(adapter, DAT_D, "tx pkt:", skb->data,
 			 min_t(size_t, skb->len, DEBUG_DUMP_DATA_MAX_LEN));
-
+out:
 	switch (ret) {
 	case -ENOSR:
 		mwifiex_dbg(adapter, DATA, "data: -ENOSR is returned\n");
@@ -129,14 +132,16 @@  int mwifiex_process_tx(struct mwifiex_private *priv, struct sk_buff *skb,
 		mwifiex_dbg(adapter, ERROR, "data: -EBUSY is returned\n");
 		break;
 	case -1:
-		mwifiex_dbg(adapter, ERROR,
-			    "mwifiex_write_data_async failed: 0x%X\n",
-			    ret);
+		mwifiex_dbg(adapter, ERROR, "%s: %s interface error\n",
+			    __func__, mwifiex_interface_name(adapter));
 		adapter->dbg.num_tx_host_to_card_failure++;
 		mwifiex_write_data_complete(adapter, skb, 0, ret);
 		break;
 	case -EINPROGRESS:
 		break;
+	case -EINVAL:
+		mwifiex_dbg(adapter, ERROR, "%s: malformed skb\n", __func__);
+		fallthrough;
 	case 0:
 		mwifiex_write_data_complete(adapter, skb, 0, ret);
 		break;
@@ -199,8 +204,8 @@  static int mwifiex_host_to_card(struct mwifiex_adapter *adapter,
 		mwifiex_dbg(adapter, ERROR, "data: -EBUSY is returned\n");
 		break;
 	case -1:
-		mwifiex_dbg(adapter, ERROR,
-			    "mwifiex_write_data_async failed: 0x%X\n", ret);
+		mwifiex_dbg(adapter, ERROR, "%s: %s interface error\n",
+			    __func__, mwifiex_interface_name(adapter));
 		adapter->dbg.num_tx_host_to_card_failure++;
 		mwifiex_write_data_complete(adapter, skb, 0, ret);
 		break;
diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
index e495f7eaea03..fe26dcc23120 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
@@ -442,8 +442,8 @@  int mwifiex_process_uap_rx_packet(struct mwifiex_private *priv,
  *      - Priority specific Tx control
  *      - Flags
  */
-void *mwifiex_process_uap_txpd(struct mwifiex_private *priv,
-			       struct sk_buff *skb)
+void mwifiex_process_uap_txpd(struct mwifiex_private *priv,
+			      struct sk_buff *skb)
 {
 	struct mwifiex_adapter *adapter = priv->adapter;
 	struct uap_txpd *txpd;
@@ -452,15 +452,6 @@  void *mwifiex_process_uap_txpd(struct mwifiex_private *priv,
 	u16 pkt_type, pkt_offset;
 	int hroom = adapter->intf_hdr_len;
 
-	if (!skb->len) {
-		mwifiex_dbg(adapter, ERROR,
-			    "Tx: bad packet length: %d\n", skb->len);
-		tx_info->status_code = -1;
-		return skb->data;
-	}
-
-	BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
-
 	pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
 
 	pad = ((uintptr_t)skb->data - (sizeof(*txpd) + hroom)) &
@@ -508,6 +499,4 @@  void *mwifiex_process_uap_txpd(struct mwifiex_private *priv,
 	if (!txpd->tx_control)
 		/* TxCtrl set by user or default */
 		txpd->tx_control = cpu_to_le32(priv->pkt_tx_ctrl);
-
-	return skb->data;
 }
diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c b/drivers/net/wireless/marvell/mwifiex/wmm.c
index 00a5679b5c51..050ce183f507 100644
--- a/drivers/net/wireless/marvell/mwifiex/wmm.c
+++ b/drivers/net/wireless/marvell/mwifiex/wmm.c
@@ -1376,7 +1376,8 @@  mwifiex_send_processed_packet(struct mwifiex_private *priv,
 		spin_unlock_bh(&priv->wmm.ra_list_spinlock);
 		break;
 	case -1:
-		mwifiex_dbg(adapter, ERROR, "host_to_card failed: %#x\n", ret);
+		mwifiex_dbg(adapter, ERROR, "%s: %s interface error\n",
+			    __func__, mwifiex_interface_name(adapter));
 		adapter->dbg.num_tx_host_to_card_failure++;
 		mwifiex_write_data_complete(adapter, skb, 0, ret);
 		break;