Removed four set but unused variables. Issue detected by gcc.
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> >
> > 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
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 <[email protected]>
> > > ---
> > >
> > > 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
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 <[email protected]>
> > > > ---
> > > >
> > > > 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
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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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
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 <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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
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 <[email protected]>
> > > > ---
> > > >
> > > > 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
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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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
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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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
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 <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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
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 <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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
> > > 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