[PATCH] staging: rtl8723au: Fix sparse warnings

[PATCH] staging: rtl8723au: Fix sparse warnings

[Krzysztof Konopko]

Some struct fields in wifi.h are meant to be __le16 bu were declared as
unsigned short.  This was reported by sparse:

  rtw_wlan_util.c:538:24: warning: cast to restricted __le16
  rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
  rtw_wlan_util.c:1546:25: warning: cast to restricted __le16

This patch changes declared types of the struct fields involved.

Signed-off-by: Krzysztof Konopko <kris@konagma.com>
---
 drivers/staging/rtl8723au/include/wifi.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
index fd3da3b..8a2adc5 100644
--- a/drivers/staging/rtl8723au/include/wifi.h
+++ b/drivers/staging/rtl8723au/include/wifi.h
@@ -28,7 +28,7 @@
 struct AC_param {
 	unsigned char		ACI_AIFSN;
 	unsigned char		CW;
-	unsigned short	TXOP_limit;
+	__le16			TXOP_limit;
 }  __packed;
 
 struct WMM_para_element {
@@ -39,9 +39,9 @@ struct WMM_para_element {
 
 struct ADDBA_request {
 	unsigned char		dialog_token;
-	unsigned short	BA_para_set;
+	__le16		BA_para_set;
 	unsigned short	BA_timeout_value;
-	unsigned short	BA_starting_seqctrl;
+	__le16		BA_starting_seqctrl;
 }  __packed;
 
 
-- 
2.1.3

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Larry Finger]

On 12/11/2014 04:23 PM, Krzysztof Konopko wrote:
> Some struct fields in wifi.h are meant to be __le16 bu were declared as
> unsigned short.  This was reported by sparse:
>
>    rtw_wlan_util.c:538:24: warning: cast to restricted __le16
>    rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
>    rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>
> This patch changes declared types of the struct fields involved.
>
> Signed-off-by: Krzysztof Konopko <kris@konagma.com>
> ---
>   drivers/staging/rtl8723au/include/wifi.h | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
> index fd3da3b..8a2adc5 100644
> --- a/drivers/staging/rtl8723au/include/wifi.h
> +++ b/drivers/staging/rtl8723au/include/wifi.h
> @@ -28,7 +28,7 @@
>   struct AC_param {
>   	unsigned char		ACI_AIFSN;
>   	unsigned char		CW;
> -	unsigned short	TXOP_limit;
> +	__le16			TXOP_limit;
>   }  __packed;
>
>   struct WMM_para_element {
> @@ -39,9 +39,9 @@ struct WMM_para_element {
>
>   struct ADDBA_request {
>   	unsigned char		dialog_token;
> -	unsigned short	BA_para_set;
> +	__le16		BA_para_set;
>   	unsigned short	BA_timeout_value;
> -	unsigned short	BA_starting_seqctrl;
> +	__le16		BA_starting_seqctrl;
>   }  __packed;

This fix may make the sparse warnings go away, but I think it introduces new 
bugs. In particular, did you test on big-endian hardware after you made this 
change? I recently found that the driver for RTL8188EU needed to have 
BA_para_set to unsigned short, and the endianess warnings needed to be fixed in 
the code. Then it would work on my PowerBook G4 with a PPC processor.

In RTL8188EU, both BA_starting_seqctrl and TXOP_limit are unsigned short.

Larry

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Krzysztof Konopko]

On 12/12/14 00:53, Larry Finger wrote:
> On 12/11/2014 04:23 PM, Krzysztof Konopko wrote:
>> Some struct fields in wifi.h are meant to be __le16 bu were declared as
>> unsigned short.  This was reported by sparse:
>>
>>    rtw_wlan_util.c:538:24: warning: cast to restricted __le16
>>    rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
>>    rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>>
>> This patch changes declared types of the struct fields involved.
>>
>> Signed-off-by: Krzysztof Konopko <kris@konagma.com>
>> ---
>>   drivers/staging/rtl8723au/include/wifi.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8723au/include/wifi.h
>> b/drivers/staging/rtl8723au/include/wifi.h
>> index fd3da3b..8a2adc5 100644
>> --- a/drivers/staging/rtl8723au/include/wifi.h
>> +++ b/drivers/staging/rtl8723au/include/wifi.h
>> @@ -28,7 +28,7 @@
>>   struct AC_param {
>>       unsigned char        ACI_AIFSN;
>>       unsigned char        CW;
>> -    unsigned short    TXOP_limit;
>> +    __le16            TXOP_limit;
>>   }  __packed;
>>
>>   struct WMM_para_element {
>> @@ -39,9 +39,9 @@ struct WMM_para_element {
>>
>>   struct ADDBA_request {
>>       unsigned char        dialog_token;
>> -    unsigned short    BA_para_set;
>> +    __le16        BA_para_set;
>>       unsigned short    BA_timeout_value;
>> -    unsigned short    BA_starting_seqctrl;
>> +    __le16        BA_starting_seqctrl;
>>   }  __packed;
> 
> This fix may make the sparse warnings go away, but I think it introduces
> new bugs.

Right, I see.  Nice try though, isn't it? ;)

> In particular, did you test on big-endian hardware after you
> made this change?

Nope.  I don't have any big-endian hardware.  I don't even have the
wireless card TBH.  But I'm happy to try to get one.  Is Rtl8723AE the
right model?

> I recently found that the driver for RTL8188EU needed
> to have BA_para_set to unsigned short, and the endianess warnings needed
> to be fixed in the code. Then it would work on my PowerBook G4 with a
> PPC processor.
> 

OK.  Does it still work with little endian?

> In RTL8188EU, both BA_starting_seqctrl and TXOP_limit are unsigned short.
> 

That's not quite the case.  `TXOP_limit` is __le16 in RTL8188EU [1].
It's __le16 even in your GitHub repo [2].  And that made me thinking
that there's probably some inconsistency in the header.

I'm _far_ from being a wireless expert but doesn't data coming out of
the wire/air have the endianess defined explicitly?  And both `AC_param`
and `ADDBA_request` come out of air?

I was hunting particularly for inconsistencies with `sparse` and came
across this one.  But I dug a bit further and I wonder why the driver is
not using standard stuff like the one in `include/linux/ieee80211.h`
where any data wider than one byte is clearly declared as __le<nn>?

Cheers,
Kris

--

[1] drivers/staging/rtl8188eu/include/wifi.h as of next-20141211
[2] https://github.com/lwfinger/rtl8188eu/blob/master/include/wifi.h

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Johannes Berg]

On Fri, 2014-12-12 at 12:35 +0100, Krzysztof Konopko wrote:

> I'm _far_ from being a wireless expert but doesn't data coming out of
> the wire/air have the endianess defined explicitly?  And both `AC_param`
> and `ADDBA_request` come out of air?

In general, data in 802.11 frames is little endian. Both of these would
appear to correspond to data received/sent over the air, so __le would
make sense.

johannes

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Jes Sorensen]

Krzysztof Konopko <kris@konagma.com> writes:
> On 12/12/14 00:53, Larry Finger wrote:
>> In RTL8188EU, both BA_starting_seqctrl and TXOP_limit are unsigned short.
>> 
>
> That's not quite the case.  `TXOP_limit` is __le16 in RTL8188EU [1].
> It's __le16 even in your GitHub repo [2].  And that made me thinking
> that there's probably some inconsistency in the header.
>
> I'm _far_ from being a wireless expert but doesn't data coming out of
> the wire/air have the endianess defined explicitly?  And both `AC_param`
> and `ADDBA_request` come out of air?
>
> I was hunting particularly for inconsistencies with `sparse` and came
> across this one.  But I dug a bit further and I wonder why the driver is
> not using standard stuff like the one in `include/linux/ieee80211.h`
> where any data wider than one byte is clearly declared as __le<nn>?

In general all over the wire data is little-endian. The driver has been
slowly moved towards using the standard defines from the Linux headers,
but this is a *lot* of work, and it requires testing. I really don't get
warm fuzzy feelings from patches that blindly make these kinds of
changes without also testing them.

Jes

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Larry Finger]

On 12/12/2014 05:35 AM, Krzysztof Konopko wrote:
> On 12/12/14 00:53, Larry Finger wrote:
>>
>> This fix may make the sparse warnings go away, but I think it introduces
>> new bugs.
>
> Right, I see.  Nice try though, isn't it? ;)
>
>> In particular, did you test on big-endian hardware after you
>> made this change?
>
> Nope.  I don't have any big-endian hardware.  I don't even have the
> wireless card TBH.  But I'm happy to try to get one.  Is Rtl8723AE the
> right model?

No. The device numbers that end in E are PCIe and use a mac80211-based driver. 
As none of my BE hardware has PCIe, I cannot test those drivers on other than LE 
hardware. I do not have the hardware either for the RTL8723AU. For that reason, 
I am careful when modifying the driver - I let Jes do that.

>> I recently found that the driver for RTL8188EU needed
>> to have BA_para_set to unsigned short, and the endianess warnings needed
>> to be fixed in the code. Then it would work on my PowerBook G4 with a
>> PPC processor.
>>
>
> OK.  Does it still work with little endian?

Of course. Making changes between u16 and __le16 do not make any difference in 
the code for little endian. As Dan Carpenter pointed out, these changes cannot 
introduce any bugs. I should have said that the changes could obscure bugs for 
big-endian systems.

>> In RTL8188EU, both BA_starting_seqctrl and TXOP_limit are unsigned short.
>>
>
> That's not quite the case.  `TXOP_limit` is __le16 in RTL8188EU [1].
> It's __le16 even in your GitHub repo [2].  And that made me thinking
> that there's probably some inconsistency in the header.

All the USB drivers are a mess. The kernel version of rtl8188eu does not work on 
PPC; however, the git repo now does. I'm working on finding the differences and 
fixing the kernel version.

> I'm _far_ from being a wireless expert but doesn't data coming out of
> the wire/air have the endianess defined explicitly?  And both `AC_param`
> and `ADDBA_request` come out of air?

Yes, in the air the endianess is explicitly defined. I'm not sure how AC_param 
is populated, but the data in ADDBA_request are individually extracted/inserted 
from/to the on-air packets. They could be kept as little endian, but the 
calculations on BA_para_set are being done in CPU endianess. For consistency 
with the kernel headers you mention below, the structures should probably use LE 
and the calculations modified.

> I was hunting particularly for inconsistencies with `sparse` and came
> across this one.  But I dug a bit further and I wonder why the driver is
> not using standard stuff like the one in `include/linux/ieee80211.h`
> where any data wider than one byte is clearly declared as __le<nn>?

That is a good question. One possibility is that those definitions do not exist 
on some of the older kernels that Realtek supports. They generally work with 
2.6.18 and newer.

To be able to fix the kernel driver for RTL8188EU on PPC, I need to sort out 
these endian problems. Once I do, I will port them to the other drivers.

Larry

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Jes Sorensen]

