| Message ID | 28c83b99b8fea0115ad7fbda7cc93a86468ec50d.1644265120.git.paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v3,1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb | expand |
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"?
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
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
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);
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(-)