[Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions

[Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions

[Shreeya Patel]

Remove unnecessary if and else conditions since both are leading to the
initialization of "phtpriv->ampdu_enable" with the same value.

Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_mlme.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
index 71fcb466019a..48e9faf27321 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
@@ -2772,13 +2772,9 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 *pie, uint ie_len, u8 channe
 
 	/* maybe needs check if ap supports rx ampdu. */
 	if (!(phtpriv->ampdu_enable) && pregistrypriv->ampdu_enable == 1) {
-		if (pregistrypriv->wifi_spec == 1) {
-			/* remove this part because testbed AP should disable RX AMPDU */
-			/* phtpriv->ampdu_enable = false; */
-			phtpriv->ampdu_enable = true;
-		} else {
-			phtpriv->ampdu_enable = true;
-		}
+		/* remove this part because testbed AP should disable RX AMPDU */
+		/* phtpriv->ampdu_enable = false; */
+		phtpriv->ampdu_enable = true;
 	} else if (pregistrypriv->ampdu_enable == 2) {
 		/* remove this part because testbed AP should disable RX AMPDU */
 		/* phtpriv->ampdu_enable = true; */
-- 
2.17.1

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions

[Lakshmi Ramasubramanian]

On 3/11/2020 6:58 AM, Shreeya Patel wrote:

> Remove unnecessary if and else conditions since both are leading to the
> initialization of "phtpriv->ampdu_enable" with the same value.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

Stating this based on the patch descriptions I have seen.
Others, please advise\correct me if I am wrong.

Patch description should state the problem first[1] and then describe 
how that is fixed in the given patch.

For example:

In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the 
same value in both if and else statements.

This patch removes this unnecessary if-else statement.


[1] Documentation\process\submitting-patches.rst
        2) Describe your changes

Thanks,
  -lakshmi

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions

[Stefano Brivio]

Hi Lakshmi,

On Wed, 11 Mar 2020 19:42:06 -0700
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote:

> On 3/11/2020 6:58 AM, Shreeya Patel wrote:
> 
> > Remove unnecessary if and else conditions since both are leading to the
> > initialization of "phtpriv->ampdu_enable" with the same value.
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>  
> 
> Stating this based on the patch descriptions I have seen.
> Others, please advise\correct me if I am wrong.
> 
> Patch description should state the problem first[1] and then describe 
> how that is fixed in the given patch.
> 
> For example:
> 
> In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the 
> same value in both if and else statements.
> 
> This patch removes this unnecessary if-else statement.

That's my general preference as well, but I can't find any point in the
"Describe your changes" section of submitting-patches.rst actually
defining the order. I wouldn't imply that from the sequence the steps
are presented in.

In case it's possible to say everything with a single statement as
Shreeya did here, though, I guess that becomes rather a linguistic
factor, and I personally prefer the concise version here.

-- 
Stefano

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions

[Julia Lawall]

On Thu, 12 Mar 2020, Stefano Brivio wrote:

> Hi Lakshmi,
>
> On Wed, 11 Mar 2020 19:42:06 -0700
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote:
>
> > On 3/11/2020 6:58 AM, Shreeya Patel wrote:
> >
> > > Remove unnecessary if and else conditions since both are leading to the
> > > initialization of "phtpriv->ampdu_enable" with the same value.
> > >
> > > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> >
> > Stating this based on the patch descriptions I have seen.
> > Others, please advise\correct me if I am wrong.
> >
> > Patch description should state the problem first[1] and then describe
> > how that is fixed in the given patch.
> >
> > For example:
> >
> > In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the
> > same value in both if and else statements.
> >
> > This patch removes this unnecessary if-else statement.
>
> That's my general preference as well, but I can't find any point in the
> "Describe your changes" section of submitting-patches.rst actually
> defining the order. I wouldn't imply that from the sequence the steps
> are presented in.
>
> In case it's possible to say everything with a single statement as
> Shreeya did here, though, I guess that becomes rather a linguistic
> factor, and I personally prefer the concise version here.

https://kernelnewbies.org/PatchPhilosophy suggests:

In patch descriptions and in the subject, it is common and preferable to
use present-tense, imperative language. Write as if you are telling git
what to do with your patch.

It provides the following as an example of a good description:

    somedriver: fix sleep while atomic in send_a_packet()

    The send_a_packet() function is called in atomic context but takes a mutex,
    causing a sleeping while atomic warning.  Change the skb_lock to be a spin
    lock instead of a mutex to fix.

So this illustrates the order that Lakshmi suggests, even though I don't
think that order is ever suggested explicitly.  On the other hand it
avoids "This patch...", which would add some clutter, in my opinion.

julia

>
> --
> Stefano
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200312113416.23d3db5c%40elisabeth.
>

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions

[Lakshmi Ramasubramanian]

On 3/12/2020 3:49 AM, Julia Lawall wrote:

Thanks for your input Julia and Stefano.

>> That's my general preference as well, but I can't find any point in the
>> "Describe your changes" section of submitting-patches.rst actually
>> defining the order. I wouldn't imply that from the sequence the steps
>> are presented in.
>>
>> In case it's possible to say everything with a single statement as
>> Shreeya did here, though, I guess that becomes rather a linguistic
>> factor, and I personally prefer the concise version here.
> 
> https://kernelnewbies.org/PatchPhilosophy suggests:
> 
> In patch descriptions and in the subject, it is common and preferable to
> use present-tense, imperative language. Write as if you are telling git
> what to do with your patch.

Use of imperative language is the approach I was thinking as well.

thanks,
  -lakshmi

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions

[Dan Carpenter]

The original patch description was basically fine.  Outreachy reviews
tend to be more pedantic about this sort of stuff than most maintainers.
There are a couple who have very strict rules, but try to avoid those
maintainers and your life will be happier.

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions

[Dan Carpenter]

On Wed, Mar 11, 2020 at 07:28:59PM +0530, Shreeya Patel wrote:
> Remove unnecessary if and else conditions since both are leading to the
> initialization of "phtpriv->ampdu_enable" with the same value.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 71fcb466019a..48e9faf27321 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -2772,13 +2772,9 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 *pie, uint ie_len, u8 channe
>  
>  	/* maybe needs check if ap supports rx ampdu. */
>  	if (!(phtpriv->ampdu_enable) && pregistrypriv->ampdu_enable == 1) {
> -		if (pregistrypriv->wifi_spec == 1) {
> -			/* remove this part because testbed AP should disable RX AMPDU */
> -			/* phtpriv->ampdu_enable = false; */
> -			phtpriv->ampdu_enable = true;
> -		} else {
> -			phtpriv->ampdu_enable = true;
> -		}
> +		/* remove this part because testbed AP should disable RX AMPDU */
> +		/* phtpriv->ampdu_enable = false; */

Please delete this dead code and the related comment.

regards,
dan carpenter