[PATCH 0/3] mac80211: HT RX reorder buffer cleanup and timeout workaround

[PATCH 0/3] mac80211: HT RX reorder buffer cleanup and timeout workaround

[Jouni Malinen]

There are some cases where frames in RX reorder buffer can get stuck for
extended time due to various errors (implementation bugs, frames getting
lost completely, etc.). The workaround added here seems to make us
recover much more quickly in some of these cases. This does not cover
all cases, but is a useful initial step in avoiding some HT aggregation
issues.

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH 1/3] mac80211: Use a shared function to release frames from RX reorder buf

[Jouni Malinen]

No need to duplicate the same code in two places (and that would be
three after the followup patch).

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 net/mac80211/rx.c |   70 +++++++++++++++++++++++-------------------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

--- wireless-testing.orig/net/mac80211/rx.c	2009-04-29 17:10:10.000000000 +0300
+++ wireless-testing/net/mac80211/rx.c	2009-05-05 20:26:33.000000000 +0300
@@ -2284,6 +2284,34 @@ static inline u16 seq_sub(u16 sq1, u16 s
 }
 
 
+static void ieee80211_release_reorder_frame(struct ieee80211_hw *hw,
+					    struct tid_ampdu_rx *tid_agg_rx,
+					    int index)
+{
+	struct ieee80211_supported_band *sband;
+	struct ieee80211_rate *rate;
+	struct ieee80211_rx_status status;
+
+	if (!tid_agg_rx->reorder_buf[index])
+		goto no_frame;
+
+	/* release the reordered frames to stack */
+	memcpy(&status, tid_agg_rx->reorder_buf[index]->cb, sizeof(status));
+	sband = hw->wiphy->bands[status.band];
+	if (status.flag & RX_FLAG_HT)
+		rate = sband->bitrates; /* TODO: HT rates */
+	else
+		rate = &sband->bitrates[status.rate_idx];
+	__ieee80211_rx_handle_packet(hw, tid_agg_rx->reorder_buf[index],
+				     &status, rate);
+	tid_agg_rx->stored_mpdu_num--;
+	tid_agg_rx->reorder_buf[index] = NULL;
+
+no_frame:
+	tid_agg_rx->head_seq_num = seq_inc(tid_agg_rx->head_seq_num);
+}
+
+
 /*
  * As it function blongs to Rx path it must be called with
  * the proper rcu_read_lock protection for its flow.
@@ -2295,12 +2323,8 @@ static u8 ieee80211_sta_manage_reorder_b
 					   u16 mpdu_seq_num,
 					   int bar_req)
 {
-	struct ieee80211_local *local = hw_to_local(hw);
-	struct ieee80211_rx_status status;
 	u16 head_seq_num, buf_size;
 	int index;
-	struct ieee80211_supported_band *sband;
-	struct ieee80211_rate *rate;
 
 	buf_size = tid_agg_rx->buf_size;
 	head_seq_num = tid_agg_rx->head_seq_num;
@@ -2325,28 +2349,8 @@ static u8 ieee80211_sta_manage_reorder_b
 			index = seq_sub(tid_agg_rx->head_seq_num,
 				tid_agg_rx->ssn)
 				% tid_agg_rx->buf_size;
-
-			if (tid_agg_rx->reorder_buf[index]) {
-				/* release the reordered frames to stack */
-				memcpy(&status,
-					tid_agg_rx->reorder_buf[index]->cb,
-					sizeof(status));
-				sband = local->hw.wiphy->bands[status.band];
-				if (status.flag & RX_FLAG_HT) {
-					/* TODO: HT rates */
-					rate = sband->bitrates;
-				} else {
-					rate = &sband->bitrates
-						[status.rate_idx];
-				}
-				__ieee80211_rx_handle_packet(hw,
-					tid_agg_rx->reorder_buf[index],
-					&status, rate);
-				tid_agg_rx->stored_mpdu_num--;
-				tid_agg_rx->reorder_buf[index] = NULL;
-			}
-			tid_agg_rx->head_seq_num =
-				seq_inc(tid_agg_rx->head_seq_num);
+			ieee80211_release_reorder_frame(hw, tid_agg_rx,
+							index);
 		}
 		if (bar_req)
 			return 1;
