[PATCH] staging: r8188eu: combine nested if statements into one

[PATCH] staging: r8188eu: combine nested if statements into one

[Chang Yu]

Combine two nested if statements into a single one

Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
index 599d3e55be76..c7564af39023 100644
--- a/drivers/staging/r8188eu/core/rtw_recv.c
+++ b/drivers/staging/r8188eu/core/rtw_recv.c
@@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
 
 	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
 
-	if (padapter) {
-		if (pfree_recv_queue == &precvpriv->free_recv_queue)
-				precvpriv->free_recvframe_cnt++;
-	}
+	if (padapter && pfree_recv_queue == &precvpriv->free_recv_queue)
+		precvpriv->free_recvframe_cnt++;
 
 	spin_unlock_bh(&pfree_recv_queue->lock);
 
-- 
2.36.1

Re: [PATCH] staging: r8188eu: combine nested if statements into one

[Philipp Hortmann]

On 6/23/22 05:15, Chang Yu wrote:
> -	if (padapter) {
> -		if (pfree_recv_queue == &precvpriv->free_recv_queue)
> -				precvpriv->free_recvframe_cnt++;
> -	}
> +	if (padapter && pfree_recv_queue == &precvpriv->free_recv_queue)
> +		precvpriv->free_recvframe_cnt++;

Hi

tested with:
#include <stdio.h>
int main() {
     char padapter = 1;
     int pfree_recv_queue = 256;
     int free_recv_queue = 256;

     if (padapter) {
     	if (pfree_recv_queue == free_recv_queue)
             printf("Executed before patch: 
precvpriv->free_recvframe_cnt++;\n");
     }

     if (padapter && pfree_recv_queue == free_recv_queue)
		printf("Executed after patch: precvpriv->free_recvframe_cnt++;\n");

    return 0;
}

Seems to work. But the rules which operation is done first && or == are 
not too easy. I would prefer to have:

if (padapter && (pfree_recv_queue == free_recv_queue))

So it is very easy to read what is evaluated first.

But this is just my opinion and does not have to be right.

Thanks for your patch.

Bye Philipp

[PATCH v2] staging: r8188eu: combine nested if statements into one

[Chang Yu]

Combine two nested if statements into a single one

Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
---
Changes in v2:
Added a pair of parentheses to make operator precedence explicit.

 drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
index 6564e82ddd66..020bc212532f 100644
--- a/drivers/staging/r8188eu/core/rtw_recv.c
+++ b/drivers/staging/r8188eu/core/rtw_recv.c
@@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
 
 	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
 
-	if (padapter) {
-		if (pfree_recv_queue == &precvpriv->free_recv_queue)
-				precvpriv->free_recvframe_cnt++;
-	}
+	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
+		precvpriv->free_recvframe_cnt++;
 
 	spin_unlock_bh(&pfree_recv_queue->lock);
 
-- 
2.36.1

Re: [PATCH v2] staging: r8188eu: combine nested if statements into one

[Philipp Hortmann]

On 6/23/22 07:14, Chang Yu wrote:
> Combine two nested if statements into a single one
> 
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> ---
> Changes in v2:
> Added a pair of parentheses to make operator precedence explicit.
> 
>   drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> index 6564e82ddd66..020bc212532f 100644
> --- a/drivers/staging/r8188eu/core/rtw_recv.c
> +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> @@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
>   
>   	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
>   
> -	if (padapter) {
> -		if (pfree_recv_queue == &precvpriv->free_recv_queue)
> -				precvpriv->free_recvframe_cnt++;
> -	}
> +	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
> +		precvpriv->free_recvframe_cnt++;
>   
>   	spin_unlock_bh(&pfree_recv_queue->lock);
>   


Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150

Re: [PATCH v2] staging: r8188eu: combine nested if statements into one

[Greg KH]

On Wed, Jun 22, 2022 at 10:14:04PM -0700, Chang Yu wrote:
> Combine two nested if statements into a single one
> 
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> ---
> Changes in v2:
> Added a pair of parentheses to make operator precedence explicit.
> 
>  drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> index 6564e82ddd66..020bc212532f 100644
> --- a/drivers/staging/r8188eu/core/rtw_recv.c
> +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> @@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
>  
>  	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
>  
> -	if (padapter) {
> -		if (pfree_recv_queue == &precvpriv->free_recv_queue)
> -				precvpriv->free_recvframe_cnt++;
> -	}
> +	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
> +		precvpriv->free_recvframe_cnt++;
>  
>  	spin_unlock_bh(&pfree_recv_queue->lock);
>  
> -- 
> 2.36.1
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what is needed in order to
  properly describe the change.

- You did not write a descriptive Subject: for the patch, allowing Greg,
  and everyone else, to know what this patch is all about.  Please read
  the section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what a proper Subject: line should
  look like.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

RE: [PATCH] staging: r8188eu: combine nested if statements into one

[David Laight]

...
> Seems to work. But the rules which operation is done first && or == are
> not too easy.

They are the way around you want them to be.
== generates a truth value.
&& and || compare truth values,

The only 'wrong' operator priorities are & and |.
The short-circuiting && and || weren't in the very early
versions of C - the bitwise & and | were used.
When K&R added && and || they left the priorities of & an | alone.
I they they've later said they should have bitten the bullet
and changed the priorities and all the existing C code

> I would prefer to have:
> 
> if (padapter && (pfree_recv_queue == free_recv_queue))
> 
> So it is very easy to read what is evaluated first.

That just starts adding too many () and makes more complex
conditionals hard to read.

	David

> 
> But this is just my opinion and does not have to be right.
> 
> Thanks for your patch.
> 
> Bye Philipp

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] staging: r8188eu: combine nested if statements into one

