Message-ID: <20210402081420.GU2088@kadam>
Date:   Fri, 2 Apr 2021 08:14:20 +0000 (UTC)
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Fabio Aiuto <fabioaiuto83@...il.com>
Cc:     gregkh@...uxfoundation.org, joe@...ches.com,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/49] staging: rtl8723bs: remove RT_TRACE logs in
 core/rtw_cmd.c

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.

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

Powered by blists - more mailing lists