All of lore.kernel.org
 help / color / mirror / Atom feed
* [Outreachy kernel] [PATCH 0/2] Removed set but unused variables
@ 2021-04-28 11:33 Fabio M. De Francesco
  2021-04-28 11:33 ` [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove " Fabio M. De Francesco
  2021-04-28 11:33 ` [Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: core: Removed set but unused variable Fabio M. De Francesco
  0 siblings, 2 replies; 16+ messages in thread
From: Fabio M. De Francesco @ 2021-04-28 11:33 UTC (permalink / raw)
  To: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel
  Cc: 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-28 11:33 [Outreachy kernel] [PATCH 0/2] Removed set but unused variables Fabio M. De Francesco
@ 2021-04-28 11:33 ` Fabio M. De Francesco
  2021-04-29  7:26   ` Fabio Aiuto
  2021-04-28 11:33 ` [Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: core: Removed set but unused variable Fabio M. De Francesco
  1 sibling, 1 reply; 16+ messages in thread
From: Fabio M. De Francesco @ 2021-04-28 11:33 UTC (permalink / raw)
  To: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: core: Removed set but unused variable.
  2021-04-28 11:33 [Outreachy kernel] [PATCH 0/2] Removed set but unused variables Fabio M. De Francesco
  2021-04-28 11:33 ` [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove " Fabio M. De Francesco
@ 2021-04-28 11:33 ` Fabio M. De Francesco
  2021-04-29 14:35   ` Dan Carpenter
  1 sibling, 1 reply; 16+ messages in thread
From: Fabio M. De Francesco @ 2021-04-28 11:33 UTC (permalink / raw)
  To: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-28 11:33 ` [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove " Fabio M. De Francesco
@ 2021-04-29  7:26   ` Fabio Aiuto
  2021-04-29  7:44     ` Fabio M. De Francesco
  0 siblings, 1 reply; 16+ messages in thread
From: Fabio Aiuto @ 2021-04-29  7:26 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel

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 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29  7:26   ` Fabio Aiuto
@ 2021-04-29  7:44     ` Fabio M. De Francesco
  2021-04-29  8:25       ` Fabio Aiuto
  0 siblings, 1 reply; 16+ messages in thread
From: Fabio M. De Francesco @ 2021-04-29  7:44 UTC (permalink / raw)
  To: Fabio Aiuto
  Cc: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel

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




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29  7:44     ` Fabio M. De Francesco
@ 2021-04-29  8:25       ` Fabio Aiuto
  2021-04-29  9:16         ` Fabio M. De Francesco
  2021-04-29 10:01         ` Greg Kroah-Hartman
  0 siblings, 2 replies; 16+ messages in thread
From: Fabio Aiuto @ 2021-04-29  8:25 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29  8:25       ` Fabio Aiuto
@ 2021-04-29  9:16         ` Fabio M. De Francesco
  2021-04-29  9:32           ` Fabio Aiuto
  2021-04-29 10:01         ` Greg Kroah-Hartman
  1 sibling, 1 reply; 16+ messages in thread
From: Fabio M. De Francesco @ 2021-04-29  9:16 UTC (permalink / raw)
  To: Fabio Aiuto
  Cc: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel

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





^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29  9:16         ` Fabio M. De Francesco
@ 2021-04-29  9:32           ` Fabio Aiuto
  2021-04-29  9:53             ` Fabio M. De Francesco
  0 siblings, 1 reply; 16+ messages in thread
From: Fabio Aiuto @ 2021-04-29  9:32 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29  9:32           ` Fabio Aiuto
@ 2021-04-29  9:53             ` Fabio M. De Francesco
  0 siblings, 0 replies; 16+ messages in thread
From: Fabio M. De Francesco @ 2021-04-29  9:53 UTC (permalink / raw)
  To: Fabio Aiuto
  Cc: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel

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





^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29  8:25       ` Fabio Aiuto
  2021-04-29  9:16         ` Fabio M. De Francesco
@ 2021-04-29 10:01         ` Greg Kroah-Hartman
  2021-04-29 10:15           ` Fabio M. De Francesco
  2021-04-29 10:22           ` Fabio Aiuto
  1 sibling, 2 replies; 16+ messages in thread
From: Greg Kroah-Hartman @ 2021-04-29 10:01 UTC (permalink / raw)
  To: Fabio Aiuto
  Cc: Fabio M. De Francesco, outreachy-kernel, linux-staging, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29 10:01         ` Greg Kroah-Hartman
@ 2021-04-29 10:15           ` Fabio M. De Francesco
  2021-04-29 11:21             ` Greg Kroah-Hartman
  2021-04-29 10:22           ` Fabio Aiuto
  1 sibling, 1 reply; 16+ messages in thread
From: Fabio M. De Francesco @ 2021-04-29 10:15 UTC (permalink / raw)
  To: Fabio Aiuto, Greg Kroah-Hartman
  Cc: outreachy-kernel, linux-staging, linux-kernel

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





^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29 10:01         ` Greg Kroah-Hartman
  2021-04-29 10:15           ` Fabio M. De Francesco
@ 2021-04-29 10:22           ` Fabio Aiuto
  2021-04-29 10:38             ` Fabio M. De Francesco
  1 sibling, 1 reply; 16+ messages in thread
From: Fabio Aiuto @ 2021-04-29 10:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Fabio M. De Francesco, outreachy-kernel, linux-staging, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29 10:22           ` Fabio Aiuto
@ 2021-04-29 10:38             ` Fabio M. De Francesco
  2021-04-29 12:07               ` Fabio Aiuto
  0 siblings, 1 reply; 16+ messages in thread
From: Fabio M. De Francesco @ 2021-04-29 10:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Fabio Aiuto
  Cc: outreachy-kernel, linux-staging, linux-kernel

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




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29 10:15           ` Fabio M. De Francesco
@ 2021-04-29 11:21             ` Greg Kroah-Hartman
  0 siblings, 0 replies; 16+ messages in thread
From: Greg Kroah-Hartman @ 2021-04-29 11:21 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Fabio Aiuto, outreachy-kernel, linux-staging, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove set but unused variables
  2021-04-29 10:38             ` Fabio M. De Francesco
@ 2021-04-29 12:07               ` Fabio Aiuto
  0 siblings, 0 replies; 16+ messages in thread
From: Fabio Aiuto @ 2021-04-29 12:07 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Greg Kroah-Hartman, outreachy-kernel, linux-staging, linux-kernel

> > > 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: core: Removed set but unused variable.
  2021-04-28 11:33 ` [Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: core: Removed set but unused variable Fabio M. De Francesco
@ 2021-04-29 14:35   ` Dan Carpenter
  0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2021-04-29 14:35 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: outreachy-kernel, Greg Kroah-Hartman, linux-staging, linux-kernel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-04-29 14:35 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-28 11:33 [Outreachy kernel] [PATCH 0/2] Removed set but unused variables Fabio M. De Francesco
2021-04-28 11:33 ` [Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove " Fabio M. De Francesco
2021-04-29  7:26   ` Fabio Aiuto
2021-04-29  7:44     ` Fabio M. De Francesco
2021-04-29  8:25       ` Fabio Aiuto
2021-04-29  9:16         ` Fabio M. De Francesco
2021-04-29  9:32           ` Fabio Aiuto
2021-04-29  9:53             ` Fabio M. De Francesco
2021-04-29 10:01         ` Greg Kroah-Hartman
2021-04-29 10:15           ` Fabio M. De Francesco
2021-04-29 11:21             ` Greg Kroah-Hartman
2021-04-29 10:22           ` Fabio Aiuto
2021-04-29 10:38             ` Fabio M. De Francesco
2021-04-29 12:07               ` Fabio Aiuto
2021-04-28 11:33 ` [Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: core: Removed set but unused variable Fabio M. De Francesco
2021-04-29 14:35   ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.