[Chang Yu]

On Thu, Jun 23, 2022 at 12:05:40PM +0000, David Laight wrote:
> ...
> > Seems to work. But the rules which operation is done first && or == are
> > not too easy.
> 
> They are the way around you want them to be.
> == generates a truth value.
> && and || compare truth values,
> 
> The only 'wrong' operator priorities are & and |.
> The short-circuiting && and || weren't in the very early
> versions of C - the bitwise & and | were used.
> When K&R added && and || they left the priorities of & an | alone.
> I they they've later said they should have bitten the bullet
> and changed the priorities and all the existing C code
> 
> > I would prefer to have:
> > 
> > if (padapter && (pfree_recv_queue == free_recv_queue))
> > 
> > So it is very easy to read what is evaluated first.
> 
> That just starts adding too many () and makes more complex
> conditionals hard to read.
> 
> 	David
> 
> > 
> > But this is just my opinion and does not have to be right.
> > 
> > Thanks for your patch.
> > 
> > Bye Philipp
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

In my humble opinion it just boils down to personal preference in this
case. The kernel coding style guidlines don't seem to have a definitive
gold standard regarding this. I will leave the patch as-is for now, but
if anybody feels strongly that the () needs to be removed please feel
free to let me know and I'll make the change.

Re: [PATCH v2] staging: r8188eu: combine nested if statements into one

[Chang Yu]

On Thu, Jun 23, 2022 at 11:45:07AM +0200, Greg KH wrote:
> On Wed, Jun 22, 2022 at 10:14:04PM -0700, Chang Yu wrote:
> > Combine two nested if statements into a single one
> > 
> > Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> > ---
> > Changes in v2:
> > Added a pair of parentheses to make operator precedence explicit.
> > 
> >  drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> > index 6564e82ddd66..020bc212532f 100644
> > --- a/drivers/staging/r8188eu/core/rtw_recv.c
> > +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> > @@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
> >  
> >  	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
> >  
> > -	if (padapter) {
> > -		if (pfree_recv_queue == &precvpriv->free_recv_queue)
> > -				precvpriv->free_recvframe_cnt++;
> > -	}
> > +	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
> > +		precvpriv->free_recvframe_cnt++;
> >  
> >  	spin_unlock_bh(&pfree_recv_queue->lock);
> >  
> > -- 
> > 2.36.1
> > 
> > 
> 
> Hi,
> 
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
> 
> You are receiving this message because of the following common error(s)
> as indicated below:
> 
> - You did not specify a description of why the patch is needed, or
>   possibly, any description at all, in the email body.  Please read the
>   section entitled "The canonical patch format" in the kernel file,
>   Documentation/SubmittingPatches for what is needed in order to
>   properly describe the change.
> 
> - You did not write a descriptive Subject: for the patch, allowing Greg,
>   and everyone else, to know what this patch is all about.  Please read
>   the section entitled "The canonical patch format" in the kernel file,
>   Documentation/SubmittingPatches for what a proper Subject: line should
>   look like.
> 
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.
> 
> thanks,
> 
> greg k-h's patch email bot

