[Outreachy kernel] [PATCH 0/2] Removed set but unused variables

[Outreachy kernel] [PATCH 0/2] Removed set but unused variables

[Fabio M. De Francesco]

Removed set but unused variables. Issue detected by gcc.

Fabio M. De Francesco (2):
  staging: rtl8723bs: hal: Remove set but unused variables.
  staging: rtl8723bs: core: Removed set but unused variable.

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c     | 4 +---
 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
 2 files changed, 1 insertion(+), 8 deletions(-)

-- 
2.31.1

[Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio M. De Francesco]

Removed four set but unused variables. Issue detected by gcc.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 082448557b53..96cb4426a0f4 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter *padapter, enum hal_def_variable variable, v
 			u32 cmd;
 			u32 ra_info1, ra_info2;
 			u32 rate_mask1, rate_mask2;
-			u8 curr_tx_rate, curr_tx_sgi, hight_rate, lowest_rate;
 
 			cmd = 0x40000100 | mac_id;
 			rtw_write32(padapter, REG_HMEBOX_DBG_2_8723B, cmd);
 			msleep(10);
 			ra_info1 = rtw_read32(padapter, 0x2F0);
-			curr_tx_rate = ra_info1&0x7F;
-			curr_tx_sgi = (ra_info1>>7)&0x01;
 
 			cmd = 0x40000400 | mac_id;
 			rtw_write32(padapter, REG_HMEBOX_DBG_2_8723B, cmd);
@@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, enum hal_def_variable variable, v
 			ra_info2 = rtw_read32(padapter, 0x2F4);
 			rate_mask1 = rtw_read32(padapter, 0x2F8);
 			rate_mask2 = rtw_read32(padapter, 0x2FC);
-			hight_rate = ra_info2&0xFF;
-			lowest_rate = (ra_info2>>8)  & 0xFF;
 
 		}
 		break;
-- 
2.31.1

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio Aiuto]

Hi Fabio,

On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> Removed four set but unused variables. Issue detected by gcc.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> index 082448557b53..96cb4426a0f4 100644
> --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter *padapter, enum hal_def_variable variable, v
>  			u32 cmd;
>  			u32 ra_info1, ra_info2;
>  			u32 rate_mask1, rate_mask2;
> -			u8 curr_tx_rate, curr_tx_sgi, hight_rate, lowest_rate;
>  
>  			cmd = 0x40000100 | mac_id;
>  			rtw_write32(padapter, REG_HMEBOX_DBG_2_8723B, cmd);
>  			msleep(10);
>  			ra_info1 = rtw_read32(padapter, 0x2F0);
> -			curr_tx_rate = ra_info1&0x7F;
> -			curr_tx_sgi = (ra_info1>>7)&0x01;
>  
>  			cmd = 0x40000400 | mac_id;
>  			rtw_write32(padapter, REG_HMEBOX_DBG_2_8723B, cmd);
> @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, enum hal_def_variable variable, v
>  			ra_info2 = rtw_read32(padapter, 0x2F4);
>  			rate_mask1 = rtw_read32(padapter, 0x2F8);
>  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> -			hight_rate = ra_info2&0xFF;
> -			lowest_rate = (ra_info2>>8)  & 0xFF;
>  
>  		}
>  		break;
> -- 
> 2.31.1
> 
>

rate_mask{1,2} and ra_info{1,2} seems to be unused as well.

thank you,

fabio 

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio M. De Francesco]

On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> Hi Fabio,
> 
> On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> > Removed four set but unused variables. Issue detected by gcc.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index 
082448557b53..96cb4426a0f4
> > 100644
> > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter *padapter, 
enum
> > hal_def_variable variable, v> 
> >  			u32 cmd;
> >  			u32 ra_info1, ra_info2;
> >  			u32 rate_mask1, rate_mask2;
> > 
> > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate, 
lowest_rate;
> > 
> >  			cmd = 0x40000100 | mac_id;
> >  			rtw_write32(padapter, 
REG_HMEBOX_DBG_2_8723B, cmd);
> >  			msleep(10);
> >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > 
> > -			curr_tx_rate = ra_info1&0x7F;
> > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > 
> >  			cmd = 0x40000400 | mac_id;
> >  			rtw_write32(padapter, 
REG_HMEBOX_DBG_2_8723B, cmd);
> > 
> > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, enum
> > hal_def_variable variable, v> 
> >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > 
> > -			hight_rate = ra_info2&0xFF;
> > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > 
> >  		}
> >  		break;
> 
> rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> 
> thank you,
> 
> fabio
>
Hello Fabio,

I'm not sure about it: rtw_read32 calls a pointer to a function. I'm don't 
know drivers programming, however that function looks like an implementation 
of a read() system call. So I wouldn't be so sure to remove those calls. 

Could calling a (*read) method have side effects on subsequent read()? I mean: 
could it update some internal data structure? If not I can remove the 
variables you mentioned above and the calls to read32.

I'm looking forward to read your reply.

Thanks,

Fabio

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio Aiuto]

On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > Hi Fabio,
> > 
> > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> > > Removed four set but unused variables. Issue detected by gcc.
> > > 
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > > 
> > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > >  1 file changed, 5 deletions(-)
> > > 
> > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index 
> 082448557b53..96cb4426a0f4
> > > 100644
> > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter *padapter, 
> enum
> > > hal_def_variable variable, v> 
> > >  			u32 cmd;
> > >  			u32 ra_info1, ra_info2;
> > >  			u32 rate_mask1, rate_mask2;
> > > 
> > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate, 
> lowest_rate;
> > > 
> > >  			cmd = 0x40000100 | mac_id;
> > >  			rtw_write32(padapter, 
> REG_HMEBOX_DBG_2_8723B, cmd);
> > >  			msleep(10);
> > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > 
> > > -			curr_tx_rate = ra_info1&0x7F;
> > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > 
> > >  			cmd = 0x40000400 | mac_id;
> > >  			rtw_write32(padapter, 
> REG_HMEBOX_DBG_2_8723B, cmd);
> > > 
> > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, enum
> > > hal_def_variable variable, v> 
> > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > 
> > > -			hight_rate = ra_info2&0xFF;
> > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > 
> > >  		}
> > >  		break;
> > 
> > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > 
> > thank you,
> > 
> > fabio
> >
> Hello Fabio,
> 
> I'm not sure about it: rtw_read32 calls a pointer to a function. I'm don't 
> know drivers programming, however that function looks like an implementation 
> of a read() system call. So I wouldn't be so sure to remove those calls. 
> 
> Could calling a (*read) method have side effects on subsequent read()? I mean: 
> could it update some internal data structure? If not I can remove the 
> variables you mentioned above and the calls to read32.
> 
> I'm looking forward to read your reply.
> 
> Thanks,
> 
> Fabio
> 
> 
> 

