diff mbox series

wifi: ath9k: cleanup ath_tx_complete_aggr()

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

Commit Message

Dmitry Antipov March 21, 2024, 12:26 p.m. UTC
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(-)

Comments

Kalle Valo March 21, 2024, 1:57 p.m. UTC | #1
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.
Toke Høiland-Jørgensen March 21, 2024, 7:44 p.m. UTC | #2
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 mbox series

Patch

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) {
 			/*