[PATCH v2] Staging: rtl8188eu: Use put_unaligned_le16

[PATCH v2] Staging: rtl8188eu: Use put_unaligned_le16

[Vaishali Thakkar]

This patch introduces the use of function put_unaligned_le16.

This is done using Coccinelle and semantic patch used is as follows:

@a@
typedef u16, __le16, uint16_t;
{u16,__le16,uint16_t} e16;
identifier tmp;
expression ptr;
expression y,e;
type T;
@@

- tmp = cpu_to_le16(y);

<+... when != tmp
(
- memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(uint16_t)\|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

Here, use of variable tim_bitmap_le and variable itself is removed. To be compatible
with changes header file is added too.

Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
---
Changes since v1:
	-Use of variable tim_bitmap_le is removed
	-Variable tim_bitmap_le is removed
	-To be compatible with changes header file is added
	-Commit log is edited

 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

Re: [Outreachy kernel] [PATCH v2] Staging: rtl8188eu: Use put_unaligned_le16

[Preeti U Murthy]

Hi Vaishali,

On 02/18/2015 05:17 PM, Vaishali Thakkar wrote:
> This patch introduces the use of function put_unaligned_le16.

Why is the use of put_unaligned_le16 better than memcpy here ?
Can this be figured out?

Regards
Preeti U Murthy
> 
> This is done using Coccinelle and semantic patch used is as follows:
> 
> @a@
> typedef u16, __le16, uint16_t;
> {u16,__le16,uint16_t} e16;
> identifier tmp;
> expression ptr;
> expression y,e;
> type T;
> @@
> 
> - tmp = cpu_to_le16(y);
> 
> <+... when != tmp
> (
> - memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(uint16_t)\|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
> 
> Here, use of variable tim_bitmap_le and variable itself is removed. To be compatible
> with changes header file is added too.
> 
> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
> ---
> Changes since v1:
> 	-Use of variable tim_bitmap_le is removed
> 	-Variable tim_bitmap_le is removed
> 	-To be compatible with changes header file is added
> 	-Commit log is edited
> 
>  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 v2] Staging: rtl8188eu: Use put_unaligned_le16

[Vaishali Thakkar]

On Wed, Feb 18, 2015 at 6:03 PM, Preeti U Murthy
<preeti@linux.vnet.ibm.com> wrote:
> Hi Vaishali,

Hello Preeti,

> On 02/18/2015 05:17 PM, Vaishali Thakkar wrote:
>> This patch introduces the use of function put_unaligned_le16.
>
> Why is the use of put_unaligned_le16 better than memcpy here ?
> Can this be figured out?

As per the definition of put_unaligned_* functions (Here:
http://lxr.free-electrons.com/source/arch/c6x/include/asm/unaligned.h#L34)
, we can use
put_unaligned_* functions in following kind of cases:

Whenever we have two lines like these:

__le16 tmp = cpu_to_le16(y);
    memcpy(ptr, &tmp, sizeof(tmp));

We can go for using put_unaligned_* functions instead:

    put_unaligned_le16(y, ptr);

This change can be applied for all put_unaligned-* functions
[ little endian and big endian] listed in this header file.

This is my understanding about these functions. Bob suggested me these
functions at first place. I worked on solving such bugs using
Coccinelle considering
many different cases and wrote semantic patch for solving all those
bugs. Though I
had some endianness issue for some cases, and I am working on it. It
may be possible
that I am missing something. If it is such a case, then it would be
good if you can correct
me. If you want me to be more specific in change log then also I can go for v3.

Thank You

> Regards
> Preeti U Murthy
>>
>> This is done using Coccinelle and semantic patch used is as follows:
>>
>> @a@
>> typedef u16, __le16, uint16_t;
>> {u16,__le16,uint16_t} e16;
>> identifier tmp;
>> expression ptr;
>> expression y,e;
>> type T;
>> @@
>>
>> - tmp = cpu_to_le16(y);
>>
>> <+... when != tmp
>> (
>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(uint16_t)\|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
>>
>> Here, use of variable tim_bitmap_le and variable itself is removed. To be compatible
>> with changes header file is added too.
>>
>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>> ---
>> Changes since v1:
>>       -Use of variable tim_bitmap_le is removed
>>       -Variable tim_bitmap_le is removed
>>       -To be compatible with changes header file is added
>>       -Commit log is edited
>>
>>  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 v2] Staging: rtl8188eu: Use put_unaligned_le16

[Jes Sorensen]

On 02/18/15 08:21, Vaishali Thakkar wrote:
> On Wed, Feb 18, 2015 at 6:03 PM, Preeti U Murthy
> <preeti@linux.vnet.ibm.com> wrote:
>> Hi Vaishali,
> 
> Hello Preeti,
> 
>> On 02/18/2015 05:17 PM, Vaishali Thakkar wrote:
>>> This patch introduces the use of function put_unaligned_le16.
>>
>> Why is the use of put_unaligned_le16 better than memcpy here ?
>> Can this be figured out?
> 
> As per the definition of put_unaligned_* functions (Here:
> http://lxr.free-electrons.com/source/arch/c6x/include/asm/unaligned.h#L34)
> , we can use
> put_unaligned_* functions in following kind of cases:
> 
> Whenever we have two lines like these:
> 
> __le16 tmp = cpu_to_le16(y);
>     memcpy(ptr, &tmp, sizeof(tmp));
> 
> We can go for using put_unaligned_* functions instead:
> 
>     put_unaligned_le16(y, ptr);
> 
> This change can be applied for all put_unaligned-* functions
> [ little endian and big endian] listed in this header file.
> 
> This is my understanding about these functions. Bob suggested me these
> functions at first place. I worked on solving such bugs using
> Coccinelle considering
> many different cases and wrote semantic patch for solving all those
> bugs. Though I
> had some endianness issue for some cases, and I am working on it. It
> may be possible
> that I am missing something. If it is such a case, then it would be
> good if you can correct
> me. If you want me to be more specific in change log then also I can go for v3.

Just a comment on this - playing with byte ordering macros and then
memcpy() is always risky, and prone to hide errors which are hard to
track down. The use of put_unaligned_le16() makes it very clear what is
happening in the code, so it is a good solution to apply here.

Cheers,
Jes

> Thank You
> 
>> Regards
>> Preeti U Murthy
>>>
>>> This is done using Coccinelle and semantic patch used is as follows:
>>>
>>> @a@
>>> typedef u16, __le16, uint16_t;
>>> {u16,__le16,uint16_t} e16;
>>> identifier tmp;
>>> expression ptr;
>>> expression y,e;
>>> type T;
>>> @@
>>>
>>> - tmp = cpu_to_le16(y);
>>>
>>> <+... when != tmp
>>> (
>>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(uint16_t)\|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
>>>
>>> Here, use of variable tim_bitmap_le and variable itself is removed. To be compatible
>>> with changes header file is added too.
>>>
>>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>>> ---
>>> Changes since v1:
>>>       -Use of variable tim_bitmap_le is removed
>>>       -Variable tim_bitmap_le is removed
>>>       -To be compatible with changes header file is added
>>>       -Commit log is edited
>>>
>>>  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 v2] Staging: rtl8188eu: Use put_unaligned_le16

[Vaishali Thakkar]

On Wed, Feb 18, 2015 at 10:49 PM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
> On 02/18/15 08:21, Vaishali Thakkar wrote:
>> On Wed, Feb 18, 2015 at 6:03 PM, Preeti U Murthy
>> <preeti@linux.vnet.ibm.com> wrote:
>>> Hi Vaishali,
>>
>> Hello Preeti,
>>
>>> On 02/18/2015 05:17 PM, Vaishali Thakkar wrote:
>>>> This patch introduces the use of function put_unaligned_le16.
>>>
>>> Why is the use of put_unaligned_le16 better than memcpy here ?
>>> Can this be figured out?
>>
>> As per the definition of put_unaligned_* functions (Here:
>> http://lxr.free-electrons.com/source/arch/c6x/include/asm/unaligned.h#L34)
>> , we can use
>> put_unaligned_* functions in following kind of cases:
>>
>> Whenever we have two lines like these:
>>
>> __le16 tmp = cpu_to_le16(y);
>>     memcpy(ptr, &tmp, sizeof(tmp));
>>
>> We can go for using put_unaligned_* functions instead:
>>
>>     put_unaligned_le16(y, ptr);
>>
>> This change can be applied for all put_unaligned-* functions
>> [ little endian and big endian] listed in this header file.
>>
>> This is my understanding about these functions. Bob suggested me these
>> functions at first place. I worked on solving such bugs using
>> Coccinelle considering
>> many different cases and wrote semantic patch for solving all those
>> bugs. Though I
>> had some endianness issue for some cases, and I am working on it. It
>> may be possible
>> that I am missing something. If it is such a case, then it would be
>> good if you can correct
>> me. If you want me to be more specific in change log then also I can go for v3.
>
> Just a comment on this - playing with byte ordering macros and then
> memcpy() is always risky, and prone to hide errors which are hard to
> track down. The use of put_unaligned_le16() makes it very clear what is
> happening in the code, so it is a good solution to apply here.

Yes. This makes sense. Thanks for the comment.

> Cheers,
> Jes
>
>> Thank You
>>
>>> Regards
>>> Preeti U Murthy
>>>>
>>>> This is done using Coccinelle and semantic patch used is as follows:
>>>>
>>>> @a@
>>>> typedef u16, __le16, uint16_t;
>>>> {u16,__le16,uint16_t} e16;
>>>> identifier tmp;
>>>> expression ptr;
>>>> expression y,e;
>>>> type T;
>>>> @@
>>>>
>>>> - tmp = cpu_to_le16(y);
>>>>
>>>> <+... when != tmp
>>>> (
>>>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(uint16_t)\|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
>>>>
>>>> Here, use of variable tim_bitmap_le and variable itself is removed. To be compatible
>>>> with changes header file is added too.
>>>>
>>>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>>>> ---
>>>> Changes since v1:
>>>>       -Use of variable tim_bitmap_le is removed
>>>>       -Variable tim_bitmap_le is removed
>>>>       -To be compatible with changes header file is added
>>>>       -Commit log is edited
>>>>
>>>>  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 v2] Staging: rtl8188eu: Use put_unaligned_le16

[Preeti U Murthy]

Hi Vaishali,

On 02/18/2015 10:49 PM, Jes Sorensen wrote:
> On 02/18/15 08:21, Vaishali Thakkar wrote:
>> On Wed, Feb 18, 2015 at 6:03 PM, Preeti U Murthy
>> <preeti@linux.vnet.ibm.com> wrote:
>>> Hi Vaishali,
>>
>> Hello Preeti,
>>
>>> On 02/18/2015 05:17 PM, Vaishali Thakkar wrote:
>>>> This patch introduces the use of function put_unaligned_le16.
>>>
>>> Why is the use of put_unaligned_le16 better than memcpy here ?
>>> Can this be figured out?
>>
>> As per the definition of put_unaligned_* functions (Here:
>> http://lxr.free-electrons.com/source/arch/c6x/include/asm/unaligned.h#L34)
>> , we can use
>> put_unaligned_* functions in following kind of cases:
>>
>> Whenever we have two lines like these:
>>
>> __le16 tmp = cpu_to_le16(y);
>>     memcpy(ptr, &tmp, sizeof(tmp));
>>
>> We can go for using put_unaligned_* functions instead:
>>
>>     put_unaligned_le16(y, ptr);
>>
>> This change can be applied for all put_unaligned-* functions
>> [ little endian and big endian] listed in this header file.
>>
>> This is my understanding about these functions. Bob suggested me these
>> functions at first place. I worked on solving such bugs using
>> Coccinelle considering
>> many different cases and wrote semantic patch for solving all those
>> bugs. Though I
>> had some endianness issue for some cases, and I am working on it. It
>> may be possible
>> that I am missing something. If it is such a case, then it would be
>> good if you can correct
>> me. If you want me to be more specific in change log then also I can go for v3.
> 
> Just a comment on this - playing with byte ordering macros and then
> memcpy() is always risky, and prone to hide errors which are hard to
> track down. The use of put_unaligned_le16() makes it very clear what is
> happening in the code, so it is a good solution to apply here.

Sorry I was ambiguous in pointing out my expectation in my comment. Yes
I was looking for a clearer changelog thats all. Including Jes's above
comment in your changelog would clarify the purpose of this patch.

Regards
Preeti u Murthy
> 
> Cheers,
> Jes
> 
>> Thank You
>>
>>> Regards
>>> Preeti U Murthy
>>>>
>>>> This is done using Coccinelle and semantic patch used is as follows:
>>>>
>>>> @a@
>>>> typedef u16, __le16, uint16_t;
>>>> {u16,__le16,uint16_t} e16;
>>>> identifier tmp;
>>>> expression ptr;
>>>> expression y,e;
>>>> type T;
>>>> @@
>>>>
>>>> - tmp = cpu_to_le16(y);
>>>>
>>>> <+... when != tmp
>>>> (
>>>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(uint16_t)\|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
>>>>
>>>> Here, use of variable tim_bitmap_le and variable itself is removed. To be compatible
>>>> with changes header file is added too.
>>>>
>>>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>>>> ---
>>>> Changes since v1:
>>>>       -Use of variable tim_bitmap_le is removed
>>>>       -Variable tim_bitmap_le is removed
>>>>       -To be compatible with changes header file is added
>>>>       -Commit log is edited
>>>>
>>>>  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 v2] Staging: rtl8188eu: Use put_unaligned_le16

[Vaishali Thakkar]

On Thu, Feb 19, 2015 at 8:33 AM, Preeti U Murthy
<preeti@linux.vnet.ibm.com> wrote:
> Hi Vaishali,

Hello Preeti

> On 02/18/2015 10:49 PM, Jes Sorensen wrote:
>> On 02/18/15 08:21, Vaishali Thakkar wrote:
>>> On Wed, Feb 18, 2015 at 6:03 PM, Preeti U Murthy
>>> <preeti@linux.vnet.ibm.com> wrote:
>>>> Hi Vaishali,
>>>
>>> Hello Preeti,
>>>
>>>> On 02/18/2015 05:17 PM, Vaishali Thakkar wrote:
>>>>> This patch introduces the use of function put_unaligned_le16.
>>>>
>>>> Why is the use of put_unaligned_le16 better than memcpy here ?
>>>> Can this be figured out?
>>>
>>> As per the definition of put_unaligned_* functions (Here:
>>> http://lxr.free-electrons.com/source/arch/c6x/include/asm/unaligned.h#L34)
>>> , we can use
>>> put_unaligned_* functions in following kind of cases:
>>>
>>> Whenever we have two lines like these:
>>>
>>> __le16 tmp = cpu_to_le16(y);
>>>     memcpy(ptr, &tmp, sizeof(tmp));
>>>
>>> We can go for using put_unaligned_* functions instead:
>>>
>>>     put_unaligned_le16(y, ptr);
>>>
>>> This change can be applied for all put_unaligned-* functions
>>> [ little endian and big endian] listed in this header file.
>>>
>>> This is my understanding about these functions. Bob suggested me these
>>> functions at first place. I worked on solving such bugs using
>>> Coccinelle considering
>>> many different cases and wrote semantic patch for solving all those
>>> bugs. Though I
>>> had some endianness issue for some cases, and I am working on it. It
>>> may be possible
>>> that I am missing something. If it is such a case, then it would be
>>> good if you can correct
>>> me. If you want me to be more specific in change log then also I can go for v3.
>>
>> Just a comment on this - playing with byte ordering macros and then
>> memcpy() is always risky, and prone to hide errors which are hard to
>> track down. The use of put_unaligned_le16() makes it very clear what is
>> happening in the code, so it is a good solution to apply here.
>
> Sorry I was ambiguous in pointing out my expectation in my comment. Yes
> I was looking for a clearer changelog thats all. Including Jes's above
> comment in your changelog would clarify the purpose of this patch.

Yes. I got your point and it makes sense too. I will add Jes's comment
in next version and will keep my change log short and clear.

Thank You

> Regards
> Preeti u Murthy
>>
>> Cheers,
>> Jes
>>
>>> Thank You
>>>
>>>> Regards
>>>> Preeti U Murthy
>>>>>
>>>>> This is done using Coccinelle and semantic patch used is as follows:
>>>>>
>>>>> @a@
>>>>> typedef u16, __le16, uint16_t;
>>>>> {u16,__le16,uint16_t} e16;
>>>>> identifier tmp;
>>>>> expression ptr;
>>>>> expression y,e;
>>>>> type T;
>>>>> @@
>>>>>
>>>>> - tmp = cpu_to_le16(y);
>>>>>
>>>>> <+... when != tmp
>>>>> (
>>>>> - memcpy(ptr, (T)&tmp, \(2\|sizeof(u16)\|sizeof(__le16)\|sizeof(uint16_t)\|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
>>>>>
>>>>> Here, use of variable tim_bitmap_le and variable itself is removed. To be compatible
>>>>> with changes header file is added too.
>>>>>
>>>>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>>>>> ---
>>>>> Changes since v1:
>>>>>       -Use of variable tim_bitmap_le is removed
>>>>>       -Variable tim_bitmap_le is removed
>>>>>       -To be compatible with changes header file is added
>>>>>       -Commit log is edited
>>>>>
>>>>>  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