[PATCH wireless-2.6] iwlagn: fix "Received BA when not expected"

[PATCH wireless-2.6] iwlagn: fix "Received BA when not expected"

[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"

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

commit 2a87c26bbe9587baeb9e56d3ce0b4971bd777643
Author: Johannes Berg <johannes.berg@intel.com>
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: stable@kernel.org # 2.6.35+
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/iwlwifi/iwl-agn-tx.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-tx.c b/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
index 494de0e..dfde372 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
@@ -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);
-- 
1.7.1

Re: [PATCH wireless-2.6] iwlagn: fix "Received BA when not expected"

[Johannes Berg]

On Thu, 2011-04-28 at 13:10 +0200, Stanislaw Gruszka wrote:
> 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.

> -	/* 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:

> +	/* 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

Re: [PATCH wireless-2.6] iwlagn: fix "Received BA when not expected"

[Johannes Berg]

On Thu, 2011-04-28 at 13:37 +0200, Johannes Berg wrote:
> On Thu, 2011-04-28 at 13:10 +0200, Stanislaw Gruszka wrote:
> > 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

Re: [PATCH wireless-2.6] iwlagn: fix "Received BA when not expected"

[Stanislaw Gruszka]

On Thu, Apr 28, 2011 at 01:37:41PM +0200, Johannes Berg wrote:
> On Thu, 2011-04-28 at 13:10 +0200, Stanislaw Gruszka wrote:
> > 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.

> > -	/* 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:
> 
> > +	/* 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?

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

Re: [PATCH wireless-2.6] iwlagn: fix "Received BA when not expected"

[Johannes Berg]

On Thu, 2011-04-28 at 13:59 +0200, Stanislaw Gruszka wrote:
> On Thu, Apr 28, 2011 at 01:37:41PM +0200, Johannes Berg wrote:
> > On Thu, 2011-04-28 at 13:10 +0200, Stanislaw Gruszka wrote:
> > > 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.

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

> > > -	/* 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:
> > 
> > > +	/* 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?
> 
> 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.

> 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

Re: [PATCH wireless-2.6] iwlagn: fix "Received BA when not expected"

[Stanislaw Gruszka]

On Thu, Apr 28, 2011 at 02:09:53PM +0200, Johannes Berg wrote:
> > > 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.
> 
> > 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).

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