LinuxLists.cc - [PATCH v3] staging: wlan-ng: replace rate macros

2023-04-18 12:36:34

Subject: [PATCH v3] 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]>
---
v2: - Fix array underflow and conditions with respect to the start at 0
v3: - Remove unnecessary spaces
---
drivers/staging/wlan-ng/p80211metastruct.h | 18 +-------
drivers/staging/wlan-ng/prism2mgmt.c | 52 +++++++---------------
2 files changed, 18 insertions(+), 52 deletions(-)

diff --git a/drivers/staging/wlan-ng/p80211metastruct.h b/drivers/staging/wlan-ng/p80211metastruct.h
index 4adc64580185..e963227f797c 100644
--- a/drivers/staging/wlan-ng/p80211metastruct.h
+++ b/drivers/staging/wlan-ng/p80211metastruct.h
@@ -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 {
diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c
index 9030a8939a9b..fc465261baa1 100644
--- a/drivers/staging/wlan-ng/prism2mgmt.c
+++ b/drivers/staging/wlan-ng/prism2mgmt.c
@@ -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)
-
- REQSUPPRATE(1);
- REQSUPPRATE(2);
- REQSUPPRATE(3);
- REQSUPPRATE(4);
- REQSUPPRATE(5);
- REQSUPPRATE(6);
- REQSUPPRATE(7);
- REQSUPPRATE(8);
+ for (int i = 0; i < 8; i++) {
+ if (count > i &&
+ DOT11_RATE5_ISBASIC_GET(item->supprates[i])) {
+ req->basicrate[i].data = item->supprates[i];
+ 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];
+ req->supprate[i].status =
+ P80211ENUM_msgitem_status_data_ok;
+ }
+ }

/* beacon period */
req->beaconperiod.status = P80211ENUM_msgitem_status_data_ok;
--
2.34.1

2023-04-18 13:11:09

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v3] staging: wlan-ng: replace rate macros

<note, your Reply-To: is very odd, please fix your email client up...>

On Tue, Apr 18, 2023 at 02:35:52PM +0200, Luke Koch wrote:
> 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]>
> ---
> v2: - Fix array underflow and conditions with respect to the start at 0
> v3: - Remove unnecessary spaces
> ---
> drivers/staging/wlan-ng/p80211metastruct.h | 18 +-------
> drivers/staging/wlan-ng/prism2mgmt.c | 52 +++++++---------------
> 2 files changed, 18 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/staging/wlan-ng/p80211metastruct.h b/drivers/staging/wlan-ng/p80211metastruct.h
> index 4adc64580185..e963227f797c 100644
> --- a/drivers/staging/wlan-ng/p80211metastruct.h
> +++ b/drivers/staging/wlan-ng/p80211metastruct.h
> @@ -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 {
> diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c
> index 9030a8939a9b..fc465261baa1 100644
> --- a/drivers/staging/wlan-ng/prism2mgmt.c
> +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> @@ -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)
> -
> - REQSUPPRATE(1);
> - REQSUPPRATE(2);
> - REQSUPPRATE(3);
> - REQSUPPRATE(4);
> - REQSUPPRATE(5);
> - REQSUPPRATE(6);
> - REQSUPPRATE(7);
> - REQSUPPRATE(8);
> + for (int i = 0; i < 8; i++) {
> + if (count > i &&
> + DOT11_RATE5_ISBASIC_GET(item->supprates[i])) {
> + req->basicrate[i].data = item->supprates[i];
> + 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];
> + req->supprate[i].status =
> + P80211ENUM_msgitem_status_data_ok;
> + }
> + }

This patch implies that these structures are set but never actually read
from, so why are they present at all? Is this a structure that is on
the wire/air or used somewhere else as an api to hardware?

I tried to unwind things in the driver, but couldn't figure it out, what
happens if you just delete these fields, does the driver still work
properly?

thanks,

greg k-h

2023-04-18 19:48:37

by Luke Koch

[permalink] [raw]

Subject: Re: [PATCH v3] staging: wlan-ng: replace rate macros

On Tue, Apr 18, 2023 at 03:04:06PM +0200, Greg KH wrote:
> This patch implies that these structures are set but never actually read
> from, so why are they present at all? Is this a structure that is on
> the wire/air or used somewhere else as an api to hardware?

I never looked that far as I was naively trying to just fix the
checkpatch warning. They seem to be part of MAC frames coming from the
hardware.

> I tried to unwind things in the driver, but couldn't figure it out, what
> happens if you just delete these fields, does the driver still work
> properly?

It compiles without issue when deleting these structs, but almost
certainly breaks the driver since it changes what the resulting message
looks like in memory. I assume the caller of the function uses the message for
something - otherwise setting all the message parameters wouldn't make any sense
at all - and changing the message like this would cause major problems.

The structs seem necessary, but changing how they are accessed to using
arrays shouldn't introduce any issues as resulting values as well as addresses
in relation to the message struct will be unchanged.

Best regards,

Luke Koch

2023-04-20 11:58:59

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v3] staging: wlan-ng: replace rate macros

On Tue, Apr 18, 2023 at 02:35:52PM +0200, Luke Koch wrote:
> 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]>

That has to be "Signed-off-by:"

2023-04-20 12:02:25

by Greg Kroah-Hartman

[permalink] [raw]