[PATCH v3 0/2] Staging: Use put_unaligned_le16

[PATCH v3 0/2] Staging: Use put_unaligned_le16

[Vaishali Thakkar]

This patch series introduces the use of function
put_unaligned_le16 as Using byte ordering functions
and then memcpy() is risky and prone to hide errors
which are hard to track down.

This change is done using Coccinelle.

Changes in v3:
	-Two separate patches are merged in a patch series 

Vaishali Thakkar (2):
  Staging: rtl8188eu: Use put_unaligned_le16
  Staging: rtl8723au: Use put_unaligned_le16

 drivers/staging/rtl8188eu/core/rtw_ap.c | 8 +++-----
 drivers/staging/rtl8723au/core/rtw_ap.c | 8 +++-----
 2 files changed, 6 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] Staging: rtl8188eu: Use put_unaligned_le16

[Vaishali Thakkar]

Using byte ordering functions and then memcpy() is risky and
prone to hide errors which are hard to track down. So, this
patch introduces the use of function put_unaligned_le16 which
makes the code clear. Here, use of variable tim_bitmap_le
and variable itself is removed. Also, to be compatible with the
changes header file is added too.

Coccinelle is used to do this change and semantic patch used for
this is as follows:

@a@
typedef __le16;
__le16 e16;
identifier tmp;
expression ptr;
expression y,e;
type T;
@@

- tmp = cpu_to_le16(y);

<+... when != tmp
(
- memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
+ put_unaligned_le16(y,ptr);
|
- memcpy(ptr, (T)&tmp, ...);
+ put_unaligned_le16(y,ptr);
)
...+>
? tmp = e

@@ type T; identifier a.tmp; @@

- T tmp;
...when != tmp

Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
---
Changes since v2:
	-Edit commit log
	-Merge this patch in a patch-series

Changes since v1:
	-Remove use of variable tim_bitmap_le
	-Remove variable tim_bitmap_le
	-Add header file to be compatible with the changes
	-Edit commit log

 drivers/staging/rtl8188eu/core/rtw_ap.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c b/drivers/staging/rtl8188eu/core/rtw_ap.c
index da19145..e65ee6e 100644
--- a/drivers/staging/rtl8188eu/core/rtw_ap.c
+++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
@@ -23,6 +23,7 @@
 #include <drv_types.h>
 #include <wifi.h>
 #include <ieee80211.h>
+#include <asm/unaligned.h>
 
 #ifdef CONFIG_88EU_AP_MODE
 
