netdev - Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <258ac12b-9ca3-9b24-30df-148f9df51582@quicinc.com>
Date:   Mon, 7 Feb 2022 13:24:01 -0800
From:   Jeff Johnson <quic_jjohnson@...cinc.com>
To:     Pavel Skripkin <paskripkin@...il.com>,
        <ath9k-devel@....qualcomm.com>, <kvalo@...nel.org>,
        <davem@...emloft.net>, <kuba@...nel.org>, <toke@...e.dk>,
        <linville@...driver.com>
CC:     <linux-wireless@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros

On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
> I've changed *STAT_* macros a bit in previous patch and I seems like
> they become really unreadable. Align these macros definitions to make
> code cleaner.
> 
> Also fixed following checkpatch warning
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> 
> Signed-off-by: Pavel Skripkin <paskripkin@...il.com>
> ---
> 
> Changes since v2:
> 	- My send-email script forgot, that mailing lists exist.
> 	  Added back all related lists
> 	- Fixed checkpatch warning
> 
> Changes since v1:
> 	- Added this patch
> 
> ---
>   drivers/net/wireless/ath/ath9k/htc.h | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
> index 141642e5e00d..b4755e21a501 100644
> --- a/drivers/net/wireless/ath/ath9k/htc.h
> +++ b/drivers/net/wireless/ath/ath9k/htc.h
> @@ -327,14 +327,14 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb)
>   }
>   
>   #ifdef CONFIG_ATH9K_HTC_DEBUGFS
> -#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
> -#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
> -#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
> -#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
> -#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
> -#define CAB_STAT_INC   priv->debug.tx_stats.cab_queued++
> -
> -#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++)
> +#define __STAT_SAVE(expr)	(hif_dev->htc_handle->drv_priv ? (expr) : 0)
> +#define TX_STAT_INC(c)		__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
> +#define TX_STAT_ADD(c, a)	__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
> +#define RX_STAT_INC(c)		__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
> +#define RX_STAT_ADD(c, a)	__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
> +#define CAB_STAT_INC		(priv->debug.tx_stats.cab_queued++)
> +
> +#define TX_QSTAT_INC(q)		(priv->debug.tx_stats.queue_stats[q]++)
>   
>   void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv,
>   			   struct ath_rx_status *rs);

It seems that these macros (both the original and the new) aren't 
following the guidance from the Coding Style which tells us under 
"Things to avoid when using macros" that we should avoid "macros that 
depend on having a local variable with a magic name". Wouldn't these 
macros be "better" is they included the hif_dev/priv as arguments rather 
than being "magic"?