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