| Message ID | 20221215165553.1950307-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ath9k: use proper statements in conditionals | expand |
Arnd Bergmann <arnd@kernel.org> writes: > From: Arnd Bergmann <arnd@arndb.de> > > A previous cleanup patch accidentally broke some conditional > expressions by replacing the safe "do {} while (0)" constructs > with empty macros. gcc points this out when extra warnings > are enabled: > > drivers/net/wireless/ath/ath9k/hif_usb.c: In function 'ath9k_skb_queue_complete': > drivers/net/wireless/ath/ath9k/hif_usb.c:251:57: error: suggest braces around empty body in an 'else' statement [-Werror=empty-body] > 251 | TX_STAT_INC(hif_dev, skb_failed); > > Make both sets of macros proper expressions again. > > Fixes: d7fc76039b74 ("ath9k: htc: clean up statistics macros") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/net/wireless/ath/ath9k/htc.h | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h > index 30f0765fb9fd..237f4ec2cffd 100644 > --- a/drivers/net/wireless/ath/ath9k/htc.h > +++ b/drivers/net/wireless/ath/ath9k/htc.h > @@ -327,9 +327,9 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) > } > > #ifdef CONFIG_ATH9K_HTC_DEBUGFS > -#define __STAT_SAFE(hif_dev, expr) ((hif_dev)->htc_handle->drv_priv ? (expr) : 0) > -#define CAB_STAT_INC(priv) ((priv)->debug.tx_stats.cab_queued++) > -#define TX_QSTAT_INC(priv, q) ((priv)->debug.tx_stats.queue_stats[q]++) > +#define __STAT_SAFE(hif_dev, expr) do { ((hif_dev)->htc_handle->drv_priv ? (expr) : 0); } while (0) > +#define CAB_STAT_INC(priv) do { ((priv)->debug.tx_stats.cab_queued++); } while (0) > +#define TX_QSTAT_INC(priv, q) do { ((priv)->debug.tx_stats.queue_stats[q]++); } while (0) Hmm, is it really necessary to wrap these in do/while constructs? AFAICT they're all simple statements already? -Toke
On Thu, Dec 15, 2022, at 18:16, Toke Høiland-Jørgensen wrote: >> index 30f0765fb9fd..237f4ec2cffd 100644 >> --- a/drivers/net/wireless/ath/ath9k/htc.h >> +++ b/drivers/net/wireless/ath/ath9k/htc.h >> @@ -327,9 +327,9 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) >> } >> >> #ifdef CONFIG_ATH9K_HTC_DEBUGFS >> -#define __STAT_SAFE(hif_dev, expr) ((hif_dev)->htc_handle->drv_priv ? (expr) : 0) >> -#define CAB_STAT_INC(priv) ((priv)->debug.tx_stats.cab_queued++) >> -#define TX_QSTAT_INC(priv, q) ((priv)->debug.tx_stats.queue_stats[q]++) >> +#define __STAT_SAFE(hif_dev, expr) do { ((hif_dev)->htc_handle->drv_priv ? (expr) : 0); } while (0) >> +#define CAB_STAT_INC(priv) do { ((priv)->debug.tx_stats.cab_queued++); } while (0) >> +#define TX_QSTAT_INC(priv, q) do { ((priv)->debug.tx_stats.queue_stats[q]++); } while (0) > > Hmm, is it really necessary to wrap these in do/while constructs? AFAICT > they're all simple statements already? It's generally safer to do the same thing on both side of the #ifdef. The "do { } while (0)" is an empty statement that is needed to fix the bug on the #else side. The expressions you have on the #ifdef side can be used as values, and wrapping them in do{}while(0) turns them into statements (without a value) as well, so fewer things can go wrong when you only test one side. I suppose the best solution would be to just use inline functions for all of them and get rid of the macros. Arnd
"Arnd Bergmann" <arnd@arndb.de> writes: > On Thu, Dec 15, 2022, at 18:16, Toke Høiland-Jørgensen wrote: >>> index 30f0765fb9fd..237f4ec2cffd 100644 >>> --- a/drivers/net/wireless/ath/ath9k/htc.h >>> +++ b/drivers/net/wireless/ath/ath9k/htc.h >>> @@ -327,9 +327,9 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) >>> } >>> >>> #ifdef CONFIG_ATH9K_HTC_DEBUGFS >>> -#define __STAT_SAFE(hif_dev, expr) ((hif_dev)->htc_handle->drv_priv ? (expr) : 0) >>> -#define CAB_STAT_INC(priv) ((priv)->debug.tx_stats.cab_queued++) >>> -#define TX_QSTAT_INC(priv, q) ((priv)->debug.tx_stats.queue_stats[q]++) >>> +#define __STAT_SAFE(hif_dev, expr) do { ((hif_dev)->htc_handle->drv_priv ? (expr) : 0); } while (0) >>> +#define CAB_STAT_INC(priv) do { ((priv)->debug.tx_stats.cab_queued++); } while (0) >>> +#define TX_QSTAT_INC(priv, q) do { ((priv)->debug.tx_stats.queue_stats[q]++); } while (0) >> >> Hmm, is it really necessary to wrap these in do/while constructs? AFAICT >> they're all simple statements already? > > It's generally safer to do the same thing on both side of the #ifdef. > > The "do { } while (0)" is an empty statement that is needed to fix > the bug on the #else side. The expressions you have on the #ifdef > side can be used as values, and wrapping them in do{}while(0) > turns them into statements (without a value) as well, so fewer > things can go wrong when you only test one side. Alright, makes sense; thanks for explaining! > I suppose the best solution would be to just use inline functions > for all of them and get rid of the macros. Let's merge this patch to fix the bug, and if someone wants to follow up with a conversion to inline functions, that would be awesome, of course :) -Toke
Arnd Bergmann <arnd@kernel.org> writes: > From: Arnd Bergmann <arnd@arndb.de> > > A previous cleanup patch accidentally broke some conditional > expressions by replacing the safe "do {} while (0)" constructs > with empty macros. gcc points this out when extra warnings > are enabled: > > drivers/net/wireless/ath/ath9k/hif_usb.c: In function 'ath9k_skb_queue_complete': > drivers/net/wireless/ath/ath9k/hif_usb.c:251:57: error: suggest braces around empty body in an 'else' statement [-Werror=empty-body] > 251 | TX_STAT_INC(hif_dev, skb_failed); > > Make both sets of macros proper expressions again. > > Fixes: d7fc76039b74 ("ath9k: htc: clean up statistics macros") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Toke Høiland-Jørgensen <toke@toke.dk> writes: > Arnd Bergmann <arnd@kernel.org> writes: > >> From: Arnd Bergmann <arnd@arndb.de> >> >> A previous cleanup patch accidentally broke some conditional >> expressions by replacing the safe "do {} while (0)" constructs >> with empty macros. gcc points this out when extra warnings >> are enabled: >> >> drivers/net/wireless/ath/ath9k/hif_usb.c: In function 'ath9k_skb_queue_complete': >> drivers/net/wireless/ath/ath9k/hif_usb.c:251:57: error: suggest braces around empty body in an 'else' statement [-Werror=empty-body] >> 251 | TX_STAT_INC(hif_dev, skb_failed); >> >> Make both sets of macros proper expressions again. >> >> Fixes: d7fc76039b74 ("ath9k: htc: clean up statistics macros") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> I'll queue this to v6.2.
Kalle Valo <kvalo@kernel.org> writes: > Toke Høiland-Jørgensen <toke@toke.dk> writes: > >> Arnd Bergmann <arnd@kernel.org> writes: >> >>> From: Arnd Bergmann <arnd@arndb.de> >>> >>> A previous cleanup patch accidentally broke some conditional >>> expressions by replacing the safe "do {} while (0)" constructs >>> with empty macros. gcc points this out when extra warnings >>> are enabled: >>> >>> drivers/net/wireless/ath/ath9k/hif_usb.c: In function 'ath9k_skb_queue_complete': >>> drivers/net/wireless/ath/ath9k/hif_usb.c:251:57: error: suggest braces around empty body in an 'else' statement [-Werror=empty-body] >>> 251 | TX_STAT_INC(hif_dev, skb_failed); >>> >>> Make both sets of macros proper expressions again. >>> >>> Fixes: d7fc76039b74 ("ath9k: htc: clean up statistics macros") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> >> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > > I'll queue this to v6.2. Sounds good, thanks! -Toke
Arnd Bergmann <arnd@kernel.org> wrote: > From: Arnd Bergmann <arnd@arndb.de> > > A previous cleanup patch accidentally broke some conditional > expressions by replacing the safe "do {} while (0)" constructs > with empty macros. gcc points this out when extra warnings > are enabled: > > drivers/net/wireless/ath/ath9k/hif_usb.c: In function 'ath9k_skb_queue_complete': > drivers/net/wireless/ath/ath9k/hif_usb.c:251:57: error: suggest braces around empty body in an 'else' statement [-Werror=empty-body] > 251 | TX_STAT_INC(hif_dev, skb_failed); > > Make both sets of macros proper expressions again. > > Fixes: d7fc76039b74 ("ath9k: htc: clean up statistics macros") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> Patch applied to wireless.git, thanks. b7dc753fe33a wifi: ath9k: use proper statements in conditionals
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index 30f0765fb9fd..237f4ec2cffd 100644 --- a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -327,9 +327,9 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) } #ifdef CONFIG_ATH9K_HTC_DEBUGFS -#define __STAT_SAFE(hif_dev, expr) ((hif_dev)->htc_handle->drv_priv ? (expr) : 0) -#define CAB_STAT_INC(priv) ((priv)->debug.tx_stats.cab_queued++) -#define TX_QSTAT_INC(priv, q) ((priv)->debug.tx_stats.queue_stats[q]++) +#define __STAT_SAFE(hif_dev, expr) do { ((hif_dev)->htc_handle->drv_priv ? (expr) : 0); } while (0) +#define CAB_STAT_INC(priv) do { ((priv)->debug.tx_stats.cab_queued++); } while (0) +#define TX_QSTAT_INC(priv, q) do { ((priv)->debug.tx_stats.queue_stats[q]++); } while (0) #define TX_STAT_INC(hif_dev, c) \ __STAT_SAFE((hif_dev), (hif_dev)->htc_handle->drv_priv->debug.tx_stats.c++) @@ -378,10 +378,10 @@ void ath9k_htc_get_et_stats(struct ieee80211_hw *hw, struct ethtool_stats *stats, u64 *data); #else -#define TX_STAT_INC(hif_dev, c) -#define TX_STAT_ADD(hif_dev, c, a) -#define RX_STAT_INC(hif_dev, c) -#define RX_STAT_ADD(hif_dev, c, a) +#define TX_STAT_INC(hif_dev, c) do { } while (0) +#define TX_STAT_ADD(hif_dev, c, a) do { } while (0) +#define RX_STAT_INC(hif_dev, c) do { } while (0) +#define RX_STAT_ADD(hif_dev, c, a) do { } while (0) #define CAB_STAT_INC(priv) #define TX_QSTAT_INC(priv, c)