[PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Rickard Strandqvist]

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 <rickard_strandqvist@spectrumdigital.se>
---
 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

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Peter Wu]

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 <rickard_strandqvist@spectrumdigital.se>
> ---
>  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

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Rickard Strandqvist]

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 <peter@lekensteyn.nl>:
> 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 <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  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
>

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Larry Finger]

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 <peter@lekensteyn.nl>:
>> 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 <rickard_strandqvist@spectrumdigital.se>
>>> ---
>>>   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

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Rickard Strandqvist]

Hi all

Good. New patches are on the way :)

Best regards
Rickard Strandqvist


2014-06-08 2:01 GMT+02:00 Larry Finger <Larry.Finger@lwfinger.net>:
> 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 <peter@lekensteyn.nl>:
>>>
>>> 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
>>>> <rickard_strandqvist@spectrumdigital.se>
>>>> ---
>>>>   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
>

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Peter Wu]

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

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Rickard Strandqvist]

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 <peter-VTkQYDcBqhK7DlmcbJSQ7g@public.gmane.org>:
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Peter Wu]

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

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Larry Finger]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: wireless: rtlwifi: rtl8192de: hw.c: Cleaning up conjunction always evaluates to false

[Peter Wu]

On Tuesday 10 June 2014 23:31:37 Rickard Strandqvist wrote:
> I guess it's ok to do the patches?

Right, you got some feedback from me and another person. You can
send patches whenever you like.

> But then again, I will then send them one by one, with the cover
> letter? +35 email?

Have you seen my other reply? Use git-format-patch --cover-letter.
Why would you need 35 mails? A commit should logically group a
change per driver. So, one commit probably touches rtlXXXX/reg.h
and rtlXXXX/hw.c (perhaps others? grep for it!)

Be sure to manually check each change. Your last patchset included
non-sensial commit messages and subjects.

> Then the macro I should add in all the:
> 
> net/wireless/rtlwifi/rtl****se/reg.h
> 
> 
> And in the:
> drivers/staging/rtl8188eu/include/rtl8188e_spec.h
> drivers/staging/rtl8192e/rtl8192e/r8192E_hw.h
> drivers/staging/rtl8712/rtl871x_wlan_sme.h
> drivers/staging/rtl8723au/include/rtl8723a_spec.h
> drivers/staging/rtl8821ae/rtl8821ae/reg.h

The staging drivers will be taken by Greg KH, see MAINTAINERS
for the addresses for staging drivers.

> And should I put it like:
> #define MSR_MASK 0x03
> 
> Or is this more accurate:
> #define MSR_MASK   (MSR_INFRA | MSR_ADHOC)

Since the integer resulting from the mask is a decimal sequence rather
than a bitmap, it makes more sense to use 0x03 instead. Please have a
look at the drivers and be sure to understand why the changes are needed.

Peter

PS. please do not top-post. For driver discussions, you should keep
the list cc'd too. That makes it more likely for someone to respond to
such queries.