| Message ID | 20240321122642.175645-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Toke Høiland-Jørgensen |
| Headers | show |
| Series | wifi: ath9k: cleanup ath_tx_complete_aggr() | expand |
Dmitry Antipov <dmantipov@yandex.ru> writes: > Since 'skb', 'tx_info' and 'fi' are actually used within > buffers processing loop only, move them inside the latter > and avoid some redundant initialization at the beginning > of 'ath_tx_complete_aggr()'. Compile tested only. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > drivers/net/wireless/ath/ath9k/xmit.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c > index d519b676a109..657862be45e5 100644 > --- a/drivers/net/wireless/ath/ath9k/xmit.c > +++ b/drivers/net/wireless/ath/ath9k/xmit.c > @@ -491,8 +491,6 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, > struct ath_tx_status *ts, int txok) > { > struct ath_node *an = NULL; > - struct sk_buff *skb; > - struct ieee80211_tx_info *tx_info; > struct ath_buf *bf_next, *bf_last = bf->bf_lastbf; > struct list_head bf_head; > struct sk_buff_head bf_pending; > @@ -501,15 +499,11 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, > int isaggr, txfail, txpending, sendbar = 0, needreset = 0, nbad = 0; > bool rc_update = true, isba; > struct ieee80211_tx_rate rates[4]; > - struct ath_frame_info *fi; > int nframes; > bool flush = !!(ts->ts_status & ATH9K_TX_FLUSH); > int i, retries; > int bar_index = -1; > > - skb = bf->bf_mpdu; > - tx_info = IEEE80211_SKB_CB(skb); > - > memcpy(rates, bf->rates, sizeof(rates)); > > retries = ts->ts_longretry + 1; > @@ -571,14 +565,13 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, > ath_tx_count_frames(sc, bf, ts, txok, &nframes, &nbad); > while (bf) { > u16 seqno = bf->bf_state.seqno; > + struct sk_buff *skb = bf->bf_mpdu; > + struct ath_frame_info *fi = get_frame_info(skb); > + struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb); FWIW I don't like this style (defining variables within a loop) but that's really up to Toke to decide. Also what does the generated code look like when assigning the same variables in every loop iteration? But honestly this just feels unnecessary code churn. Dmitry, I have to warn that these kind of cleanups (without no concrete improvement) from you are getting annoying, it's very close that I just start ignoring your patches.
Dmitry Antipov <dmantipov@yandex.ru> writes: > Since 'skb', 'tx_info' and 'fi' are actually used within > buffers processing loop only, move them inside the latter > and avoid some redundant initialization at the beginning > of 'ath_tx_complete_aggr()'. Compile tested only. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > drivers/net/wireless/ath/ath9k/xmit.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c > index d519b676a109..657862be45e5 100644 > --- a/drivers/net/wireless/ath/ath9k/xmit.c > +++ b/drivers/net/wireless/ath/ath9k/xmit.c > @@ -491,8 +491,6 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, > struct ath_tx_status *ts, int txok) > { > struct ath_node *an = NULL; > - struct sk_buff *skb; > - struct ieee80211_tx_info *tx_info; > struct ath_buf *bf_next, *bf_last = bf->bf_lastbf; > struct list_head bf_head; > struct sk_buff_head bf_pending; > @@ -501,15 +499,11 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, > int isaggr, txfail, txpending, sendbar = 0, needreset = 0, nbad = 0; > bool rc_update = true, isba; > struct ieee80211_tx_rate rates[4]; > - struct ath_frame_info *fi; > int nframes; > bool flush = !!(ts->ts_status & ATH9K_TX_FLUSH); > int i, retries; > int bar_index = -1; > > - skb = bf->bf_mpdu; > - tx_info = IEEE80211_SKB_CB(skb); > - > memcpy(rates, bf->rates, sizeof(rates)); > > retries = ts->ts_longretry + 1; > @@ -571,14 +565,13 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, > ath_tx_count_frames(sc, bf, ts, txok, &nframes, &nbad); > while (bf) { > u16 seqno = bf->bf_state.seqno; > + struct sk_buff *skb = bf->bf_mpdu; > + struct ath_frame_info *fi = get_frame_info(skb); > + struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb); > > txfail = txpending = sendbar = 0; > bf_next = bf->bf_next; > > - skb = bf->bf_mpdu; > - tx_info = IEEE80211_SKB_CB(skb); > - fi = get_frame_info(skb); > - I'm OK with moving the variable declarations inside the loop body, but please keep the assignment here as separate statements (and maintain reverse x-mas tree ordering in the declarations at the top of the loop). -Toke
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c index d519b676a109..657862be45e5 100644 --- a/drivers/net/wireless/ath/ath9k/xmit.c +++ b/drivers/net/wireless/ath/ath9k/xmit.c @@ -491,8 +491,6 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, struct ath_tx_status *ts, int txok) { struct ath_node *an = NULL; - struct sk_buff *skb; - struct ieee80211_tx_info *tx_info; struct ath_buf *bf_next, *bf_last = bf->bf_lastbf; struct list_head bf_head; struct sk_buff_head bf_pending; @@ -501,15 +499,11 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, int isaggr, txfail, txpending, sendbar = 0, needreset = 0, nbad = 0; bool rc_update = true, isba; struct ieee80211_tx_rate rates[4]; - struct ath_frame_info *fi; int nframes; bool flush = !!(ts->ts_status & ATH9K_TX_FLUSH); int i, retries; int bar_index = -1; - skb = bf->bf_mpdu; - tx_info = IEEE80211_SKB_CB(skb); - memcpy(rates, bf->rates, sizeof(rates)); retries = ts->ts_longretry + 1; @@ -571,14 +565,13 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq, ath_tx_count_frames(sc, bf, ts, txok, &nframes, &nbad); while (bf) { u16 seqno = bf->bf_state.seqno; + struct sk_buff *skb = bf->bf_mpdu; + struct ath_frame_info *fi = get_frame_info(skb); + struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb); txfail = txpending = sendbar = 0; bf_next = bf->bf_next; - skb = bf->bf_mpdu; - tx_info = IEEE80211_SKB_CB(skb); - fi = get_frame_info(skb); - if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno) || !tid->active) { /*
Since 'skb', 'tx_info' and 'fi' are actually used within buffers processing loop only, move them inside the latter and avoid some redundant initialization at the beginning of 'ath_tx_complete_aggr()'. Compile tested only. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- drivers/net/wireless/ath/ath9k/xmit.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)