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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200312143019.GN11561@kadam>
Date:   Thu, 12 Mar 2020 17:30:19 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jerome Pouiller <Jerome.Pouiller@...abs.com>
Cc:     devel@...verdev.osuosl.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S . Miller" <davem@...emloft.net>,
        Kalle Valo <kvalo@...eaurora.org>
Subject: Re: [PATCH 3/5] staging: wfx: make warning about pending frame less
 scary

On Tue, Mar 10, 2020 at 11:13:54AM +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@...abs.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@...abs.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