I'm not entirely sure how to fix this. I checked the original patch
again and the subject and the body looks OK to me. I'm still a newbie so
I might have missed a couple of things. It would be greatly appreciated
if someone could point out what's missing.

Re: [PATCH] staging: r8188eu: combine nested if statements into one

[Philipp Hortmann]

On 6/24/22 05:30, Chang Yu wrote:
> On Thu, Jun 23, 2022 at 12:05:40PM +0000, David Laight wrote:
>> ...
>>> Seems to work. But the rules which operation is done first && or == are
>>> not too easy.
>>
>> They are the way around you want them to be.
>> == generates a truth value.
>> && and || compare truth values,
>>
>> The only 'wrong' operator priorities are & and |.
>> The short-circuiting && and || weren't in the very early
>> versions of C - the bitwise & and | were used.
>> When K&R added && and || they left the priorities of & an | alone.
>> I they they've later said they should have bitten the bullet
>> and changed the priorities and all the existing C code
>>
>>> I would prefer to have:
>>>
>>> if (padapter && (pfree_recv_queue == free_recv_queue))
>>>
>>> So it is very easy to read what is evaluated first.
>>
>> That just starts adding too many () and makes more complex
>> conditionals hard to read.
>>
>> 	David
>>
>>>
>>> But this is just my opinion and does not have to be right.
>>>
>>> Thanks for your patch.
>>>
>>> Bye Philipp
>>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
>> Registration No: 1397386 (Wales)
> 
> In my humble opinion it just boils down to personal preference in this
> case. The kernel coding style guidlines don't seem to have a definitive
> gold standard regarding this. I will leave the patch as-is for now, but
> if anybody feels strongly that the () needs to be removed please feel
> free to let me know and I'll make the change.

Please consider that the person you want to sign-of for this patch is 
most likely Greg K-H. I propose to fix first what he is asking for.

Bye Philipp

Re: [PATCH v2] staging: r8188eu: combine nested if statements into one

[Dan Carpenter]

On Thu, Jun 23, 2022 at 08:34:54PM -0700, Chang Yu wrote:
> > - You did not specify a description of why the patch is needed,
>
> I'm not entirely sure how to fix this. I checked the original patch
> again and the subject and the body looks OK to me. I'm still a newbie so
> I might have missed a couple of things. It would be greatly appreciated
> if someone could point out what's missing.

What's the advantage of combining if statements?  Out of all the if
statements in the kernel why did you pick that one?  Probably it's
because the indenting was wrong, no?

Write the commit message like this:

[PATCH v3] staging: r8188eu: clean up if statement

I noticed that the if statement was strange and the code was indented
too far.  It is cleaner to combine both if statements as well.

regards,
dan carpenter

Re: [PATCH v2] staging: r8188eu: combine nested if statements into one

[Philipp Hortmann]

