| Message ID | 1402151419-18296-1-git-send-email-rickard_strandqvist@spectrumdigital.se (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lekensteyn.nl>: > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)