[PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads

[PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads

[Dmitry Antipov]

Drop redundant reads from RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A
registers in _rtl88e_phy_path_a_rx_iqk().

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitriy Antipov <Dmitriy.Antipov@softline.com>
---
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
index 12d0b3a87af7..380a813acda8 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
@@ -1475,8 +1475,6 @@ static u8 _rtl88e_phy_path_a_rx_iqk(struct ieee80211_hw *hw, bool config_pathb)
 	mdelay(IQK_DELAY_TIME);
 
 	reg_eac = rtl_get_bbreg(hw, RRX_POWER_AFTER_IQK_A_2, MASKDWORD);
-	reg_e94 = rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
-	reg_e9c = rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);
 	reg_ea4 = rtl_get_bbreg(hw, RRX_POWER_BEFORE_IQK_A_2, MASKDWORD);
 
 	if (!(reg_eac & BIT(27)) &&
-- 
2.40.1

Re: [PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads

[Larry Finger]

On 5/30/23 10:54, Dmitry Antipov wrote:
> Drop redundant reads from RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A
> registers in _rtl88e_phy_path_a_rx_iqk().
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitriy Antipov <Dmitriy.Antipov@softline.com>
> ---
>   drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
> index 12d0b3a87af7..380a813acda8 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
> @@ -1475,8 +1475,6 @@ static u8 _rtl88e_phy_path_a_rx_iqk(struct ieee80211_hw *hw, bool config_pathb)
>   	mdelay(IQK_DELAY_TIME);
>   
>   	reg_eac = rtl_get_bbreg(hw, RRX_POWER_AFTER_IQK_A_2, MASKDWORD);
> -	reg_e94 = rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
> -	reg_e9c = rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);
>   	reg_ea4 = rtl_get_bbreg(hw, RRX_POWER_BEFORE_IQK_A_2, MASKDWORD);
>   
>   	if (!(reg_eac & BIT(27)) &&

I do not know the answer to this question either, but how does your tool know 
that the statements between the first read and the second have not caused the 
firmware to change the contents of the BB registers?

NACK for this patch

Larry

Re: [PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads

[Dmitry Antipov]

On 5/30/23 20:42, Larry Finger wrote:

> I do not know the answer to this question either, but how does
> your tool know that the statements between the first read and
> the second have not caused the firmware to change the contents > of the BB registers?

This tool is a static analysis software and has no special knowledge
about any particular hardware. So I do not strongly insist on this
change which should be treated as a subject to more detailed consideration.

I've noticed 6c75eab0417b9e5b05a18dbfc373e27a8ef876d8 where rtl_get_bbreg()
is preserved but the result is ignored. Would the similar thing be a kind
of a cleanup for this case as well?

Thanks,
Dmitry

Re: [PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads

[Larry Finger]

On 5/30/23 13:26, Dmitry Antipov wrote:
> On 5/30/23 20:42, Larry Finger wrote:
> 
>> I do not know the answer to this question either, but how does
>> your tool know that the statements between the first read and
>> the second have not caused the firmware to change the contents > of the BB 
>> registers?
> 
> This tool is a static analysis software and has no special knowledge
> about any particular hardware. So I do not strongly insist on this
> change which should be treated as a subject to more detailed consideration.
> 
> I've noticed 6c75eab0417b9e5b05a18dbfc373e27a8ef876d8 where rtl_get_bbreg()
> is preserved but the result is ignored. Would the similar thing be a kind
> of a cleanup for this case as well?
> 

Yes, you could ignore the output from rtl_get_bbreg() for lines 1484 and 1485.

Larry

RE: [PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads

[Ping-Ke Shih]

> -----Original Message-----
> From: Larry Finger <larry.finger@gmail.com> On Behalf Of Larry Finger
> Sent: Wednesday, May 31, 2023 2:55 AM
> To: Dmitry Antipov <dmantipov@yandex.ru>; Ping-Ke Shih <pkshih@realtek.com>
> Cc: Kalle Valo <kvalo@kernel.org>; linux-wireless@vger.kernel.org; lvc-project@linuxtesting.org; Dmitriy
> Antipov <Dmitriy.Antipov@softline.com>
> Subject: Re: [PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads
> 
> On 5/30/23 13:26, Dmitry Antipov wrote:
> > On 5/30/23 20:42, Larry Finger wrote:
> >
> >> I do not know the answer to this question either, but how does
> >> your tool know that the statements between the first read and
> >> the second have not caused the firmware to change the contents > of the BB
> >> registers?
> >
> > This tool is a static analysis software and has no special knowledge
> > about any particular hardware. So I do not strongly insist on this
> > change which should be treated as a subject to more detailed consideration.
> >
> > I've noticed 6c75eab0417b9e5b05a18dbfc373e27a8ef876d8 where rtl_get_bbreg()
> > is preserved but the result is ignored. Would the similar thing be a kind
> > of a cleanup for this case as well?
> >
> 
> Yes, you could ignore the output from rtl_get_bbreg() for lines 1484 and 1485.
> 

Another way is to add a debug message to show them, and then static checker
shouldn't warn this. Also, people can diagnose IQK & LOK results if needed. 

--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
@@ -1479,6 +1479,10 @@ static u8 _rtl88e_phy_path_a_rx_iqk(struct ieee80211_hw *hw, bool config_pathb)
        reg_e9c = rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);
        reg_ea4 = rtl_get_bbreg(hw, RRX_POWER_BEFORE_IQK_A_2, MASKDWORD);

+       rtl_dbg(rtlpriv, COMP_IQK, DBG_LOUD,
+               "Path A Rx LOK & IQK results 0xeac=0x%x 0xe94=0x%x 0xe9c=0x%x 0xea4=0x%x\n",
+               reg_eac, reg_e94, reg_e9c, reg_ea4);
+
        if (!(reg_eac & BIT(27)) &&
            (((reg_ea4 & 0x03FF0000) >> 16) != 0x132) &&
            (((reg_eac & 0x03FF0000) >> 16) != 0x36))

Ping-Ke

Re: [PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads

[Dmitry Antipov]

On 5/31/23 03:25, Ping-Ke Shih wrote:

> Another way is to add a debug message to show them, and then static checker
> shouldn't warn this. Also, people can diagnose IQK & LOK results if needed.

1. When CONFIG_RTLWIFI_DEBUG is not set, rtl_dbg() becomes a no-op inline
function which doesn't use any of its arguments at all. This may (an will)
cause the tool to produce even more warnings.

2. I don't like an idea to add some code just to make some tool happy.

3. In general, is it always (or just sometimes) required to read (some
subset of?) BB registers even if we don't interested in their values?
Is it explicitly required by the hardware design?

Thanks,
Dmitry

RE: [PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads

[Ping-Ke Shih]

> -----Original Message-----
> From: Dmitry Antipov <dmantipov@yandex.ru>
> Sent: Wednesday, May 31, 2023 1:51 PM
> To: Ping-Ke Shih <pkshih@realtek.com>; Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Kalle Valo <kvalo@kernel.org>; linux-wireless@vger.kernel.org; lvc-project@linuxtesting.org; Dmitriy
> Antipov <Dmitriy.Antipov@softline.com>
> Subject: Re: [PATCH] rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads
> 
> 3. In general, is it always (or just sometimes) required to read (some
> subset of?) BB registers even if we don't interested in their values?
> Is it explicitly required by the hardware design?
> 

I think it isn't always required basically. However, for this case, I can't find
the author to know if we can remove the statements. There is a delay to
make sure these read operations can get correct values, so I suggest to have
similar changes like 6c75eab0417b9e5b05a18dbfc373e27a8ef876d8 you mentioned if
we really want this patch.

Ping-Ke

[PATCH] rtlwifi: rtl8188ee: mark RTX_POWER_{BEFORE,AFTER}_IQK_A reads as unused

[Dmitry Antipov]

According to Ping-Ke Shih, it may be unsafe to remove BB register reads
even if we don't interested in their values. OTOH such a reads may confuse
compilers (when the most strictness warning options are enabled) and/or
static analysis tools. So it may be helpful to convert related calls of
'rtl_get_bbreg()' to 'void' and mark such a cases with special comment
just to make them easier to find and maybe even fix in the future.

This is generally inspired by 6c75eab0417b9e5b05a18dbfc373e27a8ef876d8.

Suggested-by: Ping-Ke Shih <pkshih@realtek.com>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
index 12d0b3a87af7..856c626cc99b 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
@@ -1475,8 +1475,8 @@ static u8 _rtl88e_phy_path_a_rx_iqk(struct ieee80211_hw *hw, bool config_pathb)
 	mdelay(IQK_DELAY_TIME);
 
 	reg_eac = rtl_get_bbreg(hw, RRX_POWER_AFTER_IQK_A_2, MASKDWORD);
-	reg_e94 = rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
-	reg_e9c = rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);
+	/* unused */ (void)rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
+	/* unused */ (void)rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);
 	reg_ea4 = rtl_get_bbreg(hw, RRX_POWER_BEFORE_IQK_A_2, MASKDWORD);
 
 	if (!(reg_eac & BIT(27)) &&
-- 
2.40.1

Re: [PATCH] rtlwifi: rtl8188ee: mark RTX_POWER_{BEFORE,AFTER}_IQK_A reads as unused

[Ping-Ke Shih]

On Thu, 2023-06-01 at 13:52 +0300, Dmitry Antipov wrote:
> 
> According to Ping-Ke Shih, it may be unsafe to remove BB register reads
> even if we don't interested in their values. OTOH such a reads may confuse
> compilers (when the most strictness warning options are enabled) and/or
> static analysis tools. So it may be helpful to convert related calls of
> 'rtl_get_bbreg()' to 'void' and mark such a cases with special comment
> just to make them easier to find and maybe even fix in the future.
> 
> This is generally inspired by 6c75eab0417b9e5b05a18dbfc373e27a8ef876d8.

Normally, mention a commit by `commit <12 digits SHA1> ("subject")`

> 
> Suggested-by: Ping-Ke Shih <pkshih@realtek.com>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
> b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
> index 12d0b3a87af7..856c626cc99b 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c
> @@ -1475,8 +1475,8 @@ static u8 _rtl88e_phy_path_a_rx_iqk(struct ieee80211_hw *hw, bool
> config_pathb)
>         mdelay(IQK_DELAY_TIME);
> 
>         reg_eac = rtl_get_bbreg(hw, RRX_POWER_AFTER_IQK_A_2, MASKDWORD);
> -       reg_e94 = rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
> -       reg_e9c = rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);
> +       /* unused */ (void)rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
> +       /* unused */ (void)rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);

Why not just

rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);


>         reg_ea4 = rtl_get_bbreg(hw, RRX_POWER_BEFORE_IQK_A_2, MASKDWORD);
> 
>         if (!(reg_eac & BIT(27)) &&
> --
> 2.40.1
> 

Re: [PATCH] rtlwifi: rtl8188ee: mark RTX_POWER_{BEFORE,AFTER}_IQK_A reads as unused

[Dmitry Antipov]

On 6/1/23 15:30, Ping-Ke Shih wrote:

> Normally, mention a commit by `commit <12 digits SHA1> ("subject")`

OK

> Why not just
> 
> rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
> rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);

Compiler with -Wextra etc. or static analysis tool may complain about an unused
return value. As far as I know GCC has __attribute__((warn_unused_result)) but
lacks an opposite thing, so (somewhat ugly explicit) cast to 'void' may be helpful.

Dmitry

Re: [PATCH] rtlwifi: rtl8188ee: mark RTX_POWER_{BEFORE,AFTER}_IQK_A reads as unused

[Ping-Ke Shih]

On Thu, 2023-06-01 at 16:50 +0300, Dmitry Antipov wrote:
> 
> On 6/1/23 15:30, Ping-Ke Shih wrote:
> 
> > Normally, mention a commit by `commit <12 digits SHA1> ("subject")`
> 
> OK
> 
> > Why not just
> > 
> > rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
> > rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);
> 
> Compiler with -Wextra etc. or static analysis tool may complain about an unused
> return value. As far as I know GCC has __attribute__((warn_unused_result)) but
> lacks an opposite thing, so (somewhat ugly explicit) cast to 'void' may be helpful.
> 

Don't you think casting of 'void' only makes tool happy?

Ping-Ke

Re: [PATCH] rtlwifi: rtl8188ee: mark RTX_POWER_{BEFORE,AFTER}_IQK_A reads as unused

[Dmitry Antipov]

On 6/2/23 09:13, Ping-Ke Shih wrote:

> Don't you think casting of 'void' only makes tool happy?

The point is in commenting the pieces of code which looks like
leftovers and probably should be reconsidered. Explicit casts
to void should be considered as an extra annotations rather than
an attempts to fix something.

Dmitry

Re: [PATCH] rtlwifi: rtl8188ee: mark RTX_POWER_{BEFORE,AFTER}_IQK_A reads as unused

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

>> Why not just
>>
>> rtl_get_bbreg(hw, RTX_POWER_BEFORE_IQK_A, MASKDWORD);
>> rtl_get_bbreg(hw, RTX_POWER_AFTER_IQK_A, MASKDWORD);
>
> Compiler with -Wextra etc. or static analysis tool may complain about an unused
> return value. As far as I know GCC has __attribute__((warn_unused_result)) but
> lacks an opposite thing, so (somewhat ugly explicit) cast to 'void' may be helpful.

I assume you are referring to this attribute:

#define __must_check                    __attribute__((__warn_unused_result__))

But if __must_check is NOT set doesn't it mean that checking of the
function's return value is optional? At least that's how I interpret the
meaning.


-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] rtlwifi: rtl8188ee: mark RTX_POWER_{BEFORE,AFTER}_IQK_A reads as unused

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> According to Ping-Ke Shih, it may be unsafe to remove BB register reads
> even if we don't interested in their values. OTOH such a reads may confuse
> compilers (when the most strictness warning options are enabled) and/or
> static analysis tools. So it may be helpful to convert related calls of
> 'rtl_get_bbreg()' to 'void' and mark such a cases with special comment
> just to make them easier to find and maybe even fix in the future.
>
> This is generally inspired by 6c75eab0417b9e5b05a18dbfc373e27a8ef876d8.
>
> Suggested-by: Ping-Ke Shih <pkshih@realtek.com>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

"wifi:" missing from the title, see the wiki link below for more info.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches