iwlagn: fix "Received BA when not expected"

PATCH Superseded Stanislaw Gruszka 2 participants 6 messages Apr 28, 2011 – Apr 29, 2011 Networking drivers (wireless)

Share & tools

Stanislaw Gruszka

author

2011-04-28 11:11 UTC #0 lore raw

PATCH 1/1 iwlagn: fix "Received BA when not expected"

Need to use broadcast sta_id for management and multicast frames, otherwise we broke BA session and get messages like that:

"Received BA when not expected"

or (on older kernels):

"BA scd_flow 0 does not match txq_id 10"

This fix regression introduced in 2.6.35 during station management code rewrite by:

commit 2a87c26bbe9587baeb9e56d3ce0b4971bd777643

Author: Johannes Berg <[email protected]>
Date:   Fri Apr 30 11:30:45 2010 -0700

iwlwifi: use iwl_find_station less

Patch partially resolve:
https://bugzilla.kernel.org/show_bug.cgi?id=16691
However, there are still 11n performance problems on 4965 and 5xxx devices that need to be ivestigated.

Cc [email protected] # 2.6.35+

Signed-off-by Stanislaw Gruszka <[email protected]>

---
 drivers/net/wireless/iwlwifi/iwl-agn-tx.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

drivers/net/wireless/iwlwifi/iwl-agn-tx.c +12 -6

index 494de0e..dfde372 100644
@@ -31,6 +31,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/sched.h>
+#include <linux/etherdevice.h>
 
 #include "iwl-dev.h"
 #include "iwl-core.h"

@@ -582,12 +583,17 @@ int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb)
 
 	hdr_len = ieee80211_hdrlen(fc);
 
-	/* Find index into station table for destination station */
-	sta_id = iwl_sta_id_or_broadcast(priv, ctx, info->control.sta);
-	if (sta_id == IWL_INVALID_STATION) {
-		IWL_DEBUG_DROP(priv, "Dropping - INVALID STATION: %pM\n",
-			       hdr->addr1);
-		goto drop_unlock;
+	/* If this frame is broadcast or management, use broadcast station id */
+	if (!ieee80211_is_data(fc) || is_multicast_ether_addr(hdr->addr1))
+		sta_id = ctx->bcast_sta_id;
+	else {
+		/* Find index into station table for destination station */
+		sta_id = iwl_sta_id_or_broadcast(priv, ctx, info->control.sta);
+		if (sta_id == IWL_INVALID_STATION) {
+			IWL_DEBUG_DROP(priv, "Dropping - INVALID STATION: %pM\n",
+				       hdr->addr1);
+			goto drop_unlock;
+		}
 	}
 
 	IWL_DEBUG_TX(priv, "station Id %d\n", sta_id);

Signature · 1 line

1.7.1

Johannes Berg

maintainer

2011-04-28 11:37 UTC #1 lore raw

Quoted 8 lines · Stanislaw Gruszka

Need to use broadcast sta_id for management and multicast frames, otherwise we broke BA session and get messages like that:
"Received BA when not expected"
or (on older kernels):
"BA scd_flow 0 does not match txq_id 10"

Hmm. Interesting. I believe you, but it's hard.

drivers/net/wireless/iwlwifi/iwl-agn-tx.c int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) · line 586

Quoted 2 lines

drivers/net/wireless/iwlwifi/iwl-agn-tx.c +0 -2

-	/* Find index into station table for destination station */
-	sta_id = iwl_sta_id_or_broadcast(priv, ctx, info->control.sta);

info->control.sta should be NULL here, at least for multicast frames (which never really happen in practise unless you use AP mode). And then this should return ctx->bcast_sta_id, just like you use below:

drivers/net/wireless/iwlwifi/iwl-agn-tx.c int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) · line 588

Quoted 3 lines

drivers/net/wireless/iwlwifi/iwl-agn-tx.c +3 -0

+	/* If this frame is broadcast or management, use broadcast station id */
+	if (!ieee80211_is_data(fc) || is_multicast_ether_addr(hdr->addr1))
+		sta_id = ctx->bcast_sta_id;

So the reason has to be management frames. But why would those be affecting it? They aren't QoS frames, so there should be no influence on BA handling at all. This is a bit odd.

Maybe we should make it clear here that it's about non-data frames and remove the multicast check since that ought to be handled in the other path?

johannes

Johannes Berg

maintainer

2011-04-28 11:41 UTC #2 lore raw

Quoted 11 lines · Johannes Berg

Quoted 8 lines · Stanislaw Gruszka
Need to use broadcast sta_id for management and multicast frames, otherwise we broke BA session and get messages like that:
"Received BA when not expected"
or (on older kernels):
"BA scd_flow 0 does not match txq_id 10"
Hmm. Interesting. I believe you, but it's hard.

You're right of course that my patch changed the behaviour. I just don't (yet) understand why it's causing this particular issue.

johannes

Stanislaw Gruszka

author

2011-04-28 12:00 UTC #3 lore raw

In reply to Johannes Berg

Quoted 11 lines · Johannes Berg

Quoted 8 lines · Stanislaw Gruszka
Need to use broadcast sta_id for management and multicast frames, otherwise we broke BA session and get messages like that:
"Received BA when not expected"
or (on older kernels):
"BA scd_flow 0 does not match txq_id 10"
Hmm. Interesting. I believe you, but it's hard.

To be honest, I expected to get explanations from you (perhaps with better fix) :-) However this one is intended to work on older kernels, and is tested by me and one other user.

Quoted 16 lines

