[PATCH v2 0/2] staging: rtl8723au: core: endianness issues

[PATCH v2 0/2] staging: rtl8723au: core: endianness issues

[David Decotigny]

The code shows a couple inconsistencies (described in commit
descriptions) which would not be an issue on little-endian cpus, but
could cause breakage on non-LE cpus. Note: I could not test on real
hardware, these patches created based on sparse reports.

############################################
# Patch Set Summary:

David Decotigny (2):
  staging: rtl8723au: core: avoid bitwise arithmetic with forced
    endianness
  staging: rtl8723au: core: remove redundant endianness conversion

 drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

[PATCH v2 1/2] staging: rtl8723au: core: avoid bitwise arithmetic with forced endianness

[David Decotigny]

This fixes bitwise arithmetic performed on the host on a variable
previously converted to little-endian, and subsequently converted
again to little-endian:
  - issue_action_BA23a() called with "status" crafted in host byte order
  - "status" converted to LE
  - bitwise arithmetic on the (LE) "status", performed with masks and
    shifts in host byte order
  - result converted to LE (again) and stored in device structure

Sparse warning addressed by this patch:
  drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3806:16: warning: incorrect type in assignment (different base types)
  drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3806:16:    expected unsigned short [unsigned] status
  drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3806:16:    got restricted __le16 [usertype] <noident>

Signed-off-by: David Decotigny <ddecotig@gmail.com>
---
 drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
index 196beaf..7c3b5dd 100644
--- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
@@ -3803,8 +3803,6 @@ void issue_action_BA23a(struct rtw_adapter *padapter,
 
 	pattrib->pktlen = sizeof(struct ieee80211_hdr_3addr) + 1;
 
-	status = cpu_to_le16(status);
-
 	switch (action) {
 	case WLAN_ACTION_ADDBA_REQ:
 		pattrib->pktlen += sizeof(mgmt->u.action.u.addba_req);
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v2 1/2] staging: rtl8723au: core: avoid bitwise arithmetic with forced endianness

[Dan Carpenter]

You're CC'ing all the lustre people on this by mistake.

Can we find which patch introduced this bug, and add a Fixes: tag and
CC whoever introduced it?

Please, resend with the correct CC list.

regards,
dan carpenter

Re: [PATCH v2 1/2] staging: rtl8723au: core: avoid bitwise arithmetic with forced endianness

[David Decotigny]

This was introduced by kernel bulk commit 5e93f3520 "staging: r8723au:
Add source files for new driver - part 1", initially from github
according to commit description. On github, this traces back to
another bulk commit: 2896bda04353 "Add new files in core directory",
which is the 1st version of the driver. Not sure where to find the
parent repos.

PS: sorry for the incorrect To/Cc, going to resend to more appropriate
recipients.

On Sun, Jun 7, 2015 at 4:20 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> You're CC'ing all the lustre people on this by mistake.
>
> Can we find which patch introduced this bug, and add a Fixes: tag and
> CC whoever introduced it?
>
> Please, resend with the correct CC list.
>
> regards,
> dan carpenter
>

[PATCH v2 2/2] staging: rtl8723au: core: remove redundant endianness conversion

[David Decotigny]

Source and destination have the same little-endian annotation: this
patch removes forced conversion from host byte order to little-endian.

This addresses the following sparse warning:
drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3911:56: warning: incorrect type in argument 1 (different base types)
drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3911:56:    expected unsigned short [unsigned] [usertype] val
drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3911:56:    got restricted __le16 [usertype] BA_timeout_value

Signed-off-by: David Decotigny <ddecotig@gmail.com>
---
 drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
index 7c3b5dd..142f214 100644
--- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
@@ -3906,8 +3906,8 @@ void issue_action_BA23a(struct rtw_adapter *padapter,
 		put_unaligned_le16(BA_para_set,
 				   &mgmt->u.action.u.addba_resp.capab);
 
-		put_unaligned_le16(pmlmeinfo->ADDBA_req.BA_timeout_value,
-				   &mgmt->u.action.u.addba_resp.timeout);
+		mgmt->u.action.u.addba_resp.timeout
+			= pmlmeinfo->ADDBA_req.BA_timeout_value;
 
 		pattrib->pktlen += 8;
 		break;
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v2 2/2] staging: rtl8723au: core: remove redundant endianness conversion

[Dan Carpenter]

On Sat, Jun 06, 2015 at 05:34:00PM -0700, David Decotigny wrote:
> Source and destination have the same little-endian annotation: this
> patch removes forced conversion from host byte order to little-endian.

The patch seems correct but the changelog is a bit wrong.  It will
change it from little endian to host endian but we don't want the dest
to be host endian, we want it to be little endain.

regards,
dan carpenter

[PATCH v2 0/2] staging: rtl8723au: core: endianness issues

[David Decotigny]

The code shows a couple inconsistencies (described in commit
descriptions) which would not be an issue on little-endian cpus, but
could cause breakage on non-LE cpus. Note: I could not test on real
hardware, these patches created based on sparse reports.

Hostory:
  - resending the same patches to correct recipients, only changed
    commit descriptions (credits to Dan Carpenter)

############################################
# Patch Set Summary:

David Decotigny (2):
  staging: rtl8723au: core: avoid bitwise arithmetic with forced
    endianness
  staging: rtl8723au: core: remove redundant endianness conversion

 drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

[PATCH v2 2/2] staging: rtl8723au: core: remove redundant endianness conversion

[David Decotigny]

Source and destination have the same little-endian annotation: this
patch removes incorrect byte-swap on non-LE cpus.

This addresses the following sparse warning:
drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3911:56: warning: incorrect type in argument 1 (different base types)
drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3911:56:    expected unsigned short [unsigned] [usertype] val
drivers/staging/rtl8723au/core/rtw_mlme_ext.c:3911:56:    got restricted __le16 [usertype] BA_timeout_value

Signed-off-by: David Decotigny <ddecotig@gmail.com>
---
 drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
index 7c3b5dd..142f214 100644
--- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
@@ -3906,8 +3906,8 @@ void issue_action_BA23a(struct rtw_adapter *padapter,
 		put_unaligned_le16(BA_para_set,
 				   &mgmt->u.action.u.addba_resp.capab);
 
-		put_unaligned_le16(pmlmeinfo->ADDBA_req.BA_timeout_value,
-				   &mgmt->u.action.u.addba_resp.timeout);
+		mgmt->u.action.u.addba_resp.timeout
+			= pmlmeinfo->ADDBA_req.BA_timeout_value;
 
 		pattrib->pktlen += 8;
 		break;
-- 
2.2.0.rc0.207.ga3a616c