Skip to contentSkip to conversation

staging: wlan-ng: replace rate macros

PATCH Superseded Luke Koch 2 participants 3 messages Apr 15, 2023 Staging

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Luke Koch

author
#0 lore raw

PATCH 1/1 staging: wlan-ng: replace rate macros

Change p80211msg_dot11req_scan_results rate members to struct arrays instead of individually numbered member structs.
Replace macros to set rates with loops to avoid checkpatch warning and adhere to linux coding style.

Reported by checkpatch:

CHECK: Macro argument reuse 'N' - possible side-effects?

Signed off by: Luke Koch <[email protected]>

---
 drivers/staging/wlan-ng/p80211metastruct.h | 18 +-------
 drivers/staging/wlan-ng/prism2mgmt.c       | 52 +++++++---------------
 2 files changed, 18 insertions(+), 52 deletions(-)
drivers/staging/wlan-ng/p80211metastruct.h +2 -16
index 4adc64580185..e963227f797c 100644
@@ -114,22 +114,8 @@ struct p80211msg_dot11req_scan_results {
 	struct p80211item_uint32 cfpollreq;
 	struct p80211item_uint32 privacy;
 	struct p80211item_uint32 capinfo;
-	struct p80211item_uint32 basicrate1;
-	struct p80211item_uint32 basicrate2;
-	struct p80211item_uint32 basicrate3;
-	struct p80211item_uint32 basicrate4;
-	struct p80211item_uint32 basicrate5;
-	struct p80211item_uint32 basicrate6;
-	struct p80211item_uint32 basicrate7;
-	struct p80211item_uint32 basicrate8;
-	struct p80211item_uint32 supprate1;
-	struct p80211item_uint32 supprate2;
-	struct p80211item_uint32 supprate3;
-	struct p80211item_uint32 supprate4;
-	struct p80211item_uint32 supprate5;
-	struct p80211item_uint32 supprate6;
-	struct p80211item_uint32 supprate7;
-	struct p80211item_uint32 supprate8;
+	struct p80211item_uint32 basicrate[8];
+	struct p80211item_uint32 supprate[8];
 } __packed;
 
 struct p80211msg_dot11req_start {
drivers/staging/wlan-ng/prism2mgmt.c +16 -36

58 lines not on this page · raw message

Signature · 1 line
2.34.1
#1 lore raw
Quoted 4 lines · Luke Koch

Change p80211msg_dot11req_scan_results rate members to struct arrays instead of individually numbered member structs.
Replace macros to set rates with loops to avoid checkpatch warning and adhere to linux coding style.

Writing every sentence in imperative tense makes people sound like space aliens in human skin... You can if you want to but it's not a requirement in staging. There are two maintainers who have that requirement and the Outreachy people.

Quoted 16 lines

Reported by checkpatch:

CHECK: Macro argument reuse 'N' - possible side-effects?

Signed off by: Luke Koch <[email protected]>

---
 drivers/staging/wlan-ng/p80211metastruct.h | 18 +-------
 drivers/staging/wlan-ng/prism2mgmt.c       | 52 +++++++---------------
 2 files changed, 18 insertions(+), 52 deletions(-)
drivers/staging/wlan-ng/p80211metastruct.h +0 -0
index 4adc64580185..e963227f797c 100644

drivers/staging/wlan-ng/p80211metastruct.h struct p80211msg_dot11req_scan_results { · line 117

Quoted 4 lines
drivers/staging/wlan-ng/p80211metastruct.h +0 -1
@@ -114,22 +114,8 @@ struct p80211msg_dot11req_scan_results {
 	struct p80211item_uint32 cfpollreq;
 	struct p80211item_uint32 privacy;
 	struct p80211item_uint32 capinfo;
-	struct p80211item_uint32 basicrate1;
                                 ^^^^^^^^^^

This starts at 1 instead of 0.

Quoted 55 lines
drivers/staging/wlan-ng/p80211metastruct.h +2 -15
-	struct p80211item_uint32 basicrate2;
-	struct p80211item_uint32 basicrate3;
-	struct p80211item_uint32 basicrate4;
-	struct p80211item_uint32 basicrate5;
-	struct p80211item_uint32 basicrate6;
-	struct p80211item_uint32 basicrate7;
-	struct p80211item_uint32 basicrate8;
-	struct p80211item_uint32 supprate1;
-	struct p80211item_uint32 supprate2;
-	struct p80211item_uint32 supprate3;
-	struct p80211item_uint32 supprate4;
-	struct p80211item_uint32 supprate5;
-	struct p80211item_uint32 supprate6;
-	struct p80211item_uint32 supprate7;
-	struct p80211item_uint32 supprate8;
+	struct p80211item_uint32 basicrate[8];
+	struct p80211item_uint32 supprate[8];
 } __packed;
 
 struct p80211msg_dot11req_start {
drivers/staging/wlan-ng/prism2mgmt.c +0 -27
index 9030a8939a9b..069c39c36753 100644
@@ -437,42 +437,22 @@ int prism2mgmt_scan_results(struct wlandevice *wlandev, void *msgp)
 		if (item->supprates[count] == 0)
 			break;
 
-#define REQBASICRATE(N) \
-	do { \
-		if ((count >= (N)) && DOT11_RATE5_ISBASIC_GET(	\
-			item->supprates[(N) - 1])) { \
-			req->basicrate ## N .data = item->supprates[(N) - 1]; \
-			req->basicrate ## N .status = \
-				P80211ENUM_msgitem_status_data_ok; \
-		} \
-	} while (0)
-
-	REQBASICRATE(1);
-	REQBASICRATE(2);
-	REQBASICRATE(3);
-	REQBASICRATE(4);
-	REQBASICRATE(5);
-	REQBASICRATE(6);
-	REQBASICRATE(7);
-	REQBASICRATE(8);
-
-#define REQSUPPRATE(N) \
-	do { \
-		if (count >= (N)) {					\
-			req->supprate ## N .data = item->supprates[(N) - 1]; \
-			req->supprate ## N .status = \
-				P80211ENUM_msgitem_status_data_ok; \
-		} \
-	} while (0)

drivers/staging/wlan-ng/prism2mgmt.c int prism2mgmt_scan_results(struct wlandevice *wlandev, void *msgp) · line 440

Quoted 10 lines
drivers/staging/wlan-ng/prism2mgmt.c +1 -9
-
-	REQSUPPRATE(1);
-	REQSUPPRATE(2);
-	REQSUPPRATE(3);
-	REQSUPPRATE(4);
-	REQSUPPRATE(5);
-	REQSUPPRATE(6);
-	REQSUPPRATE(7);
-	REQSUPPRATE(8);
+	for (int i = 0; i < 8; i++) {
                 ^^^^^

Now it starts at zero.

drivers/staging/wlan-ng/prism2mgmt.c int prism2mgmt_scan_results(struct wlandevice *wlandev, void *msgp) · line 442

Quoted 2 lines
drivers/staging/wlan-ng/prism2mgmt.c +2 -0
+		if (count >= i &&
+		    DOT11_RATE5_ISBASIC_GET(item->supprates[i - 1])) {
                                                            ^^^^^

This is an array underflow.

regards,
dan carpenter

Quoted 19 lines
drivers/staging/wlan-ng/prism2mgmt.c +13 -0
+			req->basicrate[i] .data = item->supprates[i - 1];
+			req->basicrate[i] .status =
+				P80211ENUM_msgitem_status_data_ok;
+		}
+	}
+
+	for (int i = 0; i < 8; i++) {
+		if (count >= i) {
+			req->supprate[i] .data = item->supprates[i - 1];
+			req->supprate[i] .status =
+				P80211ENUM_msgitem_status_data_ok;
+		}
+	}
 
 	/* beacon period */
 	req->beaconperiod.status = P80211ENUM_msgitem_status_data_ok;
Signature · 1 line
2.34.1
author
#2 lore raw
Quoted 13 lines · Dan Carpenter

Writing every sentence in imperative tense makes people sound like space aliens in human skin... You can if you want to but it's not a requirement in staging. There are two maintainers who have that requirement and the Outreachy people.

drivers/staging/wlan-ng/p80211metastruct.h struct p80211msg_dot11req_scan_results { · line 117

Quoted 1 line
drivers/staging/wlan-ng/p80211metastruct.h +0 -1
-	struct p80211item_uint32 basicrate1;
                                 ^^^^^^^^^^

This starts at 1 instead of 0.

Review

Quoted 1 line
(quoted diff) +1 -0
+	for (int i = 0; i < 8; i++) {
                 ^^^^^

Now it starts at zero.

Quoted 7 lines

drivers/staging/wlan-ng/prism2mgmt.c int prism2mgmt_scan_results(struct wlandevice *wlandev, void *msgp) · line 442

Quoted 2 lines
drivers/staging/wlan-ng/prism2mgmt.c +2 -0
+		if (count >= i &&
+		    DOT11_RATE5_ISBASIC_GET(item->supprates[i - 1])) {
                                                            ^^^^^

This is an array underflow.

regards,
dan carpenter

Thank you for the heads up on the (lack of) necessity for alienspeak and the code problem.
Starting at 0 should not have made a difference but I totally missed that underflow... embarassing.
I'll make sure to fix that and resubmit the revised patch.

Thanks,
Luke