2022-11-03 07:11:27

by Deepak R Varma

Subject: [PATCH] staging: rtl8723bs: Use min/max macros for variable comparison

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 <[email protected]>
---

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));

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

2022-11-03 08:41:33

by Julia Lawall

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 <[email protected]>
> ---
>
> 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
>
>
>
>
>

2022-11-03 09:20:33

by Deepak R Varma

Subject: Re: [PATCH] staging: rtl8723bs: Use min/max macros for variable comparison

On Thu, Nov 03, 2022 at 08:55:32AM +0100, Julia Lawall wrote:
>
>
> 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 <[email protected]>
> > ---
> >
> > 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.

Sounds good. I will improve further and send in a revision.

Also, do you have a comment on why coccicheck did not catch the min opportunity
for max_AMPDU_len computation? I am not seeing anything different with the
if/else blocks here.

Thank you,
./drv

>
> 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
> >
> >
> >
> >
> >
>