On 6/24/22 05:34, Chang Yu wrote:
> On Thu, Jun 23, 2022 at 11:45:07AM +0200, Greg KH wrote:
>> On Wed, Jun 22, 2022 at 10:14:04PM -0700, Chang Yu wrote:
>>> Combine two nested if statements into a single one
>>>
>>> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
>>> ---
>>> Changes in v2:
>>> Added a pair of parentheses to make operator precedence explicit.
>>>
>>>   drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
>>> index 6564e82ddd66..020bc212532f 100644
>>> --- a/drivers/staging/r8188eu/core/rtw_recv.c
>>> +++ b/drivers/staging/r8188eu/core/rtw_recv.c
>>> @@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
>>>   
>>>   	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
>>>   
>>> -	if (padapter) {
>>> -		if (pfree_recv_queue == &precvpriv->free_recv_queue)
>>> -				precvpriv->free_recvframe_cnt++;
>>> -	}
>>> +	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
>>> +		precvpriv->free_recvframe_cnt++;
>>>   
>>>   	spin_unlock_bh(&pfree_recv_queue->lock);
>>>   
>>> -- 
>>> 2.36.1
>>>
>>>
>>
>> Hi,
>>
>> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
>> a patch that has triggered this response.  He used to manually respond
>> to these common problems, but in order to save his sanity (he kept
>> writing the same thing over and over, yet to different people), I was
>> created.  Hopefully you will not take offence and will fix the problem
>> in your patch and resubmit it so that it can be accepted into the Linux
>> kernel tree.
>>
>> You are receiving this message because of the following common error(s)
>> as indicated below:
>>
>> - You did not specify a description of why the patch is needed, or
>>    possibly, any description at all, in the email body.  Please read the
>>    section entitled "The canonical patch format" in the kernel file,
>>    Documentation/SubmittingPatches for what is needed in order to
>>    properly describe the change.
>>
>> - You did not write a descriptive Subject: for the patch, allowing Greg,
>>    and everyone else, to know what this patch is all about.  Please read
>>    the section entitled "The canonical patch format" in the kernel file,
>>    Documentation/SubmittingPatches for what a proper Subject: line should
>>    look like.
>>
>> If you wish to discuss this problem further, or you have questions about
>> how to resolve this issue, please feel free to respond to this email and
>> Greg will reply once he has dug out from the pending patches received
>> from other developers.
>>
>> thanks,
>>
>> greg k-h's patch email bot
> 
> I'm not entirely sure how to fix this. I checked the original patch
> again and the subject and the body looks OK to me. I'm still a newbie so
> I might have missed a couple of things. It would be greatly appreciated
> if someone could point out what's missing.
> 

description:
You wrote what you did in the description. Even when the why can be 
likely answered as well it is not sufficient for Greg K-H.

I propose something like:
Combine two nested if statements into a single one to increase readability.

Or

Combine two nested if statements into a single one to shorten code.

subject:
I am guessing. The subject could may be remain but I think it is to 
general. Please consider that we can have multiple of this subjects what 
is not good. How to know which patch is which?

I propose something like:
staging: r8188eu: combine nested if statements in function xxxx

Or

staging: r8188eu: combine nested if statements in file xxxx


But consider that the patches that were accepted do also have a not so 
specific subject. The description was very clear about the "why". There 
the reason was always checkpatch.

Bye Philipp

Re: [PATCH v2] staging: r8188eu: combine nested if statements into one

[Chang Yu]

On Fri, Jun 24, 2022 at 08:39:30AM +0300, Dan Carpenter wrote:
> On Thu, Jun 23, 2022 at 08:34:54PM -0700, Chang Yu wrote:
> > > - You did not specify a description of why the patch is needed,
> >
> > I'm not entirely sure how to fix this. I checked the original patch
> > again and the subject and the body looks OK to me. I'm still a newbie so
> > I might have missed a couple of things. It would be greatly appreciated
> > if someone could point out what's missing.
> 
> What's the advantage of combining if statements?  Out of all the if
> statements in the kernel why did you pick that one?  Probably it's
> because the indenting was wrong, no?
> 
> Write the commit message like this:
> 
> [PATCH v3] staging: r8188eu: clean up if statement
> 
> I noticed that the if statement was strange and the code was indented
> too far.  It is cleaner to combine both if statements as well.
> 
> regards,
> dan carpenter

