Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 1 Apr 2021 23:51:15 +0200
From:   Fabio Aiuto <fabioaiuto83@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     gregkh@linuxfoundation.org, joe@perches.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 08/49] staging: rtl8723bs: remove RT_TRACE logs in
 core/rtw_cmd.c
Message-ID: <20210401215114.GA15992@agape.jhs>
References: <cover.1617268327.git.fabioaiuto83@gmail.com>
 <7f51432d99459d79742639341f107115f0c224c5.1617268327.git.fabioaiuto83@gmail.com>
 <20210401095017.GR2065@kadam>
 <20210401135536.GA1691@agape.jhs>
 <20210401143235.GV2065@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210401143235.GV2065@kadam>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Thu, Apr 01, 2021 at 05:32:36PM +0300, Dan Carpenter wrote:
> On Thu, Apr 01, 2021 at 03:55:37PM +0200, Fabio Aiuto wrote:
> > 
> > Hi Dan,
> > 
> > I have the following:
> > 
> >  	if (rtw_createbss_cmd(adapter) != _SUCCESS)
> > -		RT_TRACE(_module_rtl871x_mlme_c_, _drv_err_, ("Error =>rtw_createbss_cmd status FAIL\n"));
> > +	;
> > 
> > will I leave
> > 
> > 	if (rtw_createbss_cmd(adapter) != _SUCCESS)
> > 		;
> > 
> > or just
> > 
> > 	rtw_createbss_cmd(adapter);
> > 
> > ?
> > 
> > what's best from the static analysis point of view?
> > 
> > smatch and sparse says nothing about that.
> 
> rtw_createbss_cmd() can only fail if this allocation fails:
> 
> 	pcmd = rtw_zmalloc(sizeof(struct cmd_obj));
> 
> In current kernels, that size of small allocation will never fail.  But
> we alway write code as if every allocation can fail.
> 
> Normally when an allocation fails then we want to return -ENOMEM and
> clean up.  But this code is an event handler for firmware events and
> there isn't any real clean up to do.  Since there is nothing we can do
> then this is basically working and fine.
> 
> How I would write this is:
> 
> 			ret = rtw_createbss_cmd(adapter);
> 			if (ret != _SUCCESS)
> 				goto unlock;
> 		}
> 	}
> unlock:
> 	spin_unlock_bh(&pmlmepriv->lock);
> }
> 
> That doesn't change how the code works but it signals to the the reader
> what your intention is.  If we just remove the error handling then it's
> ambiguous.
> 
> 			rtw_createbss_cmd(adapter);
> 		}
> 	}
> 	<-- Futurue programmer decides to add code here then figuring
>             that rtw_createbss_cmd() can fail is a problem.
> 
> 	spin_unlock_bh(&pmlmepriv->lock);
> }
> 
> But for something like this which is maybe more subtle than just a
> straight delete of lines of code, then consider pulling it out into its
> own separate patch.  That makes it easier to review.  Put all the stuff
> that I said in the commit message:
> 
> ---
> [PATCH] tidy up some error handling
> 
> The RT_TRACE() output is not useful so we want to delete it.  In this
> case there is no cleanup for rtw_createbss_cmd() required or even
> possible.  I've deleted the RT_TRACE() output and added a goto unlock
> to show that we can't continue if rtw_createbss_cmd() fails.
> 
> ---
> 
> > 
> > Checkpatch too seems to ignore it, maybe the first one is good,
> > but I would like to be sure before sending another over 40 patches
> > long patchset.
> 
> Don't send 40 patches.  Just send 10 at a time until you get a better
> feel for which ones are going to get applied or not. :P  It's not
> arbitrary, and I'm definitely not trying to NAK your patches.  Once you
> learn the rules I hope that it's predictable and straight forward.
> 
> regards,
> dan carpenter
> 

Hi Dan,

sorry again. In this case:

@@ -828,10 +829,11 @@ void rtw_surveydone_event_callback(struct adapter *adapter, u8 *pbuf)
 
                                        pmlmepriv->fw_state = WIFI_ADHOC_MASTER_STATE;
 
-                                       if (rtw_createbss_cmd(adapter) != _SUCCESS)
-                                               ;
-
                                        pmlmepriv->to_join = false;
+
+                                       ret = rtw_createbss_cmd(adapter);
+                                       if (ret != _SUCCESS)
+                                               goto unlock;
                                }
                        }

I decided to move the set to false of pmlepriv->to_join before 
the rtw_createbss_cmd(). In old code that statement was executed
unconditionally and seems not to be tied to the failure of 
rtw_createbss_cmd().

The eventual goto would skip this instruction so I moved it
before.

What do you think?

Thank you,

fabio