Re: [PATCH 3/5] staging: wfx: make warning about pending frame less scary

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jerome Pouiller <Jerome.Pouiller@silabs.com>
Cc: devel@driverdev.osuosl.org, linux-wireless@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"David S . Miller" <davem@davemloft.net>,
	Kalle Valo <kvalo@codeaurora.org>
Subject: Re: [PATCH 3/5] staging: wfx: make warning about pending frame less scary
Date: Thu, 12 Mar 2020 17:30:19 +0300	[thread overview]
Message-ID: <20200312143019.GN11561@kadam> (raw)
In-Reply-To: <20200310101356.182818-4-Jerome.Pouiller@silabs.com>

On Tue, Mar 10, 2020 at 11:13:54AM +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> Removing station while some traffic is in progress may happen.
> 

You're doing this in every commit where you start the commit message in
the subject and then just keep writing.  Take a look at your patch in
this URL.  Try to find the subject.

https://marc.info/?l=linux-driver-devel&m=158383526527951&w=2

The subject is far separated from the body of the commit message.  I
normally read the patch first, then I read the commit message and I
don't read the subject at all.  Or sometimes I only read the subject.

https://www.designershumor.com/2019/09/30/you-will-read-this-first-meme/

So it really helps me if the commit message restates the subject.  The
truth is that I don't really even like the advice that Josh wrote in
the howto about patch descriptions.  I normally start by explaining the
problem then how I solved it.  But I try not to be a pedant, so long as
I can understand the problem and the patch that's fine.  So how I would
write this commit message is:

    The warning message about releasing a station while Tx is in
    progress will trigger a stack trace, possibly a reboot depending
    on the configuration, and a syzbot email.  It's not necessarily
    a big deal that transmission is still in process so let's make the
    warning less scary.

> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
>  drivers/staging/wfx/sta.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 03d0f224ffdb..010e13bcd33e 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -605,7 +605,9 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(sta_priv->buffered); i++)
> -		WARN(sta_priv->buffered[i], "release station while Tx is in progress");
> +		if (sta_priv->buffered[i])
> +			dev_warn(wvif->wdev->dev, "release station while %d pending frame on queue %d",
> +				 sta_priv->buffered[i], i);

Why print a warning message at all if this is a normal situation?  Just
delete the whole thing.

regards,
dan carpenter