staging: rtl8192u: use min/max macros

PATCH In review Rebecca Mckeever 2 participants 5 messages Apr 7, 2022 Staging

Share & tools

Rebecca Mckeever

author

2022-04-07 00:09 UTC #0 lore raw

cover staging: rtl8192u: use min/max macros

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

Signature · 1 line

2.32.0

Rebecca Mckeever

author

2022-04-07 00:09 UTC #1 lore raw

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

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

---
 drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +1 -1

index 78cc8f357bbc..a10c1303695b 100644
@@ -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;

Signature · 1 line

2.32.0

Rebecca Mckeever

author

2022-04-07 00:09 UTC #3 lore raw

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

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

---
 drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c +5 -8

index dba3f2db9f48..8f2cd669c5d4 100644
@@ -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

Signature · 1 line

2.32.0

Dan Carpenter

2022-04-07 07:28 UTC #2 lore raw

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

In reply to Rebecca Mckeever

Quoted 14 lines · 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 <[email protected]>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +0 -0
index 78cc8f357bbc..a10c1303695b 100644

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c int ieee80211_wx_get_encode(struct ieee80211_device *ieee, · line 473

Quoted 5 lines

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c +1 -1

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

Dan Carpenter

2022-04-07 07:50 UTC #4 lore raw

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

In reply to Rebecca Mckeever

Quoted 16 lines · 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 <[email protected]>
---
 drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c +0 -0
index dba3f2db9f48..8f2cd669c5d4 100644

drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c void HTOnAssocRsp(struct ieee80211_device *ieee) · line 944

Quoted 9 lines

drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c +2 -4

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