Understood. Thank you for pointing this out to me. I didn't realize the
description was too general. I simply assumed that the reason for
combining if statements is obvious and did not elaborate. I see now this
is not the right assumption to make. I will revise the patch shortly.

Thank you!

Best,
Chang

Re: [PATCH v2] staging: r8188eu: combine nested if statements into one

[Chang Yu]

On Fri, Jun 24, 2022 at 07:47:12AM +0200, Philipp Hortmann wrote:
> On 6/24/22 05:34, Chang Yu wrote:
> > On Thu, Jun 23, 2022 at 11:45:07AM +0200, Greg KH wrote:
> > > On Wed, Jun 22, 2022 at 10:14:04PM -0700, Chang Yu wrote:
> > > > Combine two nested if statements into a single one
> > > > 
> > > > Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > Added a pair of parentheses to make operator precedence explicit.
> > > > 
> > > >   drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
> > > >   1 file changed, 2 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> > > > index 6564e82ddd66..020bc212532f 100644
> > > > --- a/drivers/staging/r8188eu/core/rtw_recv.c
> > > > +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> > > > @@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
> > > >   	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
> > > > -	if (padapter) {
> > > > -		if (pfree_recv_queue == &precvpriv->free_recv_queue)
> > > > -				precvpriv->free_recvframe_cnt++;
> > > > -	}
> > > > +	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
> > > > +		precvpriv->free_recvframe_cnt++;
> > > >   	spin_unlock_bh(&pfree_recv_queue->lock);
> > > > -- 
> > > > 2.36.1
> > > > 
> > > > 
> > > 
> > > Hi,
> > > 
> > > This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> > > a patch that has triggered this response.  He used to manually respond
> > > to these common problems, but in order to save his sanity (he kept
> > > writing the same thing over and over, yet to different people), I was
> > > created.  Hopefully you will not take offence and will fix the problem
> > > in your patch and resubmit it so that it can be accepted into the Linux
> > > kernel tree.
> > > 
> > > You are receiving this message because of the following common error(s)
> > > as indicated below:
> > > 
> > > - You did not specify a description of why the patch is needed, or
> > >    possibly, any description at all, in the email body.  Please read the
> > >    section entitled "The canonical patch format" in the kernel file,
> > >    Documentation/SubmittingPatches for what is needed in order to
> > >    properly describe the change.
> > > 
> > > - You did not write a descriptive Subject: for the patch, allowing Greg,
> > >    and everyone else, to know what this patch is all about.  Please read
> > >    the section entitled "The canonical patch format" in the kernel file,
> > >    Documentation/SubmittingPatches for what a proper Subject: line should
> > >    look like.
> > > 
> > > If you wish to discuss this problem further, or you have questions about
> > > how to resolve this issue, please feel free to respond to this email and
> > > Greg will reply once he has dug out from the pending patches received
> > > from other developers.
> > > 
> > > thanks,
> > > 
> > > greg k-h's patch email bot
> > 
> > I'm not entirely sure how to fix this. I checked the original patch
> > again and the subject and the body looks OK to me. I'm still a newbie so
> > I might have missed a couple of things. It would be greatly appreciated
> > if someone could point out what's missing.
> > 
> 
> description:
> You wrote what you did in the description. Even when the why can be likely
> answered as well it is not sufficient for Greg K-H.
> 
> I propose something like:
> Combine two nested if statements into a single one to increase readability.
> 
> Or
> 
> Combine two nested if statements into a single one to shorten code.
> 
> subject:
> I am guessing. The subject could may be remain but I think it is to general.
> Please consider that we can have multiple of this subjects what is not good.
> How to know which patch is which?
> 
> I propose something like:
> staging: r8188eu: combine nested if statements in function xxxx
> 
> Or
> 
> staging: r8188eu: combine nested if statements in file xxxx
> 
> 
> But consider that the patches that were accepted do also have a not so
> specific subject. The description was very clear about the "why". There the
> reason was always checkpatch.
> 
> Bye Philipp
> 
> 
> 
> 
> 
> 
> 
> 