@@ -78,11 +79,8 @@ static void update_BCNTIM(struct adapter *padapter)
 	if (true) {
 		u8 *p, *dst_ie, *premainder_ie = NULL;
 		u8 *pbackup_remainder_ie = NULL;
-		__le16 tim_bitmap_le;
 		uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
 
-		tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
-
 		p = rtw_get_ie(pie + _FIXED_IE_LENGTH_, _TIM_IE_, &tim_ielen, pnetwork_mlmeext->IELength - _FIXED_IE_LENGTH_);
 		if (p != NULL && tim_ielen > 0) {
 			tim_ielen += 2;
@@ -137,9 +135,9 @@ static void update_BCNTIM(struct adapter *padapter)
 			*dst_ie++ = 0;
 
 		if (tim_ielen == 4) {
-			*dst_ie++ = *(u8 *)&tim_bitmap_le;
+			*dst_ie++ = pstapriv->tim_bitmap & 0xff;
 		} else if (tim_ielen == 5) {
-			memcpy(dst_ie, &tim_bitmap_le, 2);
+			put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
 			dst_ie += 2;
 		}
 
-- 
1.9.1

[PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16

[Vaishali Thakkar]

Using byte ordering functions and then memcpy() is risky and
prone to hide errors which are hard to track down. So, this
patch introduces the use of function put_unaligned_le16 which
makes the code clear. Here, use of variable tim_bitmap_le
and variable itself is removed. Also, to be compatible with the
changes header file is added too.

Coccinelle is used to do this change and semantic patch used for
this is as follows:

@a@
typedef __le16;
__le16 e16;
identifier tmp;
expression ptr;
expression y,e;
type T;
@@

- tmp = cpu_to_le16(y);

<+... when != tmp
(
- memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
+ put_unaligned_le16(y,ptr);
|
- memcpy(ptr, (T)&tmp, ...);
+ put_unaligned_le16(y,ptr);
)
...+>
? tmp = e

@@ type T; identifier a.tmp; @@

- T tmp;
...when != tmp

Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
---
Changes since v1:
	-Edit commit log
	-Merge this patch in a patch series

 drivers/staging/rtl8723au/core/rtw_ap.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c b/drivers/staging/rtl8723au/core/rtw_ap.c
index c6327c0..7fa4352 100644
--- a/drivers/staging/rtl8723au/core/rtw_ap.c
+++ b/drivers/staging/rtl8723au/core/rtw_ap.c
@@ -20,6 +20,7 @@
 #include <wifi.h>
 #include <rtl8723a_cmd.h>
 #include <rtl8723a_hal.h>
+#include <asm/unaligned.h>
 
 extern unsigned char WMM_OUI23A[];
 extern unsigned char WPS_OUI23A[];
@@ -72,11 +73,8 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
 	struct wlan_bssid_ex *pnetwork_mlmeext = &pmlmeinfo->network;
 	unsigned char *pie = pnetwork_mlmeext->IEs;
 	u8 *p, *dst_ie, *premainder_ie = NULL, *pbackup_remainder_ie = NULL;
-	__le16 tim_bitmap_le;
 	uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
 
-	tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
-
 	p = rtw_get_ie23a(pie, WLAN_EID_TIM, &tim_ielen,
 			  pnetwork_mlmeext->IELength);
 	if (p != NULL && tim_ielen > 0) {
@@ -143,9 +141,9 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
 		*dst_ie++ = 0;
 
 	if (tim_ielen == 4) {
-		*dst_ie++ = *(u8 *)&tim_bitmap_le;
+		*dst_ie++ = pstapriv->tim_bitmap & 0xff;
 	} else if (tim_ielen == 5) {
-		memcpy(dst_ie, &tim_bitmap_le, 2);
+		put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
 		dst_ie += 2;
 	}
 
-- 
1.9.1

Re: [Outreachy kernel] [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16

[Julia Lawall]

On Fri, 20 Feb 2015, Vaishali Thakkar wrote:

> Using byte ordering functions and then memcpy() is risky and
> prone to hide errors which are hard to track down. So, this
> patch introduces the use of function put_unaligned_le16 which
> makes the code clear. Here, use of variable tim_bitmap_le
> and variable itself is removed. Also, to be compatible with the
> changes header file is added too.
> 
> Coccinelle is used to do this change and semantic patch used for
> this is as follows:
> 
> @a@
> typedef __le16;
> __le16 e16;
> identifier tmp;
> expression ptr;
> expression y,e;
> type T;
> @@
> 
> - tmp = cpu_to_le16(y);
> 
> <+... when != tmp
> (
> - memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
> + put_unaligned_le16(y,ptr);
> |
> - memcpy(ptr, (T)&tmp, ...);
> + put_unaligned_le16(y,ptr);
> )
> ...+>
> ? tmp = e
> 
> @@ type T; identifier a.tmp; @@
> 
> - T tmp;
> ...when != tmp
> 
> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
> ---
> Changes since v1:
> 	-Edit commit log
> 	-Merge this patch in a patch series

So in this case there is no change since v2?  Maybe it would be better to 
make that explicit.

julia


>  drivers/staging/rtl8723au/core/rtw_ap.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c b/drivers/staging/rtl8723au/core/rtw_ap.c
> index c6327c0..7fa4352 100644
> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
> @@ -20,6 +20,7 @@
>  #include <wifi.h>
>  #include <rtl8723a_cmd.h>
>  #include <rtl8723a_hal.h>
> +#include <asm/unaligned.h>
>  
>  extern unsigned char WMM_OUI23A[];
>  extern unsigned char WPS_OUI23A[];
> @@ -72,11 +73,8 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>  	struct wlan_bssid_ex *pnetwork_mlmeext = &pmlmeinfo->network;
>  	unsigned char *pie = pnetwork_mlmeext->IEs;
>  	u8 *p, *dst_ie, *premainder_ie = NULL, *pbackup_remainder_ie = NULL;
> -	__le16 tim_bitmap_le;
>  	uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
>  
> -	tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
> -
>  	p = rtw_get_ie23a(pie, WLAN_EID_TIM, &tim_ielen,
>  			  pnetwork_mlmeext->IELength);
>  	if (p != NULL && tim_ielen > 0) {
> @@ -143,9 +141,9 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>  		*dst_ie++ = 0;
>  
>  	if (tim_ielen == 4) {
> -		*dst_ie++ = *(u8 *)&tim_bitmap_le;
> +		*dst_ie++ = pstapriv->tim_bitmap & 0xff;
>  	} else if (tim_ielen == 5) {
> -		memcpy(dst_ie, &tim_bitmap_le, 2);
> +		put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
>  		dst_ie += 2;
>  	}
>  
> -- 
> 1.9.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/9defe79e0f212c883f260e27a67bff9f9ba013ba.1424402536.git.vthakkar1994%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH v3 1/2] Staging: rtl8188eu: Use put_unaligned_le16

[Preeti U Murthy]

Hi Vaishali,

On 02/20/2015 09:00 AM, Vaishali Thakkar wrote:
> Using byte ordering functions and then memcpy() is risky and
> prone to hide errors which are hard to track down. So, this
> patch introduces the use of function put_unaligned_le16 which
> makes the code clear. Here, use of variable tim_bitmap_le
> and variable itself is removed. Also, to be compatible with the
> changes header file is added too.

You don't really need to explain the inclusion of header files since it
would be understood that it is to link the newly introduced functions in
the patch. But you don't need to send out a new version just for
correcting this. The patchset looks good to me.

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> Coccinelle is used to do this change and semantic patch used for
> this is as follows:
> 
> @a@
> typedef __le16;
> __le16 e16;
> identifier tmp;
> expression ptr;
> expression y,e;
> type T;
> @@
> 
> - tmp = cpu_to_le16(y);
> 
> <+... when != tmp
> (
> - memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
> + put_unaligned_le16(y,ptr);
> |
> - memcpy(ptr, (T)&tmp, ...);
> + put_unaligned_le16(y,ptr);
> )
> ...+>
> ? tmp = e
> 
> @@ type T; identifier a.tmp; @@
> 
> - T tmp;
> ...when != tmp
> 
> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
> ---
> Changes since v2:
> 	-Edit commit log
> 	-Merge this patch in a patch-series
> 
> Changes since v1:
> 	-Remove use of variable tim_bitmap_le
> 	-Remove variable tim_bitmap_le
> 	-Add header file to be compatible with the changes
> 	-Edit commit log
> 
>  drivers/staging/rtl8188eu/core/rtw_ap.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c b/drivers/staging/rtl8188eu/core/rtw_ap.c
> index da19145..e65ee6e 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_ap.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
> @@ -23,6 +23,7 @@
>  #include <drv_types.h>
>  #include <wifi.h>
>  #include <ieee80211.h>
> +#include <asm/unaligned.h>
> 
>  #ifdef CONFIG_88EU_AP_MODE
> 
> @@ -78,11 +79,8 @@ static void update_BCNTIM(struct adapter *padapter)
>  	if (true) {
>  		u8 *p, *dst_ie, *premainder_ie = NULL;
>  		u8 *pbackup_remainder_ie = NULL;
> -		__le16 tim_bitmap_le;
>  		uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
> 
> -		tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
> -
>  		p = rtw_get_ie(pie + _FIXED_IE_LENGTH_, _TIM_IE_, &tim_ielen, pnetwork_mlmeext->IELength - _FIXED_IE_LENGTH_);
>  		if (p != NULL && tim_ielen > 0) {
>  			tim_ielen += 2;
> @@ -137,9 +135,9 @@ static void update_BCNTIM(struct adapter *padapter)
>  			*dst_ie++ = 0;
> 
>  		if (tim_ielen == 4) {
> -			*dst_ie++ = *(u8 *)&tim_bitmap_le;
> +			*dst_ie++ = pstapriv->tim_bitmap & 0xff;
>  		} else if (tim_ielen == 5) {
> -			memcpy(dst_ie, &tim_bitmap_le, 2);
> +			put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
>  			dst_ie += 2;
>  		}
> 

Re: [Outreachy kernel] [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16

[Vaishali Thakkar]

On Fri, Feb 20, 2015 at 12:47 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Fri, 20 Feb 2015, Vaishali Thakkar wrote:
>
>> Using byte ordering functions and then memcpy() is risky and
>> prone to hide errors which are hard to track down. So, this
>> patch introduces the use of function put_unaligned_le16 which
>> makes the code clear. Here, use of variable tim_bitmap_le
>> and variable itself is removed. Also, to be compatible with the
>> changes header file is added too.
>>
>> Coccinelle is used to do this change and semantic patch used for
>> this is as follows:
>>
>> @a@
>> typedef __le16;
>> __le16 e16;
>> identifier tmp;
>> expression ptr;
>> expression y,e;
>> type T;
>> @@
>>
>> - tmp = cpu_to_le16(y);
>>
>> <+... when != tmp
>> (
>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
>> + put_unaligned_le16(y,ptr);
>> |
>> - memcpy(ptr, (T)&tmp, ...);
>> + put_unaligned_le16(y,ptr);
>> )
>> ...+>
>> ? tmp = e
>>
>> @@ type T; identifier a.tmp; @@
>>
>> - T tmp;
>> ...when != tmp
>>
>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>> ---
>> Changes since v1:
>>       -Edit commit log
>>       -Merge this patch in a patch series
>
> So in this case there is no change since v2?  Maybe it would be better to
> make that explicit.

Actually there is no v2 of this patch but as this patch is merged in a
series with
v3 of patch Staging: rtl8188eu: Use put_unaligned_le16 it comes as a v3.

Should I clear this in a cover letter and then resend the patch series?

> julia
>
>
>>  drivers/staging/rtl8723au/core/rtw_ap.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c b/drivers/staging/rtl8723au/core/rtw_ap.c
>> index c6327c0..7fa4352 100644
>> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
>> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
>> @@ -20,6 +20,7 @@
>>  #include <wifi.h>
>>  #include <rtl8723a_cmd.h>
>>  #include <rtl8723a_hal.h>
>> +#include <asm/unaligned.h>
>>
>>  extern unsigned char WMM_OUI23A[];
>>  extern unsigned char WPS_OUI23A[];
>> @@ -72,11 +73,8 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>>       struct wlan_bssid_ex *pnetwork_mlmeext = &pmlmeinfo->network;
>>       unsigned char *pie = pnetwork_mlmeext->IEs;
>>       u8 *p, *dst_ie, *premainder_ie = NULL, *pbackup_remainder_ie = NULL;
>> -     __le16 tim_bitmap_le;
>>       uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
>>
>> -     tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
>> -
>>       p = rtw_get_ie23a(pie, WLAN_EID_TIM, &tim_ielen,
>>                         pnetwork_mlmeext->IELength);
>>       if (p != NULL && tim_ielen > 0) {
>> @@ -143,9 +141,9 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>>               *dst_ie++ = 0;
>>
>>       if (tim_ielen == 4) {
>> -             *dst_ie++ = *(u8 *)&tim_bitmap_le;
>> +             *dst_ie++ = pstapriv->tim_bitmap & 0xff;
>>       } else if (tim_ielen == 5) {
>> -             memcpy(dst_ie, &tim_bitmap_le, 2);
>> +             put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
>>               dst_ie += 2;
>>       }
>>
>> --
>> 1.9.1
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/9defe79e0f212c883f260e27a67bff9f9ba013ba.1424402536.git.vthakkar1994%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>>



-- 
Vaishali

Re: [Outreachy kernel] [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16

[Preeti U Murthy]

On 02/20/2015 09:01 AM, Vaishali Thakkar wrote:
> Using byte ordering functions and then memcpy() is risky and
> prone to hide errors which are hard to track down. So, this
> patch introduces the use of function put_unaligned_le16 which
> makes the code clear. Here, use of variable tim_bitmap_le
> and variable itself is removed. Also, to be compatible with the
> changes header file is added too.
> 
> Coccinelle is used to do this change and semantic patch used for
> this is as follows:
> 
> @a@
> typedef __le16;
> __le16 e16;
> identifier tmp;
> expression ptr;
> expression y,e;
> type T;
> @@
> 
> - tmp = cpu_to_le16(y);
> 
> <+... when != tmp
> (
> - memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
> + put_unaligned_le16(y,ptr);
> |
> - memcpy(ptr, (T)&tmp, ...);
> + put_unaligned_le16(y,ptr);
> )
> ...+>
> ? tmp = e
> 
> @@ type T; identifier a.tmp; @@
> 
> - T tmp;
> ...when != tmp
> 
> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
> ---
> Changes since v1:
> 	-Edit commit log
> 	-Merge this patch in a patch series
> 
>  drivers/staging/rtl8723au/core/rtw_ap.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c b/drivers/staging/rtl8723au/core/rtw_ap.c
> index c6327c0..7fa4352 100644
> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
> @@ -20,6 +20,7 @@
>  #include <wifi.h>
>  #include <rtl8723a_cmd.h>
>  #include <rtl8723a_hal.h>
> +#include <asm/unaligned.h>
> 
>  extern unsigned char WMM_OUI23A[];
>  extern unsigned char WPS_OUI23A[];
> @@ -72,11 +73,8 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>  	struct wlan_bssid_ex *pnetwork_mlmeext = &pmlmeinfo->network;
>  	unsigned char *pie = pnetwork_mlmeext->IEs;
>  	u8 *p, *dst_ie, *premainder_ie = NULL, *pbackup_remainder_ie = NULL;
> -	__le16 tim_bitmap_le;
>  	uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
> 
> -	tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
> -
>  	p = rtw_get_ie23a(pie, WLAN_EID_TIM, &tim_ielen,
>  			  pnetwork_mlmeext->IELength);
>  	if (p != NULL && tim_ielen > 0) {
> @@ -143,9 +141,9 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>  		*dst_ie++ = 0;
> 
>  	if (tim_ielen == 4) {
> -		*dst_ie++ = *(u8 *)&tim_bitmap_le;
> +		*dst_ie++ = pstapriv->tim_bitmap & 0xff;
>  	} else if (tim_ielen == 5) {
> -		memcpy(dst_ie, &tim_bitmap_le, 2);
> +		put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
>  		dst_ie += 2;
>  	}
> 
Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>

Re: [Outreachy kernel] [PATCH v3 1/2] Staging: rtl8188eu: Use put_unaligned_le16

[Vaishali Thakkar]

On Fri, Feb 20, 2015 at 12:54 PM, Preeti U Murthy
<preeti@linux.vnet.ibm.com> wrote:
> Hi Vaishali,

Hello Preeti

> On 02/20/2015 09:00 AM, Vaishali Thakkar wrote:
>> Using byte ordering functions and then memcpy() is risky and
>> prone to hide errors which are hard to track down. So, this
>> patch introduces the use of function put_unaligned_le16 which
>> makes the code clear. Here, use of variable tim_bitmap_le
>> and variable itself is removed. Also, to be compatible with the
>> changes header file is added too.
>
> You don't really need to explain the inclusion of header files since it
> would be understood that it is to link the newly introduced functions in
> the patch. But you don't need to send out a new version just for
> correcting this. The patchset looks good to me.

Ok. I will keep this in mind for next patches.

Thank You

> Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>
>> Coccinelle is used to do this change and semantic patch used for
>> this is as follows:
>>
>> @a@
>> typedef __le16;
>> __le16 e16;
>> identifier tmp;
>> expression ptr;
>> expression y,e;
>> type T;
>> @@
>>
>> - tmp = cpu_to_le16(y);
>>
>> <+... when != tmp
>> (
>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
>> + put_unaligned_le16(y,ptr);
>> |
>> - memcpy(ptr, (T)&tmp, ...);
>> + put_unaligned_le16(y,ptr);
>> )
>> ...+>
>> ? tmp = e
>>
>> @@ type T; identifier a.tmp; @@
>>
>> - T tmp;
>> ...when != tmp
>>
>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>> ---
>> Changes since v2:
>>       -Edit commit log
>>       -Merge this patch in a patch-series
>>
>> Changes since v1:
>>       -Remove use of variable tim_bitmap_le
>>       -Remove variable tim_bitmap_le
>>       -Add header file to be compatible with the changes
>>       -Edit commit log
>>
>>  drivers/staging/rtl8188eu/core/rtw_ap.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c b/drivers/staging/rtl8188eu/core/rtw_ap.c
>> index da19145..e65ee6e 100644
>> --- a/drivers/staging/rtl8188eu/core/rtw_ap.c
>> +++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
>> @@ -23,6 +23,7 @@
>>  #include <drv_types.h>
>>  #include <wifi.h>
>>  #include <ieee80211.h>
>> +#include <asm/unaligned.h>
>>
>>  #ifdef CONFIG_88EU_AP_MODE
>>
>> @@ -78,11 +79,8 @@ static void update_BCNTIM(struct adapter *padapter)
>>       if (true) {
>>               u8 *p, *dst_ie, *premainder_ie = NULL;
>>               u8 *pbackup_remainder_ie = NULL;
>> -             __le16 tim_bitmap_le;
>>               uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
>>
>> -             tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
>> -
>>               p = rtw_get_ie(pie + _FIXED_IE_LENGTH_, _TIM_IE_, &tim_ielen, pnetwork_mlmeext->IELength - _FIXED_IE_LENGTH_);
>>               if (p != NULL && tim_ielen > 0) {
>>                       tim_ielen += 2;
>> @@ -137,9 +135,9 @@ static void update_BCNTIM(struct adapter *padapter)
>>                       *dst_ie++ = 0;
>>
>>               if (tim_ielen == 4) {
>> -                     *dst_ie++ = *(u8 *)&tim_bitmap_le;
>> +                     *dst_ie++ = pstapriv->tim_bitmap & 0xff;
>>               } else if (tim_ielen == 5) {
>> -                     memcpy(dst_ie, &tim_bitmap_le, 2);
>> +                     put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
>>                       dst_ie += 2;
>>               }
>>
>



-- 
Vaishali

Re: [Outreachy kernel] [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16

[Preeti U Murthy]

On 02/20/2015 12:54 PM, Vaishali Thakkar wrote:
> On Fri, Feb 20, 2015 at 12:47 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> On Fri, 20 Feb 2015, Vaishali Thakkar wrote:
>>
>>> Using byte ordering functions and then memcpy() is risky and
>>> prone to hide errors which are hard to track down. So, this
>>> patch introduces the use of function put_unaligned_le16 which
>>> makes the code clear. Here, use of variable tim_bitmap_le
>>> and variable itself is removed. Also, to be compatible with the
>>> changes header file is added too.
>>>
>>> Coccinelle is used to do this change and semantic patch used for
>>> this is as follows:
>>>
>>> @a@
>>> typedef __le16;
>>> __le16 e16;
>>> identifier tmp;
>>> expression ptr;
>>> expression y,e;
>>> type T;
>>> @@
>>>
>>> - tmp = cpu_to_le16(y);
>>>
>>> <+... when != tmp
>>> (
>>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
>>> + put_unaligned_le16(y,ptr);
>>> |
>>> - memcpy(ptr, (T)&tmp, ...);
>>> + put_unaligned_le16(y,ptr);
>>> )
>>> ...+>
>>> ? tmp = e
>>>
>>> @@ type T; identifier a.tmp; @@
>>>
>>> - T tmp;
>>> ...when != tmp
>>>
>>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>>> ---
>>> Changes since v1:
>>>       -Edit commit log
>>>       -Merge this patch in a patch series
>>
>> So in this case there is no change since v2?  Maybe it would be better to
>> make that explicit.
> 
> Actually there is no v2 of this patch but as this patch is merged in a
> series with
> v3 of patch Staging: rtl8188eu: Use put_unaligned_le16 it comes as a v3.
> 
> Should I clear this in a cover letter and then resend the patch series?

Yes I would suggest that. I missed this. Additionally I would recommend
in this case to not prefix any version number to the cover letter.
Instead prefix the appropriate version numbers to the individual patches
mentioning the changes from the previous versions.

Regards
Preeti U Murthy
> 
>> julia
>>
>>
>>>  drivers/staging/rtl8723au/core/rtw_ap.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c b/drivers/staging/rtl8723au/core/rtw_ap.c
>>> index c6327c0..7fa4352 100644
>>> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
>>> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
>>> @@ -20,6 +20,7 @@
>>>  #include <wifi.h>
>>>  #include <rtl8723a_cmd.h>
>>>  #include <rtl8723a_hal.h>
>>> +#include <asm/unaligned.h>
>>>
>>>  extern unsigned char WMM_OUI23A[];
>>>  extern unsigned char WPS_OUI23A[];
>>> @@ -72,11 +73,8 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>>>       struct wlan_bssid_ex *pnetwork_mlmeext = &pmlmeinfo->network;
>>>       unsigned char *pie = pnetwork_mlmeext->IEs;
>>>       u8 *p, *dst_ie, *premainder_ie = NULL, *pbackup_remainder_ie = NULL;
>>> -     __le16 tim_bitmap_le;
>>>       uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
>>>
>>> -     tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
>>> -
>>>       p = rtw_get_ie23a(pie, WLAN_EID_TIM, &tim_ielen,
>>>                         pnetwork_mlmeext->IELength);
>>>       if (p != NULL && tim_ielen > 0) {
>>> @@ -143,9 +141,9 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>>>               *dst_ie++ = 0;
>>>
>>>       if (tim_ielen == 4) {
>>> -             *dst_ie++ = *(u8 *)&tim_bitmap_le;
>>> +             *dst_ie++ = pstapriv->tim_bitmap & 0xff;
>>>       } else if (tim_ielen == 5) {
>>> -             memcpy(dst_ie, &tim_bitmap_le, 2);
>>> +             put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
>>>               dst_ie += 2;
>>>       }
>>>
>>> --
>>> 1.9.1
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/9defe79e0f212c883f260e27a67bff9f9ba013ba.1424402536.git.vthakkar1994%40gmail.com.
>>> For more options, visit https://groups.google.com/d/optout.
>>>
> 
> 
> 

Re: [Outreachy kernel] [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16

[Vaishali Thakkar]

On Fri, Feb 20, 2015 at 1:08 PM, Preeti U Murthy
<preeti@linux.vnet.ibm.com> wrote:
>
> On 02/20/2015 12:54 PM, Vaishali Thakkar wrote:
>> On Fri, Feb 20, 2015 at 12:47 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>>> On Fri, 20 Feb 2015, Vaishali Thakkar wrote:
>>>
>>>> Using byte ordering functions and then memcpy() is risky and
>>>> prone to hide errors which are hard to track down. So, this
>>>> patch introduces the use of function put_unaligned_le16 which
>>>> makes the code clear. Here, use of variable tim_bitmap_le
>>>> and variable itself is removed. Also, to be compatible with the
>>>> changes header file is added too.
>>>>
>>>> Coccinelle is used to do this change and semantic patch used for
>>>> this is as follows:
>>>>
>>>> @a@
>>>> typedef __le16;
>>>> __le16 e16;
>>>> identifier tmp;
>>>> expression ptr;
>>>> expression y,e;
>>>> type T;
>>>> @@
>>>>
>>>> - tmp = cpu_to_le16(y);
>>>>
>>>> <+... when != tmp
>>>> (
>>>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(__le16)\|sizeof(e16)\));
>>>> + put_unaligned_le16(y,ptr);
>>>> |
>>>> - memcpy(ptr, (T)&tmp, ...);
>>>> + put_unaligned_le16(y,ptr);
>>>> )
>>>> ...+>
>>>> ? tmp = e
>>>>
>>>> @@ type T; identifier a.tmp; @@
>>>>
>>>> - T tmp;
>>>> ...when != tmp
>>>>
>>>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>>>> ---
>>>> Changes since v1:
>>>>       -Edit commit log
>>>>       -Merge this patch in a patch series
>>>
>>> So in this case there is no change since v2?  Maybe it would be better to
>>> make that explicit.
>>
>> Actually there is no v2 of this patch but as this patch is merged in a
>> series with
>> v3 of patch Staging: rtl8188eu: Use put_unaligned_le16 it comes as a v3.
>>
>> Should I clear this in a cover letter and then resend the patch series?
>
> Yes I would suggest that. I missed this. Additionally I would recommend
> in this case to not prefix any version number to the cover letter.
> Instead prefix the appropriate version numbers to the individual patches
> mentioning the changes from the previous versions.

Ok. So, in next patch-series cover letter will not have any version number and
both of these patches will have v4 as I already prefixed them as v3.
And for this
particular patch I should write that there is no v2 under signed off
by line. Right??

> Regards
> Preeti U Murthy
>>
>>> julia
>>>
>>>
>>>>  drivers/staging/rtl8723au/core/rtw_ap.c | 8 +++-----
>>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c b/drivers/staging/rtl8723au/core/rtw_ap.c
>>>> index c6327c0..7fa4352 100644
>>>> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
>>>> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
>>>> @@ -20,6 +20,7 @@
>>>>  #include <wifi.h>
>>>>  #include <rtl8723a_cmd.h>
>>>>  #include <rtl8723a_hal.h>
>>>> +#include <asm/unaligned.h>
>>>>
>>>>  extern unsigned char WMM_OUI23A[];
>>>>  extern unsigned char WPS_OUI23A[];
>>>> @@ -72,11 +73,8 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>>>>       struct wlan_bssid_ex *pnetwork_mlmeext = &pmlmeinfo->network;
>>>>       unsigned char *pie = pnetwork_mlmeext->IEs;
>>>>       u8 *p, *dst_ie, *premainder_ie = NULL, *pbackup_remainder_ie = NULL;
>>>> -     __le16 tim_bitmap_le;
>>>>       uint offset, tmp_len, tim_ielen, tim_ie_offset, remainder_ielen;
>>>>
>>>> -     tim_bitmap_le = cpu_to_le16(pstapriv->tim_bitmap);
>>>> -
>>>>       p = rtw_get_ie23a(pie, WLAN_EID_TIM, &tim_ielen,
>>>>                         pnetwork_mlmeext->IELength);
>>>>       if (p != NULL && tim_ielen > 0) {
>>>> @@ -143,9 +141,9 @@ static void update_BCNTIM(struct rtw_adapter *padapter)
>>>>               *dst_ie++ = 0;
>>>>
>>>>       if (tim_ielen == 4) {
>>>> -             *dst_ie++ = *(u8 *)&tim_bitmap_le;
>>>> +             *dst_ie++ = pstapriv->tim_bitmap & 0xff;
>>>>       } else if (tim_ielen == 5) {
>>>> -             memcpy(dst_ie, &tim_bitmap_le, 2);
>>>> +             put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
>>>>               dst_ie += 2;
>>>>       }
>>>>
>>>> --
>>>> 1.9.1
>>>>
>>>> --
>>>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>>>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>>>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>>>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/9defe79e0f212c883f260e27a67bff9f9ba013ba.1424402536.git.vthakkar1994%40gmail.com.
>>>> For more options, visit https://groups.google.com/d/optout.
>>>>
>>
>>
>>
>



-- 
Vaishali

Re: [Outreachy kernel] [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16

[Preeti U Murthy]

On 02/20/2015 01:20 PM, Vaishali Thakkar wrote:
>>
>> Yes I would suggest that. I missed this. Additionally I would recommend
>> in this case to not prefix any version number to the cover letter.
>> Instead prefix the appropriate version numbers to the individual patches
>> mentioning the changes from the previous versions.
> 
> Ok. So, in next patch-series cover letter will not have any version number and
> both of these patches will have v4 as I already prefixed them as v3.
> And for this
> particular patch I should write that there is no v2 under signed off
> by line. Right??

1. No version number for the cover letter.
2. Retain [PATCH v3 1/2] Staging: rtl8188eu: Use put_unaligned_le16 as
v3 itself.
3. Change [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16: to
v2. So it would be:
[PATCH v2 2/2] Staging: rtl8723au: Use put_unaligned_le16, since it has
undergone just one revision.

The reason I ask not to change the version numbers again is because
there is nothing that has significantly changed from the previous
versions. There will be a change only in the manner of sending patches.

So now the question is about how you indicate to the maintainer that the
fresh series that you will post now is the one to be picked up. Usually
under such circumstances we append RESEND to the patches.
So now you can resend the next version as mentioned in the points above
but with a RESEND tag to the cover letter and patches.

Something like: [RESEND PATCH 0/2] Staging: Use put_unaligned_le16.
The maintainer will thus know that this is the one to be picked up.

Regards
Preeti U Murthy


> 
>> Regards
>> Preeti U Murthy

Re: [Outreachy kernel] [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16

[Vaishali Thakkar]

On Fri, Feb 20, 2015 at 1:32 PM, Preeti U Murthy
<preeti@linux.vnet.ibm.com> wrote:
> On 02/20/2015 01:20 PM, Vaishali Thakkar wrote:
>>>
>>> Yes I would suggest that. I missed this. Additionally I would recommend
>>> in this case to not prefix any version number to the cover letter.
>>> Instead prefix the appropriate version numbers to the individual patches
>>> mentioning the changes from the previous versions.
>>
>> Ok. So, in next patch-series cover letter will not have any version number and
>> both of these patches will have v4 as I already prefixed them as v3.
>> And for this
>> particular patch I should write that there is no v2 under signed off
>> by line. Right??
>
> 1. No version number for the cover letter.
> 2. Retain [PATCH v3 1/2] Staging: rtl8188eu: Use put_unaligned_le16 as
> v3 itself.
> 3. Change [PATCH v3 2/2] Staging: rtl8723au: Use put_unaligned_le16: to
> v2. So it would be:
> [PATCH v2 2/2] Staging: rtl8723au: Use put_unaligned_le16, since it has
> undergone just one revision.
>
> The reason I ask not to change the version numbers again is because
> there is nothing that has significantly changed from the previous
> versions. There will be a change only in the manner of sending patches.
>
> So now the question is about how you indicate to the maintainer that the
> fresh series that you will post now is the one to be picked up. Usually
> under such circumstances we append RESEND to the patches.
> So now you can resend the next version as mentioned in the points above
> but with a RESEND tag to the cover letter and patches.
>
> Something like: [RESEND PATCH 0/2] Staging: Use put_unaligned_le16.
> The maintainer will thus know that this is the one to be picked up.

Yes. This makes sense. I was confused about how maintainer will know what
to pick in these kind of situations. Thanks for clearing this. :)

> Regards
> Preeti U Murthy
>
>
>>
>>> Regards
>>> Preeti U Murthy
>



-- 
Vaishali