Larry Finger <Larry.Finger@lwfinger.net> writes:
> On 12/12/2014 05:35 AM, Krzysztof Konopko wrote:
>> I was hunting particularly for inconsistencies with `sparse` and came
>> across this one.  But I dug a bit further and I wonder why the driver is
>> not using standard stuff like the one in `include/linux/ieee80211.h`
>> where any data wider than one byte is clearly declared as __le<nn>?
>
> That is a good question. One possibility is that those definitions do
> not exist on some of the older kernels that Realtek supports. They
> generally work with 2.6.18 and newer.

The reason the 8723au driver doesn't use the defines from there is that
in ieee80211.h they are part of struct ieee80211_mgmt, while the 8723au
driver access the addba etc. elements without the full struct in place.

Jes

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Krzysztof Konopko]

On 12/12/14 19:52, Jes Sorensen wrote:
> Larry Finger <Larry.Finger@lwfinger.net> writes:
>> On 12/12/2014 05:35 AM, Krzysztof Konopko wrote:
>>> I was hunting particularly for inconsistencies with `sparse` and came
>>> across this one.  But I dug a bit further and I wonder why the driver is
>>> not using standard stuff like the one in `include/linux/ieee80211.h`
>>> where any data wider than one byte is clearly declared as __le<nn>?
>>
>> That is a good question. One possibility is that those definitions do
>> not exist on some of the older kernels that Realtek supports. They
>> generally work with 2.6.18 and newer.
> 
> The reason the 8723au driver doesn't use the defines from there is that
> in ieee80211.h they are part of struct ieee80211_mgmt, while the 8723au
> driver access the addba etc. elements without the full struct in place.
> 

