diff mbox series

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

Message ID 20230601105215.27013-1-dmantipov@yandex.ru (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series rtlwifi: rtl8188ee: mark RTX_POWER_{BEFORE,AFTER}_IQK_A reads as unused | expand

Commit Message

Dmitry Antipov June 1, 2023, 10:52 a.m. UTC
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(-)

Comments

Ping-Ke Shih June 1, 2023, 12:30 p.m. UTC | #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 <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
>
Dmitry Antipov June 1, 2023, 1:50 p.m. UTC | #2
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
Ping-Ke Shih June 2, 2023, 6:13 a.m. UTC | #3
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
Dmitry Antipov June 5, 2023, 9:43 a.m. UTC | #4
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
Kalle Valo June 13, 2023, 8:17 a.m. UTC | #5
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.
Kalle Valo June 13, 2023, 8:18 a.m. UTC | #6
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.
diff mbox series

Patch

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