Received-SPF: pass (google.com: domain of linux-wireless+bounces-5114-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
To: Dmitry Antipov <dmantipov@yandex.ru>
Cc: Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org,
 lvc-project@linuxtesting.org, Dmitry Antipov <dmantipov@yandex.ru>
Subject: Re: [PATCH] wifi: ath9k: cleanup ath_tx_complete_aggr()
In-Reply-To: <20240321122642.175645-1-dmantipov@yandex.ru>
References: <20240321122642.175645-1-dmantipov@yandex.ru>
Date: Thu, 21 Mar 2024 20:44:10 +0100
Message-ID: <87o7b7fjzp.fsf@toke.dk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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