And why is that the case?
(I'm trying to understand, not debunk)

Looks to me that this driver has been kept out of the tree for quite a
while (by Realtek) and now suffers from locally invented stuff.  I
understand this is a lot of work to unify the codebase with ieee80211.h,
but are there any technical hurdles?  I'm just curious.

Thanks,
Kris

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Jes Sorensen]

Krzysztof Konopko <kris@konagma.com> writes:
> On 12/12/14 19:52, Jes Sorensen wrote:
>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>> On 12/12/2014 05:35 AM, Krzysztof Konopko wrote:
>>>> I was hunting particularly for inconsistencies with `sparse` and came
>>>> across this one.  But I dug a bit further and I wonder why the driver is
>>>> not using standard stuff like the one in `include/linux/ieee80211.h`
>>>> where any data wider than one byte is clearly declared as __le<nn>?
>>>
>>> That is a good question. One possibility is that those definitions do
>>> not exist on some of the older kernels that Realtek supports. They
>>> generally work with 2.6.18 and newer.
>> 
>> The reason the 8723au driver doesn't use the defines from there is that
>> in ieee80211.h they are part of struct ieee80211_mgmt, while the 8723au
>> driver access the addba etc. elements without the full struct in place.
>> 
>
> And why is that the case?
> (I'm trying to understand, not debunk)
>
> Looks to me that this driver has been kept out of the tree for quite a
> while (by Realtek) and now suffers from locally invented stuff.  I
> understand this is a lot of work to unify the codebase with ieee80211.h,
> but are there any technical hurdles?  I'm just curious.

The main issue is that the RTL driver maintains a partial copy of the
management frame, ie. without the front block containing the MAC
addresses. Switching this over to carry a full copy of the frame is
extremely intrusive as it's mixed in pretty much everywhere in the
driver.

The driver is derived from Realtek's multi-OS vendor driver, which
included code for pretty much every OS on the planet. If you want to see
how it looked initially, check out Larry's github tree and go back to
the initial checkins .... Realtek seem to copy it into a new tree and
devel from there whenever they do a new chip, so the legacy in this
codebase is huge.

Cheers,
Jes

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Krzysztof Konopko]

