Expression '(X & 0xfc) == 0x3' is always false
I chose to remove this code, because it will not make any difference.
But obviously it is rather a properly designed if statement that is needed.
This was partly found using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist <[email protected]>
---
drivers/net/wireless/rtlwifi/rtl8192de/hw.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
index 2b08671..a1520d5 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
@@ -1128,10 +1128,7 @@ static int _rtl92de_set_media_status(struct ieee80211_hw *hw,
}
rtl_write_byte(rtlpriv, REG_CR + 2, bt_msr);
rtlpriv->cfg->ops->led_control(hw, ledaction);
- if ((bt_msr & 0xfc) == MSR_AP)
- rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x00);
- else
- rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
+ rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
return 0;
}
--
1.7.10.4
Hi!
Yes, 0x3 was one of the most likely :)
But wanted someone who knows the code better would be heard.
All agreed? Then I do a new patch.
Looks like it is the same error in the files below, I'll fix them all them to.
rtl8192cu/hw.c:1363: if ((bt_msr & 0xfc) == MSR_AP)
rtl8192ce/hw.c:1209: if ((bt_msr & 0xfc) == MSR_AP)
rtl8188ee/hw.c:1234: if ((bt_msr & 0xfc) == MSR_AP)
rtl8192de/hw.c:1131: if ((bt_msr & 0xfc) == MSR_AP)
Best regards
Rickard Strandqvist
2014-06-07 17:02 GMT+02:00 Peter Wu <[email protected]>:
> On Saturday 07 June 2014 16:30:19 Rickard Strandqvist wrote:
>> Expression '(X & 0xfc) == 0x3' is always false
>
> While this is true, I believe that some other mistake is made.
>
>> I chose to remove this code, because it will not make any difference.
>> But obviously it is rather a properly designed if statement that is needed.
>>
>> This was partly found using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist <[email protected]>
>> ---
>> drivers/net/wireless/rtlwifi/rtl8192de/hw.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>> index 2b08671..a1520d5 100644
>> --- a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>> +++ b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>> @@ -1128,10 +1128,7 @@ static int _rtl92de_set_media_status(struct ieee80211_hw *hw,
>> }
>> rtl_write_byte(rtlpriv, REG_CR + 2, bt_msr);
>> rtlpriv->cfg->ops->led_control(hw, ledaction);
>> - if ((bt_msr & 0xfc) == MSR_AP)
>
> If you look a few lines up, then you see that bt_msr is OR-ed with MSR_AP
> for AP interfaces. The 0xfc should be 0x03, see other drivers for example:
>
> rtl8723ae/hw.c:1112: if ((bt_msr & 0x03) == MSR_AP)
> rtl8723be/hw.c:1200: if ((bt_msr & 0x03) == MSR_AP)
> rtl8192cu/hw.c:1363: if ((bt_msr & 0xfc) == MSR_AP)
> rtl8192ce/hw.c:1209: if ((bt_msr & 0xfc) == MSR_AP)
> rtl8188ee/hw.c:1234: if ((bt_msr & 0xfc) == MSR_AP)
> rtl8192de/hw.c:1131: if ((bt_msr & 0xfc) == MSR_AP)
>
>> - rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x00);
>> - else
>> - rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
>> + rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
>> return 0;
>> }
>
> Kind regards,
> Peter
>
On Saturday 07 June 2014 16:30:19 Rickard Strandqvist wrote:
> Expression '(X & 0xfc) == 0x3' is always false
While this is true, I believe that some other mistake is made.
> I chose to remove this code, because it will not make any difference.
> But obviously it is rather a properly designed if statement that is needed.
>
> This was partly found using a static code analysis program called cppcheck.
>
> Signed-off-by: Rickard Strandqvist <[email protected]>
> ---
> drivers/net/wireless/rtlwifi/rtl8192de/hw.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
> index 2b08671..a1520d5 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
> @@ -1128,10 +1128,7 @@ static int _rtl92de_set_media_status(struct ieee80211_hw *hw,
> }
> rtl_write_byte(rtlpriv, REG_CR + 2, bt_msr);
> rtlpriv->cfg->ops->led_control(hw, ledaction);
> - if ((bt_msr & 0xfc) == MSR_AP)
If you look a few lines up, then you see that bt_msr is OR-ed with MSR_AP
for AP interfaces. The 0xfc should be 0x03, see other drivers for example:
rtl8723ae/hw.c:1112: if ((bt_msr & 0x03) == MSR_AP)
rtl8723be/hw.c:1200: if ((bt_msr & 0x03) == MSR_AP)
rtl8192cu/hw.c:1363: if ((bt_msr & 0xfc) == MSR_AP)
rtl8192ce/hw.c:1209: if ((bt_msr & 0xfc) == MSR_AP)
rtl8188ee/hw.c:1234: if ((bt_msr & 0xfc) == MSR_AP)
rtl8192de/hw.c:1131: if ((bt_msr & 0xfc) == MSR_AP)
> - rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x00);
> - else
> - rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
> + rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
> return 0;
> }
Kind regards,
Peter
On 06/07/2014 10:24 AM, Rickard Strandqvist wrote:
> Hi!
>
> Yes, 0x3 was one of the most likely :)
> But wanted someone who knows the code better would be heard.
> All agreed? Then I do a new patch.
>
> Looks like it is the same error in the files below, I'll fix them all them to.
>
> rtl8192cu/hw.c:1363: if ((bt_msr & 0xfc) == MSR_AP)
> rtl8192ce/hw.c:1209: if ((bt_msr & 0xfc) == MSR_AP)
> rtl8188ee/hw.c:1234: if ((bt_msr & 0xfc) == MSR_AP)
> rtl8192de/hw.c:1131: if ((bt_msr & 0xfc) == MSR_AP)
>
>
> Best regards
> Rickard Strandqvist
>
>
> 2014-06-07 17:02 GMT+02:00 Peter Wu <[email protected]>:
>> On Saturday 07 June 2014 16:30:19 Rickard Strandqvist wrote:
>>> Expression '(X & 0xfc) == 0x3' is always false
>>
>> While this is true, I believe that some other mistake is made.
>>
>>> I chose to remove this code, because it will not make any difference.
>>> But obviously it is rather a properly designed if statement that is needed.
>>>
>>> This was partly found using a static code analysis program called cppcheck.
>>>
>>> Signed-off-by: Rickard Strandqvist <[email protected]>
>>> ---
>>> drivers/net/wireless/rtlwifi/rtl8192de/hw.c | 5 +----
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>>> index 2b08671..a1520d5 100644
>>> --- a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>>> +++ b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>>> @@ -1128,10 +1128,7 @@ static int _rtl92de_set_media_status(struct ieee80211_hw *hw,
>>> }
>>> rtl_write_byte(rtlpriv, REG_CR + 2, bt_msr);
>>> rtlpriv->cfg->ops->led_control(hw, ledaction);
>>> - if ((bt_msr & 0xfc) == MSR_AP)
>>
>> If you look a few lines up, then you see that bt_msr is OR-ed with MSR_AP
>> for AP interfaces. The 0xfc should be 0x03, see other drivers for example:
>>
>> rtl8723ae/hw.c:1112: if ((bt_msr & 0x03) == MSR_AP)
>> rtl8723be/hw.c:1200: if ((bt_msr & 0x03) == MSR_AP)
>> rtl8192cu/hw.c:1363: if ((bt_msr & 0xfc) == MSR_AP)
>> rtl8192ce/hw.c:1209: if ((bt_msr & 0xfc) == MSR_AP)
>> rtl8188ee/hw.c:1234: if ((bt_msr & 0xfc) == MSR_AP)
>> rtl8192de/hw.c:1131: if ((bt_msr & 0xfc) == MSR_AP)
>>
>>> - rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x00);
>>> - else
>>> - rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
>>> + rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
>>> return 0;
>>> }
Peter,
As you have learned here, automatically making changes suggested by some tool
may convert a visible bug into one that is invisible, and only found by a
detailed line-by-line examination of the code, and that is unlikely to happen.
Please be careful.
From everything I see, the test in all drivers should be
if ((bt_msr & MSR_AP) == MSR_AP)
Larry
Hi all
Good. New patches are on the way :)
Best regards
Rickard Strandqvist
2014-06-08 2:01 GMT+02:00 Larry Finger <[email protected]>:
> On 06/07/2014 10:24 AM, Rickard Strandqvist wrote:
>>
>> Hi!
>>
>> Yes, 0x3 was one of the most likely :)
>> But wanted someone who knows the code better would be heard.
>> All agreed? Then I do a new patch.
>>
>> Looks like it is the same error in the files below, I'll fix them all them
>> to.
>>
>> rtl8192cu/hw.c:1363: if ((bt_msr & 0xfc) == MSR_AP)
>> rtl8192ce/hw.c:1209: if ((bt_msr & 0xfc) == MSR_AP)
>> rtl8188ee/hw.c:1234: if ((bt_msr & 0xfc) == MSR_AP)
>> rtl8192de/hw.c:1131: if ((bt_msr & 0xfc) == MSR_AP)
>>
>>
>> Best regards
>> Rickard Strandqvist
>>
>>
>> 2014-06-07 17:02 GMT+02:00 Peter Wu <[email protected]>:
>>>
>>> On Saturday 07 June 2014 16:30:19 Rickard Strandqvist wrote:
>>>>
>>>> Expression '(X & 0xfc) == 0x3' is always false
>>>
>>>
>>> While this is true, I believe that some other mistake is made.
>>>
>>>> I chose to remove this code, because it will not make any difference.
>>>> But obviously it is rather a properly designed if statement that is
>>>> needed.
>>>>
>>>> This was partly found using a static code analysis program called
>>>> cppcheck.
>>>>
>>>> Signed-off-by: Rickard Strandqvist
>>>> <[email protected]>
>>>> ---
>>>> drivers/net/wireless/rtlwifi/rtl8192de/hw.c | 5 +----
>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>>>> b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>>>> index 2b08671..a1520d5 100644
>>>> --- a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>>>> +++ b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>>>> @@ -1128,10 +1128,7 @@ static int _rtl92de_set_media_status(struct
>>>> ieee80211_hw *hw,
>>>> }
>>>> rtl_write_byte(rtlpriv, REG_CR + 2, bt_msr);
>>>> rtlpriv->cfg->ops->led_control(hw, ledaction);
>>>> - if ((bt_msr & 0xfc) == MSR_AP)
>>>
>>>
>>> If you look a few lines up, then you see that bt_msr is OR-ed with MSR_AP
>>> for AP interfaces. The 0xfc should be 0x03, see other drivers for
>>> example:
>>>
>>> rtl8723ae/hw.c:1112: if ((bt_msr & 0x03) == MSR_AP)
>>> rtl8723be/hw.c:1200: if ((bt_msr & 0x03) == MSR_AP)
>>> rtl8192cu/hw.c:1363: if ((bt_msr & 0xfc) == MSR_AP)
>>> rtl8192ce/hw.c:1209: if ((bt_msr & 0xfc) == MSR_AP)
>>> rtl8188ee/hw.c:1234: if ((bt_msr & 0xfc) == MSR_AP)
>>> rtl8192de/hw.c:1131: if ((bt_msr & 0xfc) == MSR_AP)
>>>
>>>> - rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x00);
>>>> - else
>>>> - rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
>>>> + rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66);
>>>> return 0;
>>>> }
>
>
> Peter,
>
> As you have learned here, automatically making changes suggested by some
> tool may convert a visible bug into one that is invisible, and only found by
> a detailed line-by-line examination of the code, and that is unlikely to
> happen. Please be careful.
>
> From everything I see, the test in all drivers should be
>
> if ((bt_msr & MSR_AP) == MSR_AP)
>
> Larry
>
On Saturday 07 June 2014 19:01:20 Larry Finger wrote:
> As you have learned here, automatically making changes suggested by some tool
> may convert a visible bug into one that is invisible, and only found by a
> detailed line-by-line examination of the code, and that is unlikely to happen.
> Please be careful.
>
> From everything I see, the test in all drivers should be
>
> if ((bt_msr & MSR_AP) == MSR_AP)
That only happens to be case because MSR_INFRA | MSR_ADHOC == MSR_AP. This
seems to be the intent:
#define MSR_MASK 0x03
if ((bt_msr & MSR_MASK) == MSR_AP)
In rtl8192se, there are also MSR_LINK_... constants covering MSR_...
and in addition, there is a MSR_LINK_MASK. These macros are quite
redundant though given the other definitions, but the mask is still
nice to have I guess.
Also, personally I would submit just one patch touching all drivers, but
I see that Rickard has submitted a bunch of patches (without cover letter
either, making it more difficult to group them). What would you prefer,
a single patch touching multiple drivers (as the changes are mostly the
same) or split patches?
Kind regards,
Peter
Hi
Damn, there I was bit too fast :-(
Then we use MSR_MASK instead, new patch then. But I will wait a day?
Or what is long enough to be sure that nobody else have any
objections? How is this usually resolved?
Sure, I can send a patch for all the files instead. However, earlier
received complaints when I sent patches extending over more than one
file.
I'll check again how the cover letter works. Although when I try with
my send patch mail script it did not work as I wanted.
Best regards
Rickard Strandqvist
2014-06-08 11:26 GMT+02:00 Peter Wu <[email protected]>:
> On Saturday 07 June 2014 19:01:20 Larry Finger wrote:
>> As you have learned here, automatically making changes suggested by some tool
>> may convert a visible bug into one that is invisible, and only found by a
>> detailed line-by-line examination of the code, and that is unlikely to happen.
>> Please be careful.
>>
>> From everything I see, the test in all drivers should be
>>
>> if ((bt_msr & MSR_AP) == MSR_AP)
>
> That only happens to be case because MSR_INFRA | MSR_ADHOC == MSR_AP. This
> seems to be the intent:
>
> #define MSR_MASK 0x03
> if ((bt_msr & MSR_MASK) == MSR_AP)
>
> In rtl8192se, there are also MSR_LINK_... constants covering MSR_...
> and in addition, there is a MSR_LINK_MASK. These macros are quite
> redundant though given the other definitions, but the mask is still
> nice to have I guess.
>
> Also, personally I would submit just one patch touching all drivers, but
> I see that Rickard has submitted a bunch of patches (without cover letter
> either, making it more difficult to group them). What would you prefer,
> a single patch touching multiple drivers (as the changes are mostly the
> same) or split patches?
>
> Kind regards,
> Peter
>
On Sunday 08 June 2014 12:36:11 Rickard Strandqvist wrote:
> Then we use MSR_MASK instead, new patch then. But I will wait a day?
> Or what is long enough to be sure that nobody else have any
> objections? How is this usually resolved?
Well, Larry is the maintainer, so he will ultimately pick up patches.
One or two days should give people some time to read and reply.
As for MSR_MASK, that macro does not exist yet, I was wondering whether
it's OK to add a new macro? (Larry?)
> Sure, I can send a patch for all the files instead. However, earlier
> received complaints when I sent patches extending over more than one
> file.
>
> I'll check again how the cover letter works. Although when I try with
> my send patch mail script it did not work as I wanted.
See the manual page git-send-email(1) and git-format-patch(1). In
particular the Example section of git-send-email(1).
Kind regards,
Peter
On 06/08/2014 05:43 AM, Peter Wu wrote:
> On Sunday 08 June 2014 12:36:11 Rickard Strandqvist wrote:
>> Then we use MSR_MASK instead, new patch then. But I will wait a day?
>> Or what is long enough to be sure that nobody else have any
>> objections? How is this usually resolved?
>
> Well, Larry is the maintainer, so he will ultimately pick up patches.
> One or two days should give people some time to read and reply.
My role as maintainer is a little different than others. As I have a private
broadband connection with only 1 Mbps upload, it is not practical for me to
operate a git server. I used to have an account at kernel.org, but I lost it
after the break-in there. As I have never met face-to-face with another Linux
developer, I have had no chance to have my credentials signed, so that resource
is unavailable. As a result, I ACK or NACK patches and they are picked up by
John Linville for drivers in the regular wireless tree, and Greg Kroah-Hartman
for the staging drivers.
> As for MSR_MASK, that macro does not exist yet, I was wondering whether
> it's OK to add a new macro? (Larry?)
Yes, that is OK.
>> Sure, I can send a patch for all the files instead. However, earlier
>> received complaints when I sent patches extending over more than one
>> file.
These do really need to be split a little. There must be at least one patch for
the wireless tree, and a second for staging. In fact, I prefer the way they are
with one for each driver.
Larry