@@ -2380,19 +2384,7 @@ static u8 ieee80211_sta_manage_reorder_b
 	index = seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn)
 						% tid_agg_rx->buf_size;
 	while (tid_agg_rx->reorder_buf[index]) {
-		/* release the reordered frame back to stack */
-		memcpy(&status, tid_agg_rx->reorder_buf[index]->cb,
-			sizeof(status));
-		sband = local->hw.wiphy->bands[status.band];
-		if (status.flag & RX_FLAG_HT)
-			rate = sband->bitrates; /* TODO: HT rates */
-		else
-			rate = &sband->bitrates[status.rate_idx];
-		__ieee80211_rx_handle_packet(hw, tid_agg_rx->reorder_buf[index],
-					     &status, rate);
-		tid_agg_rx->stored_mpdu_num--;
-		tid_agg_rx->reorder_buf[index] = NULL;
-		tid_agg_rx->head_seq_num = seq_inc(tid_agg_rx->head_seq_num);
+		ieee80211_release_reorder_frame(hw, tid_agg_rx, index);
 		index =	seq_sub(tid_agg_rx->head_seq_num,
 			tid_agg_rx->ssn) % tid_agg_rx->buf_size;
 	}

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Jouni Malinen]

This patch allows skbs to be released from the RX reorder buffer in
case they have been there for an unexpectedly long time without us
having received the missing frames before them. Previously, these
frames were only released when the reorder window moved and that could
take very long time unless new frames were received constantly (e.g.,
TCP connections could be killed more or less indefinitely).

This situation should not happen very frequently, but it looks like
there are some scenarious that trigger it for some reason. As such,
this should be considered mostly a workaround to speed up recovery
from unexpected siutation that could result in connections hanging for
long periods of time.

The changes here will only check for timeout situation when adding new
RX frames to the reorder buffer. It does not handle all possible
cases, but seems to help for most cases that could result from common
network usage (e.g., TCP retrying at least couple of times). For more
completely coverage, a timer could be used to periodically check
whether there are any frames remaining in the reorder buffer if no new
frames are received.

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 net/mac80211/agg-rx.c   |    8 +++++++-
 net/mac80211/rx.c       |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
 net/mac80211/sta_info.h |    2 ++
 3 files changed, 55 insertions(+), 2 deletions(-)