Thank you very much for the valuable input. I will reword the subject
and the description and re-send the patch momentarily.

[PATCH v3] staging: r8188eu: core/rtw_recv.c: clean up nested if statements

[Chang Yu]

Combine two nested if statements into a single one to fix indentation
issue and improve readability, as suggested by checkpatch.pl

Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
index 6564e82ddd66..020bc212532f 100644
--- a/drivers/staging/r8188eu/core/rtw_recv.c
+++ b/drivers/staging/r8188eu/core/rtw_recv.c
@@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
 
 	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
 
-	if (padapter) {
-		if (pfree_recv_queue == &precvpriv->free_recv_queue)
-				precvpriv->free_recvframe_cnt++;
-	}
+	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
+		precvpriv->free_recvframe_cnt++;
 
 	spin_unlock_bh(&pfree_recv_queue->lock);
 
-- 
2.36.1

Re: [PATCH v3] staging: r8188eu: core/rtw_recv.c: clean up nested if statements

[Greg KH]

On Thu, Jun 23, 2022 at 11:27:05PM -0700, Chang Yu wrote:
> Combine two nested if statements into a single one to fix indentation
> issue and improve readability, as suggested by checkpatch.pl
> 
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> index 6564e82ddd66..020bc212532f 100644
> --- a/drivers/staging/r8188eu/core/rtw_recv.c
> +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> @@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
>  
>  	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
>  
> -	if (padapter) {
> -		if (pfree_recv_queue == &precvpriv->free_recv_queue)
> -				precvpriv->free_recvframe_cnt++;
> -	}
> +	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
> +		precvpriv->free_recvframe_cnt++;
>  
>  	spin_unlock_bh(&pfree_recv_queue->lock);
>  
> -- 
> 2.36.1
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH v4] staging: r8188eu: core/rtw_recv.c: clean up nested if statements

[Chang Yu]

Combine two nested if statements into a single one to fix indentation
issue and improve readability, as suggested by checkpatch.pl

Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
---
Changes in v4:
 - Added missing change log and resend

Changes in v3:
 - Modified subject and description to be more descriptive

Changes in v2:
 - Added a pair of parentheses to make operator precedence explicit


 drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
index 6564e82ddd66..020bc212532f 100644
--- a/drivers/staging/r8188eu/core/rtw_recv.c
+++ b/drivers/staging/r8188eu/core/rtw_recv.c
@@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
 
 	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
 
-	if (padapter) {
-		if (pfree_recv_queue == &precvpriv->free_recv_queue)
-				precvpriv->free_recvframe_cnt++;
-	}
+	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
+		precvpriv->free_recvframe_cnt++;
 
 	spin_unlock_bh(&pfree_recv_queue->lock);
 
-- 
2.36.1

Re: [PATCH v4] staging: r8188eu: core/rtw_recv.c: clean up nested if statements

[Philipp Hortmann]

On 6/24/22 16:45, Chang Yu wrote:
> Combine two nested if statements into a single one to fix indentation
> issue and improve readability, as suggested by checkpatch.pl
> 
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> ---
> Changes in v4:
>   - Added missing change log and resend
> 
> Changes in v3:
>   - Modified subject and description to be more descriptive
> 
> Changes in v2:
>   - Added a pair of parentheses to make operator precedence explicit
> 
> 
>   drivers/staging/r8188eu/core/rtw_recv.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> index 6564e82ddd66..020bc212532f 100644
> --- a/drivers/staging/r8188eu/core/rtw_recv.c
> +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> @@ -166,10 +166,8 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
>   
>   	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
>   
> -	if (padapter) {
> -		if (pfree_recv_queue == &precvpriv->free_recv_queue)
> -				precvpriv->free_recvframe_cnt++;
> -	}
> +	if (padapter && (pfree_recv_queue == &precvpriv->free_recv_queue))
> +		precvpriv->free_recvframe_cnt++;
>   
>   	spin_unlock_bh(&pfree_recv_queue->lock);
>   

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150