[PATCH] wifi: ath9k: cleanup ath_tx_complete_aggr()

[PATCH] wifi: ath9k: cleanup ath_tx_complete_aggr()

[Dmitry Antipov]

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);
-
 		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno) ||
 		    !tid->active) {
 			/*
-- 
2.44.0

Re: [PATCH] wifi: ath9k: cleanup ath_tx_complete_aggr()

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath9k: cleanup ath_tx_complete_aggr()

[Toke Høiland-Jørgensen]

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