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

[Jes Sorensen <jes.sorensen@gmail.com>]

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;
>>>               }
>>>
>>
> 
> 
>