Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall <julia.lawall@inria.fr>]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 12:03:14 PM CEST you wrote:
> > On Sat, 10 Apr 2021, Julia Lawall wrote:
> > > On Sat, 10 Apr 2021, Greg KH wrote:
> > > > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco
> wrote:
> > > > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > > > it is used everywhere as a bool and, accordingly, it should be
> > > > > declared as a bool. Shorten the controlling
> > > > > expression of an 'if' statement.
> > > > >
> > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > ---
> > > > >
> > > > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > > > > 96fe172ced8d..8dc4dd8c6d4c 100644
> > > > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter
> > > > > *padapter)
> > > > >
> > > > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > > > >  {
> > > > >
> > > > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode ==
> true) {
> > > > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > > > >
> > > > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > > > >
> > > > >  			padapter-
> >HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> > > > >  			function caller is in interrupt context
> */> > >
> > > > >  	}
> > > > >
> > > > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > > > > 0a48f1653be5..0767dbb84199 100644
> > > > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > > > >
> > > > >  	u8 LpsIdleCount;
> > > > >  	u8 power_mgnt;
> > > > >  	u8 org_power_mgnt;
> > > > >
> > > > > -	u8 fw_current_in_ps_mode;
> > > > > +	bool fw_current_in_ps_mode;
> > > > >
> > > > >  	unsigned long	DelayLPSLastTimeStamp;
> > > > >  	s32		pnp_current_pwr_state;
> > > > >  	u8 pnp_bstop_trx;
> > > >
> > > > If this is only checked, how can it ever be true?  Who ever sets this
> > > > value?
> > >
> > > I think it's already updated everywhere with true and false, so there
> > > is
> > > nothing to change.  But it would be good to make that clear in the log
> > > message.
> >
> > Oops, I was thinking of the field, not the local variable.
> > If the field is never set, that seems like a big problem...
> >
> > julia
> >
> >
> Julia, I'm a bit confused...
>
> I've anew examined that line and noticed that fw_current_in_ps_mode is
> referenced by through a pointer returned by adapter_to_pwrctl(padapter).
> padapter is a pointer set in other files of the driver.
>
> To me it seems that is perfectly reasonable that, as a result,
> fw_current_in_ps_mode could be set (externally) either with true or false.
>
> I'm sure I'm missing something, since both you and Greg have a larger
> experience than I could ever have. Can you please elaborate this topic a
> bit more in order to make me understand whatever I can't see?

OK, I was just following my recollection of the code.  It seems that the
only thing that is needed is to explain in the log message how the
variable is initialized and why changing the type doesn't mean that you
have to change the initializations.  A weakness of the patch notation is
that you only see what changed, and not all of the code that is actually
relevant.  So if there is something that is not visible and that is
important to convince the reader that the change is correct, then it
should be mentioned in the log message.

Again, not having looked at the code, but does this pointer already have a
bool type?  Or does it have a u8 type?  Perhaps the pointer has to be
changed too?

julia