Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <258ac12b-9ca3-9b24-30df-148f9df51582@quicinc.com>
Date:   Mon, 7 Feb 2022 13:24:01 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros
Content-Language: en-US
To:     Pavel Skripkin <paskripkin@gmail.com>,
        <ath9k-devel@qca.qualcomm.com>, <kvalo@kernel.org>,
        <davem@davemloft.net>, <kuba@kernel.org>, <toke@toke.dk>,
        <linville@tuxdriver.com>
CC:     <linux-wireless@vger.kernel.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <80962aae265995d1cdb724f5362c556d494c7566.1644265120.git.paskripkin@gmail.com>
 <28c83b99b8fea0115ad7fbda7cc93a86468ec50d.1644265120.git.paskripkin@gmail.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <28c83b99b8fea0115ad7fbda7cc93a86468ec50d.1644265120.git.paskripkin@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@gmail.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"?