--- wireless-testing.orig/net/mac80211/agg-rx.c	2009-04-29 17:10:10.000000000 +0300
+++ wireless-testing/net/mac80211/agg-rx.c	2009-05-05 20:21:33.000000000 +0300
@@ -68,6 +68,7 @@ void __ieee80211_stop_rx_ba_session(stru
 	spin_lock_bh(&sta->lock);
 	/* free resources */
 	kfree(sta->ampdu_mlme.tid_rx[tid]->reorder_buf);
+	kfree(sta->ampdu_mlme.tid_rx[tid]->reorder_time);
 
 	if (!sta->ampdu_mlme.tid_rx[tid]->shutdown) {
 		kfree(sta->ampdu_mlme.tid_rx[tid]);
@@ -268,13 +269,18 @@ void ieee80211_process_addba_request(str
 	/* prepare reordering buffer */
 	tid_agg_rx->reorder_buf =
 		kcalloc(buf_size, sizeof(struct sk_buff *), GFP_ATOMIC);
-	if (!tid_agg_rx->reorder_buf) {
+	tid_agg_rx->reorder_time =
+		kcalloc(buf_size, sizeof(unsigned long), GFP_ATOMIC);
+	if (!tid_agg_rx->reorder_buf || !tid_agg_rx->reorder_time) {
 #ifdef CONFIG_MAC80211_HT_DEBUG
 		if (net_ratelimit())
 			printk(KERN_ERR "can not allocate reordering buffer "
 			       "to tid %d\n", tid);
 #endif
+		kfree(tid_agg_rx->reorder_buf);
+		kfree(tid_agg_rx->reorder_time);
 		kfree(sta->ampdu_mlme.tid_rx[tid]);
+		sta->ampdu_mlme.tid_rx[tid] = NULL;
 		goto end;
 	}
 
--- wireless-testing.orig/net/mac80211/sta_info.h	2009-04-21 15:34:55.000000000 +0300
+++ wireless-testing/net/mac80211/sta_info.h	2009-05-05 20:21:33.000000000 +0300
@@ -88,6 +88,7 @@ struct tid_ampdu_tx {
  * struct tid_ampdu_rx - TID aggregation information (Rx).
  *
  * @reorder_buf: buffer to reorder incoming aggregated MPDUs
+ * @reorder_time: jiffies when skb was added
  * @session_timer: check if peer keeps Tx-ing on the TID (by timeout value)
  * @head_seq_num: head sequence number in reordering buffer.
  * @stored_mpdu_num: number of MPDUs in reordering buffer
@@ -99,6 +100,7 @@ struct tid_ampdu_tx {
  */
 struct tid_ampdu_rx {
 	struct sk_buff **reorder_buf;
+	unsigned long *reorder_time;
 	struct timer_list session_timer;
 	u16 head_seq_num;
 	u16 stored_mpdu_num;
--- wireless-testing.orig/net/mac80211/rx.c	2009-05-05 20:21:24.000000000 +0300
+++ wireless-testing/net/mac80211/rx.c	2009-05-05 20:26:31.000000000 +0300
@@ -2313,6 +2313,15 @@ no_frame:
 
 
 /*
+ * Timeout (in jiffies) for skb's that are waiting in the RX reorder buffer. If
+ * the skb was added to the buffer longer than this time ago, the earlier
+ * frames that have not yet been received are assumed to be lost and the skb
+ * can be released for processing. This may also release other skb's from the
+ * reorder buffer if there are no additional gaps between the frames.
+ */
+#define HT_RX_REORDER_BUF_TIMEOUT (HZ / 10)
+
+/*
  * As it function blongs to Rx path it must be called with
  * the proper rcu_read_lock protection for its flow.
  */
@@ -2377,13 +2386,49 @@ static u8 ieee80211_sta_manage_reorder_b
 
 	/* put the frame in the reordering buffer */
 	tid_agg_rx->reorder_buf[index] = skb;
+	tid_agg_rx->reorder_time[index] = jiffies;
 	memcpy(tid_agg_rx->reorder_buf[index]->cb, rxstatus,
 	       sizeof(*rxstatus));
 	tid_agg_rx->stored_mpdu_num++;
 	/* release the buffer until next missing frame */
 	index = seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn)
 						% tid_agg_rx->buf_size;
-	while (tid_agg_rx->reorder_buf[index]) {
+	if (!tid_agg_rx->reorder_buf[index] &&
+	    tid_agg_rx->stored_mpdu_num > 1) {
+		/*
+		 * No buffers ready to be released, but check whether any
+		 * frames in the reorder buffer have timed out.
+		 */
+		int j;
+		int skipped = 1;
+		for (j = (index + 1) % tid_agg_rx->buf_size; j != index;
+		     j = (j + 1) % tid_agg_rx->buf_size) {
+			if (tid_agg_rx->reorder_buf[j] == NULL) {
+				skipped++;
+				continue;
+			}
+			if (!time_after(jiffies, tid_agg_rx->reorder_time[j] +
+					HZ / 10))
+				break;
+
+#ifdef CONFIG_MAC80211_HT_DEBUG
+			if (net_ratelimit())
+				printk(KERN_DEBUG "%s: release an RX reorder "
+				       "frame due to timeout on earlier "
+				       "frames\n",
+				       wiphy_name(hw->wiphy));
+#endif
+			ieee80211_release_reorder_frame(hw, tid_agg_rx, j);
+
+			/*
+			 * Increment the head seq# also for the skipped slots.
+			 */
+			tid_agg_rx->head_seq_num =
+				(tid_agg_rx->head_seq_num + skipped) &
+				SEQ_MASK;
+			skipped = 0;
+		}
+	} else while (tid_agg_rx->reorder_buf[index]) {
 		ieee80211_release_reorder_frame(hw, tid_agg_rx, index);
 		index =	seq_sub(tid_agg_rx->head_seq_num,
 			tid_agg_rx->ssn) % tid_agg_rx->buf_size;

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Marcel Holtmann]

Hi Jouni,

> This patch allows skbs to be released from the RX reorder buffer in
> case they have been there for an unexpectedly long time without us
> having received the missing frames before them. Previously, these
> frames were only released when the reorder window moved and that could
> take very long time unless new frames were received constantly (e.g.,
> TCP connections could be killed more or less indefinitely).
> 
> This situation should not happen very frequently, but it looks like
> there are some scenarious that trigger it for some reason. As such,
> this should be considered mostly a workaround to speed up recovery
> from unexpected siutation that could result in connections hanging for
> long periods of time.

I can confirm that this used to be a regular situation between my X200
and a D-Link access point. I was originally thinking this was a driver
issue, but your patch makes it possible to use wireless-testing tree
again. I am not kidding here, before your patch it was impossible to use
it at all. For me it was a real frequent situation. It is still present,
but now it handles it more gracefully:

Open BA session requested for 00:1c:f0:xx:xx:xx tid 0
iwlagn 0000:03:00.0: iwl_tx_agg_start on ra = 00:1c:f0:xx:xx:xx tid = 0
activated addBA response timer on tid 0
switched off addBA timer for tid 0 
Aggregation is on for tid 0 
Rx A-MPDU request on tid 0 result 0
unexpected AddBA Req from 00:1c:f0:xx:xx:xx on tid 0
phy0: release an RX reorder frame due to timeout on earlier frames
phy0: release an RX reorder frame due to timeout on earlier frames

My VPN connection doesn't always like it, but that could also be a total
different issue with the provider.

Regards

Marcel

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Marcel Holtmann]

Hi Johannes,

> > This patch allows skbs to be released from the RX reorder buffer in
> > case they have been there for an unexpectedly long time without us
> > having received the missing frames before them. Previously, these
> > frames were only released when the reorder window moved and that could
> > take very long time unless new frames were received constantly (e.g.,
> > TCP connections could be killed more or less indefinitely).
> > 
> > This situation should not happen very frequently, but it looks like
> > there are some scenarious that trigger it for some reason. As such,
> > this should be considered mostly a workaround to speed up recovery
> > from unexpected siutation that could result in connections hanging for
> > long periods of time.
> 
> I can confirm that this used to be a regular situation between my X200
> and a D-Link access point. I was originally thinking this was a driver
> issue, but your patch makes it possible to use wireless-testing tree
> again. I am not kidding here, before your patch it was impossible to use
> it at all. For me it was a real frequent situation. It is still present,
> but now it handles it more gracefully:
> 
> Open BA session requested for 00:1c:f0:xx:xx:xx tid 0
> iwlagn 0000:03:00.0: iwl_tx_agg_start on ra = 00:1c:f0:xx:xx:xx tid = 0
> activated addBA response timer on tid 0
> switched off addBA timer for tid 0 
> Aggregation is on for tid 0 
> Rx A-MPDU request on tid 0 result 0
> unexpected AddBA Req from 00:1c:f0:xx:xx:xx on tid 0
> phy0: release an RX reorder frame due to timeout on earlier frames
> phy0: release an RX reorder frame due to timeout on earlier frames

so I finally got the debug output for you. Took only over a day :)

phy0: AddBA: ssn=195, dialog_token=1 tid=0 timeout=0ba_policy=1
addba: d0 00 40 01 00 16 eb 05 46 5c 00 1c f0 62 88 5b 
addba: 00 1c f0 62 88 5b 50 d7 03 00 01 02 10 00 00 30 
addba: 0c 
Rx A-MPDU request on tid 0 result 0
Open BA session requested for 00:1c:f0:xx:xx:xx tid 0
iwlagn 0000:03:00.0: iwl_tx_agg_start on ra = 00:1c:f0:xx:xx:xx tid = 0
activated addBA response timer on tid 0
switched off addBA timer for tid 0 
Aggregation is on for tid 0 
phy0: AddBA: ssn=371, dialog_token=2 tid=0 timeout=0ba_policy=1
addba: d0 00 40 01 00 16 eb 05 46 5c 00 1c f0 62 88 5b 
addba: 00 1c f0 62 88 5b 90 f0 03 00 02 02 10 00 00 30 
addba: 17 
phy0: RX reorder buf: head_seq=371 ssn=195 buf_size=64 stored_mpdu_num=0 timeout=0 index=48; - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
unexpected AddBA Req from 00:1c:f0:xx:xx:xx on tid 0
phy0: release an RX reorder frame due to timeout on earlier frames
phy0: RX reorder buf: head_seq=2001 ssn=195 buf_size=64 stored_mpdu_num=3 timeout=0 index=14; - - - - - - - - - - - - - - - - 236 229 0 - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
phy0: release an RX reorder frame due to timeout on earlier frames
phy0: RX reorder buf: head_seq=2004 ssn=195 buf_size=64 stored_mpdu_num=2 timeout=0 index=17; - - - - - - - - - - - - - - - - - 229 0 - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
phy0: release an RX reorder frame due to timeout on earlier frames
phy0: RX reorder buf: head_seq=2197 ssn=195 buf_size=64 stored_mpdu_num=3 timeout=0 index=18; - - - - - - - - - - - - - - - - - - - 124 124 0 - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
phy0: release an RX reorder frame due to timeout on earlier frames
phy0: RX reorder buf: head_seq=2199 ssn=195 buf_size=64 stored_mpdu_num=2 timeout=0 index=20; - - - - - - - - - - - - - - - - - - - - 124 0 - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Regards

Marcel

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Jouni Malinen]

On Fri, May 08, 2009 at 06:48:23PM -0700, Marcel Holtmann wrote:

> > I can confirm that this used to be a regular situation between my X200
> > and a D-Link access point.

Which D-Link model is that AP? I think I should try to get one of those
added to me test bed.. ;-)

> so I finally got the debug output for you. Took only over a day :)

Thanks! Would you happen to have timing information available for these
(e.g., from klogd)? It looks like the AP is sending out an ADDBA Request
to update some parameters, but we currently ignore that request.
However, at least in this particular case, our RX reorder bug head_seq
matches with the ssn from the ADDBA Request, so I'm not sure whether
ignoring the ADDBA contents is really causing harm here (anyway, we
should really process these updates, too).

The timeouts on RX reorder frames look similar to what I have seen in my
tests and the workaround was indeed trying to address that type of
issue, so it is nice to hear that it helped in this case, too.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Marcel Holtmann]

Hi Jouni,

> > > I can confirm that this used to be a regular situation between my X200
> > > and a D-Link access point.
> 
> Which D-Link model is that AP? I think I should try to get one of those
> added to me test bed.. ;-)

