ath9k: use proper statements in conditionals

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

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