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

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

On 02/17/15 05:11, Vaishali Thakkar wrote:
> On Tue, Feb 17, 2015 at 2:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tuesday 17 February 2015 10:12:57 Vaishali Thakkar wrote:
>>> diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c b/drivers/staging/rtl8188eu/core/rtw_ap.c
>>> index da19145..0b7b156 100644
>>> --- a/drivers/staging/rtl8188eu/core/rtw_ap.c
>>> +++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
>>> @@ -81,8 +81,6 @@ static void update_BCNTIM(struct adapter *padapter)
>>>                 __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;
>>> @@ -139,7 +137,7 @@ static void update_BCNTIM(struct adapter *padapter)
>>>                 if (tim_ielen == 4) {
>>>                         *dst_ie++ = *(u8 *)&tim_bitmap_le;
>>>                 } else if (tim_ielen == 5) {
>>> -                       memcpy(dst_ie, &tim_bitmap_le, 2);
>>> +                       put_unaligned_le16(pstapriv->tim_bitmap, dst_ie);
>>>                         dst_ie += 2;
>>
>> I think you are introducing the use of an uninitialized variable here, when
>> tim_bitmap_le is accessed first.
>>
>> You should always compile the code you change to check for new warnings,
>> and when you remove the use of a variable, check who else is using it
>> and if it can be removed. In this case, I'd suggest removing the
>> tim_bitmap_le variable entirely and fixing the other use as well.
> 
> I compiled the code before sending this. But it seems something went
> wrong from my side.I am sorry for this. Can you please tell me which errors are
> introduced by this patch as it is stillnot showing me any errors??
> 
> Also, here this Coccinelle semantic patch performs check for the variable
> and if it is not used anywhere else then it removes the variable.Here,
> in this case
> tim_bitmap_le variable is used " *dst_ie++ = *(u8 *)&tim_bitmap_le;"
> here. So, do
> you want me to fix this line and then remove this variable?? I am
> sorry but I am
> confused here that's why asking again.

Your use of put_unaligned_le16() if good, the main issue is the above
assignment as you mention. Since you got rid of the tim_bitmap_le =
portion, the *dst_ie++ = is going to assigning random garbage since
tim_bitmap_le is not set.

One way to solve the problem would be to make that line say

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

and hopefully eliminate the tim_bitmap_le variable completely.

Jes