Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 3:24:43 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > > > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > > > That variable has global scope and is assigned at least in:
> > > > > > What do you mean by global scope? None of the following look
> > > > > > like
> > > > > > references to global variables.
> > > > > >
> > > > > > julia
> > > > >
> > > > > I just mean that fw_current_in_ps_mode is a field of a struct in a
> > > > > .h
> > > > > file included everywhere in this driver and that the functions whom
> > > > > the following assignments belong to have not the "static" type
> > > > > modifier.
> > > >
> > > > OK, but a field in a structure is not a variable, and this is not
> > > > what
> > > > scope means.
> > >
> > > You're right, a field in a structure is not a variable.
> > >
> > > > int x;
> > > >
> > > > outside of anything is a global variable (global scope).
> > > >
> > > > int foo() {
> > > >
> > > > int x;
> > > > ...
> > > >
> > > > }
> > > >
> > > > Here x is a local variable. Its scope is the body of the function.
> > > >
> > > > int foo() {
> > > >
> > > > if (abc) {
> > > >
> > > > int x;
> > > > ...
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > Here x is a local variable, but its scope is only in the if branch.
> > >
> > > And you're right again: I needed a little refresh of my knowledge of C.
> > >
> > > I've searched again in the code for the purpose of finding out if that
> > > struct is initialized with global scope but I didn't find anything. I
> > > didn't even find any dynamic allocation within functions that returns
> > > pointers to that struct.
> > >
> > > Therefore, according to Greg's request, I'll delete that stupid 'if'
> > > statement in the patch series v2 that I'm about to submit.
> >
> > I'm really not clear on why the if should be deleted.
> >
> > julia
> >
> I'm supposed to delete it because of the review made by Greg. In a couple
> of previous messages he wrote:
>
> "If this is only checked, how can it ever be true? Who ever sets this
> value?"
>
> and then:
>
> "Just delete the variable from the structure entirely and when it is
> used.".
>
> However, like you, I'm not sure yet.
Be sure. Greg already said that he misinterpreted the patch, because he
thought that the name also changed.
julia
>
> Thanks,
>
> Fabio
> >
> > > I've really appreciated your help.
> > >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > julia
> > > >
> > > > > Thanks,
> > > > >
> > > > > Fabio
> > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Sat, Apr 10, 2021 at 03:53:38PM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 3:24:43 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > > > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > > > That variable has global scope and is assigned at least in:
> > > > > > What do you mean by global scope? None of the following look
> > > > > > like
> > > > > > references to global variables.
> > > > > >
> > > > > > julia
> > > > >
> > > > > I just mean that fw_current_in_ps_mode is a field of a struct in a
> > > > > .h
> > > > > file included everywhere in this driver and that the functions whom
> > > > > the following assignments belong to have not the "static" type
> > > > > modifier.
> > > >
> > > > OK, but a field in a structure is not a variable, and this is not
> > > > what
> > > > scope means.
> > >
> > > You're right, a field in a structure is not a variable.
> > >
> > > > int x;
> > > >
> > > > outside of anything is a global variable (global scope).
> > > >
> > > > int foo() {
> > > >
> > > > int x;
> > > > ...
> > > >
> > > > }
> > > >
> > > > Here x is a local variable. Its scope is the body of the function.
> > > >
> > > > int foo() {
> > > >
> > > > if (abc) {
> > > >
> > > > int x;
> > > > ...
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > Here x is a local variable, but its scope is only in the if branch.
> > >
> > > And you're right again: I needed a little refresh of my knowledge of C.
> > >
> > > I've searched again in the code for the purpose of finding out if that
> > > struct is initialized with global scope but I didn't find anything. I
> > > didn't even find any dynamic allocation within functions that returns
> > > pointers to that struct.
> > >
> > > Therefore, according to Greg's request, I'll delete that stupid 'if'
> > > statement in the patch series v2 that I'm about to submit.
> >
> > I'm really not clear on why the if should be deleted.
> >
> > julia
> >
> I'm supposed to delete it because of the review made by Greg. In a couple
> of previous messages he wrote:
>
> "If this is only checked, how can it ever be true? Who ever sets this
> value?"
>
> and then:
>
> "Just delete the variable from the structure entirely and when it is
> used.".
>
> However, like you, I'm not sure yet.
I don't think any of us are, try fixing up what you think is right and
resend and we can go from there :)
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Saturday, April 10, 2021 3:24:43 PM CEST Julia Lawall wrote:
> On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > > That variable has global scope and is assigned at least in:
> > > > > What do you mean by global scope? None of the following look
> > > > > like
> > > > > references to global variables.
> > > > >
> > > > > julia
> > > >
> > > > I just mean that fw_current_in_ps_mode is a field of a struct in a
> > > > .h
> > > > file included everywhere in this driver and that the functions whom
> > > > the following assignments belong to have not the "static" type
> > > > modifier.
> > >
> > > OK, but a field in a structure is not a variable, and this is not
> > > what
> > > scope means.
> >
> > You're right, a field in a structure is not a variable.
> >
> > > int x;
> > >
> > > outside of anything is a global variable (global scope).
> > >
> > > int foo() {
> > >
> > > int x;
> > > ...
> > >
> > > }
> > >
> > > Here x is a local variable. Its scope is the body of the function.
> > >
> > > int foo() {
> > >
> > > if (abc) {
> > >
> > > int x;
> > > ...
> > >
> > > }
> > >
> > > }
> > >
> > > Here x is a local variable, but its scope is only in the if branch.
> >
> > And you're right again: I needed a little refresh of my knowledge of C.
> >
> > I've searched again in the code for the purpose of finding out if that
> > struct is initialized with global scope but I didn't find anything. I
> > didn't even find any dynamic allocation within functions that returns
> > pointers to that struct.
> >
> > Therefore, according to Greg's request, I'll delete that stupid 'if'
> > statement in the patch series v2 that I'm about to submit.
>
> I'm really not clear on why the if should be deleted.
>
> julia
>
I'm supposed to delete it because of the review made by Greg. In a couple
of previous messages he wrote:
"If this is only checked, how can it ever be true? Who ever sets this
value?"
and then:
"Just delete the variable from the structure entirely and when it is
used.".
However, like you, I'm not sure yet.
Thanks,
Fabio
>
> > I've really appreciated your help.
> >
> > Thanks,
> >
> > Fabio
> >
> > > julia
> > >
> > > > Thanks,
> > > >
> > > > Fabio
> > > >
> > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > > >
> > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > > >
> > > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > > >
> > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > That variable has global scope and is assigned at least in:
> > > > What do you mean by global scope? None of the following look like
> > > > references to global variables.
> > > >
> > > > julia
> > >
> > > I just mean that fw_current_in_ps_mode is a field of a struct in a .h
> > > file included everywhere in this driver and that the functions whom
> > > the following assignments belong to have not the "static" type
> > > modifier.
> > OK, but a field in a structure is not a variable, and this is not what
> > scope means.
> >
> You're right, a field in a structure is not a variable.
> >
> > int x;
> >
> > outside of anything is a global variable (global scope).
> >
> > int foo() {
> > int x;
> > ...
> > }
> >
> > Here x is a local variable. Its scope is the body of the function.
> >
> > int foo() {
> > if (abc) {
> > int x;
> > ...
> > }
> > }
> >
> > Here x is a local variable, but its scope is only in the if branch.
> >
> And you're right again: I needed a little refresh of my knowledge of C.
>
> I've searched again in the code for the purpose of finding out if that
> struct is initialized with global scope but I didn't find anything. I
> didn't even find any dynamic allocation within functions that returns
> pointers to that struct.
>
> Therefore, according to Greg's request, I'll delete that stupid 'if'
> statement in the patch series v2 that I'm about to submit.
I'm really not clear on why the if should be deleted.
julia
>
> I've really appreciated your help.
>
> Thanks,
>
> Fabio
> >
> > julia
> >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > >
> > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > That variable has global scope and is assigned at least in:
> > > What do you mean by global scope? None of the following look like
> > > references to global variables.
> > >
> > > julia
> >
> > I just mean that fw_current_in_ps_mode is a field of a struct in a .h
> > file included everywhere in this driver and that the functions whom
> > the following assignments belong to have not the "static" type
> > modifier.
> OK, but a field in a structure is not a variable, and this is not what
> scope means.
>
You're right, a field in a structure is not a variable.
>
> int x;
>
> outside of anything is a global variable (global scope).
>
> int foo() {
> int x;
> ...
> }
>
> Here x is a local variable. Its scope is the body of the function.
>
> int foo() {
> if (abc) {
> int x;
> ...
> }
> }
>
> Here x is a local variable, but its scope is only in the if branch.
>
And you're right again: I needed a little refresh of my knowledge of C.
I've searched again in the code for the purpose of finding out if that
struct is initialized with global scope but I didn't find anything. I
didn't even find any dynamic allocation within functions that returns
pointers to that struct.
Therefore, according to Greg's request, I'll delete that stupid 'if'
statement in the patch series v2 that I'm about to submit.
I've really appreciated your help.
Thanks,
Fabio
>
> julia
>
> > Thanks,
> >
> > Fabio
> >
> > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > pwrpriv->fw_current_in_ps_mode = false;
> > > >
> > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > pwrpriv->fw_current_in_ps_mode = true;
> > > >
> > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > >
> > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > pwrctrlpriv->fw_current_in_ps_mode = false;
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > That variable has global scope and is assigned at least in:
> > What do you mean by global scope? None of the following look like
> > references to global variables.
> >
> > julia
> I just mean that fw_current_in_ps_mode is a field of a struct in a .h file
> included everywhere in this driver and that the functions whom the
> following assignments belong to have not the "static" type modifier.
OK, but a field in a structure is not a variable, and this is not what
scope means.
int x;
outside of anything is a global variable (global scope).
int foo() {
int x;
...
}
Here x is a local variable. Its scope is the body of the function.
int foo() {
if (abc) {
int x;
...
}
}
Here x is a local variable, but its scope is only in the if branch.
julia
>
> Thanks,
>
> Fabio
> >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > pwrpriv->fw_current_in_ps_mode = false;
> > >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > pwrpriv->fw_current_in_ps_mode = true;
> > >
> > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote: > > That variable has global scope and is assigned at least in: > What do you mean by global scope? None of the following look like > references to global variables. > > julia I just mean that fw_current_in_ps_mode is a field of a struct in a .h file included everywhere in this driver and that the functions whom the following assignments belong to have not the "static" type modifier. Thanks, Fabio > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368: > > pwrpriv->fw_current_in_ps_mode = false; > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380: > > pwrpriv->fw_current_in_ps_mode = true; > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433: > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false; > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981: > > pwrctrlpriv->fw_current_in_ps_mode = false;
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
> That variable has global scope and is assigned at least in: What do you mean by global scope? None of the following look like references to global variables. julia > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368: > pwrpriv->fw_current_in_ps_mode = false; > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380: > pwrpriv->fw_current_in_ps_mode = true; > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433: > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false; > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981: > pwrctrlpriv->fw_current_in_ps_mode = false;
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Sat, Apr 10, 2021 at 01:23:21PM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 1:03:34 PM CEST Greg KH wrote:
> > On Sat, Apr 10, 2021 at 12:58:06PM +0200, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 12:31:27 PM CEST Greg KH wrote:
> > > > On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco
> wrote:
> > > > > On Saturday, April 10, 2021 11:31:16 AM CEST 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
> > > > > > > ---
> > > > > > >
> > > > > > > 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?
> > > > >
> > > > > You're right. It is not set, therefore the "if" control expression
> > > > > cannot ever be "true".
> > > > >
> > > > > Can I delete this statement in a new patch? Or you prefer I send
> > > > > the
> > > > > whole series again with this change in patch 4/4?
> > > >
> > > > Just delete the variable from the structure entirely and when it is
> > > > used.
> > >
> > > I've read the code of the function whom that 'if' statement belongs to.
> > > This function takes a pointer whose name is 'padapter' and this is has
> > > global scope. I think that since fw_current_in_ps_mode is dereferenced
> > > by the function adapter_to_pwrctl(padapter) it can and is indeed
> > > initialized and modified in some other files of the driver.
> >
> > Where does that happen, and why did the build not break when you changed
> > the variable name?
> >
> The build didn't break because I changed the name of that variable
> everywhere it is used in the driver. This patch is against all the affected
> files of each subdirectory of staging/rtl8723bs.
Right, and as you only had to change it in one place, that tested if
that variable was set to a specific value, and there is no code that
actually sets the variable, it seems like that variable is not ever used
and can be removed.
> > Is the whole variable assigned to a specific
> > location in memory in the device? Where is it initialized?
> >
> That variable has global scope and is assigned at least in:
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> pwrpriv->fw_current_in_ps_mode = false;
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> pwrpriv->fw_current_in_ps_mode = true;
>
> drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Saturday, April 10, 2021 1:03:34 PM CEST Greg KH wrote:
> On Sat, Apr 10, 2021 at 12:58:06PM +0200, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 12:31:27 PM CEST Greg KH wrote:
> > > On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco
wrote:
> > > > On Saturday, April 10, 2021 11:31:16 AM CEST 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
> > > > > > ---
> > > > > >
> > > > > > 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?
> > > >
> > > > You're right. It is not set, therefore the "if" control expression
> > > > cannot ever be "true".
> > > >
> > > > Can I delete this statement in a new patch? Or you prefer I send
> > > > the
> > > > whole series again with this change in patch 4/4?
> > >
> > > Just delete the variable from the structure entirely and when it is
> > > used.
> >
> > I've read the code of the function whom that 'if' statement belongs to.
> > This function takes a pointer whose name is 'padapter' and this is has
> > global scope. I think that since fw_current_in_ps_mode is dereferenced
> > by the function adapter_to_pwrctl(padapter) it can and is indeed
> > initialized and modified in some other files of the driver.
>
> Where does that happen, and why did the build not break when you changed
> the variable name?
>
The build didn't break because I changed the name of that variable
everywhere it is used in the driver. This patch is against all the affected
files of each subdirectory of staging/rtl8723bs.
>
> Is the whole variable assigned to a specific
> location in memory in the device? Where is it initialized?
>
That variable has global scope and is assigned at least in:
drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
pwrpriv->fw_current_in_ps_mode = false;
drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
pwrpriv->fw_current_in_ps_mode = true;
drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
pwrctrlpriv->fw_current_in_ps_mode = false;
Thanks,
Fabio
>
> > That's why I'll leave the if statement as is now. If I am wrong there's
> > time to change it later in a future patch.
>
> Don't change obviously wrong code, we can clean it up properly :)
>
> thanks,
>
> greg k-h
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Sat, Apr 10, 2021 at 12:58:06PM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 12:31:27 PM CEST Greg KH wrote:
> > On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 11:31:16 AM CEST 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
> > > > > ---
> > > > >
> > > > > 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?
> > >
> > > You're right. It is not set, therefore the "if" control expression
> > > cannot ever be "true".
> > >
> > > Can I delete this statement in a new patch? Or you prefer I send the
> > > whole series again with this change in patch 4/4?
> >
> > Just delete the variable from the structure entirely and when it is
> > used.
> >
> I've read the code of the function whom that 'if' statement belongs to.
> This function takes a pointer whose name is 'padapter' and this is has
> global scope. I think that since fw_current_in_ps_mode is dereferenced by
> the function adapter_to_pwrctl(padapter) it can and is indeed initialized
> and modified in some other files of the driver.
Where does that happen, and why did the build not break when you changed
the variable name? Is the whole variable assigned to a specific
location in memory in the device? Where is it initialized?
> That's why I'll leave the if statement as is now. If I am wrong there's
> time to change it later in a future patch.
Don't change obviously wrong code, we can clean it up properly :)
thanks,
greg k-h
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Saturday, April 10, 2021 12:31:27 PM CEST Greg KH wrote:
> On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 11:31:16 AM CEST 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
> > > > ---
> > > >
> > > > 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?
> >
> > You're right. It is not set, therefore the "if" control expression
> > cannot ever be "true".
> >
> > Can I delete this statement in a new patch? Or you prefer I send the
> > whole series again with this change in patch 4/4?
>
> Just delete the variable from the structure entirely and when it is
> used.
>
I've read the code of the function whom that 'if' statement belongs to.
This function takes a pointer whose name is 'padapter' and this is has
global scope. I think that since fw_current_in_ps_mode is dereferenced by
the function adapter_to_pwrctl(padapter) it can and is indeed initialized
and modified in some other files of the driver.
That's why I'll leave the if statement as is now. If I am wrong there's
time to change it later in a future patch.
Thanks for your kind review,
Fabio
>
>
> thanks,
>
> greg k-h
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 11:31:16 AM CEST 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
> > > ---
> > >
> > > 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?
> >
> You're right. It is not set, therefore the "if" control expression cannot
> ever be "true".
>
> Can I delete this statement in a new patch? Or you prefer I send the whole
> series again with this change in patch 4/4?
Just delete the variable from the structure entirely and when it is
used.
thanks,
greg k-h
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
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
> > > ---
> > > 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
>
> >
> > 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+unsubscr...@googlegroups.com.
> > To view this discussion on the web visit
> > https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQN%40kroah.com.
> >
>
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
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
> > ---
> > 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.
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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQN%40kroah.com.
>
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
On Saturday, April 10, 2021 11:31:16 AM CEST 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
> > ---
> >
> > 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?
>
You're right. It is not set, therefore the "if" control expression cannot
ever be "true".
Can I delete this statement in a new patch? Or you prefer I send the whole
series again with this change in patch 4/4?
Thanks,
Fabio
>
> thanks,
>
> greg k-h
Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable
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
> ---
> 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?
thanks,
greg k-h