hi Fabio,

rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in core/rtw_io.c):

u32 _rtw_read32(struct adapter *adapter, u32 addr)
{
        u32 r_val;
        /* struct       io_queue        *pio_queue = (struct io_queue *)adapter->pio_queue; */
        struct io_priv *pio_priv = &adapter->iopriv;
        struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
        u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);

        _read32 = pintfhdl->io_ops._read32;

        r_val = _read32(pintfhdl, addr);
        return rtw_le32_to_cpu(r_val);

}

the actual read seems to be performed by the handler contained in

	pintfhdl->io_ops._read32;

so:

$ grep -r '\b_read32' drivers/staging/rtl8723bs/

drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 = &sdio_read32;

this is the place where _read32 is stored with sdio_read32 reference... 

drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 = pintfhdl->io_ops._read32;
...

if you check it in hal/sdio_ops.c, nothing is written, just reads are
performed (and it's not odd, for a read function isn't supposed to write
something under the hood ;)).

I think those variables could be easily removed as W=1 gcc suggests.

thank you,

fabio

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio M. De Francesco]

On Thursday, April 29, 2021 10:25:53 AM CEST Fabio Aiuto wrote:
> On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> > On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > > Hi Fabio,
> > > 
> > > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> > > > Removed four set but unused variables. Issue detected by gcc.
> > > > 
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > > >  1 file changed, 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index
> > 
> > 082448557b53..96cb4426a0f4
> > 
> > > > 100644
> > > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter *padapter,
> > 
> > enum
> > 
> > > > hal_def_variable variable, v>
> > > > 
> > > >  			u32 cmd;
> > > >  			u32 ra_info1, ra_info2;
> > > >  			u32 rate_mask1, rate_mask2;
> > > > 
> > > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate,
> > 
> > lowest_rate;
> > 
> > > >  			cmd = 0x40000100 | mac_id;
> > > >  			rtw_write32(padapter,
> > 
> > REG_HMEBOX_DBG_2_8723B, cmd);
> > 
> > > >  			msleep(10);
> > > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > > 
> > > > -			curr_tx_rate = ra_info1&0x7F;
> > > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > > 
> > > >  			cmd = 0x40000400 | mac_id;
> > > >  			rtw_write32(padapter,
> > 
> > REG_HMEBOX_DBG_2_8723B, cmd);
> > 
> > > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, 
enum
> > > > hal_def_variable variable, v>
> > > > 
> > > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > > 
> > > > -			hight_rate = ra_info2&0xFF;
> > > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > > 
> > > >  		}
> > > >  		break;
> > > 
> > > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > > 
> > > thank you,
> > > 
> > > fabio
> > 
> > Hello Fabio,
> > 
> > I'm not sure about it: rtw_read32 calls a pointer to a function. I'm don't
> > know drivers programming, however that function looks like an 
implementation
> > of a read() system call. So I wouldn't be so sure to remove those calls.
> > 
> > Could calling a (*read) method have side effects on subsequent read()? I 
mean:
> > could it update some internal data structure? If not I can remove the
> > variables you mentioned above and the calls to read32.
> > 
> > I'm looking forward to read your reply.
> > 
> > Thanks,
> > 
> > Fabio
> 
> hi Fabio,
> 
> rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in core/
rtw_io.c):
>
Hi Fabio,

Thanks a lot for your reply.

However, There is something less than clear to me... how did you find that 
rtw_read32 is a macro wrapping _rtw_read32()? I mean: I knew that, in vim, one 
can go to the definition of something by using ctrl-] key.

If I do that on rtw_read32 it takes me to a static definition of it in 
drivers/net/wireless/realtek/rtw88/hci.h. This is a one line function:

static inline void rtw_write32(struct rtw_dev *rtwdev, u32 addr, u32 val)
{       
        rtwdev->hci.ops->write32(rtwdev, addr, val);
}

When I use ctrl-] on write32() it takes me to struct rtw_hci_ops in drivers/
net/wireless/realtek/rtw88/hci.h.

After that I wanted to find where the member (*read32)() is assigned but I 
don't know where and how to grep it: I tried "grep -rn "\bwrite32\b=" drivers/
staging/rtl8723bs/" but I found nothing...

Can you please explain what I'm doing wrong in following the path I mentioned 
above and how you find out that macro?

Thanks for your time,

Fabio 
> 
> u32 _rtw_read32(struct adapter *adapter, u32 addr)
> {
>         u32 r_val;
>         /* struct       io_queue        *pio_queue = (struct io_queue
> *)adapter->pio_queue; */ struct io_priv *pio_priv = &adapter->iopriv;
>         struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
>         u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> 
>         _read32 = pintfhdl->io_ops._read32;
> 
>         r_val = _read32(pintfhdl, addr);
>         return rtw_le32_to_cpu(r_val);
> 
> }
> 
> the actual read seems to be performed by the handler contained in
> 
> 	pintfhdl->io_ops._read32;
> 
> so:
> 
> $ grep -r '\b_read32' drivers/staging/rtl8723bs/
> 
> drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 = 
&sdio_read32;
> 
> this is the place where _read32 is stored with sdio_read32 reference...
> 
> drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct 
intf_hdl *pintfhdl, u32
> addr); drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 = pintfhdl-
>io_ops._read32;
> ...
> 
> if you check it in hal/sdio_ops.c, nothing is written, just reads are
> performed (and it's not odd, for a read function isn't supposed to write
> something under the hood ;)).
> 
> I think those variables could be easily removed as W=1 gcc suggests.
> 
> thank you,
> 
> fabio

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio Aiuto]