On 12/12/14 18:35, Larry Finger wrote:
> On 12/12/2014 05:35 AM, Krzysztof Konopko wrote:
>> On 12/12/14 00:53, Larry Finger wrote:
>>> In particular, did you test on big-endian hardware after you
>>> made this change?
>>
>> Nope.  I don't have any big-endian hardware.  I don't even have the
>> wireless card TBH.  But I'm happy to try to get one.  Is Rtl8723AE the
>> right model?
> 
> No. The device numbers that end in E are PCIe and use a mac80211-based
> driver. As none of my BE hardware has PCIe, I cannot test those drivers
> on other than LE hardware. I do not have the hardware either for the
> RTL8723AU. For that reason, I am careful when modifying the driver - I
> let Jes do that.
> 

Silly me.  'U' stands for USB here.  But can't find this device on any
auction.  It's included in some ultrabooks but can't afford that for the
sake of fixing some sparse warnings :)

>>> In RTL8188EU, both BA_starting_seqctrl and TXOP_limit are unsigned
>>> short.
>>>
>>
>> That's not quite the case.  `TXOP_limit` is __le16 in RTL8188EU [1].
>> It's __le16 even in your GitHub repo [2].  And that made me thinking
>> that there's probably some inconsistency in the header.
> 
> All the USB drivers are a mess. The kernel version of rtl8188eu does not
> work on PPC; however, the git repo now does. I'm working on finding the
> differences and fixing the kernel version.
> 

Right.  I found your introductory message:
https://lkml.org/lkml/2013/4/1/280

>> I was hunting particularly for inconsistencies with `sparse` and came
>> across this one.  But I dug a bit further and I wonder why the driver is
>> not using standard stuff like the one in `include/linux/ieee80211.h`
>> where any data wider than one byte is clearly declared as __le<nn>?
> 
> That is a good question. One possibility is that those definitions do
> not exist on some of the older kernels that Realtek supports. They
> generally work with 2.6.18 and newer.
>

That would be important if the driver was kept out of the tree.  Isn't
it the point of having the driver in the mainline to keep up with the
kernel and don't bother with older versions?

> To be able to fix the kernel driver for RTL8188EU on PPC, I need to sort
> out these endian problems. Once I do, I will port them to the other
> drivers.
> 

Isn't `sparse` useful here? :)

Kris

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Larry Finger]

On 12/12/2014 04:50 PM, Krzysztof Konopko wrote:
> On 12/12/14 18:35, Larry Finger wrote:
>> On 12/12/2014 05:35 AM, Krzysztof Konopko wrote:
>>> On 12/12/14 00:53, Larry Finger wrote:
>>>> In particular, did you test on big-endian hardware after you
>>>> made this change?
>>>
>>> Nope.  I don't have any big-endian hardware.  I don't even have the
>>> wireless card TBH.  But I'm happy to try to get one.  Is Rtl8723AE the
>>> right model?
>>
>> No. The device numbers that end in E are PCIe and use a mac80211-based
>> driver. As none of my BE hardware has PCIe, I cannot test those drivers
>> on other than LE hardware. I do not have the hardware either for the
>> RTL8723AU. For that reason, I am careful when modifying the driver - I
>> let Jes do that.
>>
>
> Silly me.  'U' stands for USB here.  But can't find this device on any
> auction.  It's included in some ultrabooks but can't afford that for the
> sake of fixing some sparse warnings :)

There are no stand-alone USB devices that I have found for either RTL8723AU or 
RTL8723BU. The closest are modules CM-8723U and CM-8723BU by CC&C 
(http://www.ccandc.com.tw/) with RTL8723AU and RTL8723BU, respectively. The 
former is obsolete and no longer on the web site. These modules have D+ and D- 
connectors for USB, but they take 3.3 V, not 5. As a result, one would need some 
sort of voltage regulator circuit. That would not be complicated as it would 
consist of a TI LM2937-3.3 and a couple of capacitors. I wrote to them to see if 
I could get samples, but no response yet.

>> To be able to fix the kernel driver for RTL8188EU on PPC, I need to sort
>> out these endian problems. Once I do, I will port them to the other
>> drivers.
>>
>
> Isn't `sparse` useful here? :)

Yes, but the git repo works, and the kernel version does not, even though both 
do not have any Sparse warhings.

Larry

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Dan Carpenter]

