[PATCH 0/2] staging: rtl8192u: use min/max macros

[PATCH 0/2] staging: rtl8192u: use min/max macros

[Rebecca Mckeever]

These patches replace ternary and if else statements with an
equivalent min/max macro. Found with minmax coccinelle script.

---
I noticed that ieee80211.h includes linux/kernel.h and linux/jiffies.h,
which both include linux/minmax.h. Would it be a good idea to add
#include <linux/minmax.h>
in ieee80211.h in case the content of linux/kernel.h or linux/jiffies.h
changes?
---

Rebecca Mckeever (2):
  staging: rtl8192u: use max macro instead of ternary operator
  staging: rtl8192u: use min_t/max_t macros instead of if else

 drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c   |  2 +-
 drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
 2 files changed, 6 insertions(+), 9 deletions(-)

-- 
2.32.0

[PATCH 1/2] staging: rtl8192u: use max macro instead of ternary operator

[Rebecca Mckeever]

Replace ternary operator with max macro to increase readability
and conform to Linux kernel coding style.
Found with minmax coccinelle script.

Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
index 78cc8f357bbc..a10c1303695b 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
@@ -470,7 +470,7 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
 		return 0;
 	}
 	len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
-	erq->length = (len >= 0 ? len : 0);
+	erq->length = max(len, 0);
 
 	erq->flags |= IW_ENCODE_ENABLED;
 
-- 
2.32.0

Re: [PATCH 1/2] staging: rtl8192u: use max macro instead of ternary operator

[Dan Carpenter]

On Wed, Apr 06, 2022 at 07:09:43PM -0500, Rebecca Mckeever wrote:
> Replace ternary operator with max macro to increase readability
> and conform to Linux kernel coding style.
> Found with minmax coccinelle script.
> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> ---
>  drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> index 78cc8f357bbc..a10c1303695b 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> @@ -470,7 +470,7 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
>  		return 0;
>  	}
>  	len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> -	erq->length = (len >= 0 ? len : 0);
> +	erq->length = max(len, 0);

Neither before nor after is really readable.  It would be better to
write it as:

	len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
	if (len < 0)
		len = 0;
	erq->length = len;

There are few rules/patterns that apply here:
1) Do error handling right away.
2) Do error handling not success handling.
3) Try to keep the error path and the success path separate.
4) Try to keep the success path indented one tab and the error path
   indented two tabs.  It looks like this to the reader:

	a = frob();
	if (fail)
		cleanup;
	b = frob();
	if (fail)
		cleanup;


	success
	check
		fail
	success
	check
		fail

So then if you want to understand what a function does at a high level
you can just skim the code which is indented one tab and ignore the
error handling code.

regards,
dan carpenter

[PATCH 2/2] staging: rtl8192u: use min_t/max_t macros instead of if else

[Rebecca Mckeever]

Replace if else statement with min_t or max_t macros to increase
readability and conform to Linux kernel coding style. The _t versions
of the macros must be used to avoid applying typeof to the bit fields
pPeerHTCap->MaxRxAMPDUFactor, and pPeerHTCap->MPDUDensity.
Found with minmax coccinelle script.

Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
---
 drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
index dba3f2db9f48..8f2cd669c5d4 100644
--- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
+++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
@@ -940,10 +940,8 @@ void HTOnAssocRsp(struct ieee80211_device *ieee)
 			else
 				pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K;
 		} else {
-			if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K)
-				pHTInfo->CurrentAMPDUFactor = pPeerHTCap->MaxRxAMPDUFactor;
-			else
-				pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K;
+			pHTInfo->CurrentAMPDUFactor = min_t(u8, pPeerHTCap->MaxRxAMPDUFactor,
+							    HT_AGG_SIZE_32K);
 		}
 	}
 
@@ -951,10 +949,9 @@ void HTOnAssocRsp(struct ieee80211_device *ieee)
 	 * <2> Set AMPDU Minimum MPDU Start Spacing
 	 * 802.11n 3.0 section 9.7d.3
 	 */
-	if (pHTInfo->MPDU_Density > pPeerHTCap->MPDUDensity)
-		pHTInfo->CurrentMPDUDensity = pHTInfo->MPDU_Density;
-	else
-		pHTInfo->CurrentMPDUDensity = pPeerHTCap->MPDUDensity;
+	pHTInfo->CurrentMPDUDensity = max_t(u8, pHTInfo->MPDU_Density,
+					    pPeerHTCap->MPDUDensity);
+
 	if (ieee->pairwise_key_type != KEY_TYPE_NA)
 		pHTInfo->CurrentMPDUDensity	= 7; // 8us
 	// Force TX AMSDU
-- 
2.32.0

Re: [PATCH 2/2] staging: rtl8192u: use min_t/max_t macros instead of if else

[Dan Carpenter]

On Wed, Apr 06, 2022 at 07:09:44PM -0500, Rebecca Mckeever wrote:
> Replace if else statement with min_t or max_t macros to increase
> readability and conform to Linux kernel coding style. The _t versions
> of the macros must be used to avoid applying typeof to the bit fields
> pPeerHTCap->MaxRxAMPDUFactor, and pPeerHTCap->MPDUDensity.
> Found with minmax coccinelle script.
> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> ---
>  drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> index dba3f2db9f48..8f2cd669c5d4 100644
> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> @@ -940,10 +940,8 @@ void HTOnAssocRsp(struct ieee80211_device *ieee)
>  			else
>  				pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_64K;
>  		} else {
> -			if (pPeerHTCap->MaxRxAMPDUFactor < HT_AGG_SIZE_32K)
> -				pHTInfo->CurrentAMPDUFactor = pPeerHTCap->MaxRxAMPDUFactor;
> -			else
> -				pHTInfo->CurrentAMPDUFactor = HT_AGG_SIZE_32K;
> +			pHTInfo->CurrentAMPDUFactor = min_t(u8, pPeerHTCap->MaxRxAMPDUFactor,
> +							    HT_AGG_SIZE_32K);

The u8 cast doesn't cause a problem, but it makes me nervous so I have
to look up the types involved.

pPeerHTCap->MaxRxAMPDUFactor is 2 bits of a u8.
HT_AGG_SIZE_32K is an enum, which in real life terms of the compilers we
use is going to be a uint.

With min_t() you want to do the casts to a large enough unsigned type
so that it doesn't truncate.  And so u8 works.  But it would be less
scary if you cast it to a u32 or uint because a read will assume that
probably pPeerHTCap->MaxRxAMPDUFactor is a u32 and HT_AGG_SIZE_32K is an
int.  These are false assumptions, but probably that's what most people
will assume.

The reader in this case will feel safe and not worry about truncation so
they won't have to look up the types.

regards,
dan carpenter