On Thu, Apr 29, 2021 at 11:16:24AM +0200, Fabio M. De Francesco wrote:
> On Thursday, April 29, 2021 10:25:53 AM CEST Fabio Aiuto wrote:
> > On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> > > On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > > > Hi Fabio,
> > > > 
> > > > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> > > > > Removed four set but unused variables. Issue detected by gcc.
> > > > > 
> > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > > > >  1 file changed, 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index
> > > 
> > > 082448557b53..96cb4426a0f4
> > > 
> > > > > 100644
> > > > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter *padapter,
> > > 
> > > enum
> > > 
> > > > > hal_def_variable variable, v>
> > > > > 
> > > > >  			u32 cmd;
> > > > >  			u32 ra_info1, ra_info2;
> > > > >  			u32 rate_mask1, rate_mask2;
> > > > > 
> > > > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate,
> > > 
> > > lowest_rate;
> > > 
> > > > >  			cmd = 0x40000100 | mac_id;
> > > > >  			rtw_write32(padapter,
> > > 
> > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > 
> > > > >  			msleep(10);
> > > > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > > > 
> > > > > -			curr_tx_rate = ra_info1&0x7F;
> > > > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > > > 
> > > > >  			cmd = 0x40000400 | mac_id;
> > > > >  			rtw_write32(padapter,
> > > 
> > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > 
> > > > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, 
> enum
> > > > > hal_def_variable variable, v>
> > > > > 
> > > > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > > > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > > > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > > > 
> > > > > -			hight_rate = ra_info2&0xFF;
> > > > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > > > 
> > > > >  		}
> > > > >  		break;
> > > > 
> > > > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > > > 
> > > > thank you,
> > > > 
> > > > fabio
> > > 
> > > Hello Fabio,
> > > 
> > > I'm not sure about it: rtw_read32 calls a pointer to a function. I'm don't
> > > know drivers programming, however that function looks like an 
> implementation
> > > of a read() system call. So I wouldn't be so sure to remove those calls.
> > > 
> > > Could calling a (*read) method have side effects on subsequent read()? I 
> mean:
> > > could it update some internal data structure? If not I can remove the
> > > variables you mentioned above and the calls to read32.
> > > 
> > > I'm looking forward to read your reply.
> > > 
> > > Thanks,
> > > 
> > > Fabio
> > 
> > hi Fabio,
> > 
> > rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in core/
> rtw_io.c):
> >
> Hi Fabio,
> 
> Thanks a lot for your reply.
> 
> However, There is something less than clear to me... how did you find that 
> rtw_read32 is a macro wrapping _rtw_read32()? I mean: I knew that, in vim, one 
> can go to the definition of something by using ctrl-] key.
> 
> If I do that on rtw_read32 it takes me to a static definition of it in 
> drivers/net/wireless/realtek/rtw88/hci.h. This is a one line function:
> 
> static inline void rtw_write32(struct rtw_dev *rtwdev, u32 addr, u32 val)
> {       
>         rtwdev->hci.ops->write32(rtwdev, addr, val);
> }
> 
> When I use ctrl-] on write32() it takes me to struct rtw_hci_ops in drivers/
> net/wireless/realtek/rtw88/hci.h.
> 
> After that I wanted to find where the member (*read32)() is assigned but I 
> don't know where and how to grep it: I tried "grep -rn "\bwrite32\b=" drivers/
> staging/rtl8723bs/" but I found nothing...
> 
> Can you please explain what I'm doing wrong in following the path I mentioned 
> above and how you find out that macro?

$grep -r 'define\s*\brtw_read32' drivers/staging/rtl8723bs/

it's a grep I always do to check if a function is actally
a function or a macro..


> 
> Thanks for your time,
> 
> Fabio 
> > 
> > u32 _rtw_read32(struct adapter *adapter, u32 addr)
> > {
> >         u32 r_val;
> >         /* struct       io_queue        *pio_queue = (struct io_queue
> > *)adapter->pio_queue; */ struct io_priv *pio_priv = &adapter->iopriv;
> >         struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
> >         u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> > 
> >         _read32 = pintfhdl->io_ops._read32;
> > 
> >         r_val = _read32(pintfhdl, addr);
> >         return rtw_le32_to_cpu(r_val);
> > 
> > }
> > 
> > the actual read seems to be performed by the handler contained in
> > 
> > 	pintfhdl->io_ops._read32;
> > 
> > so:
> > 
> > $ grep -r '\b_read32' drivers/staging/rtl8723bs/
> > 
> > drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 = 
> &sdio_read32;
> > 
> > this is the place where _read32 is stored with sdio_read32 reference...
> > 
> > drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct 
> intf_hdl *pintfhdl, u32
> > addr); drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 = pintfhdl-
> >io_ops._read32;
> > ...
> > 
> > if you check it in hal/sdio_ops.c, nothing is written, just reads are
> > performed (and it's not odd, for a read function isn't supposed to write
> > something under the hood ;)).
> > 
> > I think those variables could be easily removed as W=1 gcc suggests.
> > 
> > thank you,
> > 
> > fabio
> 
> 
> 
> 

thanks,

fabio

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio M. De Francesco]

On Thursday, April 29, 2021 11:32:52 AM CEST Fabio Aiuto wrote:
> On Thu, Apr 29, 2021 at 11:16:24AM +0200, Fabio M. De Francesco wrote:
> > On Thursday, April 29, 2021 10:25:53 AM CEST Fabio Aiuto wrote:
> > > On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> > > > On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > > > > Hi Fabio,
> > > > > 
> > > > > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco 
wrote:
> > > > > > Removed four set but unused variables. Issue detected by gcc.
> > > > > > 
> > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > > > > >  1 file changed, 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index
> > > > 
> > > > 082448557b53..96cb4426a0f4
> > > > 
> > > > > > 100644
> > > > > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter 
*padapter,
> > > > 
> > > > enum
> > > > 
> > > > > > hal_def_variable variable, v>
> > > > > > 
> > > > > >  			u32 cmd;
> > > > > >  			u32 ra_info1, ra_info2;
> > > > > >  			u32 rate_mask1, rate_mask2;
> > > > > > 
> > > > > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate,
> > > > 
> > > > lowest_rate;
> > > > 
> > > > > >  			cmd = 0x40000100 | mac_id;
> > > > > >  			rtw_write32(padapter,
> > > > 
> > > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > 
> > > > > >  			msleep(10);
> > > > > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > > > > 
> > > > > > -			curr_tx_rate = ra_info1&0x7F;
> > > > > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > > > > 
> > > > > >  			cmd = 0x40000400 | mac_id;
> > > > > >  			rtw_write32(padapter,
> > > > 
> > > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > 
> > > > > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter 
*padapter,
> > 
> > enum
> > 
> > > > > > hal_def_variable variable, v>
> > > > > > 
> > > > > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > > > > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > > > > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > > > > 
> > > > > > -			hight_rate = ra_info2&0xFF;
> > > > > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > > > > 
> > > > > >  		}
> > > > > >  		break;
> > > > > 
> > > > > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > > > > 
> > > > > thank you,
> > > > > 
> > > > > fabio
> > > > 
> > > > Hello Fabio,
> > > > 
> > > > I'm not sure about it: rtw_read32 calls a pointer to a function. I'm 
don't
> > > > know drivers programming, however that function looks like an
> > 
> > implementation
> > 
> > > > of a read() system call. So I wouldn't be so sure to remove those 
calls.
> > > > 
> > > > Could calling a (*read) method have side effects on subsequent read()? 
I
> > 
> > mean:
> > > > could it update some internal data structure? If not I can remove the
> > > > variables you mentioned above and the calls to read32.
> > > > 
> > > > I'm looking forward to read your reply.
> > > > 
> > > > Thanks,
> > > > 
> > > > Fabio
> > > 
> > > hi Fabio,
> > > 
> > > rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in 
core/
> > 
> > rtw_io.c):
> > 
> > Hi Fabio,
> > 
> > Thanks a lot for your reply.
> > 
> > However, There is something less than clear to me... how did you find that
> > rtw_read32 is a macro wrapping _rtw_read32()? I mean: I knew that, in vim, 
one
> > can go to the definition of something by using ctrl-] key.
> > 
> > If I do that on rtw_read32 it takes me to a static definition of it in
> > drivers/net/wireless/realtek/rtw88/hci.h. This is a one line function:
> > 
> > static inline void rtw_write32(struct rtw_dev *rtwdev, u32 addr, u32 val)
> > {
> > 
> >         rtwdev->hci.ops->write32(rtwdev, addr, val);
> > 
> > }
> > 
> > When I use ctrl-] on write32() it takes me to struct rtw_hci_ops in 
drivers/
> > net/wireless/realtek/rtw88/hci.h.
> > 
> > After that I wanted to find where the member (*read32)() is assigned but I
> > don't know where and how to grep it: I tried "grep -rn "\bwrite32\b=" 
drivers/
> > staging/rtl8723bs/" but I found nothing...
> > 
> > Can you please explain what I'm doing wrong in following the path I 
mentioned
> > above and how you find out that macro?
> 
> $grep -r 'define\s*\brtw_read32' drivers/staging/rtl8723bs/
> 
Really interesting... does it imply that ctrl-] is broken? Or, does it just 
imply that I should use ctrl-] *if_and_only_if* the the command you showed 
above outputs nothing?

