| Message ID | 20230530155446.555091-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtlwifi: rtl8188ee: drop RTX_POWER_BEFORE_IQK_A and RTX_POWER_AFTER_IQK_A reads | expand |
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
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 <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
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 <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
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)) &&
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(-)