[PATCH] Staging: rtl8188eu: Use put_unaligned_le16

[PATCH] 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;
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

Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
---
 drivers/staging/rtl8188eu/core/rtw_ap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

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

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

[Arnd Bergmann]

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.

	Arnd

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

[Vaishali Thakkar]

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.

>         Arnd



-- 
Vaishali

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

[Jes Sorensen]

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

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

[Vaishali Thakkar]

On Wed, Feb 18, 2015 at 12:42 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
> 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.

Yes. This makes sense. And as there is only one use of tim_bitmap_le
variable this will eliminate it completely. Should I go for v2 now, with this
change?? I have one more such case in rtl8723au driver also. I think now
I can go for it also.

> Jes
>



-- 
Vaishali

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

[Arnd Bergmann]

On Wednesday 18 February 2015 07:02:34 Vaishali Thakkar wrote:
> On Wed, Feb 18, 2015 at 12:42 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
> > On 02/17/15 05:11, Vaishali Thakkar wrote:
> > 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.
> 
> Yes. This makes sense. And as there is only one use of tim_bitmap_le
> variable this will eliminate it completely. Should I go for v2 now, with this
> change?? I have one more such case in rtl8723au driver also. I think now
> I can go for it also.

Yes, please submit the change for both drivers as separate patches.

	Arnd

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

[Jes Sorensen]

On 02/18/15 05:50, Arnd Bergmann wrote:
> On Wednesday 18 February 2015 07:02:34 Vaishali Thakkar wrote:
>> On Wed, Feb 18, 2015 at 12:42 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
>>> On 02/17/15 05:11, Vaishali Thakkar wrote:
>>> 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.
>>
>> Yes. This makes sense. And as there is only one use of tim_bitmap_le
>> variable this will eliminate it completely. Should I go for v2 now, with this
>> change?? I have one more such case in rtl8723au driver also. I think now
>> I can go for it also.
> 
> Yes, please submit the change for both drivers as separate patches.

As Arnd says, please use two patches. The reason is that the two drivers
are completely independent and also have separate maintainers (me owning
rtl8723au fwiw).

Cheers,
Jes

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

[Vaishali Thakkar]

On Wed, Feb 18, 2015 at 10:40 PM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
> On 02/18/15 05:50, Arnd Bergmann wrote:
>> On Wednesday 18 February 2015 07:02:34 Vaishali Thakkar wrote:
>>> On Wed, Feb 18, 2015 at 12:42 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
>>>> On 02/17/15 05:11, Vaishali Thakkar wrote:
>>>> 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.
>>>
>>> Yes. This makes sense. And as there is only one use of tim_bitmap_le
>>> variable this will eliminate it completely. Should I go for v2 now, with this
>>> change?? I have one more such case in rtl8723au driver also. I think now
>>> I can go for it also.
>>
>> Yes, please submit the change for both drivers as separate patches.
>
> As Arnd says, please use two patches. The reason is that the two drivers
> are completely independent and also have separate maintainers (me owning
> rtl8723au fwiw).

Sorry I didn't get you.

Yes, I know both drivers are independent. So, I will send 2 different patches.
Here, I sent only one patch for rtl8188eu. I didn't send any patch for
rtl8723au.
I am working on it and supposed to send it now.

> Cheers,
> Jes
>
>



-- 
Vaishali

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

[Jes Sorensen]

On 02/18/15 13:08, Vaishali Thakkar wrote:
> On Wed, Feb 18, 2015 at 10:40 PM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
>> On 02/18/15 05:50, Arnd Bergmann wrote:
>>> On Wednesday 18 February 2015 07:02:34 Vaishali Thakkar wrote:
>>>> On Wed, Feb 18, 2015 at 12:42 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
>>>>> On 02/17/15 05:11, Vaishali Thakkar wrote:
>>>>> 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.
>>>>
>>>> Yes. This makes sense. And as there is only one use of tim_bitmap_le
>>>> variable this will eliminate it completely. Should I go for v2 now, with this
>>>> change?? I have one more such case in rtl8723au driver also. I think now
>>>> I can go for it also.
>>>
>>> Yes, please submit the change for both drivers as separate patches.
>>
>> As Arnd says, please use two patches. The reason is that the two drivers
>> are completely independent and also have separate maintainers (me owning
>> rtl8723au fwiw).
> 
> Sorry I didn't get you.
> 
> Yes, I know both drivers are independent. So, I will send 2 different patches.
> Here, I sent only one patch for rtl8188eu. I didn't send any patch for
> rtl8723au.
> I am working on it and supposed to send it now.