I think that automated finding of definitions is a fundamental tool, so I'd 
like to understand, in general, what are the steps and in which order they 
should be carried out. I'd really appreciate if you can list all the necessary 
steps. Other newcomers would benefit from that, IMO.

Apart the above question, I'm going to remove the other unused variables and 
add a Suggested-by in v2.

Thanks again,

Fabio
>
> it's a grep I always do to check if a function is actually
> a function or a macro..
> 
> > Thanks for your time,
> > 
> > Fabio
> > 
> > > u32 _rtw_read32(struct adapter *adapter, u32 addr)
> > > {
> > > 
> > >         u32 r_val;
> > >         /* struct       io_queue        *pio_queue = (struct io_queue
> > > 
> > > *)adapter->pio_queue; */ struct io_priv *pio_priv = &adapter->iopriv;
> > > 
> > >         struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
> > >         u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> > >         
> > >         _read32 = pintfhdl->io_ops._read32;
> > >         
> > >         r_val = _read32(pintfhdl, addr);
> > >         return rtw_le32_to_cpu(r_val);
> > > 
> > > }
> > > 
> > > the actual read seems to be performed by the handler contained in
> > > 
> > > 	pintfhdl->io_ops._read32;
> > > 
> > > so:
> > > 
> > > $ grep -r '\b_read32' drivers/staging/rtl8723bs/
> > > 
> > > drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 =
> > 
> > &sdio_read32;
> > 
> > > this is the place where _read32 is stored with sdio_read32 reference...
> > > 
> > > drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct
> > 
> > intf_hdl *pintfhdl, u32
> > 
> > > addr); drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 = pintfhdl-
> > >
> > >io_ops._read32;
> > >
> > > ...
> > > 
> > > if you check it in hal/sdio_ops.c, nothing is written, just reads are
> > > performed (and it's not odd, for a read function isn't supposed to write
> > > something under the hood ;)).
> > > 
> > > I think those variables could be easily removed as W=1 gcc suggests.
> > > 
> > > thank you,
> > > 
> > > fabio
> 
> thanks,
> 
> fabio

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Greg Kroah-Hartman]

On Thu, Apr 29, 2021 at 10:25:53AM +0200, Fabio Aiuto wrote:
> On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> > On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > > Hi Fabio,
> > > 
> > > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> > > > Removed four set but unused variables. Issue detected by gcc.
> > > > 
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > > >  1 file changed, 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index 
> > 082448557b53..96cb4426a0f4
> > > > 100644
> > > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter *padapter, 
> > enum
> > > > hal_def_variable variable, v> 
> > > >  			u32 cmd;
> > > >  			u32 ra_info1, ra_info2;
> > > >  			u32 rate_mask1, rate_mask2;
> > > > 
> > > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate, 
> > lowest_rate;
> > > > 
> > > >  			cmd = 0x40000100 | mac_id;
> > > >  			rtw_write32(padapter, 
> > REG_HMEBOX_DBG_2_8723B, cmd);
> > > >  			msleep(10);
> > > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > > 
> > > > -			curr_tx_rate = ra_info1&0x7F;
> > > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > > 
> > > >  			cmd = 0x40000400 | mac_id;
> > > >  			rtw_write32(padapter, 
> > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > 
> > > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, enum
> > > > hal_def_variable variable, v> 
> > > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > > 
> > > > -			hight_rate = ra_info2&0xFF;
> > > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > > 
> > > >  		}
> > > >  		break;
> > > 
> > > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > > 
> > > thank you,
> > > 
> > > fabio
> > >
> > Hello Fabio,
> > 
> > I'm not sure about it: rtw_read32 calls a pointer to a function. I'm don't 
> > know drivers programming, however that function looks like an implementation 
> > of a read() system call. So I wouldn't be so sure to remove those calls. 
> > 
> > Could calling a (*read) method have side effects on subsequent read()? I mean: 
> > could it update some internal data structure? If not I can remove the 
> > variables you mentioned above and the calls to read32.
> > 
> > I'm looking forward to read your reply.
> > 
> > Thanks,
> > 
> > Fabio
> > 
> > 
> > 
> 
> hi Fabio,
> 
> rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in core/rtw_io.c):
> 
> u32 _rtw_read32(struct adapter *adapter, u32 addr)
> {
>         u32 r_val;
>         /* struct       io_queue        *pio_queue = (struct io_queue *)adapter->pio_queue; */
>         struct io_priv *pio_priv = &adapter->iopriv;
>         struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
>         u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> 
>         _read32 = pintfhdl->io_ops._read32;
> 
>         r_val = _read32(pintfhdl, addr);
>         return rtw_le32_to_cpu(r_val);
> 
> }
> 
> the actual read seems to be performed by the handler contained in
> 
> 	pintfhdl->io_ops._read32;
> 
> so:
> 
> $ grep -r '\b_read32' drivers/staging/rtl8723bs/
> 
> drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 = &sdio_read32;
> 
> this is the place where _read32 is stored with sdio_read32 reference... 
> 
> drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 = pintfhdl->io_ops._read32;
> ...
> 
> if you check it in hal/sdio_ops.c, nothing is written, just reads are
> performed (and it's not odd, for a read function isn't supposed to write
> something under the hood ;)).