from the WPS information element:

	 * Version: 1.0
	 * Manufacturer: D-Link Systems
	 * Model: DIR-615
	 * Device name: Wireless N Router
	 * Config methods: Label

I bought it over a year ago in Canada. It is actually my second of these
since I fried the first one :)

> > so I finally got the debug output for you. Took only over a day :)
> 
> Thanks! Would you happen to have timing information available for these
> (e.g., from klogd)? It looks like the AP is sending out an ADDBA Request
> to update some parameters, but we currently ignore that request.
> However, at least in this particular case, our RX reorder bug head_seq
> matches with the ssn from the ADDBA Request, so I'm not sure whether
> ignoring the ADDBA contents is really causing harm here (anyway, we
> should really process these updates, too).
> 
> The timeouts on RX reorder frames look similar to what I have seen in my
> tests and the workaround was indeed trying to address that type of
> issue, so it is nice to hear that it helped in this case, too.

I don't have the timing details since my Fedora for some reason doesn't
log them to the filesystem. I have to fix that.

Also re-producing this is not as simple as you think. I have no idea
when this happens. There is no way to actually trigger it from my side.

Regards

Marcel

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Jouni Malinen]

On Sat, May 09, 2009 at 10:05:53AM -0700, Marcel Holtmann wrote:

> from the WPS information element:
> 
> 	 * Version: 1.0
> 	 * Manufacturer: D-Link Systems
> 	 * Model: DIR-615
> 	 * Device name: Wireless N Router
> 	 * Config methods: Label
> 
> I bought it over a year ago in Canada. It is actually my second of these
> since I fried the first one :)

