Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
To:     Jeff Johnson <quic_jjohnson@quicinc.com>,
        Pavel Skripkin <paskripkin@gmail.com>,
        ath9k-devel@qca.qualcomm.com, kvalo@kernel.org,
        davem@davemloft.net, kuba@kernel.org, linville@tuxdriver.com
Cc:     linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros
In-Reply-To: <258ac12b-9ca3-9b24-30df-148f9df51582@quicinc.com>
References: <80962aae265995d1cdb724f5362c556d494c7566.1644265120.git.paskripkin@gmail.com>
 <28c83b99b8fea0115ad7fbda7cc93a86468ec50d.1644265120.git.paskripkin@gmail.com>
 <258ac12b-9ca3-9b24-30df-148f9df51582@quicinc.com>
Date:   Tue, 08 Feb 2022 16:32:13 +0100
Message-ID: <87ee4d9xxe.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> 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"?

Hmm, yeah, that's a good point; looks like the non-HTC ath9k stats
macros have already been converted to take the container as a parameter,
so taking this opportunity to fix these macros is not a bad idea. While
we're at it, let's switch to the do{} while(0) syntax the other macros
are using instead of that weird usage of ?:. And there's not really any
reason for the duplication between ADD/INC either. So I'm thinking
something like:

#define __STAT_SAVE(_priv, _member, _n) do { if (_priv) (_priv)->_member += (_n); } while(0)

#define TX_STAT_ADD(_priv, _c, _a) __STAT_SAVE(_priv, debug.tx_stats._c, _a)
#define TX_STAT_INC(_priv, _c) TX_STAT_ADD(_priv, _c, 1)

[... etc ...]

-Toke