drivers/net/wireless/iwlwifi/iwl-agn-tx.c int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) · line 586
Quoted 2 lines
drivers/net/wireless/iwlwifi/iwl-agn-tx.c +0 -2
-	/* Find index into station table for destination station */
-	sta_id = iwl_sta_id_or_broadcast(priv, ctx, info->control.sta);
info->control.sta should be NULL here, at least for multicast frames (which never really happen in practise unless you use AP mode). And then this should return ctx->bcast_sta_id, just like you use below:
drivers/net/wireless/iwlwifi/iwl-agn-tx.c int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) · line 588
Quoted 3 lines
drivers/net/wireless/iwlwifi/iwl-agn-tx.c +3 -0
+	/* If this frame is broadcast or management, use broadcast station id */
+	if (!ieee80211_is_data(fc) || is_multicast_ether_addr(hdr->addr1))
+		sta_id = ctx->bcast_sta_id;
So the reason has to be management frames. But why would those be affecting it? They aren't QoS frames, so there should be no influence on BA handling at all. This is a bit odd.

Quoted 3 lines

Maybe we should make it clear here that it's about non-data frames and remove the multicast check since that ought to be handled in the other path?

We call iwl_sta_modify_sleep_tx_count(priv, sta_id, 1); for any frame as long as some other conditions are true.

Also multicast frames are important. When we send data and multicast frame we probably modify

priv->stations[sta_id].tid[tid]

with wrong sta_id (other station with unicast address), what make related aggregation data wrong for that other station.

Stanislaw

Johannes Berg

maintainer

2011-04-28 12:09 UTC #4 lore raw

Quoted 9 lines · Stanislaw Gruszka

Quoted 8 lines · Johannes Berg
Quoted 7 lines · Stanislaw Gruszka
Need to use broadcast sta_id for management and multicast frames, otherwise we broke BA session and get messages like that:
"Received BA when not expected"
or (on older kernels):

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
"BA scd_flow 0 does not match txq_id 10"
Hmm. Interesting. I believe you, but it's hard.
To be honest, I expected to get explanations from you (perhaps with better fix) :-) However this one is intended to work on older kernels, and is tested by me and one other user.

:-)
I'll try to dig ...

Quoted 16 lines

Quoted 16 lines
drivers/net/wireless/iwlwifi/iwl-agn-tx.c int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) · line 586
Quoted 2 lines
drivers/net/wireless/iwlwifi/iwl-agn-tx.c +0 -2
-	/* Find index into station table for destination station */
-	sta_id = iwl_sta_id_or_broadcast(priv, ctx, info->control.sta);
info->control.sta should be NULL here, at least for multicast frames (which never really happen in practise unless you use AP mode). And then this should return ctx->bcast_sta_id, just like you use below:
drivers/net/wireless/iwlwifi/iwl-agn-tx.c int iwlagn_tx_skb(struct iwl_priv *priv, struct sk_buff *skb) · line 588
Quoted 3 lines
drivers/net/wireless/iwlwifi/iwl-agn-tx.c +3 -0
+	/* If this frame is broadcast or management, use broadcast station id */
+	if (!ieee80211_is_data(fc) || is_multicast_ether_addr(hdr->addr1))
+		sta_id = ctx->bcast_sta_id;
So the reason has to be management frames. But why would those be affecting it? They aren't QoS frames, so there should be no influence on BA handling at all. This is a bit odd.

Quoted 6 lines

Quoted 3 lines
Maybe we should make it clear here that it's about non-data frames and remove the multicast check since that ought to be handled in the other path?
We call iwl_sta_modify_sleep_tx_count(priv, sta_id, 1); for any frame as long as some other conditions are true.

That can only happen in AP mode, and if the station is asleep, I don't think it's a concern.

Quoted 7 lines

Also multicast frames are important. When we send data and multicast frame we probably modify
priv->stations[sta_id].tid[tid]
with wrong sta_id (other station with unicast address), what make related aggregation data wrong for that other station.

Yeah, but multicast is already handled properly, since in that case info->control.sta will be set to NULL by mac80211. Keep in mind that again this is AP mode only, since a client connected to an AP never sends real multicast frames (if it sends 802.3 multicast frames they're still unicast to the AP).

johannes

Stanislaw Gruszka

author

2011-04-29 13:57 UTC #5 lore raw

Quoted 15 lines · Johannes Berg

Quoted 6 lines
Quoted 3 lines
Maybe we should make it clear here that it's about non-data frames and remove the multicast check since that ought to be handled in the other path?
We call iwl_sta_modify_sleep_tx_count(priv, sta_id, 1); for any frame as long as some other conditions are true.
That can only happen in AP mode, and if the station is asleep, I don't think it's a concern.
Quoted 5 lines
Also multicast frames are important. When we send data and multicast frame we probably modify
priv->stations[sta_id].tid[tid]

Quoted 8 lines

Quoted 2 lines
with wrong sta_id (other station with unicast address), what make related aggregation data wrong for that other station.
Yeah, but multicast is already handled properly, since in that case info->control.sta will be set to NULL by mac80211. Keep in mind that again this is AP mode only, since a client connected to an AP never sends real multicast frames (if it sends 802.3 multicast frames they're still unicast to the AP).

We use sta_id for building tx command in:

if (info->control.hw_key)

	iwlagn_tx_cmd_build_hwcrypto(priv, info, tx_cmd, skb, sta_id);

/* TODO need this for burst mode later on */ iwlagn_tx_cmd_build_basic(priv, skb, tx_cmd, info, hdr, sta_id);

so problems seems to be that the firmware do not handle unicast sta_id for management frames. I'll repost patches that check only ieee80211_is_data(fc).

BTW, iwlagn_tx_skb function needs serious cleanup, it's completely unreadable!

Stanislaw