Skip to contentSkip to conversation

wifi: mwifiex: Fix a loop in mwifiex_update_ampdu_rxwinsize()

PATCH In review Dan Carpenter 3 participants 3 messages Jan 8 – Jan 12

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 wifi: mwifiex: Fix a loop in mwifiex_update_ampdu_rxwinsize()

The "i" iterator variable is used to count two different things but unfortunately we can't store two different numbers in the same variable. Use "i" for the outside loop and "j" for the inside loop.

Fixes d219b7eb3792 ("mwifiex: handle BT coex event to adjust Rx BA window size")

---
This was found via static analysis so I'm not positive on the impact of this bug.

 drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c +3 -3
index 90831a1350f5..91166b89f918 100644
@@ -826,7 +826,7 @@ void mwifiex_update_rxreor_flags(struct mwifiex_adapter *adapter, u8 flags)
 static void mwifiex_update_ampdu_rxwinsize(struct mwifiex_adapter *adapter,
 					   bool coex_flag)
 {
-	u8 i;
+	u8 i, j;
 	u32 rx_win_size;
 	struct mwifiex_private *priv;
 
@@ -864,8 +864,8 @@ static void mwifiex_update_ampdu_rxwinsize(struct mwifiex_adapter *adapter,
 		if (rx_win_size != priv->add_ba_param.rx_win_size) {
 			if (!priv->media_connected)
 				continue;
-			for (i = 0; i < MAX_NUM_TID; i++)
-				mwifiex_11n_delba(priv, i);
+			for (j = 0; j < MAX_NUM_TID; j++)
+				mwifiex_11n_delba(priv, j);
 		}
 	}
 }
Signature · 1 line
2.51.0
#1 lore raw
Quoted 1 line · Dan Carpenter

The "i" iterator variable is used to count two different things but

nice catch

Quoted 1 line

unfortunately we can't store two different numbers in the same variable.

:-)

Quoted 2 lines

This was found via static analysis so I'm not positive on the impact of this bug.

I think it basically means anything other than the first interface (using adapter->priv[i] with i>0) will not be updated correctly for A- MPDU buffer usage (?) if you use more than two interfaces. Given that most people probably only use a single interface, I suppose the impact would be rather low.

johannes

Reviewed-by
#2 lore raw
Quoted 12 lines · Johannes Berg
Quoted 1 line · Dan Carpenter

The "i" iterator variable is used to count two different things but

nice catch

Quoted 1 line

unfortunately we can't store two different numbers in the same variable.

:-)

Quoted 2 lines

This was found via static analysis so I'm not positive on the impact of this bug.

Quoted 8 lines

I think it basically means anything other than the first interface (using adapter->priv[i] with i>0) will not be updated correctly for A- MPDU buffer usage (?) if you use more than two interfaces. Given that most people probably only use a single interface, I suppose the impact would be rather low.

johannes

Johannes is right — with AP and STA both active, whichever interface first meets media_connected (AP or STA) will trigger the inner loop to overwrite the outer interface index, so only that first interface gets the per‑TID delba while the other is skipped.
For reference: mwifiex_update_ampdu_txwinsize() already uses separate iterators , so aligning the RX-side loop to the same pattern makes sense.

Dan’s change to use a distinct inner iterator (j) here is correct. Please apply.