Thanks! I do actually have a DIR-615. However, it may be completely
different WLAN chipset depending on the hardware revision (I love the
vendors who do this..). If you have a chance, could you please take a
look at which hardware revision your AP is? It is likely in the label on
the AP (something like "H/W Ver.: D1  F/W Ver.: 4.00).

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Marcel Holtmann]

Hi Jouni,

> > from the WPS information element:
> > 
> > 	 * Version: 1.0
> > 	 * Manufacturer: D-Link Systems
> > 	 * Model: DIR-615
> > 	 * Device name: Wireless N Router
> > 	 * Config methods: Label
> > 
> > I bought it over a year ago in Canada. It is actually my second of these
> > since I fried the first one :)
> 
> Thanks! I do actually have a DIR-615. However, it may be completely
> different WLAN chipset depending on the hardware revision (I love the
> vendors who do this..). If you have a chance, could you please take a
> look at which hardware revision your AP is? It is likely in the label on
> the AP (something like "H/W Ver.: D1  F/W Ver.: 4.00).

it is Hardware Version: B2 and Firmware Version: 2.23.

Regards

Marcel

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Dan Williams]

On Sun, 2009-05-10 at 14:07 -0700, Marcel Holtmann wrote:
> Hi Jouni,
> 
> > > from the WPS information element:
> > > 
> > > 	 * Version: 1.0
> > > 	 * Manufacturer: D-Link Systems
> > > 	 * Model: DIR-615
> > > 	 * Device name: Wireless N Router
> > > 	 * Config methods: Label
> > > 
> > > I bought it over a year ago in Canada. It is actually my second of these
> > > since I fried the first one :)
> > 
> > Thanks! I do actually have a DIR-615. However, it may be completely
> > different WLAN chipset depending on the hardware revision (I love the
> > vendors who do this..). If you have a chance, could you please take a
> > look at which hardware revision your AP is? It is likely in the label on
> > the AP (something like "H/W Ver.: D1  F/W Ver.: 4.00).
> 
> it is Hardware Version: B2 and Firmware Version: 2.23.

B and later uses a Ubicom integrated chipset; A series was Marvel
TopDog.

Dan

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Marcel Holtmann]

