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

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

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