Skip to contentSkip to conversation

staging: r8821ae: a couple macro expansion bugs

PATCH In review Dan Carpenter 4 participants 5 messages Jan 28, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 staging: r8821ae: a couple macro expansion bugs

These macros need parentheses, otherwise it causes a macro expansion bug when they are used like this:

	ch->flags &= ~IEEE80211_CHAN_NO_IBSS;

This was found using Smatch:
drivers/staging/rtl8821ae/regd.c:200 _rtl_reg_apply_beaconing_flags()
warn: the 'IEEE80211_CHAN_NO_IBSS' macro might need parens

drivers/staging/rtl8821ae/regd.h +2 -2
index abc60ab8165c..dceb3f18200b 100644
@@ -30,8 +30,8 @@
 #ifndef __RTL_REGD_H__
 #define __RTL_REGD_H__
 
-#define IEEE80211_CHAN_NO_IBSS		1<<2
-#define IEEE80211_CHAN_PASSIVE_SCAN	1<<1
+#define IEEE80211_CHAN_NO_IBSS		(1 << 2)
+#define IEEE80211_CHAN_PASSIVE_SCAN	(1 << 1)
 #define WIPHY_FLAG_CUSTOM_REGULATORY	BIT(0)
 #define WIPHY_FLAG_STRICT_REGULATORY	BIT(1)
 #define WIPHY_FLAG_DISABLE_BEACON_HINTS	BIT(2)
#1 lore raw

Am 28.01.2014 15:00, schrieb Dan Carpenter:

Quoted 17 lines

These macros need parentheses, otherwise it causes a macro expansion bug when they are used like this:

	ch->flags &= ~IEEE80211_CHAN_NO_IBSS;

This was found using Smatch:
drivers/staging/rtl8821ae/regd.c:200 _rtl_reg_apply_beaconing_flags()
warn: the 'IEEE80211_CHAN_NO_IBSS' macro might need parens

drivers/staging/rtl8821ae/regd.h +0 -0
index abc60ab8165c..dceb3f18200b 100644
@@ -30,8 +30,8 @@
 #ifndef __RTL_REGD_H__

drivers/staging/rtl8821ae/regd.h

Quoted 10 lines
drivers/staging/rtl8821ae/regd.h +2 -3
 #define __RTL_REGD_H__
 
-#define IEEE80211_CHAN_NO_IBSS		1<<2
-#define IEEE80211_CHAN_PASSIVE_SCAN	1<<1
+#define IEEE80211_CHAN_NO_IBSS		(1 << 2)
+#define IEEE80211_CHAN_PASSIVE_SCAN	(1 << 1)
 #define WIPHY_FLAG_CUSTOM_REGULATORY	BIT(0)
 #define WIPHY_FLAG_STRICT_REGULATORY	BIT(1)
 #define WIPHY_FLAG_DISABLE_BEACON_HINTS	BIT(2)
--

just one minor hint ...
could we settle for a common semantic here either 1<<2 or BIT(2) ?

just my 2 cents,
re,
wh

#2 lore raw
In reply to Dan Carpenter
Quoted 22 lines · walter harms

Am 28.01.2014 15:00, schrieb Dan Carpenter:

Quoted 19 lines

These macros need parentheses, otherwise it causes a macro expansion bug when they are used like this:

	ch->flags &= ~IEEE80211_CHAN_NO_IBSS;

This was found using Smatch:
drivers/staging/rtl8821ae/regd.c:200 _rtl_reg_apply_beaconing_flags()
warn: the 'IEEE80211_CHAN_NO_IBSS' macro might need parens

drivers/staging/rtl8821ae/regd.h +0 -0
index abc60ab8165c..dceb3f18200b 100644
@@ -30,8 +30,8 @@
 #ifndef __RTL_REGD_H__
 #define __RTL_REGD_H__
 
Quoted 13 lines

drivers/staging/rtl8821ae/regd.h

Quoted 8 lines
drivers/staging/rtl8821ae/regd.h +2 -3
-#define IEEE80211_CHAN_NO_IBSS		1<<2
-#define IEEE80211_CHAN_PASSIVE_SCAN	1<<1
+#define IEEE80211_CHAN_NO_IBSS		(1 << 2)
+#define IEEE80211_CHAN_PASSIVE_SCAN	(1 << 1)
 #define WIPHY_FLAG_CUSTOM_REGULATORY	BIT(0)
 #define WIPHY_FLAG_STRICT_REGULATORY	BIT(1)
 #define WIPHY_FLAG_DISABLE_BEACON_HINTS	BIT(2)