Hi Jouni,

> > so I finally got the debug output for you. Took only over a day :)
> 
> Thanks! Would you happen to have timing information available for these
> (e.g., from klogd)? It looks like the AP is sending out an ADDBA Request
> to update some parameters, but we currently ignore that request.
> However, at least in this particular case, our RX reorder bug head_seq
> matches with the ssn from the ADDBA Request, so I'm not sure whether
> ignoring the ADDBA contents is really causing harm here (anyway, we
> should really process these updates, too).
> 
> The timeouts on RX reorder frames look similar to what I have seen in my
> tests and the workaround was indeed trying to address that type of
> issue, so it is nice to hear that it helped in this case, too.

so I enabled timing information for dmesg and got another report. It is
not exactly the same and I don't know how and why it happened.

[  249.579036] phy0: AddBA: ssn=50, dialog_token=1 tid=0 timeout=0ba_policy=1
[  249.579046] addba: d0 00 40 01 00 16 eb 05 46 5c 00 1c f0 62 88 5b 
[  249.579052] addba: 00 1c f0 62 88 5b 30 8f 03 00 01 02 10 00 00 20 
[  249.579057] addba: 03 
[  249.579073] Rx A-MPDU request on tid 0 result 0
[  250.572022] phy0: AddBA: ssn=178, dialog_token=2 tid=0 timeout=0ba_policy=1
[  250.572032] addba: d0 00 40 01 00 16 eb 05 46 5c 00 1c f0 62 88 5b 
[  250.572038] addba: 00 1c f0 62 88 5b e0 8f 03 00 02 02 10 00 00 20 
[  250.572042] addba: 0b 
[  250.572048] phy0: RX reorder buf: head_seq=178 ssn=50 buf_size=64 stored_mpdu_num=0 timeout=0 index=0; - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[  250.572108] unexpected AddBA Req from 00:1c:f0:xx:xx:xx on tid 0
[  266.263651] Open BA session requested for 00:1c:f0:xx:xx:xx tid 0
[  266.263659] iwlagn 0000:03:00.0: iwl_tx_agg_start on ra = 00:1c:f0:xx:xx:xx tid = 0
[  266.263726] activated addBA response timer on tid 0
[  266.265534] wrong addBA response token, tid 0
[  267.263467] addBA response timer expired on tid 0
[  267.263516] Stopping Tx BA session for 00:1c:f0:xx:xx:xx tid 0
[  326.083081] Open BA session requested for 00:1c:f0:xx:xx:xx tid 0
[  326.083097] iwlagn 0000:03:00.0: iwl_tx_agg_start on ra = 00:1c:f0:xx:xx:xx tid = 0
[  326.083197] activated addBA response timer on tid 0
[  326.088517] wrong addBA response token, tid 0
[  327.080206] addBA response timer expired on tid 0
[  327.080262] Stopping Tx BA session for 00:1c:f0:xx:xx:xx tid 0