On Thu, Dec 11, 2014 at 05:53:30PM -0600, Larry Finger wrote:
> On 12/11/2014 04:23 PM, Krzysztof Konopko wrote:
> >Some struct fields in wifi.h are meant to be __le16 bu were declared as
> >unsigned short.  This was reported by sparse:
> >
> >   rtw_wlan_util.c:538:24: warning: cast to restricted __le16
> >   rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
> >   rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
> >
> >This patch changes declared types of the struct fields involved.
> >
> >Signed-off-by: Krzysztof Konopko <kris@konagma.com>
> >---
> >  drivers/staging/rtl8723au/include/wifi.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
> >index fd3da3b..8a2adc5 100644
> >--- a/drivers/staging/rtl8723au/include/wifi.h
> >+++ b/drivers/staging/rtl8723au/include/wifi.h
> >@@ -28,7 +28,7 @@
> >  struct AC_param {
> >  	unsigned char		ACI_AIFSN;
> >  	unsigned char		CW;
> >-	unsigned short	TXOP_limit;
> >+	__le16			TXOP_limit;
> >  }  __packed;
> >
> >  struct WMM_para_element {
> >@@ -39,9 +39,9 @@ struct WMM_para_element {
> >
> >  struct ADDBA_request {
> >  	unsigned char		dialog_token;
> >-	unsigned short	BA_para_set;
> >+	__le16		BA_para_set;
> >  	unsigned short	BA_timeout_value;
> >-	unsigned short	BA_starting_seqctrl;
> >+	__le16		BA_starting_seqctrl;
> >  }  __packed;
> 
> This fix may make the sparse warnings go away, but I think it
> introduces new bugs.

This kind of change, doesn't change the compiled code only how Sparse
sees it.  It can't introduce bugs.

But it may well be that the calls to le16_to_cpu() should be removed.  I
looked at it a bit but I don't know.

regards,
dan carpenter

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Larry Finger]

On 12/12/2014 06:52 AM, Dan Carpenter wrote:
> On Thu, Dec 11, 2014 at 05:53:30PM -0600, Larry Finger wrote:
>> On 12/11/2014 04:23 PM, Krzysztof Konopko wrote:
>>> Some struct fields in wifi.h are meant to be __le16 bu were declared as
>>> unsigned short.  This was reported by sparse:
>>>
>>>    rtw_wlan_util.c:538:24: warning: cast to restricted __le16
>>>    rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
>>>    rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>>>
>>> This patch changes declared types of the struct fields involved.
>>>
>>> Signed-off-by: Krzysztof Konopko <kris@konagma.com>
>>> ---
>>>   drivers/staging/rtl8723au/include/wifi.h | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
>>> index fd3da3b..8a2adc5 100644
>>> --- a/drivers/staging/rtl8723au/include/wifi.h
>>> +++ b/drivers/staging/rtl8723au/include/wifi.h
>>> @@ -28,7 +28,7 @@
>>>   struct AC_param {
>>>   	unsigned char		ACI_AIFSN;
>>>   	unsigned char		CW;
>>> -	unsigned short	TXOP_limit;
>>> +	__le16			TXOP_limit;
>>>   }  __packed;
>>>
>>>   struct WMM_para_element {
>>> @@ -39,9 +39,9 @@ struct WMM_para_element {
>>>
>>>   struct ADDBA_request {
>>>   	unsigned char		dialog_token;
>>> -	unsigned short	BA_para_set;
>>> +	__le16		BA_para_set;
>>>   	unsigned short	BA_timeout_value;
>>> -	unsigned short	BA_starting_seqctrl;
>>> +	__le16		BA_starting_seqctrl;
>>>   }  __packed;
>>
>> This fix may make the sparse warnings go away, but I think it
>> introduces new bugs.
>
> This kind of change, doesn't change the compiled code only how Sparse
> sees it.  It can't introduce bugs.
>
> But it may well be that the calls to le16_to_cpu() should be removed.  I
> looked at it a bit but I don't know.

Your point regarding bugs is taken. What I should have said is that blindly 
making _le changes to hide Sparse messages may hide existing bugs for BE hardware.

Larry

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Krzysztof Konopko]

On 12/12/14 17:43, Larry Finger wrote:
> On 12/12/2014 06:52 AM, Dan Carpenter wrote:
>> On Thu, Dec 11, 2014 at 05:53:30PM -0600, Larry Finger wrote:
>>> On 12/11/2014 04:23 PM, Krzysztof Konopko wrote:
>>>> Some struct fields in wifi.h are meant to be __le16 bu were declared as
>>>> unsigned short.  This was reported by sparse:
>>>>
>>>>    rtw_wlan_util.c:538:24: warning: cast to restricted __le16
>>>>    rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
>>>>    rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>>>>
>>>> This patch changes declared types of the struct fields involved.
>>>>
>>>> Signed-off-by: Krzysztof Konopko <kris@konagma.com>
>>>> ---
>>>>   drivers/staging/rtl8723au/include/wifi.h | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/rtl8723au/include/wifi.h
>>>> b/drivers/staging/rtl8723au/include/wifi.h
>>>> index fd3da3b..8a2adc5 100644
>>>> --- a/drivers/staging/rtl8723au/include/wifi.h
>>>> +++ b/drivers/staging/rtl8723au/include/wifi.h
>>>> @@ -28,7 +28,7 @@
>>>>   struct AC_param {
>>>>       unsigned char        ACI_AIFSN;
>>>>       unsigned char        CW;
>>>> -    unsigned short    TXOP_limit;
>>>> +    __le16            TXOP_limit;
>>>>   }  __packed;
>>>>
>>>>   struct WMM_para_element {
>>>> @@ -39,9 +39,9 @@ struct WMM_para_element {
>>>>
>>>>   struct ADDBA_request {
>>>>       unsigned char        dialog_token;
>>>> -    unsigned short    BA_para_set;
>>>> +    __le16        BA_para_set;
>>>>       unsigned short    BA_timeout_value;
>>>> -    unsigned short    BA_starting_seqctrl;
>>>> +    __le16        BA_starting_seqctrl;
>>>>   }  __packed;
>>>
>>> This fix may make the sparse warnings go away, but I think it
>>> introduces new bugs.
>>
>> This kind of change, doesn't change the compiled code only how Sparse
>> sees it.  It can't introduce bugs.
>>
>> But it may well be that the calls to le16_to_cpu() should be removed.  I
>> looked at it a bit but I don't know.
> 
> Your point regarding bugs is taken. What I should have said is that
> blindly making _le changes to hide Sparse messages may hide existing
> bugs for BE hardware.
> 
> Larry
> 
> 

Yes, I started it off blindly but dug further and now have a better
understanding.  Looking in ieee80211.h and getting your feedback helped
me to get a better understanding of the situation.

I see nothing wrong in declaring data that is supposed to be
little-endian as __le.  You say that making these changes blindly may
hide existing bugs but:

* not doing anything about it is not helpful either

* this is no longer changing anything blindly
Relevant structs: `addba_req` and `ieee80211_wmm_ac_param` do declare
their fields as __le where needed.

I do take a point though about making this change inconsistently
(blindly) in my initial patch.

Kris

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Jes Sorensen]

Krzysztof Konopko <kris@konagma.com> writes:
> Some struct fields in wifi.h are meant to be __le16 bu were declared as
> unsigned short.  This was reported by sparse:
>
>   rtw_wlan_util.c:538:24: warning: cast to restricted __le16
>   rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
>   rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>
> This patch changes declared types of the struct fields involved.
>
> Signed-off-by: Krzysztof Konopko <kris@konagma.com>
> ---
>  drivers/staging/rtl8723au/include/wifi.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
> index fd3da3b..8a2adc5 100644
> --- a/drivers/staging/rtl8723au/include/wifi.h
> +++ b/drivers/staging/rtl8723au/include/wifi.h
> @@ -28,7 +28,7 @@
>  struct AC_param {
>  	unsigned char		ACI_AIFSN;
>  	unsigned char		CW;
> -	unsigned short	TXOP_limit;
> +	__le16			TXOP_limit;
>  }  __packed;
>  
>  struct WMM_para_element {
> @@ -39,9 +39,9 @@ struct WMM_para_element {
>  
>  struct ADDBA_request {
>  	unsigned char		dialog_token;
> -	unsigned short	BA_para_set;
> +	__le16		BA_para_set;
>  	unsigned short	BA_timeout_value;
> -	unsigned short	BA_starting_seqctrl;
> +	__le16		BA_starting_seqctrl;
>  }  __packed;

If you are going to make the struct comply with the on-wire data format,
be consistent. Don't just change half the elements of the struct - that
will just lead to confusion.

Jes

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Krzysztof Konopko]

On 12/12/14 18:12, Jes Sorensen wrote:
> Krzysztof Konopko <kris@konagma.com> writes:
>> Some struct fields in wifi.h are meant to be __le16 bu were declared as
>> unsigned short.  This was reported by sparse:
>>
>>   rtw_wlan_util.c:538:24: warning: cast to restricted __le16
>>   rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
>>   rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>>
>> This patch changes declared types of the struct fields involved.
>>
>> Signed-off-by: Krzysztof Konopko <kris@konagma.com>
>> ---
>>  drivers/staging/rtl8723au/include/wifi.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
>> index fd3da3b..8a2adc5 100644
>> --- a/drivers/staging/rtl8723au/include/wifi.h
>> +++ b/drivers/staging/rtl8723au/include/wifi.h
>> @@ -28,7 +28,7 @@
>>  struct AC_param {
>>  	unsigned char		ACI_AIFSN;
>>  	unsigned char		CW;
>> -	unsigned short	TXOP_limit;
>> +	__le16			TXOP_limit;
>>  }  __packed;
>>  
>>  struct WMM_para_element {
>> @@ -39,9 +39,9 @@ struct WMM_para_element {
>>  
>>  struct ADDBA_request {
>>  	unsigned char		dialog_token;
>> -	unsigned short	BA_para_set;
>> +	__le16		BA_para_set;
>>  	unsigned short	BA_timeout_value;
>> -	unsigned short	BA_starting_seqctrl;
>> +	__le16		BA_starting_seqctrl;
>>  }  __packed;
> 
> If you are going to make the struct comply with the on-wire data format,
> be consistent. Don't just change half the elements of the struct - that
> will just lead to confusion.
> 
> Jes
> 

Yes, my change was inconsistent.  Looking at `addba_req` and
`ieee80211_wmm_ac_param` in include/linux/ieee80211.h, all data wider
than 1 byte should be declared as __le.  I'll send through a patch that
makes this change consistently.

Thanks,
Kris

[PATCH] staging: rtl8723au: Fix sparse warnings

[Krzysztof Konopko]

Some struct fields in wifi.h are meant to be __le16 but were declared as
unsigned short.  This was reported by sparse:

  rtw_wlan_util.c:538:24: warning: cast to restricted __le16
  rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
  rtw_wlan_util.c:1546:25: warning: cast to restricted __le16

This patch changes the types of the struct fields involved to be
little-endian which is what is received over the air and consistent with
relevant structs in include/linux/ieee80211.h.

Signed-off-by: Krzysztof Konopko <kris@konagma.com>
---
 drivers/staging/rtl8723au/include/wifi.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
index fd3da3b..266c43e 100644
--- a/drivers/staging/rtl8723au/include/wifi.h
+++ b/drivers/staging/rtl8723au/include/wifi.h
@@ -28,7 +28,7 @@
 struct AC_param {
 	unsigned char		ACI_AIFSN;
 	unsigned char		CW;
-	unsigned short	TXOP_limit;
+	__le16			TXOP_limit;
 }  __packed;
 
 struct WMM_para_element {
@@ -39,9 +39,9 @@ struct WMM_para_element {
 
 struct ADDBA_request {
 	unsigned char		dialog_token;
-	unsigned short	BA_para_set;
-	unsigned short	BA_timeout_value;
-	unsigned short	BA_starting_seqctrl;
+	__le16		BA_para_set;
+	__le16		BA_timeout_value;
+	__le16		BA_starting_seqctrl;
 }  __packed;
 
 
-- 
2.1.3

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Joe Perches]

On Fri, 2014-12-12 at 23:58 +0100, Krzysztof Konopko wrote:
> This patch changes the types of the struct fields involved to be
> little-endian which is what is received over the air and consistent with
> relevant structs in include/linux/ieee80211.h.
[]
> diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
[]
> @@ -28,7 +28,7 @@
>  struct AC_param {
>  	unsigned char		ACI_AIFSN;
>  	unsigned char		CW;
> -	unsigned short	TXOP_limit;
> +	__le16			TXOP_limit;
>  }  __packed;
>  
>  struct WMM_para_element {
> @@ -39,9 +39,9 @@ struct WMM_para_element {
>  
>  struct ADDBA_request {
>  	unsigned char		dialog_token;
> -	unsigned short	BA_para_set;
> -	unsigned short	BA_timeout_value;
> -	unsigned short	BA_starting_seqctrl;
> +	__le16		BA_para_set;
> +	__le16		BA_timeout_value;
> +	__le16		BA_starting_seqctrl;
>  }  __packed;

If I did this, I would also change the
unsigned char uses to u8 at the same time.

[PATCH] staging: rtl8723au: Fix sparse warnings

[Krzysztof Konopko]

Some struct fields in wifi.h are meant to be __le16 but were declared as
unsigned short.  This was reported by sparse:

  rtw_wlan_util.c:538:24: warning: cast to restricted __le16
  rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
  rtw_wlan_util.c:1546:25: warning: cast to restricted __le16

This patch updates the types of the fields in `AC_param` and
`ADDBA_request` structs to be consistent with relevant structs in
include/linux/ieee80211.h.

Signed-off-by: Krzysztof Konopko <kris@konagma.com>
---
 drivers/staging/rtl8723au/include/wifi.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
index fd3da3b..25d573c 100644
--- a/drivers/staging/rtl8723au/include/wifi.h
+++ b/drivers/staging/rtl8723au/include/wifi.h
@@ -26,9 +26,9 @@
 ------------------------------------------------------------------------------*/
 
 struct AC_param {
-	unsigned char		ACI_AIFSN;
-	unsigned char		CW;
-	unsigned short	TXOP_limit;
+	u8			ACI_AIFSN;
+	u8			CW;
+	__le16			TXOP_limit;
 }  __packed;
 
 struct WMM_para_element {
@@ -38,10 +38,10 @@ struct WMM_para_element {
 }  __packed;
 
 struct ADDBA_request {
-	unsigned char		dialog_token;
-	unsigned short	BA_para_set;
-	unsigned short	BA_timeout_value;
-	unsigned short	BA_starting_seqctrl;
+	u8		dialog_token;
+	__le16		BA_para_set;
+	__le16		BA_timeout_value;
+	__le16		BA_starting_seqctrl;
 }  __packed;
 
 
-- 
2.1.3

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Jes Sorensen]

Krzysztof Konopko <kris@konagma.com> writes:
> Some struct fields in wifi.h are meant to be __le16 but were declared as
> unsigned short.  This was reported by sparse:
>
>   rtw_wlan_util.c:538:24: warning: cast to restricted __le16
>   rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
>   rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>
> This patch updates the types of the fields in `AC_param` and
> `ADDBA_request` structs to be consistent with relevant structs in
> include/linux/ieee80211.h.
>
> Signed-off-by: Krzysztof Konopko <kris@konagma.com>
> ---
>  drivers/staging/rtl8723au/include/wifi.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Well the u8 change is really in the nit picking space, but I am fine
with that too.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

>
> diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
> index fd3da3b..25d573c 100644
> --- a/drivers/staging/rtl8723au/include/wifi.h
> +++ b/drivers/staging/rtl8723au/include/wifi.h
> @@ -26,9 +26,9 @@
>  ------------------------------------------------------------------------------*/
>  
>  struct AC_param {
> -	unsigned char		ACI_AIFSN;
> -	unsigned char		CW;
> -	unsigned short	TXOP_limit;
> +	u8			ACI_AIFSN;
> +	u8			CW;
> +	__le16			TXOP_limit;
>  }  __packed;
>  
>  struct WMM_para_element {
> @@ -38,10 +38,10 @@ struct WMM_para_element {
>  }  __packed;
>  
>  struct ADDBA_request {
> -	unsigned char		dialog_token;
> -	unsigned short	BA_para_set;
> -	unsigned short	BA_timeout_value;
> -	unsigned short	BA_starting_seqctrl;
> +	u8		dialog_token;
> +	__le16		BA_para_set;
> +	__le16		BA_timeout_value;
> +	__le16		BA_starting_seqctrl;
>  }  __packed;

Re: [PATCH] staging: rtl8723au: Fix sparse warnings

[Jes Sorensen]

Krzysztof Konopko <kris@konagma.com> writes:
> Some struct fields in wifi.h are meant to be __le16 but were declared as
> unsigned short.  This was reported by sparse:
>
>   rtw_wlan_util.c:538:24: warning: cast to restricted __le16
>   rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
>   rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>
> This patch changes the types of the struct fields involved to be
> little-endian which is what is received over the air and consistent with
> relevant structs in include/linux/ieee80211.h.
>
> Signed-off-by: Krzysztof Konopko <kris@konagma.com>
> ---
>  drivers/staging/rtl8723au/include/wifi.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Looks OK

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>


>
> diff --git a/drivers/staging/rtl8723au/include/wifi.h b/drivers/staging/rtl8723au/include/wifi.h
> index fd3da3b..266c43e 100644
> --- a/drivers/staging/rtl8723au/include/wifi.h
> +++ b/drivers/staging/rtl8723au/include/wifi.h
> @@ -28,7 +28,7 @@
>  struct AC_param {
>  	unsigned char		ACI_AIFSN;
>  	unsigned char		CW;
> -	unsigned short	TXOP_limit;
> +	__le16			TXOP_limit;
>  }  __packed;
>  
>  struct WMM_para_element {
> @@ -39,9 +39,9 @@ struct WMM_para_element {
>  
>  struct ADDBA_request {
>  	unsigned char		dialog_token;
> -	unsigned short	BA_para_set;
> -	unsigned short	BA_timeout_value;
> -	unsigned short	BA_starting_seqctrl;
> +	__le16		BA_para_set;
> +	__le16		BA_timeout_value;
> +	__le16		BA_starting_seqctrl;
>  }  __packed;