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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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
> -----Original Message-----
> From: Larry Finger <[email protected]> On Behalf Of Larry Finger
> Sent: Wednesday, May 31, 2023 2:55 AM
> To: Dmitry Antipov <[email protected]>; Ping-Ke Shih <[email protected]>
> Cc: Kalle Valo <[email protected]>; [email protected]; [email protected]; Dmitriy
> Antipov <[email protected]>
> 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
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
> -----Original Message-----
> From: Dmitry Antipov <[email protected]>
> Sent: Wednesday, May 31, 2023 1:51 PM
> To: Ping-Ke Shih <[email protected]>; Larry Finger <[email protected]>
> Cc: Kalle Valo <[email protected]>; [email protected]; [email protected]; Dmitriy
> Antipov <[email protected]>
> 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
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 <[email protected]>
Signed-off-by: Dmitry Antipov <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> 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
>
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
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
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
Dmitry Antipov <[email protected]> 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
Dmitry Antipov <[email protected]> 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 <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>
"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