Yes, but many types of hardware _REQUIRE_ reads to do something.  So
"read that does nothing" is a requirement for some operations.

As an example, a write is only guaranteed to have been finished if you
do a read of the same location back from it on some hardware busses.
The bus can reorder things, but a write/read of the same location can
not be reordered.

Sometimes you have to do reads multiple times to get things to "stick".

Other times reading from a location changes a state in the hardware
(horrid but HW designers aren't the brightest at times...)

So you can NOT just remove reads without knowing that the hardware does
not require this.  This is an issue where GCC "warnings" mean nothing as
gcc does not actually know what hardware does, or does not, do for many
things.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio M. De Francesco]

On Thursday, April 29, 2021 12:01:45 PM CEST Greg Kroah-Hartman wrote:
> On Thu, Apr 29, 2021 at 10:25:53AM +0200, Fabio Aiuto wrote:
> > On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> > > On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > > > Hi Fabio,
> > > > 
> > > > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> > > > > Removed four set but unused variables. Issue detected by gcc.
> > > > > 
> > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > > > >  1 file changed, 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index
> > > 
> > > 082448557b53..96cb4426a0f4
> > > 
> > > > > 100644
> > > > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter 
*padapter,
> > > 
> > > enum
> > > 
> > > > > hal_def_variable variable, v>
> > > > > 
> > > > >  			u32 cmd;
> > > > >  			u32 ra_info1, ra_info2;
> > > > >  			u32 rate_mask1, rate_mask2;
> > > > > 
> > > > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate,
> > > 
> > > lowest_rate;
> > > 
> > > > >  			cmd = 0x40000100 | mac_id;
> > > > >  			rtw_write32(padapter,
> > > 
> > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > 
> > > > >  			msleep(10);
> > > > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > > > 
> > > > > -			curr_tx_rate = ra_info1&0x7F;
> > > > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > > > 
> > > > >  			cmd = 0x40000400 | mac_id;
> > > > >  			rtw_write32(padapter,
> > > 
> > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > 
> > > > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, 
enum
> > > > > hal_def_variable variable, v>
> > > > > 
> > > > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > > > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > > > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > > > 
> > > > > -			hight_rate = ra_info2&0xFF;
> > > > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > > > 
> > > > >  		}
> > > > >  		break;
> > > > 
> > > > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > > > 
> > > > thank you,
> > > > 
> > > > fabio
> > > 
> > > Hello Fabio,
> > > 
> > > I'm not sure about it: rtw_read32 calls a pointer to a function. I'm 
don't
> > > know drivers programming, however that function looks like an 
implementation
> > > of a read() system call. So I wouldn't be so sure to remove those calls.
> > > 
> > > Could calling a (*read) method have side effects on subsequent read()? I 
mean:
> > > could it update some internal data structure? If not I can remove the
> > > variables you mentioned above and the calls to read32.
> > > 
> > > I'm looking forward to read your reply.
> > > 
> > > Thanks,
> > > 
> > > Fabio
> > 
> > hi Fabio,
> > 
> > rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in core/
rtw_io.c):
> > 
> > u32 _rtw_read32(struct adapter *adapter, u32 addr)
> > {
> > 
> >         u32 r_val;
> >         /* struct       io_queue        *pio_queue = (struct io_queue
> >         *)adapter->pio_queue; */
> >         struct io_priv *pio_priv = &adapter->iopriv;
> >         struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
> >         u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> >         
> >         _read32 = pintfhdl->io_ops._read32;
> >         
> >         r_val = _read32(pintfhdl, addr);
> >         return rtw_le32_to_cpu(r_val);
> > 
> > }
> > 
> > the actual read seems to be performed by the handler contained in
> > 
> > 	pintfhdl->io_ops._read32;
> > 
> > so:
> > 
> > $ grep -r '\b_read32' drivers/staging/rtl8723bs/
> > 
> > drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 = 
&sdio_read32;
> > 
> > this is the place where _read32 is stored with sdio_read32 reference...
> > 
> > drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct 
intf_hdl *pintfhdl, u32
> > addr); drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 = pintfhdl-
>io_ops._read32;
> > ...
> > 
> > if you check it in hal/sdio_ops.c, nothing is written, just reads are
> > performed (and it's not odd, for a read function isn't supposed to write
> > something under the hood ;)).
> 
> Yes, but many types of hardware _REQUIRE_ reads to do something.  So
> "read that does nothing" is a requirement for some operations.
> 
> As an example, a write is only guaranteed to have been finished if you
> do a read of the same location back from it on some hardware busses.
> The bus can reorder things, but a write/read of the same location can
> not be reordered.
> 
> Sometimes you have to do reads multiple times to get things to "stick".
> 
> Other times reading from a location changes a state in the hardware
> (horrid but HW designers aren't the brightest at times...)
> 
> So you can NOT just remove reads without knowing that the hardware does
> not require this.  This is an issue where GCC "warnings" mean nothing as
> gcc does not actually know what hardware does, or does not, do for many
> things.
> 
> thanks,
> 
> greg k-h
>
Just to be sure... does it mean that v1 can be applied as-is?

Thanks,

Fabio

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Greg Kroah-Hartman]

On Thu, Apr 29, 2021 at 12:15:40PM +0200, Fabio M. De Francesco wrote:
> On Thursday, April 29, 2021 12:01:45 PM CEST Greg Kroah-Hartman wrote:
> > On Thu, Apr 29, 2021 at 10:25:53AM +0200, Fabio Aiuto wrote:
> > > On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> > > > On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > > > > Hi Fabio,
> > > > > 
> > > > > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> > > > > > Removed four set but unused variables. Issue detected by gcc.
> > > > > > 
> > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > > > > >  1 file changed, 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index
> > > > 
> > > > 082448557b53..96cb4426a0f4
> > > > 
> > > > > > 100644
> > > > > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter 
> *padapter,
> > > > 
> > > > enum
> > > > 
> > > > > > hal_def_variable variable, v>
> > > > > > 
> > > > > >  			u32 cmd;
> > > > > >  			u32 ra_info1, ra_info2;
> > > > > >  			u32 rate_mask1, rate_mask2;
> > > > > > 
> > > > > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate,
> > > > 
> > > > lowest_rate;
> > > > 
> > > > > >  			cmd = 0x40000100 | mac_id;
> > > > > >  			rtw_write32(padapter,
> > > > 
> > > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > 
> > > > > >  			msleep(10);
> > > > > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > > > > 
> > > > > > -			curr_tx_rate = ra_info1&0x7F;
> > > > > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > > > > 
> > > > > >  			cmd = 0x40000400 | mac_id;
> > > > > >  			rtw_write32(padapter,
> > > > 
> > > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > 
> > > > > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, 
> enum
> > > > > > hal_def_variable variable, v>
> > > > > > 
> > > > > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > > > > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > > > > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > > > > 
> > > > > > -			hight_rate = ra_info2&0xFF;
> > > > > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > > > > 
> > > > > >  		}
> > > > > >  		break;
> > > > > 
> > > > > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > > > > 
> > > > > thank you,
> > > > > 
> > > > > fabio
> > > > 
> > > > Hello Fabio,
> > > > 
> > > > I'm not sure about it: rtw_read32 calls a pointer to a function. I'm 
> don't
> > > > know drivers programming, however that function looks like an 
> implementation
> > > > of a read() system call. So I wouldn't be so sure to remove those calls.
> > > > 
> > > > Could calling a (*read) method have side effects on subsequent read()? I 
> mean:
> > > > could it update some internal data structure? If not I can remove the
> > > > variables you mentioned above and the calls to read32.
> > > > 
> > > > I'm looking forward to read your reply.
> > > > 
> > > > Thanks,
> > > > 
> > > > Fabio
> > > 
> > > hi Fabio,
> > > 
> > > rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in core/
> rtw_io.c):
> > > 
> > > u32 _rtw_read32(struct adapter *adapter, u32 addr)
> > > {
> > > 
> > >         u32 r_val;
> > >         /* struct       io_queue        *pio_queue = (struct io_queue
> > >         *)adapter->pio_queue; */
> > >         struct io_priv *pio_priv = &adapter->iopriv;
> > >         struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
> > >         u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> > >         
> > >         _read32 = pintfhdl->io_ops._read32;
> > >         
> > >         r_val = _read32(pintfhdl, addr);
> > >         return rtw_le32_to_cpu(r_val);
> > > 
> > > }
> > > 
> > > the actual read seems to be performed by the handler contained in
> > > 
> > > 	pintfhdl->io_ops._read32;
> > > 
> > > so:
> > > 
> > > $ grep -r '\b_read32' drivers/staging/rtl8723bs/
> > > 
> > > drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 = 
> &sdio_read32;
> > > 
> > > this is the place where _read32 is stored with sdio_read32 reference...
> > > 
> > > drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct 
> intf_hdl *pintfhdl, u32
> > > addr); drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 = pintfhdl-
> >io_ops._read32;
> > > ...
> > > 
> > > if you check it in hal/sdio_ops.c, nothing is written, just reads are
> > > performed (and it's not odd, for a read function isn't supposed to write
> > > something under the hood ;)).
> > 
> > Yes, but many types of hardware _REQUIRE_ reads to do something.  So
> > "read that does nothing" is a requirement for some operations.
> > 
> > As an example, a write is only guaranteed to have been finished if you
> > do a read of the same location back from it on some hardware busses.
> > The bus can reorder things, but a write/read of the same location can
> > not be reordered.
> > 
> > Sometimes you have to do reads multiple times to get things to "stick".
> > 
> > Other times reading from a location changes a state in the hardware
> > (horrid but HW designers aren't the brightest at times...)
> > 
> > So you can NOT just remove reads without knowing that the hardware does
> > not require this.  This is an issue where GCC "warnings" mean nothing as
> > gcc does not actually know what hardware does, or does not, do for many
> > things.
> > 
> > thanks,
> > 
> > greg k-h
> >
> Just to be sure... does it mean that v1 can be applied as-is?

I will review it when I get the time...

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio Aiuto]

