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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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?

Thanks for your kind help,

Fabio
>
> 
> > julia
> > 
> > > thanks,
> > > 
> > > greg k-h
> > > 
> > > --
> > > You received this message because you are subscribed to the Google
> > > Groups "outreachy-kernel" group. To unsubscribe from this group and
> > > stop receiving emails from it, send an email to
> > > outreachy-kernel+unsubscribe@googlegroups.com. To view this
> > > discussion on the web visit
> > > https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQ
> > > N%40kroah.com.