--

just one minor hint ...
could we settle for a common semantic here either 1<<2 or BIT(2) ?

Yeah... This driver needs a lot of cleanup in later patches.

regards,
dan carpenter

#3 lore raw
In reply to Dan Carpenter
Quoted 37 lines · Dan Carpenter
Quoted 22 lines · walter harms

Am 28.01.2014 15:00, schrieb Dan Carpenter:

Quoted 19 lines

These macros need parentheses, otherwise it causes a macro expansion bug when they are used like this:

	ch->flags &= ~IEEE80211_CHAN_NO_IBSS;

This was found using Smatch:
drivers/staging/rtl8821ae/regd.c:200 _rtl_reg_apply_beaconing_flags()
warn: the 'IEEE80211_CHAN_NO_IBSS' macro might need parens

drivers/staging/rtl8821ae/regd.h +0 -0
index abc60ab8165c..dceb3f18200b 100644
@@ -30,8 +30,8 @@
 #ifndef __RTL_REGD_H__
 #define __RTL_REGD_H__
 
Quoted 13 lines

drivers/staging/rtl8821ae/regd.h

Quoted 8 lines
drivers/staging/rtl8821ae/regd.h +2 -3
-#define IEEE80211_CHAN_NO_IBSS		1<<2
-#define IEEE80211_CHAN_PASSIVE_SCAN	1<<1
+#define IEEE80211_CHAN_NO_IBSS		(1 << 2)
+#define IEEE80211_CHAN_PASSIVE_SCAN	(1 << 1)
 #define WIPHY_FLAG_CUSTOM_REGULATORY	BIT(0)
 #define WIPHY_FLAG_STRICT_REGULATORY	BIT(1)
 #define WIPHY_FLAG_DISABLE_BEACON_HINTS	BIT(2)
--

just one minor hint ...
could we settle for a common semantic here either 1<<2 or BIT(2) ?

Quoted 1 line

Yeah... This driver needs a lot of cleanup in later patches.

That will happen soon, for 3.15, in the netdev tree, but for 3.14 we want this out there as people have the hardware and want to use it (i.e. me and 1000 others that got the hardware at a recent conference.)

thanks,

greg k-h

#4 lore raw
In reply to Dan Carpenter
Quoted 38 lines · Greg Kroah-Hartman
Quoted 37 lines · Dan Carpenter
Quoted 35 lines · walter harms

Am 28.01.2014 15:00, schrieb Dan Carpenter:

Quoted 17 lines

These macros need parentheses, otherwise it causes a macro expansion bug when they are used like this:

	ch->flags &= ~IEEE80211_CHAN_NO_IBSS;

This was found using Smatch:
drivers/staging/rtl8821ae/regd.c:200 _rtl_reg_apply_beaconing_flags()
warn: the 'IEEE80211_CHAN_NO_IBSS' macro might need parens

drivers/staging/rtl8821ae/regd.h +0 -0
index abc60ab8165c..dceb3f18200b 100644
@@ -30,8 +30,8 @@
  #ifndef __RTL_REGD_H__

drivers/staging/rtl8821ae/regd.h

Quoted 10 lines
drivers/staging/rtl8821ae/regd.h +2 -3
  #define __RTL_REGD_H__
 
-#define IEEE80211_CHAN_NO_IBSS		1<<2
-#define IEEE80211_CHAN_PASSIVE_SCAN	1<<1
+#define IEEE80211_CHAN_NO_IBSS		(1 << 2)
+#define IEEE80211_CHAN_PASSIVE_SCAN	(1 << 1)
  #define WIPHY_FLAG_CUSTOM_REGULATORY	BIT(0)
  #define WIPHY_FLAG_STRICT_REGULATORY	BIT(1)
  #define WIPHY_FLAG_DISABLE_BEACON_HINTS	BIT(2)
--

just one minor hint ...
could we settle for a common semantic here either 1<<2 or BIT(2) ?

Quoted 5 lines
Quoted 1 line

Yeah... This driver needs a lot of cleanup in later patches.

That will happen soon, for 3.15, in the netdev tree, but for 3.14 we want this out there as people have the hardware and want to use it (i.e. me and 1000 others that got the hardware at a recent conference.)

Please clean this up as much as you want, but be aware that this is a temporary solution that was done in a hurry to handle some inconsistencies between staging-next and linux-next. Neither Smatch nor Sparse were run on that version. I suspect there are also endian issues that have not been repaired. I will be reading such E-mails and will include your fixes, but the major cleanup is being done on separate source.

Larry