linux-kernel - Re: [PATCH] staging: rtl8723bs: Use min/max macros for variable comparison

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.22.394.2211030854260.17767@hadrien>
Date:   Thu, 3 Nov 2022 08:55:32 +0100 (CET)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Deepak R Varma <drv@...lo.com>
cc:     outreachy@...ts.linux.dev,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: Use min/max macros for variable
 comparison



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@...lo.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
>
>
>
>
>