This kernel is running latest wireless-testing.git with debug patch and
the disassoc change (for testing) from Johannes.

Regards

Marcel

Re: [PATCH 2/3] mac80211: Add a timeout for frames in the RX reorder buffer

[Marcel Holtmann]

Hi Jouni,

> > > so I finally got the debug output for you. Took only over a day :)
> > 
> > Thanks! Would you happen to have timing information available for these
> > (e.g., from klogd)? It looks like the AP is sending out an ADDBA Request
> > to update some parameters, but we currently ignore that request.
> > However, at least in this particular case, our RX reorder bug head_seq
> > matches with the ssn from the ADDBA Request, so I'm not sure whether
> > ignoring the ADDBA contents is really causing harm here (anyway, we
> > should really process these updates, too).
> > 
> > The timeouts on RX reorder frames look similar to what I have seen in my
> > tests and the workaround was indeed trying to address that type of
> > issue, so it is nice to hear that it helped in this case, too.
> 
> so I enabled timing information for dmesg and got another report. It is
> not exactly the same and I don't know how and why it happened.
> 
> [  249.579036] phy0: AddBA: ssn=50, dialog_token=1 tid=0 timeout=0ba_policy=1
> [  249.579046] addba: d0 00 40 01 00 16 eb 05 46 5c 00 1c f0 62 88 5b 
> [  249.579052] addba: 00 1c f0 62 88 5b 30 8f 03 00 01 02 10 00 00 20 
> [  249.579057] addba: 03 
> [  249.579073] Rx A-MPDU request on tid 0 result 0
> [  250.572022] phy0: AddBA: ssn=178, dialog_token=2 tid=0 timeout=0ba_policy=1
> [  250.572032] addba: d0 00 40 01 00 16 eb 05 46 5c 00 1c f0 62 88 5b 
> [  250.572038] addba: 00 1c f0 62 88 5b e0 8f 03 00 02 02 10 00 00 20 
> [  250.572042] addba: 0b 
> [  250.572048] phy0: RX reorder buf: head_seq=178 ssn=50 buf_size=64 stored_mpdu_num=0 timeout=0 index=0; - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
> [  250.572108] unexpected AddBA Req from 00:1c:f0:xx:xx:xx on tid 0
> [  266.263651] Open BA session requested for 00:1c:f0:xx:xx:xx tid 0
> [  266.263659] iwlagn 0000:03:00.0: iwl_tx_agg_start on ra = 00:1c:f0:xx:xx:xx tid = 0
> [  266.263726] activated addBA response timer on tid 0
> [  266.265534] wrong addBA response token, tid 0
> [  267.263467] addBA response timer expired on tid 0
> [  267.263516] Stopping Tx BA session for 00:1c:f0:xx:xx:xx tid 0
> [  326.083081] Open BA session requested for 00:1c:f0:xx:xx:xx tid 0
> [  326.083097] iwlagn 0000:03:00.0: iwl_tx_agg_start on ra = 00:1c:f0:xx:xx:xx tid = 0
> [  326.083197] activated addBA response timer on tid 0
> [  326.088517] wrong addBA response token, tid 0
> [  327.080206] addBA response timer expired on tid 0
> [  327.080262] Stopping Tx BA session for 00:1c:f0:xx:xx:xx tid 0
> 
> This kernel is running latest wireless-testing.git with debug patch and
> the disassoc change (for testing) from Johannes.

I had problems with my DIR-615 and Intel 5350 card. No stable connection
at all. No idea why it breaks down. Worked fine for the last few days
and today it breaks after a few bytes. Maybe my environment is not clean
enough. So I switched to use a Time Capsule in A-band only mode. Seems
more stable with exactly the same kernel. The reorder details from the
debug patch are still showing up:

