Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Jun 2023 13:12:56 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Dmitry Antipov <dmantipov@yandex.ru>
Cc:     Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org
Subject: Re: [PATCH 3/3] [v4] wifi: mwifiex: drop BUG_ON() from TX error
 handling
Message-ID: <ZJ3lyIQy7GPbA9YL@google.com>
References: <20230629085115.180499-1-dmantipov@yandex.ru>
 <20230629085115.180499-3-dmantipov@yandex.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230629085115.180499-3-dmantipov@yandex.ru>
Precedence: bulk

On Thu, Jun 29, 2023 at 11:51:02AM +0300, Dmitry Antipov wrote:
> Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and
> 'mwifiex_process_uap_txpd()'. In case of insufficient
> headrom, issue warning and return NULL, which should be
> gracefully handled in 'mwifiex_process_tx()'. Also mark
> error handling branches with 'unlikely()' and adjust
> format specifiers to match actual 'unsigned int' type.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v4: initial version to match series
> ---
>  drivers/net/wireless/marvell/mwifiex/sta_tx.c   | 13 +++++++++----
>  drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 13 +++++++++----
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> index 13c0e67ededf..d43f6ec1ad37 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> @@ -39,14 +39,19 @@ void *mwifiex_process_sta_txpd(struct mwifiex_private *priv,
>  	u16 pkt_type, pkt_offset;
>  	int hroom = adapter->intf_hdr_len;
>  
> -	if (!skb->len) {
> +	if (unlikely(!skb->len)) {
>  		mwifiex_dbg(adapter, ERROR,
> -			    "Tx: bad packet length: %d\n", skb->len);
> +			    "Tx: bad packet length: %u\n", skb->len);
>  		tx_info->status_code = -1;
>  		return skb->data;
>  	}
> -
> -	BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
> +	if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
> +		mwifiex_dbg(adapter, ERROR,
> +			    "Tx: insufficient skb headroom: %u\n",
> +			    skb_headroom(skb));
> +		tx_info->status_code = -1;
> +		return NULL;

I'm not sure why this return (NULL) should be different than the one for
skb->len==0 (skb->data). mwifiex_process_tx() has...weird handling for
both.

For NULL, we fall into a default (ret==-1) case where the error message
is wrong ("mwifiex_write_data_async failed: ...").

For non-NULL skb->data, we still try to queue or transmit the
skb...which seems wrong.

I think they should both be returning NULL, and mwifiex_process_tx()
should improve its error handling to more explicitly handle that case,
instead of printing the wrong error message.

(Now, I expect neither failure cases are actually exercised in practice,
which makes most of this moot...)

I'm also not sure why this is part of the same series as the others.

Brian

> +	}
>  
>  	pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> index e495f7eaea03..b27266742795 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> @@ -452,14 +452,19 @@ void *mwifiex_process_uap_txpd(struct mwifiex_private *priv,
>  	u16 pkt_type, pkt_offset;
>  	int hroom = adapter->intf_hdr_len;
>  
> -	if (!skb->len) {
> +	if (unlikely(!skb->len)) {
>  		mwifiex_dbg(adapter, ERROR,
> -			    "Tx: bad packet length: %d\n", skb->len);
> +			    "Tx: bad packet length: %u\n", skb->len);
>  		tx_info->status_code = -1;
>  		return skb->data;
>  	}
> -
> -	BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
> +	if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
> +		mwifiex_dbg(adapter, ERROR,
> +			    "Tx: insufficient skb headroom: %u\n",
> +			    skb_headroom(skb));
> +		tx_info->status_code = -1;
> +		return NULL;
> +	}
>  
>  	pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
>  
> -- 
> 2.41.0
>