Sorry if I wasn't clear here.

The key is to make the changes in two commits. In general you can post
them as part of a multi-commit patch set if you are fixing something in
a large number of places, or changing an API etc. For more independent
changes like these, posting them individually is preferred.

I hope this makes more sense.

Jes

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

[Vaishali Thakkar]

[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]

On Feb 19, 2015 4:01 AM, "Jes Sorensen" <jes.sorensen@gmail.com> wrote:
>
> On 02/18/15 13:08, Vaishali Thakkar wrote:
> > On Wed, Feb 18, 2015 at 10:40 PM, Jes Sorensen <jes.sorensen@gmail.com>
wrote:
> >> On 02/18/15 05:50, Arnd Bergmann wrote:
> >>> On Wednesday 18 February 2015 07:02:34 Vaishali Thakkar wrote:
> >>>> On Wed, Feb 18, 2015 at 12:42 AM, Jes Sorensen <
jes.sorensen@gmail.com> wrote:
> >>>>> On 02/17/15 05:11, Vaishali Thakkar wrote:
> >>>>> 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.
> >>>>
> >>>> Yes. This makes sense. And as there is only one use of tim_bitmap_le
> >>>> variable this will eliminate it completely. Should I go for v2 now,
with this
> >>>> change?? I have one more such case in rtl8723au driver also. I think
now
> >>>> I can go for it also.
> >>>
> >>> Yes, please submit the change for both drivers as separate patches.
> >>
> >> As Arnd says, please use two patches. The reason is that the two
drivers
> >> are completely independent and also have separate maintainers (me
owning
> >> rtl8723au fwiw).
> >
> > Sorry I didn't get you.
> >
> > Yes, I know both drivers are independent. So, I will send 2 different
patches.
> > Here, I sent only one patch for rtl8188eu. I didn't send any patch for
> > rtl8723au.
> > I am working on it and supposed to send it now.
>
> Sorry if I wasn't clear here.
>
> The key is to make the changes in two commits. In general you can post
> them as part of a multi-commit patch set if you are fixing something in
> a large number of places, or changing an API etc. For more independent
> changes like these, posting them individually is preferred.
>
> I hope this makes more sense.

Yes. Ok. I will send this patch as a series of patches. One for
put_unaligned_le16

function and other one for removing use of variable.

> Jes
>

[-- Attachment #2: Type: text/html, Size: 3732 bytes --]

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

[Arnd Bergmann]

On Thursday 19 February 2015 08:35:46 Vaishali Thakkar wrote:
> On Feb 19, 2015 4:01 AM, "Jes Sorensen" <jes.sorensen@gmail.com> wrote:
> >
> > The key is to make the changes in two commits. In general you can post
> > them as part of a multi-commit patch set if you are fixing something in
> > a large number of places, or changing an API etc. For more independent
> > changes like these, posting them individually is preferred.
> >
> > I hope this makes more sense.
> 
> Yes. Ok. I will send this patch as a series of patches. One for
> put_unaligned_le16 function and other one for removing use of variable.

There are lots of ways for splitting a patch series, and usually doing
a multiple smaller changes is better than combining them, but there is a
limit. In this case you have four changes that could be done separately:

a) use put_unaligned_le16 in rtl8188eu
b) remove tim_bitmap_le variable in rtl8188eu
c) use put_unaligned_le16 in rtl8723au
d) remove tim_bitmap_le variable in rtl8723au

What Jes and I were saying is that you should do a) combined with b) as
one patch, and c) with d) as a second patch, but send them as a series,
not a) and c) combined as one patch followed by b) and d) as you understood.

There are other times when it makes sense to combine things differently,
e.g. when you do the exact same one-line change in 50 drivers, you should
not send 50 patches but rather group them by maintainers and send one
patch per recipient, or even a large combined patch.

	Arnd

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