[20236.165674] Open BA session requested for 00:1f:f3:xx:x:xx tid 0
[20236.165688] iwlagn 0000:03:00.0: iwl_tx_agg_start on ra = 00:1f:f3:xx:xx:xx tid = 0
[20236.165769] activated addBA response timer on tid 0
[20236.166197] switched off addBA timer for tid 0 
[20236.166202] Aggregation is on for tid 0 
[21108.747984] phy0: AddBA: ssn=347, dialog_token=1 tid=0 timeout=0ba_policy=1
[21108.747994] addba: d0 00 3c 00 00 16 eb 05 46 5c 00 1f f3 c3 a3 21 
[21108.748000] addba: 00 1f f3 c3 a3 21 20 7e 03 00 01 02 10 00 00 b0 
[21108.748005] addba: 15 
[21108.748021] Rx A-MPDU request on tid 0 result 0
[21108.996253] phy0: AddBA: ssn=609, dialog_token=2 tid=0 timeout=0ba_policy=1
[21108.996264] addba: d0 00 3c 00 00 16 eb 05 46 5c 00 1f f3 c3 a3 21 
[21108.996270] addba: 00 1f f3 c3 a3 21 50 7e 03 00 02 02 10 00 00 10 
[21108.996274] addba: 26 
[21108.996281] phy0: RX reorder buf: head_seq=609 ssn=347 buf_size=64 stored_mpdu_num=0 timeout=0 index=6; - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[21108.996342] unexpected AddBA Req from 00:1f:f3:xx:xx:xx on tid 0
[21114.164106] phy0: AddBA: ssn=686, dialog_token=3 tid=1 timeout=0ba_policy=1
[21114.164110] addba: d0 00 3c 00 00 16 eb 05 46 5c 00 1f f3 c3 a3 21 
[21114.164112] addba: 00 1f f3 c3 a3 21 f0 81 03 00 03 06 10 00 00 e0 
[21114.164114] addba: 2a 
[21114.164122] Rx A-MPDU request on tid 1 result 0
[21122.003948] delba from 00:1f:f3:c3:a3:21 (initiator) tid 1 reason code 1
[21122.003958] Rx BA session stop requested for 00:1f:f3:xx:xx:xx tid 1

Regards

Marcel

[PATCH 3/3] mac80211: Comment the order of HT RX reorder handler vs. RX handlers

[Jouni Malinen]

We are currently processing block ack reordering as a separate task
before all other RX handlers. In theory, this is wrong since this step
should be done only after duplicate removal (see Figure 6-1 in IEEE
802.11n). However, moving this needs some work and the current
situation is not too bad. Add a comment here so that this small detail
does not get forgotten and who knows, maybe someone has some extra
time to take a look at cleaning this up.

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

---
 net/mac80211/rx.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

--- wireless-testing.orig/net/mac80211/rx.c	2009-05-05 20:21:33.000000000 +0300
+++ wireless-testing/net/mac80211/rx.c	2009-05-05 20:21:39.000000000 +0300
@@ -2551,6 +2551,18 @@ void __ieee80211_rx(struct ieee80211_hw 
 		return;
 	}
 
+	/*
+	 * In theory, the block ack reordering should happen after duplicate
+	 * removal (ieee80211_rx_h_check(), which is an RX handler). As such,
+	 * the call to ieee80211_rx_reorder_ampdu() should really be moved to
+	 * happen as a new RX handler between ieee80211_rx_h_check and
+	 * ieee80211_rx_h_decrypt. This cleanup may eventually happen, but for
+	 * the time being, the call can be here since RX reorder buf processing
+	 * will implicitly skip duplicates. We could, in theory at least,
+	 * process frames that ieee80211_rx_h_passive_scan would drop (e.g.,
+	 * frames from other than operational channel), but that should not
+	 * happen in normal networks.
+	 */
 	if (!ieee80211_rx_reorder_ampdu(local, skb, status))
 		__ieee80211_rx_handle_packet(hw, skb, status, rate);
 

-- 

-- 
Jouni Malinen                                            PGP id EFC895FA