On Thu, Apr 29, 2021 at 12:01:45PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 29, 2021 at 10:25:53AM +0200, Fabio Aiuto wrote:
> > On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> > > On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > > > Hi Fabio,
> > > > 
> > > > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco wrote:
> > > > > Removed four set but unused variables. Issue detected by gcc.
> > > > > 
> > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > > > >  1 file changed, 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index 
> > > 082448557b53..96cb4426a0f4
> > > > > 100644
> > > > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter *padapter, 
> > > enum
> > > > > hal_def_variable variable, v> 
> > > > >  			u32 cmd;
> > > > >  			u32 ra_info1, ra_info2;
> > > > >  			u32 rate_mask1, rate_mask2;
> > > > > 
> > > > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate, 
> > > lowest_rate;
> > > > > 
> > > > >  			cmd = 0x40000100 | mac_id;
> > > > >  			rtw_write32(padapter, 
> > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > >  			msleep(10);
> > > > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > > > 
> > > > > -			curr_tx_rate = ra_info1&0x7F;
> > > > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > > > 
> > > > >  			cmd = 0x40000400 | mac_id;
> > > > >  			rtw_write32(padapter, 
> > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > > 
> > > > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter *padapter, enum
> > > > > hal_def_variable variable, v> 
> > > > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > > > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > > > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > > > 
> > > > > -			hight_rate = ra_info2&0xFF;
> > > > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > > > 
> > > > >  		}
> > > > >  		break;
> > > > 
> > > > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > > > 
> > > > thank you,
> > > > 
> > > > fabio
> > > >
> > > Hello Fabio,
> > > 
> > > I'm not sure about it: rtw_read32 calls a pointer to a function. I'm don't 
> > > know drivers programming, however that function looks like an implementation 
> > > of a read() system call. So I wouldn't be so sure to remove those calls. 
> > > 
> > > Could calling a (*read) method have side effects on subsequent read()? I mean: 
> > > could it update some internal data structure? If not I can remove the 
> > > variables you mentioned above and the calls to read32.
> > > 
> > > I'm looking forward to read your reply.
> > > 
> > > Thanks,
> > > 
> > > Fabio
> > > 
> > > 
> > > 
> > 
> > hi Fabio,
> > 
> > rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in core/rtw_io.c):
> > 
> > u32 _rtw_read32(struct adapter *adapter, u32 addr)
> > {
> >         u32 r_val;
> >         /* struct       io_queue        *pio_queue = (struct io_queue *)adapter->pio_queue; */
> >         struct io_priv *pio_priv = &adapter->iopriv;
> >         struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
> >         u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> > 
> >         _read32 = pintfhdl->io_ops._read32;
> > 
> >         r_val = _read32(pintfhdl, addr);
> >         return rtw_le32_to_cpu(r_val);
> > 
> > }
> > 
> > the actual read seems to be performed by the handler contained in
> > 
> > 	pintfhdl->io_ops._read32;
> > 
> > so:
> > 
> > $ grep -r '\b_read32' drivers/staging/rtl8723bs/
> > 
> > drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 = &sdio_read32;
> > 
> > this is the place where _read32 is stored with sdio_read32 reference... 
> > 
> > drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> > drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 = pintfhdl->io_ops._read32;
> > ...
> > 
> > if you check it in hal/sdio_ops.c, nothing is written, just reads are
> > performed (and it's not odd, for a read function isn't supposed to write
> > something under the hood ;)).
> 
> Yes, but many types of hardware _REQUIRE_ reads to do something.  So
> "read that does nothing" is a requirement for some operations.
> 
> As an example, a write is only guaranteed to have been finished if you
> do a read of the same location back from it on some hardware busses.
> The bus can reorder things, but a write/read of the same location can
> not be reordered.
> 
> Sometimes you have to do reads multiple times to get things to "stick".
> 
> Other times reading from a location changes a state in the hardware
> (horrid but HW designers aren't the brightest at times...)
> 
> So you can NOT just remove reads without knowing that the hardware does
> not require this.  This is an issue where GCC "warnings" mean nothing as
> gcc does not actually know what hardware does, or does not, do for many
> things.
> 
> thanks,
> 
> greg k-h

thank you for explanation, my hardware knowledge is poor:(
Sorry for noise.

fabio

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio M. De Francesco]

On Thursday, April 29, 2021 12:22:02 PM CEST Fabio Aiuto wrote:
> On Thu, Apr 29, 2021 at 12:01:45PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Apr 29, 2021 at 10:25:53AM +0200, Fabio Aiuto wrote:
> > > On Thu, Apr 29, 2021 at 09:44:47AM +0200, Fabio M. De Francesco wrote:
> > > > On Thursday, April 29, 2021 9:26:20 AM CEST Fabio Aiuto wrote:
> > > > > Hi Fabio,
> > > > > 
> > > > > On Wed, Apr 28, 2021 at 01:33:45PM +0200, Fabio M. De Francesco 
wrote:
> > > > > > Removed four set but unused variables. Issue detected by gcc.
> > > > > > 
> > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 -----
> > > > > >  1 file changed, 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index
> > > > 
> > > > 082448557b53..96cb4426a0f4
> > > > 
> > > > > > 100644
> > > > > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> > > > > > @@ -3900,14 +3900,11 @@ u8 GetHalDefVar8723B(struct adapter 
*padapter,
> > > > 
> > > > enum
> > > > 
> > > > > > hal_def_variable variable, v>
> > > > > > 
> > > > > >  			u32 cmd;
> > > > > >  			u32 ra_info1, ra_info2;
> > > > > >  			u32 rate_mask1, rate_mask2;
> > > > > > 
> > > > > > -			u8 curr_tx_rate, curr_tx_sgi, hight_rate,
> > > > 
> > > > lowest_rate;
> > > > 
> > > > > >  			cmd = 0x40000100 | mac_id;
> > > > > >  			rtw_write32(padapter,
> > > > 
> > > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > 
> > > > > >  			msleep(10);
> > > > > >  			ra_info1 = rtw_read32(padapter, 0x2F0);
> > > > > > 
> > > > > > -			curr_tx_rate = ra_info1&0x7F;
> > > > > > -			curr_tx_sgi = (ra_info1>>7)&0x01;
> > > > > > 
> > > > > >  			cmd = 0x40000400 | mac_id;
> > > > > >  			rtw_write32(padapter,
> > > > 
> > > > REG_HMEBOX_DBG_2_8723B, cmd);
> > > > 
> > > > > > @@ -3916,8 +3913,6 @@ u8 GetHalDefVar8723B(struct adapter 
*padapter, enum
> > > > > > hal_def_variable variable, v>
> > > > > > 
> > > > > >  			ra_info2 = rtw_read32(padapter, 0x2F4);
> > > > > >  			rate_mask1 = rtw_read32(padapter, 0x2F8);
> > > > > >  			rate_mask2 = rtw_read32(padapter, 0x2FC);
> > > > > > 
> > > > > > -			hight_rate = ra_info2&0xFF;
> > > > > > -			lowest_rate = (ra_info2>>8)  & 0xFF;
> > > > > > 
> > > > > >  		}
> > > > > >  		break;
> > > > > 
> > > > > rate_mask{1,2} and ra_info{1,2} seems to be unused as well.
> > > > > 
> > > > > thank you,
> > > > > 
> > > > > fabio
> > > > 
> > > > Hello Fabio,
> > > > 
> > > > I'm not sure about it: rtw_read32 calls a pointer to a function. I'm 
don't
> > > > know drivers programming, however that function looks like an 
implementation
> > > > of a read() system call. So I wouldn't be so sure to remove those 
calls.
> > > > 
> > > > Could calling a (*read) method have side effects on subsequent read()? 
I mean:
> > > > could it update some internal data structure? If not I can remove the
> > > > variables you mentioned above and the calls to read32.
> > > > 
> > > > I'm looking forward to read your reply.
> > > > 
> > > > Thanks,
> > > > 
> > > > Fabio
> > > 
> > > hi Fabio,
> > > 
> > > rtw_read32 is a macro wrapping _rtw_read32() defined as follows (in 
core/rtw_io.c):
> > > 
> > > u32 _rtw_read32(struct adapter *adapter, u32 addr)
> > > {
> > > 
> > >         u32 r_val;
> > >         /* struct       io_queue        *pio_queue = (struct io_queue
> > >         *)adapter->pio_queue; */
> > >         struct io_priv *pio_priv = &adapter->iopriv;
> > >         struct  intf_hdl                *pintfhdl = &(pio_priv->intf);
> > >         u32 (*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> > >         
> > >         _read32 = pintfhdl->io_ops._read32;
> > >         
> > >         r_val = _read32(pintfhdl, addr);
> > >         return rtw_le32_to_cpu(r_val);
> > > 
> > > }
> > > 
> > > the actual read seems to be performed by the handler contained in
> > > 
> > > 	pintfhdl->io_ops._read32;
> > > 
> > > so:
> > > 
> > > $ grep -r '\b_read32' drivers/staging/rtl8723bs/
> > > 
> > > drivers/staging/rtl8723bs/hal/sdio_ops.c:	ops->_read32 = 
&sdio_read32;
> > > 
> > > this is the place where _read32 is stored with sdio_read32 reference...
> > > 
> > > drivers/staging/rtl8723bs/core/rtw_io.c:	u32 (*_read32)(struct 
intf_hdl *pintfhdl,
> > > u32 addr); drivers/staging/rtl8723bs/core/rtw_io.c:	_read32 =
> > > pintfhdl->io_ops._read32;
> > > ...
> > > 
> > > if you check it in hal/sdio_ops.c, nothing is written, just reads are
> > > performed (and it's not odd, for a read function isn't supposed to write
> > > something under the hood ;)).
> > 
> > Yes, but many types of hardware _REQUIRE_ reads to do something.  So
> > "read that does nothing" is a requirement for some operations.
> > 
> > As an example, a write is only guaranteed to have been finished if you
> > do a read of the same location back from it on some hardware busses.
> > The bus can reorder things, but a write/read of the same location can
> > not be reordered.
> > 
> > Sometimes you have to do reads multiple times to get things to "stick".
> > 
> > Other times reading from a location changes a state in the hardware
> > (horrid but HW designers aren't the brightest at times...)
> > 
> > So you can NOT just remove reads without knowing that the hardware does
> > not require this.  This is an issue where GCC "warnings" mean nothing as
> > gcc does not actually know what hardware does, or does not, do for many
> > things.
> > 
> > thanks,
> > 
> > greg k-h
> 
> thank you for explanation, my hardware knowledge is poor:(
> Sorry for noise.
> 
> fabio
>
I suspected that removing those variables could have been a source of troubles 
(but I was thinking of possible side effects on internal kernel's data, not of 
hardware related idiosyncrasies), however I think that you did well to point 
it out because:

1) We learned something new from Greg;
2) I learned that, for the purpose of finding definitions, vim's ctrl-] is not 
the right way to work out the problem.

If you have time, I'd appreciate some comments on the topic of line (2).

Thanks,

Fabio

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables

[Fabio Aiuto]

> > > Yes, but many types of hardware _REQUIRE_ reads to do something.  So
> > > "read that does nothing" is a requirement for some operations.
> > > 
> > > As an example, a write is only guaranteed to have been finished if you
> > > do a read of the same location back from it on some hardware busses.
> > > The bus can reorder things, but a write/read of the same location can
> > > not be reordered.
> > > 
> > > Sometimes you have to do reads multiple times to get things to "stick".
> > > 
> > > Other times reading from a location changes a state in the hardware
> > > (horrid but HW designers aren't the brightest at times...)
> > > 
> > > So you can NOT just remove reads without knowing that the hardware does
> > > not require this.  This is an issue where GCC "warnings" mean nothing as
> > > gcc does not actually know what hardware does, or does not, do for many
> > > things.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > thank you for explanation, my hardware knowledge is poor:(
> > Sorry for noise.
> > 
> > fabio
> >
> I suspected that removing those variables could have been a source of troubles 
> (but I was thinking of possible side effects on internal kernel's data, not of 
> hardware related idiosyncrasies), however I think that you did well to point 
> it out because:
> 
> 1) We learned something new from Greg;

yes that's been very good for me

> 2) I learned that, for the purpose of finding definitions, vim's ctrl-] is not 
> the right way to work out the problem.

3) I learned that with ctrl-] in vim one could (in some misterious conditions)
   see a function definition :-D

It seems that you know more than me about vim, I make intensive use of grep
for finding function defs and usages in code.

> 
> If you have time, I'd appreciate some comments on the topic of line (2).
> 
> Thanks,
> 
> Fabio
> 
> 
> 

thank you,

fabio

[Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: core: Removed set but unused variable.

[Fabio M. De Francesco]

Removed set but unused variable. Issue detected by gcc.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index a2fdd5ea5fc4..b0d2ba5ed129 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1019,7 +1019,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
 	u16 capab_info;
 	struct rtw_ieee802_11_elems elems;
 	struct sta_info *pstat;
-	unsigned char 	reassoc, *p, *pos, *wpa_ie;
+	unsigned char 	*p, *pos, *wpa_ie;
 	unsigned char WMM_IE[] = {0x00, 0x50, 0xf2, 0x02, 0x00, 0x01};
 	int		i, ie_len, wpa_ie_len, left;
 	unsigned char 	supportRate[16];
@@ -1040,10 +1040,8 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
 
 	frame_type = GetFrameSubType(pframe);
 	if (frame_type == WIFI_ASSOCREQ) {
-		reassoc = 0;
 		ie_offset = _ASOCREQ_IE_OFFSET_;
 	} else { /*  WIFI_REASSOCREQ */
-		reassoc = 1;
 		ie_offset = _REASOCREQ_IE_OFFSET_;
 	}
 
-- 
2.31.1

Re: [Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: core: Removed set but unused variable.

[Dan Carpenter]

On Wed, Apr 28, 2021 at 01:33:46PM +0200, Fabio M. De Francesco wrote:
> Removed set but unused variable. Issue detected by gcc.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index a2fdd5ea5fc4..b0d2ba5ed129 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -1019,7 +1019,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
>  	u16 capab_info;
>  	struct rtw_ieee802_11_elems elems;
>  	struct sta_info *pstat;
> -	unsigned char 	reassoc, *p, *pos, *wpa_ie;
> +	unsigned char 	*p, *pos, *wpa_ie;
>  	unsigned char WMM_IE[] = {0x00, 0x50, 0xf2, 0x02, 0x00, 0x01};
>  	int		i, ie_len, wpa_ie_len, left;
>  	unsigned char 	supportRate[16];
> @@ -1040,10 +1040,8 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
>  
>  	frame_type = GetFrameSubType(pframe);
>  	if (frame_type == WIFI_ASSOCREQ) {
> -		reassoc = 0;
>  		ie_offset = _ASOCREQ_IE_OFFSET_;
>  	} else { /*  WIFI_REASSOCREQ */
> -		reassoc = 1;
>  		ie_offset = _REASOCREQ_IE_OFFSET_;
>  	}

Get rid of the curly braces as well, otherwise the patch introduces a
checkpatch warning.

regards,
dan carpenter