[Vaishali Thakkar]

On Thu, Feb 19, 2015 at 1:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 19 February 2015 08:35:46 Vaishali Thakkar wrote:
>> On Feb 19, 2015 4:01 AM, "Jes Sorensen" <jes.sorensen@gmail.com> wrote:
>> >
>> > The key is to make the changes in two commits. In general you can post
>> > them as part of a multi-commit patch set if you are fixing something in
>> > a large number of places, or changing an API etc. For more independent
>> > changes like these, posting them individually is preferred.
>> >
>> > I hope this makes more sense.
>>
>> Yes. Ok. I will send this patch as a series of patches. One for
>> put_unaligned_le16 function and other one for removing use of variable.
>
> There are lots of ways for splitting a patch series, and usually doing
> a multiple smaller changes is better than combining them, but there is a
> limit. In this case you have four changes that could be done separately:
>
> a) use put_unaligned_le16 in rtl8188eu
> b) remove tim_bitmap_le variable in rtl8188eu
> c) use put_unaligned_le16 in rtl8723au
> d) remove tim_bitmap_le variable in rtl8723au
>
> What Jes and I were saying is that you should do a) combined with b) as
> one patch, and c) with d) as a second patch, but send them as a series,
> not a) and c) combined as one patch followed by b) and d) as you understood.

Oh. ok. Actually there is one misunderstanding here. I already sent 2 separate
patches. First one combining a) and b) and second one combining c) and d).
But after Jes's comment what I thought that he wants me to send 2 patch-series
with 4 different patches where [ a) and b) ]goes for 1st patch-series
and [c) and d) ]
goes for 2nd patch-series. But thanks for clearing this.

I will send a patch-series[ 2 patches- one patch for rtl8188eu and
second for  rtl8723au]
as you explained.

> There are other times when it makes sense to combine things differently,
> e.g. when you do the exact same one-line change in 50 drivers, you should
> not send 50 patches but rather group them by maintainers and send one
> patch per recipient, or even a large combined patch.
>
>         Arnd



-- 
Vaishali

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

[Jes Sorensen]

On 02/19/15 03:43, Vaishali Thakkar wrote:
> On Thu, Feb 19, 2015 at 1:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thursday 19 February 2015 08:35:46 Vaishali Thakkar wrote:
>>> On Feb 19, 2015 4:01 AM, "Jes Sorensen" <jes.sorensen@gmail.com> wrote:
>>>>
>>>> The key is to make the changes in two commits. In general you can post
>>>> them as part of a multi-commit patch set if you are fixing something in
>>>> a large number of places, or changing an API etc. For more independent
>>>> changes like these, posting them individually is preferred.
>>>>
>>>> I hope this makes more sense.
>>>
>>> Yes. Ok. I will send this patch as a series of patches. One for
>>> put_unaligned_le16 function and other one for removing use of variable.
>>
>> There are lots of ways for splitting a patch series, and usually doing
>> a multiple smaller changes is better than combining them, but there is a
>> limit. In this case you have four changes that could be done separately:
>>
>> a) use put_unaligned_le16 in rtl8188eu
>> b) remove tim_bitmap_le variable in rtl8188eu
>> c) use put_unaligned_le16 in rtl8723au
>> d) remove tim_bitmap_le variable in rtl8723au
>>
>> What Jes and I were saying is that you should do a) combined with b) as
>> one patch, and c) with d) as a second patch, but send them as a series,
>> not a) and c) combined as one patch followed by b) and d) as you understood.
> 
> Oh. ok. Actually there is one misunderstanding here. I already sent 2 separate
> patches. First one combining a) and b) and second one combining c) and d).
> But after Jes's comment what I thought that he wants me to send 2 patch-series
> with 4 different patches where [ a) and b) ]goes for 1st patch-series
> and [c) and d) ]
> goes for 2nd patch-series. But thanks for clearing this.
> 
> I will send a patch-series[ 2 patches- one patch for rtl8188eu and
> second for  rtl8723au]
> as you explained.

Sorry for being ambiguous on this one, Arnd is absolutely right. The a+b
c+d solution is the right way - you are basically fixing up two small
closely related issues, so it is best to keep them together in the same
patch.

Cheers,
Jes