| Message ID | 20231124020352.1660621-2-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2,1/2] wifi: rtlwifi: rtl8821ae: phy: remove some useless code | expand |
> -----Original Message----- > From: Su Hui <suhui@nfschina.com> > Sent: Friday, November 24, 2023 10:04 AM > To: dan.carpenter@linaro.org; Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org; nathan@kernel.org; > ndesaulniers@google.com; trix@redhat.com > Cc: Su Hui <suhui@nfschina.com>; lizetao1@huawei.com; linville@tuxdriver.com; Larry.Finger@lwfinger.net; > linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; llvm@lists.linux.dev; > kernel-janitors@vger.kernel.org > Subject: [PATCH v2 2/2] wifi: rtlwifi: rtl8821ae: phy: fix an undefined bitwise shift behavior > [...] > > diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c > b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c > index 6df270e29e66..52ab1b0761c0 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c > +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c > @@ -31,7 +31,12 @@ static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask) > { > u32 i = ffs(bitmask); > > - return i ? i - 1 : 32; > + if (!i) { > + WARN_ON_ONCE(1); > + return 0; > + } > + > + return i - 1; > } Personally, I prefer to use __ffs(), because in normal case no need additional '-1', and abnormal cases should not happen.
On 2023/11/24 16:51, Ping-Ke Shih wrote: > Subject: [PATCH v2 2/2] wifi: rtlwifi: rtl8821ae: phy: fix an undefined bitwise shift behavior > > [...] >> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >> b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >> index 6df270e29e66..52ab1b0761c0 100644 >> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >> @@ -31,7 +31,12 @@ static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask) >> { >> u32 i = ffs(bitmask); >> >> - return i ? i - 1 : 32; >> + if (!i) { >> + WARN_ON_ONCE(1); >> + return 0; >> + } >> + >> + return i - 1; >> } > Personally, I prefer to use __ffs(), because in normal case no need additional '-1', > and abnormal cases should not happen. Hi, Ping-Ke Replace _rtl8821ae_phy_calculate_bit_shift() by __ffs(bitmask) is better, but I'm not sure what callers should do when callers check bitmask is 0 before calling. Maybe this check is useless? I can send a v3 patch if using __ffs(bitmask) and no check for bitmask is fine. Or could you send this patch if you have a better idea? Thanks for your suggestion! Su Hui
On Fri, 2023-11-24 at 18:06 +0800, Su Hui wrote: > > On 2023/11/24 16:51, Ping-Ke Shih wrote: > > Subject: [PATCH v2 2/2] wifi: rtlwifi: rtl8821ae: phy: fix an undefined bitwise shift behavior > > > > [...] > > > diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c > > > b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c > > > index 6df270e29e66..52ab1b0761c0 100644 > > > --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c > > > +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c > > > @@ -31,7 +31,12 @@ static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask) > > > { > > > u32 i = ffs(bitmask); > > > > > > - return i ? i - 1 : 32; > > > + if (!i) { > > > + WARN_ON_ONCE(1); > > > + return 0; > > > + } > > > + > > > + return i - 1; > > > } > > Personally, I prefer to use __ffs(), because in normal case no need additional '-1', > > and abnormal cases should not happen. > > Hi, Ping-Ke > > Replace _rtl8821ae_phy_calculate_bit_shift() by __ffs(bitmask) is better, > but I'm not sure what callers should do when callers check bitmask is 0 before calling. > Maybe this check is useless? > > I can send a v3 patch if using __ffs(bitmask) and no check for bitmask is fine. > Or could you send this patch if you have a better idea? > Thanks for your suggestion! > Can this work to you? static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask) { if (WARN_ON_ONCE(!bitmask)) return 0; return __ffs(bitmask); }
On 2023/11/24 19:19, Ping-Ke Shih wrote: > On Fri, 2023-11-24 at 18:06 +0800, Su Hui wrote: >> On 2023/11/24 16:51, Ping-Ke Shih wrote: >>> Subject: [PATCH v2 2/2] wifi: rtlwifi: rtl8821ae: phy: fix an undefined bitwise shift behavior >>> >>> [...] >>>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >>>> b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >>>> index 6df270e29e66..52ab1b0761c0 100644 >>>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >>>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >>>> @@ -31,7 +31,12 @@ static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask) >>>> { >>>> u32 i = ffs(bitmask); >>>> >>>> - return i ? i - 1 : 32; >>>> + if (!i) { >>>> + WARN_ON_ONCE(1); >>>> + return 0; >>>> + } >>>> + >>>> + return i - 1; >>>> } >>> Personally, I prefer to use __ffs(), because in normal case no need additional '-1', >>> and abnormal cases should not happen. >> Hi, Ping-Ke >> >> Replace _rtl8821ae_phy_calculate_bit_shift() by __ffs(bitmask) is better, >> but I'm not sure what callers should do when callers check bitmask is 0 before calling. >> Maybe this check is useless? >> >> I can send a v3 patch if using __ffs(bitmask) and no check for bitmask is fine. >> Or could you send this patch if you have a better idea? >> Thanks for your suggestion! >> > Can this work to you? Looks good to me, briefer and better! I will send v3 soon. > static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask) > { > if (WARN_ON_ONCE(!bitmask)) > return 0; > > return __ffs(bitmask); > }
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c index 6df270e29e66..52ab1b0761c0 100644 --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c @@ -31,7 +31,12 @@ static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask) { u32 i = ffs(bitmask); - return i ? i - 1 : 32; + if (!i) { + WARN_ON_ONCE(1); + return 0; + } + + return i - 1; } static bool _rtl8821ae_phy_bb8821a_config_parafile(struct ieee80211_hw *hw); /*static bool _rtl8812ae_phy_config_mac_with_headerfile(struct ieee80211_hw *hw);*/
Clang staic checker warning: drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c:184:49: The result of the left shift is undefined due to shifting by '32', which is greater or equal to the width of type 'u32'. [core.UndefinedBinaryOperatorResult] If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.[1][2] For example, when using different gcc's compilation optimizaation options (-O0 or -O2), the result of '(u32)data << 32' is different. One is 0, the other is old value of data. Let _rtl8821ae_phy_calculate_bit_shift()'s return value less than 32 to fix this problem. Warn if bitmask is zero. [1]:https://stackoverflow.com/questions/11270492/what-does-the-c- standard-say-about-bitshifting-more-bits-than-the-width-of-type [2]:https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf Fixes: 21e4b0726dc6 ("rtlwifi: rtl8821ae: Move driver from staging to regular tree") Signed-off-by: Su Hui <suhui@nfschina.com> --- v2: - fix the subject prefix problem - silence the warning by not return 32 bits rather than adding a type cast.(Thanks to Dan and Ping-Ke) By the way, there some similar problems in _rtl88e_phy_calculate_bit_shift(), _rtl92c_phy_calculate_bit_shift() and so on... drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)