from:"Krzysztof Konopko"

[PATCH] staging: rtl8723au: Fix sparse warnings

2014-12-11 Thread 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 k...@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

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

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

2014-12-12 Thread 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 k...@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 charACI_AIFSN;
   unsigned charCW;
 -unsigned shortTXOP_limit;
 +__le16TXOP_limit;
   }  __packed;

   struct WMM_para_element {
 @@ -39,9 +39,9 @@ struct WMM_para_element {

   struct ADDBA_request {
   unsigned chardialog_token;
 -unsigned shortBA_para_set;
 +__le16BA_para_set;
   unsigned shortBA_timeout_value;
 -unsigned shortBA_starting_seqctrl;
 +__le16BA_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 __lenn?

Cheers,
Kris

--

[1] drivers/staging/rtl8188eu/include/wifi.h as of next-20141211
[2] https://github.com/lwfinger/rtl8188eu/blob/master/include/wifi.h
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-12-12 Thread 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 k...@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 charACI_AIFSN;
   unsigned charCW;
 -unsigned shortTXOP_limit;
 +__le16TXOP_limit;
   }  __packed;

   struct WMM_para_element {
 @@ -39,9 +39,9 @@ struct WMM_para_element {

   struct ADDBA_request {
   unsigned chardialog_token;
 -unsigned shortBA_para_set;
 +__le16BA_para_set;
   unsigned shortBA_timeout_value;
 -unsigned shortBA_starting_seqctrl;
 +__le16BA_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
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-12-12 Thread Krzysztof Konopko

On 12/12/14 18:12, Jes Sorensen wrote:
 Krzysztof Konopko k...@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 k...@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
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-12-12 Thread 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 __lenn?
 
 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

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

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

2014-12-12 Thread Krzysztof Konopko

On 12/12/14 19:52, Jes Sorensen wrote:
 Larry Finger larry.fin...@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 __lenn?

 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

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

[PATCH] staging: rtl8723au: Fix sparse warnings

2014-12-12 Thread 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 k...@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

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

[PATCH] staging: rtl8723au: Fix sparse warnings

2014-12-15 Thread 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 k...@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

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

[PATCH] staging: rtl8723au: Fix sparse warnings

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

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

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

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

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

[PATCH] staging: rtl8723au: Fix sparse warnings

[PATCH] staging: rtl8723au: Fix sparse warnings

8 matches

Site Navigation

Mail list logo

Footer information