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

From: Julia Lawall <julia.lawall@inria.fr>
To: Deepak Varma <mh12gx2825@gmail.com>
Cc: outreachy-kernel <outreachy-kernel@googlegroups.com>
Subject: Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
Date: Wed, 11 Mar 2020 21:26:42 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.21.2003112125030.6093@hadrien> (raw)
In-Reply-To: <92b5d69a-d4e6-480d-82ef-3a01b893fbd0@googlegroups.com>

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

On Wed, 11 Mar 2020, Deepak Varma wrote:

>
>
> On Wednesday, 11 March 2020 19:29:13 UTC+5:30, 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....@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
>
>  Juia, can you comment if the 2 commented lines [with '+' sign now] may also
> be removed to further simplify the code?

I feel uneasy about the whole change, without having more knowledge of the
device.  Someone already suggested that the code should be removed, but it
seems they didn't do it.  Maybe they didn't do it for a reason...

On the other hand, if the "if" is useless, the comments don't seem useful
either, since they applied to only one of the branches.

julia

>
> Deepak.
>
> --
> 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 visithttps://groups.google.com/d/msgid/outreachy-kernel/92b5d69a-d4e6-480d-82ef-
> 3a01b893fbd0%40googlegroups.com.
>
>