Re: [PATCH 08/49] staging: rtl8723bs: remove RT_TRACE logs in core/rtw_cmd.c

[Fabio Aiuto <fabioaiuto83@gmail.com>]

On Fri, Apr 02, 2021 at 08:14:20AM +0000, Dan Carpenter wrote:
> On Thu, Apr 01, 2021 at 11:51:15PM +0200, Fabio Aiuto wrote:
> > 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?
> 
> So when you're sending patches like this which have the potential to
> change the behavior then we want to see your thought process explained a
> bit in the message.

you are right, I skip a lot of steps in the message, next time I will
explain better.

> 
> For example, when I'm reviewing patches in my email client, then I don't
> know if rtw_createbss_cmd() uses pmlmepriv->to_join.  It turns out it
> doesn't.  I also don't know what ->to_join is for really.  Your patch
> preserves the original logic, but it's not totally clear that the
> original code was correct.  See how it's done in rtw_do_join():
> 
> drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
>    107                                  rtw_generate_random_ibss(pibss);
>    108  
>    109                                  if (rtw_createbss_cmd(padapter) != _SUCCESS) {
>    110                                          RT_TRACE(_module_rtl871x_ioctl_set_c_, _drv_err_, ("***Error =>do_goin: rtw_createbss_cmd status FAIL***\n "));
>    111                                          ret =  false;
>    112                                          goto exit;
>    113                                  }
>    114  
>    115                                  pmlmepriv->to_join = false;
>    116  
> 
> So you could make an argument that the original code is wrong.
> 
> Also rtw_createbss_cmd() can't actually fail.
> 
> The other option is to replace that particular RT_TRACE() with a dev_err()
> message.  Another option is to just skip that one and come back to it
> later.  Maybe the code will be more clear after we have cleaned it up.
> 
> It doesn't matter so long as the commit message defends your choice then
> probably we would accept any of these patches.
> 
> regards,
> dan carpenter

ok I will leave the logic untouched moving the

pmlmepriv->to_join = false;

before the rtw_create_bss() call, for they not interfere.

thank you,

fabio