Clang static checker warning:
Value stored to 'v1' is never read [deadcode.DeadStores]
Value stored to 'channel' is never read [deadcode.DeadStores]
Remove them to save some place.
Signed-off-by: Su Hui <[email protected]>
Acked-by: Ping-Ke Shih <[email protected]>
---
v2:
- fix the subject prefix problem
drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
index 5323ead30db0..6df270e29e66 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
@@ -2038,15 +2038,9 @@ static bool _rtl8821ae_phy_config_bb_with_pgheaderfile(struct ieee80211_hw *hw,
/*don't need the hw_body*/
if (!_rtl8821ae_check_condition(hw, v1)) {
i += 2; /* skip the pair of expression*/
- v1 = array[i];
v2 = array[i+1];
- v3 = array[i+2];
- while (v2 != 0xDEAD) {
+ while (v2 != 0xDEAD)
i += 3;
- v1 = array[i];
- v2 = array[i+1];
- v3 = array[i+2];
- }
}
}
}
@@ -3543,7 +3537,6 @@ u8 rtl8821ae_phy_sw_chnl(struct ieee80211_hw *hw)
struct rtl_phy *rtlphy = &rtlpriv->phy;
struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
u32 timeout = 1000, timecount = 0;
- u8 channel = rtlphy->current_channel;
if (rtlphy->sw_chnl_inprogress)
return 0;
@@ -3566,8 +3559,6 @@ u8 rtl8821ae_phy_sw_chnl(struct ieee80211_hw *hw)
rtl8821ae_phy_switch_wirelessband(hw, BAND_ON_2_4G);
rtlphy->sw_chnl_inprogress = true;
- if (channel == 0)
- channel = 1;
rtl_dbg(rtlpriv, COMP_SCAN, DBG_TRACE,
"switch to channel%d, band type is %d\n",
--
2.30.2
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 <[email protected]>
---
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(-)
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);*/
--
2.30.2
> -----Original Message-----
> From: Su Hui <[email protected]>
> Sent: Friday, November 24, 2023 10:04 AM
> To: [email protected]; Ping-Ke Shih <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: Su Hui <[email protected]>; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> 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);
> }