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 <[email protected]>
---
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);
--
2.34.1
Hi Toke,
On 2/8/22 18:32, Toke Høiland-Jørgensen wrote:
>> 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)
>
Good point, thank you. Will redo these macros in v4
With regards,
Pavel Skripkin
Jeff Johnson <[email protected]> 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 <[email protected]>
>> ---
>>
>> 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