Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Nov 2022 08:55:32 +0100 (CET)
From:   Julia Lawall <julia.lawall@inria.fr>
To:     Deepak R Varma <drv@mailo.com>
cc:     outreachy@lists.linux.dev,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: Use min/max macros for variable
 comparison
In-Reply-To: <Y2NncTaRJhLvJ4fV@qemulion>
Message-ID: <alpine.DEB.2.22.394.2211030854260.17767@hadrien>
References: <Y2NncTaRJhLvJ4fV@qemulion>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk


On Thu, 3 Nov 2022, Deepak R Varma wrote:

> Simplify code by using min and max helper macros in place of lengthy
> if/else block oriented logical evaluation and value assignment. This
> issue is identified by coccicheck using the minmax.cocci file.
>
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
>
> Please note:
>    1. Using min for max_AMPDU_len computation warning was NOT auto generated by
>       the cocciecheck command. This was caught while surround code review and
>       was manually changed.
>    2. Checkpatch script continues to complaint about min_MPDU_spacing
>       computation line being more than 100 character in length. I did not find a
>       better formatting that will address this checkpatch warning. Any
>       suggestions are most welcome.
>    3. Proposed changes are compile tested only on my x86 based VM.
>
>
>  drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 12 ++++--------
>  drivers/staging/rtl8723bs/hal/odm_DIG.c        |  5 +----
>  2 files changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> index 18ba846c0b7b..dcda587b84bc 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> @@ -986,15 +986,11 @@ void HT_caps_handler(struct adapter *padapter, struct ndis_80211_var_ie *pIE)
>  			pmlmeinfo->HT_caps.u.HT_cap[i] &= (pIE->data[i]);
>  		} else {
>  			/* modify from  fw by Thomas 2010/11/17 */
> -			if ((pmlmeinfo->HT_caps.u.HT_cap_element.AMPDU_para & 0x3) > (pIE->data[i] & 0x3))
> -				max_AMPDU_len = (pIE->data[i] & 0x3);
> -			else
> -				max_AMPDU_len = (pmlmeinfo->HT_caps.u.HT_cap_element.AMPDU_para & 0x3);
> +			max_AMPDU_len = min((pmlmeinfo->HT_caps.u.HT_cap_element.AMPDU_para & 0x3),
> +					    (pIE->data[i] & 0x3));
>
> -			if ((pmlmeinfo->HT_caps.u.HT_cap_element.AMPDU_para & 0x1c) > (pIE->data[i] & 0x1c))
> -				min_MPDU_spacing = (pmlmeinfo->HT_caps.u.HT_cap_element.AMPDU_para & 0x1c);
> -			else
> -				min_MPDU_spacing = (pIE->data[i] & 0x1c);
> +			min_MPDU_spacing = max((pmlmeinfo->HT_caps.u.HT_cap_element.AMPDU_para & 0x1c),
> +					       (pIE->data[i] & 0x1c));

There seem to be a lot of unnecessary parentheses.  Admittedly they were
there before, but since you are creating the max call from scratch in this
patch, perhaps the parentheses around the arguments could be dropped at
the same time.

julia

>
>  			pmlmeinfo->HT_caps.u.HT_cap_element.AMPDU_para = max_AMPDU_len | min_MPDU_spacing;
>  		}
> diff --git a/drivers/staging/rtl8723bs/hal/odm_DIG.c b/drivers/staging/rtl8723bs/hal/odm_DIG.c
> index 07edf74ccfe5..97a51546463a 100644
> --- a/drivers/staging/rtl8723bs/hal/odm_DIG.c
> +++ b/drivers/staging/rtl8723bs/hal/odm_DIG.c
> @@ -598,10 +598,7 @@ void odm_DIGbyRSSI_LPS(void *pDM_VOID)
>  	/* Lower bound checking */
>
>  	/* RSSI Lower bound check */
> -	if ((pDM_Odm->RSSI_Min-10) > DM_DIG_MIN_NIC)
> -		RSSI_Lower = pDM_Odm->RSSI_Min-10;
> -	else
> -		RSSI_Lower = DM_DIG_MIN_NIC;
> +	RSSI_Lower = max(pDM_Odm->RSSI_Min - 10, DM_DIG_MIN_NIC);
>
>  	/* Upper and Lower Bound checking */
>  	if (CurrentIGI > DM_DIG_MAX_NIC)
> --
> 2.34.1
>
>
>
>
>