by Fabio Aiuto

[permalink] [raw]

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

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