[PATCH 0/8] mac80211 aggregation improvements

[PATCH 0/8] mac80211 aggregation improvements

[Johannes Berg]

Hi,
   
Here's my patch set for improving aggregation, it contains

 * mac80211: rewrite fragmentation
 * mac80211: fix A-MPDU queue assignment
 * mac80211: rework the pending packets code
 * mac80211: clean up __ieee80211_tx args   
 * mac80211: unify and fix TX aggregation start
 * mac80211: add skb length sanity checking
 * mac80211: fix aggregation to not require queue stop
 * mac80211/iwlwifi: move virtual A-MDPU queue bookkeeping to iwlwifi

The skb length sanity checking isn't strictly part of this series,
but it would help debug things like the zd1211 problem Jouni fixed
recently, and I wanted to do it after the third patch due to the  
amount of code changes in that.

The rest of the code changes fix/rework some things related to
pending frames and fragmentation (to be able to work with that
code for pending aggregation frames), clean up some aggregation
code and finally remove a lot of the code that was necessary for
trying to handle hardware aggregation queues in mac80211 -- the 
code for handling it in the driver is much simpler (because it  
can be less generic) too.

Despite adding new features, long comments and reworking things
the diffstat thus looks fairly good with not many new code lines
in total:

 drivers/net/wireless/ath9k/main.c           |    2
 drivers/net/wireless/iwlwifi/iwl-3945.c     |    2
 drivers/net/wireless/iwlwifi/iwl-4965.c     |    7
 drivers/net/wireless/iwlwifi/iwl-5000.c     |    7
 drivers/net/wireless/iwlwifi/iwl-core.c     |    3
 drivers/net/wireless/iwlwifi/iwl-dev.h      |    6
 drivers/net/wireless/iwlwifi/iwl-helpers.h  |   52 ++
 drivers/net/wireless/iwlwifi/iwl-tx.c       |    8   
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    2   
 drivers/net/wireless/mac80211_hwsim.c       |    1   
 include/net/mac80211.h                      |   22   
 net/mac80211/agg-tx.c                       |  243 ++++-----
 net/mac80211/ieee80211_i.h                  |   31 -
 net/mac80211/main.c                         |   13  
 net/mac80211/sta_info.c                     |   17  
 net/mac80211/sta_info.h                     |    4  
 net/mac80211/tx.c                           |  704 +++++++++++++++-------------
 net/mac80211/util.c                         |   97 +--
 net/mac80211/wep.c                          |   21    
 net/mac80211/wpa.c                          |   28 -  
 20 files changed, 656 insertions(+), 614 deletions(-) 


I've tested this with various Intel hardware, Sujith has also
tested with ath9k hardware (thanks!)

johannes

[PATCH 1/8] mac80211: rewrite fragmentation

[Johannes Berg]

Fragmentation currently uses an allocated array to store the
fragment skbs, and then keeps track of which have been sent
and which are still pending etc. This is rather complicated;
make it simpler by just chaining the fragments into skb->next
and removing from that list when sent. Also simplifies all
code that needs to touch fragments, since it now only needs
to walk the skb->next list.

This is a prerequisite for fixing the stored packet code,
which I need to do for proper aggregation packet storing.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/ieee80211_i.h |    7 
 net/mac80211/tx.c          |  328 +++++++++++++++++++++------------------------
 net/mac80211/util.c        |   17 --
 net/mac80211/wep.c         |   21 --
 net/mac80211/wpa.c         |   28 +--
 5 files changed, 175 insertions(+), 226 deletions(-)

--- wireless-testing.orig/net/mac80211/ieee80211_i.h	2009-03-21 17:06:34.000000000 +0100
+++ wireless-testing/net/mac80211/ieee80211_i.h	2009-03-21 17:08:48.000000000 +0100
@@ -149,11 +149,6 @@ struct ieee80211_tx_data {
 
 	struct ieee80211_channel *channel;
 
-	/* Extra fragments (in addition to the first fragment
-	 * in skb) */
-	struct sk_buff **extra_frag;
-	int num_extra_frag;
-
 	u16 ethertype;
 	unsigned int flags;
 };
@@ -191,8 +186,6 @@ struct ieee80211_rx_data {
 
 struct ieee80211_tx_stored_packet {
 	struct sk_buff *skb;
-	struct sk_buff **extra_frag;
-	int num_extra_frag;
 };
 
 struct beacon_data {
--- wireless-testing.orig/net/mac80211/tx.c	2009-03-21 17:06:18.000000000 +0100
+++ wireless-testing/net/mac80211/tx.c	2009-03-21 17:08:48.000000000 +0100
@@ -34,8 +34,7 @@
 
 #define IEEE80211_TX_OK		0
 #define IEEE80211_TX_AGAIN	1
-#define IEEE80211_TX_FRAG_AGAIN	2
-#define IEEE80211_TX_PENDING	3
+#define IEEE80211_TX_PENDING	2
 
 /* misc utils */
 
@@ -702,17 +701,62 @@ ieee80211_tx_h_sequence(struct ieee80211
 	return TX_CONTINUE;
 }
 
+static int ieee80211_fragment(struct ieee80211_local *local,
+			      struct sk_buff *skb, int hdrlen,
+			      int frag_threshold)
+{
+	struct sk_buff *tail = skb, *tmp;
+	int per_fragm = frag_threshold - hdrlen - FCS_LEN;
+	int pos = hdrlen + per_fragm;
+	int rem = skb->len - hdrlen - per_fragm;
+
+	if (WARN_ON(rem < 0))
+		return -EINVAL;
+
+	while (rem) {
+		int fraglen = per_fragm;
+
+		if (fraglen > rem)
+			fraglen = rem;
+		rem -= fraglen;
+		tmp = dev_alloc_skb(local->tx_headroom +
+				    frag_threshold +
+				    IEEE80211_ENCRYPT_HEADROOM +
+				    IEEE80211_ENCRYPT_TAILROOM);
+		if (!tmp)
+			return -ENOMEM;
+		tail->next = tmp;
+		tail = tmp;
+		skb_reserve(tmp, local->tx_headroom +
+				 IEEE80211_ENCRYPT_HEADROOM);
+		/* copy control information */
+		memcpy(tmp->cb, skb->cb, sizeof(tmp->cb));
+		skb_copy_queue_mapping(tmp, skb);
+		tmp->priority = skb->priority;
+		tmp->do_not_encrypt = skb->do_not_encrypt;
+		tmp->dev = skb->dev;
+		tmp->iif = skb->iif;
+
+		/* copy header and data */
+		memcpy(skb_put(tmp, hdrlen), skb->data, hdrlen);
+		memcpy(skb_put(tmp, fraglen), skb->data + pos, fraglen);
+
+		pos += fraglen;
+	}
+
+	skb->len = hdrlen + per_fragm;
+	return 0;
+}
+
 static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_fragment(struct ieee80211_tx_data *tx)
 {
-	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
-	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
-	size_t hdrlen, per_fragm, num_fragm, payload_len, left;
-	struct sk_buff **frags, *first, *frag;
-	int i;
-	u16 seq;
-	u8 *pos;
+	struct sk_buff *skb = tx->skb;
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+	struct ieee80211_hdr *hdr = (void *)skb->data;
 	int frag_threshold = tx->local->fragmentation_threshold;
+	int hdrlen;
+	int fragnum;
 
 	if (!(tx->flags & IEEE80211_TX_FRAGMENTED))
 		return TX_CONTINUE;
@@ -725,58 +769,35 @@ ieee80211_tx_h_fragment(struct ieee80211
 	if (WARN_ON(info->flags & IEEE80211_TX_CTL_AMPDU))
 		return TX_DROP;
 
-	first = tx->skb;
-
 	hdrlen = ieee80211_hdrlen(hdr->frame_control);
-	payload_len = first->len - hdrlen;
-	per_fragm = frag_threshold - hdrlen - FCS_LEN;
-	num_fragm = DIV_ROUND_UP(payload_len, per_fragm);
-
-	frags = kzalloc(num_fragm * sizeof(struct sk_buff *), GFP_ATOMIC);
-	if (!frags)
-		goto fail;
 
-	hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_MOREFRAGS);
-	seq = le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_SEQ;
-	pos = first->data + hdrlen + per_fragm;
-	left = payload_len - per_fragm;
-	for (i = 0; i < num_fragm - 1; i++) {
-		struct ieee80211_hdr *fhdr;
-		size_t copylen;
-
-		if (left <= 0)
-			goto fail;
+	/* internal error, why is TX_FRAGMENTED set? */
+	if (WARN_ON(skb->len <= frag_threshold))
+		return TX_DROP;
 
-		/* reserve enough extra head and tail room for possible
-		 * encryption */
-		frag = frags[i] =
-			dev_alloc_skb(tx->local->tx_headroom +
-				      frag_threshold +
-				      IEEE80211_ENCRYPT_HEADROOM +
-				      IEEE80211_ENCRYPT_TAILROOM);
-		if (!frag)
-			goto fail;
+	/*
+	 * Now fragment the frame. This will allocate all the fragments and
+	 * chain them (using skb as the first fragment) to skb->next.
+	 * During transmission, we will remove the successfully transmitted
+	 * fragments from this list. When the low-level driver rejects one
+	 * of the fragments then we will simply pretend to accept the skb
+	 * but store it away as pending.
+	 */
+	if (ieee80211_fragment(tx->local, skb, hdrlen, frag_threshold))
+		return TX_DROP;
 
-		/* Make sure that all fragments use the same priority so
-		 * that they end up using the same TX queue */
-		frag->priority = first->priority;
-
-		skb_reserve(frag, tx->local->tx_headroom +
-				  IEEE80211_ENCRYPT_HEADROOM);
-
-		/* copy TX information */
-		info = IEEE80211_SKB_CB(frag);
-		memcpy(info, first->cb, sizeof(frag->cb));
-
-		/* copy/fill in 802.11 header */
-		fhdr = (struct ieee80211_hdr *) skb_put(frag, hdrlen);
-		memcpy(fhdr, first->data, hdrlen);
-		fhdr->seq_ctrl = cpu_to_le16(seq | ((i + 1) & IEEE80211_SCTL_FRAG));
-
-		if (i == num_fragm - 2) {
-			/* clear MOREFRAGS bit for the last fragment */
-			fhdr->frame_control &= cpu_to_le16(~IEEE80211_FCTL_MOREFRAGS);
-		} else {
+	/* update duration/seq/flags of fragments */
+	fragnum = 0;
+	do {
+		int next_len;
+		const __le16 morefrags = cpu_to_le16(IEEE80211_FCTL_MOREFRAGS);
+
+		hdr = (void *)skb->data;
+		info = IEEE80211_SKB_CB(skb);
+
+		if (skb->next) {
+			hdr->frame_control |= morefrags;
+			next_len = skb->next->len;
 			/*
 			 * No multi-rate retries for fragmented frames, that
 			 * would completely throw off the NAV at other STAs.
@@ -787,37 +808,16 @@ ieee80211_tx_h_fragment(struct ieee80211
 			info->control.rates[4].idx = -1;
 			BUILD_BUG_ON(IEEE80211_TX_MAX_RATES != 5);
 			info->flags &= ~IEEE80211_TX_CTL_RATE_CTRL_PROBE;
+		} else {
+			hdr->frame_control &= ~morefrags;
+			next_len = 0;
 		}
-
-		/* copy data */
-		copylen = left > per_fragm ? per_fragm : left;
-		memcpy(skb_put(frag, copylen), pos, copylen);
-
-		skb_copy_queue_mapping(frag, first);
-
-		frag->do_not_encrypt = first->do_not_encrypt;
-		frag->dev = first->dev;
-		frag->iif = first->iif;
-
-		pos += copylen;
-		left -= copylen;
-	}
-	skb_trim(first, hdrlen + per_fragm);
-
-	tx->num_extra_frag = num_fragm - 1;
-	tx->extra_frag = frags;
+		hdr->duration_id = ieee80211_duration(tx, 0, next_len);
+		hdr->seq_ctrl |= cpu_to_le16(fragnum & IEEE80211_SCTL_FRAG);
+		fragnum++;
+	} while ((skb = skb->next));
 
 	return TX_CONTINUE;
-
- fail:
-	if (frags) {
-		for (i = 0; i < num_fragm - 1; i++)
-			if (frags[i])
-				dev_kfree_skb(frags[i]);
-		kfree(frags);
-	}
-	I802_DEBUG_INC(tx->local->tx_handlers_drop_fragment);
-	return TX_DROP;
 }
 
 static ieee80211_tx_result debug_noinline
@@ -845,27 +845,19 @@ ieee80211_tx_h_encrypt(struct ieee80211_
 static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_calculate_duration(struct ieee80211_tx_data *tx)
 {
-	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
-	int next_len, i;
-	int group_addr = is_multicast_ether_addr(hdr->addr1);
-
-	if (!(tx->flags & IEEE80211_TX_FRAGMENTED)) {
-		hdr->duration_id = ieee80211_duration(tx, group_addr, 0);
-		return TX_CONTINUE;
-	}
-
-	hdr->duration_id = ieee80211_duration(tx, group_addr,
-					      tx->extra_frag[0]->len);
-
-	for (i = 0; i < tx->num_extra_frag; i++) {
-		if (i + 1 < tx->num_extra_frag)
-			next_len = tx->extra_frag[i + 1]->len;
-		else
-			next_len = 0;
+	struct sk_buff *skb = tx->skb;
+	struct ieee80211_hdr *hdr;
+	int next_len;
+	bool group_addr;
 
-		hdr = (struct ieee80211_hdr *)tx->extra_frag[i]->data;
-		hdr->duration_id = ieee80211_duration(tx, 0, next_len);
-	}
+	do {
+		hdr = (void *) skb->data;
+		next_len = skb->next ? skb->next->len : 0;
+		group_addr = is_multicast_ether_addr(hdr->addr1);
+
+		hdr->duration_id =
+			ieee80211_duration(tx, group_addr, next_len);
+	} while ((skb = skb->next));
 
 	return TX_CONTINUE;
 }
@@ -873,19 +865,16 @@ ieee80211_tx_h_calculate_duration(struct
 static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_stats(struct ieee80211_tx_data *tx)
 {
-	int i;
+	struct sk_buff *skb = tx->skb;
 
 	if (!tx->sta)
 		return TX_CONTINUE;
 
 	tx->sta->tx_packets++;
-	tx->sta->tx_fragments++;
-	tx->sta->tx_bytes += tx->skb->len;
-	if (tx->extra_frag) {
-		tx->sta->tx_fragments += tx->num_extra_frag;
-		for (i = 0; i < tx->num_extra_frag; i++)
-			tx->sta->tx_bytes += tx->extra_frag[i]->len;
-	}
+	do {
+		tx->sta->tx_fragments++;
+		tx->sta->tx_bytes += skb->len;
+	} while ((skb = skb->next));
 
 	return TX_CONTINUE;
 }
@@ -1099,45 +1088,36 @@ static int ieee80211_tx_prepare(struct i
 	return 0;
 }
 
-static int __ieee80211_tx(struct ieee80211_local *local, struct sk_buff *skb,
+static int __ieee80211_tx(struct ieee80211_local *local,
 			  struct ieee80211_tx_data *tx)
 {
+	struct sk_buff *skb = tx->skb, *next;
 	struct ieee80211_tx_info *info;
-	int ret, i;
+	int ret;
+	bool fragm = false;
 
-	if (skb) {
+	local->mdev->trans_start = jiffies;
+
+	while (skb) {
 		if (ieee80211_queue_stopped(&local->hw,
 					    skb_get_queue_mapping(skb)))
 			return IEEE80211_TX_PENDING;
 
-		ret = local->ops->tx(local_to_hw(local), skb);
-		if (ret)
-			return IEEE80211_TX_AGAIN;
-		local->mdev->trans_start = jiffies;
-		ieee80211_led_tx(local, 1);
-	}
-	if (tx->extra_frag) {
-		for (i = 0; i < tx->num_extra_frag; i++) {
-			if (!tx->extra_frag[i])
-				continue;
-			info = IEEE80211_SKB_CB(tx->extra_frag[i]);
+		if (fragm) {
+			info = IEEE80211_SKB_CB(skb);
 			info->flags &= ~(IEEE80211_TX_CTL_CLEAR_PS_FILT |
 					 IEEE80211_TX_CTL_FIRST_FRAGMENT);
-			if (ieee80211_queue_stopped(&local->hw,
-					skb_get_queue_mapping(tx->extra_frag[i])))
-				return IEEE80211_TX_FRAG_AGAIN;
-
-			ret = local->ops->tx(local_to_hw(local),
-					    tx->extra_frag[i]);
-			if (ret)
-				return IEEE80211_TX_FRAG_AGAIN;
-			local->mdev->trans_start = jiffies;
-			ieee80211_led_tx(local, 1);
-			tx->extra_frag[i] = NULL;
 		}
-		kfree(tx->extra_frag);
-		tx->extra_frag = NULL;
+
+		next = skb->next;
+		ret = local->ops->tx(local_to_hw(local), skb);
+		if (ret != NETDEV_TX_OK)
+			return IEEE80211_TX_AGAIN;
+		tx->skb = skb = next;
+		ieee80211_led_tx(local, 1);
+		fragm = true;
 	}
+
 	return IEEE80211_TX_OK;
 }
 
@@ -1149,7 +1129,6 @@ static int invoke_tx_handlers(struct iee
 {
 	struct sk_buff *skb = tx->skb;
 	ieee80211_tx_result res = TX_DROP;
-	int i;
 
 #define CALL_TXH(txh)		\
 	res = txh(tx);		\
@@ -1173,11 +1152,13 @@ static int invoke_tx_handlers(struct iee
  txh_done:
 	if (unlikely(res == TX_DROP)) {
 		I802_DEBUG_INC(tx->local->tx_handlers_drop);
-		dev_kfree_skb(skb);
-		for (i = 0; i < tx->num_extra_frag; i++)
-			if (tx->extra_frag[i])
-				dev_kfree_skb(tx->extra_frag[i]);
-		kfree(tx->extra_frag);
+		while (skb) {
+			struct sk_buff *next;
+
+			next = skb->next;
+			dev_kfree_skb(skb);
+			skb = next;
+		}
 		return -1;
 	} else if (unlikely(res == TX_QUEUED)) {
 		I802_DEBUG_INC(tx->local->tx_handlers_queued);
@@ -1194,7 +1175,7 @@ static int ieee80211_tx(struct net_devic
 	struct ieee80211_tx_data tx;
 	ieee80211_tx_result res_prepare;
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-	int ret, i;
+	int ret;
 	u16 queue;
 
 	queue = skb_get_queue_mapping(skb);
@@ -1225,7 +1206,7 @@ static int ieee80211_tx(struct net_devic
 		goto out;
 
 retry:
-	ret = __ieee80211_tx(local, skb, &tx);
+	ret = __ieee80211_tx(local, &tx);
 	if (ret) {
 		struct ieee80211_tx_stored_packet *store;
 
@@ -1240,9 +1221,6 @@ retry:
 
 		store = &local->pending_packet[queue];
 
-		if (ret == IEEE80211_TX_FRAG_AGAIN)
-			skb = NULL;
-
 		set_bit(queue, local->queues_pending);
 		smp_mb();
 		/*
@@ -1260,22 +1238,23 @@ retry:
 			clear_bit(queue, local->queues_pending);
 			goto retry;
 		}
-		store->skb = skb;
-		store->extra_frag = tx.extra_frag;
-		store->num_extra_frag = tx.num_extra_frag;
+		store->skb = tx.skb;
 	}
  out:
 	rcu_read_unlock();
 	return 0;
 
  drop:
-	if (skb)
-		dev_kfree_skb(skb);
-	for (i = 0; i < tx.num_extra_frag; i++)
-		if (tx.extra_frag[i])
-			dev_kfree_skb(tx.extra_frag[i]);
-	kfree(tx.extra_frag);
 	rcu_read_unlock();
+
+	skb = tx.skb;
+	while (skb) {
+		struct sk_buff *next;
+
+		next = skb->next;
+		dev_kfree_skb(skb);
+		skb = next;
+	}
 	return 0;
 }
 
@@ -1810,17 +1789,21 @@ int ieee80211_subif_start_xmit(struct sk
  */
 void ieee80211_clear_tx_pending(struct ieee80211_local *local)
 {
-	int i, j;
-	struct ieee80211_tx_stored_packet *store;
+	struct sk_buff *skb;
+	int i;
 
 	for (i = 0; i < local->hw.queues; i++) {
 		if (!test_bit(i, local->queues_pending))
 			continue;
-		store = &local->pending_packet[i];
-		kfree_skb(store->skb);
-		for (j = 0; j < store->num_extra_frag; j++)
-			kfree_skb(store->extra_frag[j]);
-		kfree(store->extra_frag);
+
+		skb = local->pending_packet[i].skb;
+		while (skb) {
+			struct sk_buff *next;
+
+			next = skb->next;
+			dev_kfree_skb(skb);
+			skb = next;
+		}
 		clear_bit(i, local->queues_pending);
 	}
 }
@@ -1854,14 +1837,11 @@ void ieee80211_tx_pending(unsigned long 
 		netif_start_subqueue(local->mdev, i);
 
 		store = &local->pending_packet[i];
-		tx.extra_frag = store->extra_frag;
-		tx.num_extra_frag = store->num_extra_frag;
 		tx.flags = 0;
-		ret = __ieee80211_tx(local, store->skb, &tx);
-		if (ret) {
-			if (ret == IEEE80211_TX_FRAG_AGAIN)
-				store->skb = NULL;
-		} else {
+		tx.skb = store->skb;
+		ret = __ieee80211_tx(local, &tx);
+		store->skb = tx.skb;
+		if (!ret) {
 			clear_bit(i, local->queues_pending);
 			ieee80211_wake_queue(&local->hw, i);
 		}
--- wireless-testing.orig/net/mac80211/wep.c	2009-03-21 17:06:18.000000000 +0100
+++ wireless-testing/net/mac80211/wep.c	2009-03-21 17:08:48.000000000 +0100
@@ -329,24 +329,17 @@ static int wep_encrypt_skb(struct ieee80
 ieee80211_tx_result
 ieee80211_crypto_wep_encrypt(struct ieee80211_tx_data *tx)
 {
-	int i;
+	struct sk_buff *skb;
 
 	ieee80211_tx_set_protected(tx);
 
-	if (wep_encrypt_skb(tx, tx->skb) < 0) {
-		I802_DEBUG_INC(tx->local->tx_handlers_drop_wep);
-		return TX_DROP;
-	}
-
-	if (tx->extra_frag) {
-		for (i = 0; i < tx->num_extra_frag; i++) {
-			if (wep_encrypt_skb(tx, tx->extra_frag[i])) {
-				I802_DEBUG_INC(tx->local->
-					       tx_handlers_drop_wep);
-				return TX_DROP;
-			}
+	skb = tx->skb;
+	do {
+		if (wep_encrypt_skb(tx, skb) < 0) {
+			I802_DEBUG_INC(tx->local->tx_handlers_drop_wep);
+			return TX_DROP;
 		}
-	}
+	} while ((skb = skb->next));
 
 	return TX_CONTINUE;
 }
--- wireless-testing.orig/net/mac80211/wpa.c	2009-03-21 17:06:18.000000000 +0100
+++ wireless-testing/net/mac80211/wpa.c	2009-03-21 17:08:48.000000000 +0100
@@ -196,19 +196,13 @@ ieee80211_tx_result
 ieee80211_crypto_tkip_encrypt(struct ieee80211_tx_data *tx)
 {
 	struct sk_buff *skb = tx->skb;
-	int i;
 
 	ieee80211_tx_set_protected(tx);
 
-	if (tkip_encrypt_skb(tx, skb) < 0)
-		return TX_DROP;
-
-	if (tx->extra_frag) {
-		for (i = 0; i < tx->num_extra_frag; i++) {
-			if (tkip_encrypt_skb(tx, tx->extra_frag[i]))
-				return TX_DROP;
-		}
-	}
+	do {
+		if (tkip_encrypt_skb(tx, skb) < 0)
+			return TX_DROP;
+	} while ((skb = skb->next));
 
 	return TX_CONTINUE;
 }
@@ -428,19 +422,13 @@ ieee80211_tx_result
 ieee80211_crypto_ccmp_encrypt(struct ieee80211_tx_data *tx)
 {
 	struct sk_buff *skb = tx->skb;
-	int i;
 
 	ieee80211_tx_set_protected(tx);
 
-	if (ccmp_encrypt_skb(tx, skb) < 0)
-		return TX_DROP;
-
-	if (tx->extra_frag) {
-		for (i = 0; i < tx->num_extra_frag; i++) {
-			if (ccmp_encrypt_skb(tx, tx->extra_frag[i]))
-				return TX_DROP;
-		}
-	}
+	do {
+		if (ccmp_encrypt_skb(tx, skb) < 0)
+			return TX_DROP;
+	} while ((skb = skb->next));
 
 	return TX_CONTINUE;
 }
--- wireless-testing.orig/net/mac80211/util.c	2009-03-21 17:06:18.000000000 +0100
+++ wireless-testing/net/mac80211/util.c	2009-03-21 17:08:48.000000000 +0100
@@ -166,18 +166,13 @@ int ieee80211_get_mesh_hdrlen(struct iee
 
 void ieee80211_tx_set_protected(struct ieee80211_tx_data *tx)
 {
-	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) tx->skb->data;
+	struct sk_buff *skb = tx->skb;
+	struct ieee80211_hdr *hdr;
 
-	hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PROTECTED);
-	if (tx->extra_frag) {
-		struct ieee80211_hdr *fhdr;
-		int i;
-		for (i = 0; i < tx->num_extra_frag; i++) {
-			fhdr = (struct ieee80211_hdr *)
-				tx->extra_frag[i]->data;
-			fhdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PROTECTED);
-		}
-	}
+	do {
+		hdr = (struct ieee80211_hdr *) skb->data;
+		hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PROTECTED);
+	} while ((skb = skb->next));
 }
 
 int ieee80211_frame_duration(struct ieee80211_local *local, size_t len,

-- 

Re: [PATCH 1/8] mac80211: rewrite fragmentation

[Luis R. Rodriguez]

T24gTW9uLCBNYXIgMjMsIDIwMDkgYXQgOToyOCBBTSwgSm9oYW5uZXMgQmVyZwo8am9oYW5uZXNA
c2lwc29sdXRpb25zLm5ldD4gd3JvdGU6Cj4gRnJhZ21lbnRhdGlvbiBjdXJyZW50bHkgdXNlcyBh
biBhbGxvY2F0ZWQgYXJyYXkgdG8gc3RvcmUgdGhlCj4gZnJhZ21lbnQgc2ticywgYW5kIHRoZW4g
a2VlcHMgdHJhY2sgb2Ygd2hpY2ggaGF2ZSBiZWVuIHNlbnQKPiBhbmQgd2hpY2ggYXJlIHN0aWxs
IHBlbmRpbmcgZXRjLiBUaGlzIGlzIHJhdGhlciBjb21wbGljYXRlZDsKPiBtYWtlIGl0IHNpbXBs
ZXIgYnkganVzdCBjaGFpbmluZyB0aGUgZnJhZ21lbnRzIGludG8gc2tiLT5uZXh0Cj4gYW5kIHJl
bW92aW5nIGZyb20gdGhhdCBsaXN0IHdoZW4gc2VudC4gQWxzbyBzaW1wbGlmaWVzIGFsbAo+IGNv
ZGUgdGhhdCBuZWVkcyB0byB0b3VjaCBmcmFnbWVudHMsIHNpbmNlIGl0IG5vdyBvbmx5IG5lZWRz
Cj4gdG8gd2FsayB0aGUgc2tiLT5uZXh0IGxpc3QuCj4KPiBUaGlzIGlzIGEgcHJlcmVxdWlzaXRl
IGZvciBmaXhpbmcgdGhlIHN0b3JlZCBwYWNrZXQgY29kZSwKPiB3aGljaCBJIG5lZWQgdG8gZG8g
Zm9yIHByb3BlciBhZ2dyZWdhdGlvbiBwYWNrZXQgc3RvcmluZy4KPgo+IFNpZ25lZC1vZmYtYnk6
IEpvaGFubmVzIEJlcmcgPGpvaGFubmVzQHNpcHNvbHV0aW9ucy5uZXQ+Cgo8LS0gc25pcCAtLT4K
Cj4gQEAgLTEwOTksNDUgKzEwODgsMzYgQEAgc3RhdGljIGludCBpZWVlODAyMTFfdHhfcHJlcGFy
ZShzdHJ1Y3QgaQo+IMKgIMKgIMKgIMKgcmV0dXJuIDA7Cj4gwqB9Cj4KPiAtc3RhdGljIGludCBf
X2llZWU4MDIxMV90eChzdHJ1Y3QgaWVlZTgwMjExX2xvY2FsICpsb2NhbCwgc3RydWN0IHNrX2J1
ZmYgKnNrYiwKPiArc3RhdGljIGludCBfX2llZWU4MDIxMV90eChzdHJ1Y3QgaWVlZTgwMjExX2xv
Y2FsICpsb2NhbCwKPiDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoHN0cnVj
dCBpZWVlODAyMTFfdHhfZGF0YSAqdHgpCj4gwqB7Cj4gKyDCoCDCoCDCoCBzdHJ1Y3Qgc2tfYnVm
ZiAqc2tiID0gdHgtPnNrYiwgKm5leHQ7Cj4gwqAgwqAgwqAgwqBzdHJ1Y3QgaWVlZTgwMjExX3R4
X2luZm8gKmluZm87Cj4gLSDCoCDCoCDCoCBpbnQgcmV0LCBpOwo+ICsgwqAgwqAgwqAgaW50IHJl
dDsKPiArIMKgIMKgIMKgIGJvb2wgZnJhZ20gPSBmYWxzZTsKPgo+IC0gwqAgwqAgwqAgaWYgKHNr
Yikgewo+ICsgwqAgwqAgwqAgbG9jYWwtPm1kZXYtPnRyYW5zX3N0YXJ0ID0gamlmZmllczsKPiAr
Cj4gKyDCoCDCoCDCoCB3aGlsZSAoc2tiKSB7Cj4gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqBpZiAo
aWVlZTgwMjExX3F1ZXVlX3N0b3BwZWQoJmxvY2FsLT5odywKPiDCoCDCoCDCoCDCoCDCoCDCoCDC
oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoHNrYl9nZXRfcXVl
dWVfbWFwcGluZyhza2IpKSkKPiDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoHJl
dHVybiBJRUVFODAyMTFfVFhfUEVORElORzsKPgo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAgcmV0
ID0gbG9jYWwtPm9wcy0+dHgobG9jYWxfdG9faHcobG9jYWwpLCBza2IpOwo+IC0gwqAgwqAgwqAg
wqAgwqAgwqAgwqAgaWYgKHJldCkKPiAtIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
IHJldHVybiBJRUVFODAyMTFfVFhfQUdBSU47Cj4gLSDCoCDCoCDCoCDCoCDCoCDCoCDCoCBsb2Nh
bC0+bWRldi0+dHJhbnNfc3RhcnQgPSBqaWZmaWVzOwo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAg
aWVlZTgwMjExX2xlZF90eChsb2NhbCwgMSk7Cj4gLSDCoCDCoCDCoCB9Cj4gLSDCoCDCoCDCoCBp
ZiAodHgtPmV4dHJhX2ZyYWcpIHsKPiAtIMKgIMKgIMKgIMKgIMKgIMKgIMKgIGZvciAoaSA9IDA7
IGkgPCB0eC0+bnVtX2V4dHJhX2ZyYWc7IGkrKykgewo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAg
wqAgwqAgwqAgwqAgaWYgKCF0eC0+ZXh0cmFfZnJhZ1tpXSkKPiAtIMKgIMKgIMKgIMKgIMKgIMKg
IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIGNvbnRpbnVlOwo+IC0gwqAgwqAgwqAgwqAgwqAg
wqAgwqAgwqAgwqAgwqAgwqAgaW5mbyA9IElFRUU4MDIxMV9TS0JfQ0IodHgtPmV4dHJhX2ZyYWdb
aV0pOwo+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgaWYgKGZyYWdtKSB7Cj4gKyDCoCDCoCDCoCDC
oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCBpbmZvID0gSUVFRTgwMjExX1NLQl9DQihza2IpOwo+IMKg
IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgaW5mby0+ZmxhZ3MgJj0gfihJRUVFODAy
MTFfVFhfQ1RMX0NMRUFSX1BTX0ZJTFQgfAo+IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIElFRUU4MDIxMV9UWF9DVExfRklSU1RfRlJB
R01FTlQpOwo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgaWYgKGllZWU4MDIx
MV9xdWV1ZV9zdG9wcGVkKCZsb2NhbC0+aHcsCj4gLSDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCBza2JfZ2V0X3F1ZXVlX21hcHBpbmcodHgt
PmV4dHJhX2ZyYWdbaV0pKSkKPiAtIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
IMKgIMKgIMKgIHJldHVybiBJRUVFODAyMTFfVFhfRlJBR19BR0FJTjsKPiAtCj4gLSDCoCDCoCDC
oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCByZXQgPSBsb2NhbC0+b3BzLT50eChsb2NhbF90b19o
dyhsb2NhbCksCj4gLSDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
oCDCoCDCoCDCoCDCoCDCoCDCoCB0eC0+ZXh0cmFfZnJhZ1tpXSk7Cj4gLSDCoCDCoCDCoCDCoCDC
oCDCoCDCoCDCoCDCoCDCoCDCoCBpZiAocmV0KQo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAg
wqAgwqAgwqAgwqAgwqAgwqAgwqAgcmV0dXJuIElFRUU4MDIxMV9UWF9GUkFHX0FHQUlOOwo+IC0g
wqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgbG9jYWwtPm1kZXYtPnRyYW5zX3N0YXJ0
ID0gamlmZmllczsKPiAtIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIGllZWU4MDIx
MV9sZWRfdHgobG9jYWwsIDEpOwo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAg
dHgtPmV4dHJhX2ZyYWdbaV0gPSBOVUxMOwo+IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgfQo+IC0g
wqAgwqAgwqAgwqAgwqAgwqAgwqAga2ZyZWUodHgtPmV4dHJhX2ZyYWcpOwo+IC0gwqAgwqAgwqAg
wqAgwqAgwqAgwqAgdHgtPmV4dHJhX2ZyYWcgPSBOVUxMOwo+ICsKPiArIMKgIMKgIMKgIMKgIMKg
IMKgIMKgIG5leHQgPSBza2ItPm5leHQ7Cj4gKyDCoCDCoCDCoCDCoCDCoCDCoCDCoCByZXQgPSBs
b2NhbC0+b3BzLT50eChsb2NhbF90b19odyhsb2NhbCksIHNrYik7Cj4gKyDCoCDCoCDCoCDCoCDC
oCDCoCDCoCBpZiAocmV0ICE9IE5FVERFVl9UWF9PSykKPiArIMKgIMKgIMKgIMKgIMKgIMKgIMKg
IMKgIMKgIMKgIMKgIHJldHVybiBJRUVFODAyMTFfVFhfQUdBSU47Cj4gKyDCoCDCoCDCoCDCoCDC
oCDCoCDCoCB0eC0+c2tiID0gc2tiID0gbmV4dDsKPiArIMKgIMKgIMKgIMKgIMKgIMKgIMKgIGll
ZWU4MDIxMV9sZWRfdHgobG9jYWwsIDEpOwo+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgZnJhZ20g
PSB0cnVlOwo+IMKgIMKgIMKgIMKgfQo+ICsKPiDCoCDCoCDCoCDCoHJldHVybiBJRUVFODAyMTFf
VFhfT0s7Cj4gwqB9Cj4KCkkgc2VlIG9ubHkgb25lIHBvc3NpYmxlIHJlZ3Jlc3Npb24gd2l0aCB0
aGlzIHBhdGNoIGFuZCB0aGF0IGlzIHRoYXQgaXQKc2VlbXMgeW91IGZvcmdvdCB0byB1cGRhdGUg
dGhlIG1kZXYtPnRyYW5zX3N0YXJ0IGluIHRoZSBuZXcgbG9vcCBvbgplYWNoIGZyYWdtZW50LgoK
QXMgZmFyIGFzIEkgY2FuIHRlbGwgdGhpcyB3b3VsZCBvbmx5IGludHJvZHVjZSBhIHJlZ3Jlc3Np
b24gd2l0aCBpZgpzb21lb25lIGlzIHVzaW5nIGEgbmV0ZGV2IHdhdGNoZG9nIG9uIHRoZSBtZGV2
LCBpZiB3ZSBkb24ndCBjYXJlIGFib3V0CnRoYXQgYW5kIEknbSBub3QgbWlzc2luZyBhbnkgb3Ro
ZXIgY2FzZSB3aGVyZSB0aGlzIGNvdWxkIGJlIGFmZmVjdGVkCndlIG1pZ2h0IGFzIHdlbGwgcmVt
b3ZlIHRoYXQgZmlyc3QgdXBkYXRlIGFzIGF0IGxlYXN0IGZvciBtYWM4MDIxMSBpdHMKY29tcGxl
dGVseSB1bnRvdWNoZWQuCgpPdGhlciB0aGFuIHRoYXQ6CgpSZXZpZXdlZC1ieTogTHVpcyBSLiBS
b2RyaWd1ZXogPGxyb2RyaWd1ZXpAYXRoZXJvcy5jb20+CgogIEx1aXMK

Re: [PATCH 1/8] mac80211: rewrite fragmentation

[Julian Calaby]

2009/3/26 Luis R. Rodriguez <mcgrof@gmail.com>:
> T24gTW9uLCBNYXIgMjMsIDIwMDkgYXQgOToyOCBBTSwgSm9oYW5uZXMgQmVyZwo8am9oYW5uZXNA
> c2lwc29sdXRpb25zLm5ldD4gd3JvdGU6Cj4gRnJhZ21lbnRhdGlvbiBjdXJyZW50bHkgdXNlcyBh
> biBhbGxvY2F0ZWQgYXJyYXkgdG8gc3RvcmUgdGhlCj4gZnJhZ21lbnQgc2ticywgYW5kIHRoZW4g
> a2VlcHMgdHJhY2sgb2Ygd2hpY2ggaGF2ZSBiZWVuIHNlbnQKPiBhbmQgd2hpY2ggYXJlIHN0aWxs
> IHBlbmRpbmcgZXRjLiBUaGlzIGlzIHJhdGhlciBjb21wbGljYXRlZDsKPiBtYWtlIGl0IHNpbXBs
> ZXIgYnkganVzdCBjaGFpbmluZyB0aGUgZnJhZ21lbnRzIGludG8gc2tiLT5uZXh0Cj4gYW5kIHJl
> bW92aW5nIGZyb20gdGhhdCBsaXN0IHdoZW4gc2VudC4gQWxzbyBzaW1wbGlmaWVzIGFsbAo+IGNv
> ZGUgdGhhdCBuZWVkcyB0byB0b3VjaCBmcmFnbWVudHMsIHNpbmNlIGl0IG5vdyBvbmx5IG5lZWRz
> Cj4gdG8gd2FsayB0aGUgc2tiLT5uZXh0IGxpc3QuCj4KPiBUaGlzIGlzIGEgcHJlcmVxdWlzaXRl
> IGZvciBmaXhpbmcgdGhlIHN0b3JlZCBwYWNrZXQgY29kZSwKPiB3aGljaCBJIG5lZWQgdG8gZG8g
> Zm9yIHByb3BlciBhZ2dyZWdhdGlvbiBwYWNrZXQgc3RvcmluZy4KPgo+IFNpZ25lZC1vZmYtYnk6
> IEpvaGFubmVzIEJlcmcgPGpvaGFubmVzQHNpcHNvbHV0aW9ucy5uZXQ+Cgo8LS0gc25pcCAtLT4K
> Cj4gQEAgLTEwOTksNDUgKzEwODgsMzYgQEAgc3RhdGljIGludCBpZWVlODAyMTFfdHhfcHJlcGFy
> ZShzdHJ1Y3QgaQo+IMKgIMKgIMKgIMKgcmV0dXJuIDA7Cj4gwqB9Cj4KPiAtc3RhdGljIGludCBf
> X2llZWU4MDIxMV90eChzdHJ1Y3QgaWVlZTgwMjExX2xvY2FsICpsb2NhbCwgc3RydWN0IHNrX2J1
> ZmYgKnNrYiwKPiArc3RhdGljIGludCBfX2llZWU4MDIxMV90eChzdHJ1Y3QgaWVlZTgwMjExX2xv
> Y2FsICpsb2NhbCwKPiDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoHN0cnVj
> dCBpZWVlODAyMTFfdHhfZGF0YSAqdHgpCj4gwqB7Cj4gKyDCoCDCoCDCoCBzdHJ1Y3Qgc2tfYnVm
> ZiAqc2tiID0gdHgtPnNrYiwgKm5leHQ7Cj4gwqAgwqAgwqAgwqBzdHJ1Y3QgaWVlZTgwMjExX3R4
> X2luZm8gKmluZm87Cj4gLSDCoCDCoCDCoCBpbnQgcmV0LCBpOwo+ICsgwqAgwqAgwqAgaW50IHJl
> dDsKPiArIMKgIMKgIMKgIGJvb2wgZnJhZ20gPSBmYWxzZTsKPgo+IC0gwqAgwqAgwqAgaWYgKHNr
> Yikgewo+ICsgwqAgwqAgwqAgbG9jYWwtPm1kZXYtPnRyYW5zX3N0YXJ0ID0gamlmZmllczsKPiAr
> Cj4gKyDCoCDCoCDCoCB3aGlsZSAoc2tiKSB7Cj4gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqBpZiAo
> aWVlZTgwMjExX3F1ZXVlX3N0b3BwZWQoJmxvY2FsLT5odywKPiDCoCDCoCDCoCDCoCDCoCDCoCDC
> oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoHNrYl9nZXRfcXVl
> dWVfbWFwcGluZyhza2IpKSkKPiDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoHJl
> dHVybiBJRUVFODAyMTFfVFhfUEVORElORzsKPgo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAgcmV0
> ID0gbG9jYWwtPm9wcy0+dHgobG9jYWxfdG9faHcobG9jYWwpLCBza2IpOwo+IC0gwqAgwqAgwqAg
> wqAgwqAgwqAgwqAgaWYgKHJldCkKPiAtIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
> IHJldHVybiBJRUVFODAyMTFfVFhfQUdBSU47Cj4gLSDCoCDCoCDCoCDCoCDCoCDCoCDCoCBsb2Nh
> bC0+bWRldi0+dHJhbnNfc3RhcnQgPSBqaWZmaWVzOwo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAg
> aWVlZTgwMjExX2xlZF90eChsb2NhbCwgMSk7Cj4gLSDCoCDCoCDCoCB9Cj4gLSDCoCDCoCDCoCBp
> ZiAodHgtPmV4dHJhX2ZyYWcpIHsKPiAtIMKgIMKgIMKgIMKgIMKgIMKgIMKgIGZvciAoaSA9IDA7
> IGkgPCB0eC0+bnVtX2V4dHJhX2ZyYWc7IGkrKykgewo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAg
> wqAgwqAgwqAgwqAgaWYgKCF0eC0+ZXh0cmFfZnJhZ1tpXSkKPiAtIMKgIMKgIMKgIMKgIMKgIMKg
> IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIGNvbnRpbnVlOwo+IC0gwqAgwqAgwqAgwqAgwqAg
> wqAgwqAgwqAgwqAgwqAgwqAgaW5mbyA9IElFRUU4MDIxMV9TS0JfQ0IodHgtPmV4dHJhX2ZyYWdb
> aV0pOwo+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgaWYgKGZyYWdtKSB7Cj4gKyDCoCDCoCDCoCDC
> oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCBpbmZvID0gSUVFRTgwMjExX1NLQl9DQihza2IpOwo+IMKg
> IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgaW5mby0+ZmxhZ3MgJj0gfihJRUVFODAy
> MTFfVFhfQ1RMX0NMRUFSX1BTX0ZJTFQgfAo+IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
> IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIElFRUU4MDIxMV9UWF9DVExfRklSU1RfRlJB
> R01FTlQpOwo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgaWYgKGllZWU4MDIx
> MV9xdWV1ZV9zdG9wcGVkKCZsb2NhbC0+aHcsCj4gLSDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
> oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCBza2JfZ2V0X3F1ZXVlX21hcHBpbmcodHgt
> PmV4dHJhX2ZyYWdbaV0pKSkKPiAtIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
> IMKgIMKgIMKgIHJldHVybiBJRUVFODAyMTFfVFhfRlJBR19BR0FJTjsKPiAtCj4gLSDCoCDCoCDC
> oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCByZXQgPSBsb2NhbC0+b3BzLT50eChsb2NhbF90b19o
> dyhsb2NhbCksCj4gLSDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
> oCDCoCDCoCDCoCDCoCDCoCDCoCB0eC0+ZXh0cmFfZnJhZ1tpXSk7Cj4gLSDCoCDCoCDCoCDCoCDC
> oCDCoCDCoCDCoCDCoCDCoCDCoCBpZiAocmV0KQo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAg
> wqAgwqAgwqAgwqAgwqAgwqAgwqAgcmV0dXJuIElFRUU4MDIxMV9UWF9GUkFHX0FHQUlOOwo+IC0g
> wqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgbG9jYWwtPm1kZXYtPnRyYW5zX3N0YXJ0
> ID0gamlmZmllczsKPiAtIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIGllZWU4MDIx
> MV9sZWRfdHgobG9jYWwsIDEpOwo+IC0gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAg
> dHgtPmV4dHJhX2ZyYWdbaV0gPSBOVUxMOwo+IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgfQo+IC0g
> wqAgwqAgwqAgwqAgwqAgwqAgwqAga2ZyZWUodHgtPmV4dHJhX2ZyYWcpOwo+IC0gwqAgwqAgwqAg
> wqAgwqAgwqAgwqAgdHgtPmV4dHJhX2ZyYWcgPSBOVUxMOwo+ICsKPiArIMKgIMKgIMKgIMKgIMKg
> IMKgIMKgIG5leHQgPSBza2ItPm5leHQ7Cj4gKyDCoCDCoCDCoCDCoCDCoCDCoCDCoCByZXQgPSBs
> b2NhbC0+b3BzLT50eChsb2NhbF90b19odyhsb2NhbCksIHNrYik7Cj4gKyDCoCDCoCDCoCDCoCDC
> oCDCoCDCoCBpZiAocmV0ICE9IE5FVERFVl9UWF9PSykKPiArIMKgIMKgIMKgIMKgIMKgIMKgIMKg
> IMKgIMKgIMKgIMKgIHJldHVybiBJRUVFODAyMTFfVFhfQUdBSU47Cj4gKyDCoCDCoCDCoCDCoCDC
> oCDCoCDCoCB0eC0+c2tiID0gc2tiID0gbmV4dDsKPiArIMKgIMKgIMKgIMKgIMKgIMKgIMKgIGll
> ZWU4MDIxMV9sZWRfdHgobG9jYWwsIDEpOwo+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgZnJhZ20g
> PSB0cnVlOwo+IMKgIMKgIMKgIMKgfQo+ICsKPiDCoCDCoCDCoCDCoHJldHVybiBJRUVFODAyMTFf
> VFhfT0s7Cj4gwqB9Cj4KCkkgc2VlIG9ubHkgb25lIHBvc3NpYmxlIHJlZ3Jlc3Npb24gd2l0aCB0
> aGlzIHBhdGNoIGFuZCB0aGF0IGlzIHRoYXQgaXQKc2VlbXMgeW91IGZvcmdvdCB0byB1cGRhdGUg
> dGhlIG1kZXYtPnRyYW5zX3N0YXJ0IGluIHRoZSBuZXcgbG9vcCBvbgplYWNoIGZyYWdtZW50LgoK
> QXMgZmFyIGFzIEkgY2FuIHRlbGwgdGhpcyB3b3VsZCBvbmx5IGludHJvZHVjZSBhIHJlZ3Jlc3Np
> b24gd2l0aCBpZgpzb21lb25lIGlzIHVzaW5nIGEgbmV0ZGV2IHdhdGNoZG9nIG9uIHRoZSBtZGV2
> LCBpZiB3ZSBkb24ndCBjYXJlIGFib3V0CnRoYXQgYW5kIEknbSBub3QgbWlzc2luZyBhbnkgb3Ro
> ZXIgY2FzZSB3aGVyZSB0aGlzIGNvdWxkIGJlIGFmZmVjdGVkCndlIG1pZ2h0IGFzIHdlbGwgcmVt
> b3ZlIHRoYXQgZmlyc3QgdXBkYXRlIGFzIGF0IGxlYXN0IGZvciBtYWM4MDIxMSBpdHMKY29tcGxl
> dGVseSB1bnRvdWNoZWQuCgpPdGhlciB0aGFuIHRoYXQ6CgpSZXZpZXdlZC1ieTogTHVpcyBSLiBS
> b2RyaWd1ZXogPGxyb2RyaWd1ZXpAYXRoZXJvcy5jb20+CgogIEx1aXMK

W T F..............?

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH 1/8] mac80211: rewrite fragmentation

[Luis R. Rodriguez]

2009/3/25 Julian Calaby <julian.calaby@gmail.com>:
> 2009/3/26 Luis R. Rodriguez <mcgrof@gmail.com>:
>> 1bnRvdWNoZWQuCgpPdGhlciB0aGFuIHRoYXQ6CgpSZXZpZXdlZC1ieTogTHVpcyBSLiBS
>> b2RyaWd1ZXogPGxyb2RyaWd1ZXpAYXRoZXJvcy5jb20+CgogIEx1aXMK
>
> W T F..............?

WTF^pi.. I don't know I used fscking gmail with firefox... hmmm. Let try again..

  Luis

Re: [PATCH 1/8] mac80211: rewrite fragmentation

[Luis R. Rodriguez]

On Wed, Mar 25, 2009 at 6:21 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> I see only one possible regression with this patch and that is that it
> seems you forgot to update the mdev->trans_start in the new loop on
> each fragment.
>
> As far as I can tell this would only introduce a regression with if
> someone is using a netdev watchdog on the mdev, if we don't care about
> that and I'm not missing any other case where this could be affected
> we might as well remove that first update as at least for mac80211 its
> completely untouched.
>
> Other than that:
>
> Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

Above is my actual reply, seems gmail stored it properly... hmm.......

  Luis

Re: [PATCH 1/8] mac80211: rewrite fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]

On Wed, 2009-03-25 at 18:21 -0700, Luis R. Rodriguez wrote:

> > -static int __ieee80211_tx(struct ieee80211_local *local, struct sk_buff *skb,
> > +static int __ieee80211_tx(struct ieee80211_local *local,
> >                          struct ieee80211_tx_data *tx)
> >  {
> > +       struct sk_buff *skb = tx->skb, *next;
> >        struct ieee80211_tx_info *info;
> > -       int ret, i;
> > +       int ret;
> > +       bool fragm = false;
> >
> > -       if (skb) {
> > +       local->mdev->trans_start = jiffies;
> > +
> > +       while (skb) {
> >                if (ieee80211_queue_stopped(&local->hw,
> >                                            skb_get_queue_mapping(skb)))
> >                        return IEEE80211_TX_PENDING;
> >
> > -               ret = local->ops->tx(local_to_hw(local), skb);
> > -               if (ret)
> > -                       return IEEE80211_TX_AGAIN;
> > -               local->mdev->trans_start = jiffies;
> > -               ieee80211_led_tx(local, 1);
> > -       }

> I see only one possible regression with this patch and that is that it
> seems you forgot to update the mdev->trans_start in the new loop on
> each fragment.

Well, that was actually intentional. Since none of the code here can
block the time between the various assignments of trans_start would be
_very_ small, and as such the multiple assignments can't really be
necessary.

> As far as I can tell this would only introduce a regression with if
> someone is using a netdev watchdog on the mdev, if we don't care about
> that and I'm not missing any other case where this could be affected
> we might as well remove that first update as at least for mac80211 its
> completely untouched.

netdev watchdog is always running for all netdevs :)

> Other than that:
> 
> Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

Thanks for looking!

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/8] mac80211: fix A-MPDU queue assignment

[Johannes Berg]

Internally, mac80211 requires the skb's queue mapping to be set
to the AC queue, not the virtual A-MPDU queue. This is not done
correctly currently, this patch moves the code down to directly
before the driver is invoked and adds a comment that it will be
moved into the driver later.

Since this requires __ieee80211_tx() to have the sta pointer,
make sure to provide it in ieee80211_tx_pending().

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/tx.c |   35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

--- wireless-testing.orig/net/mac80211/tx.c	2009-03-23 13:59:20.000000000 +0100
+++ wireless-testing/net/mac80211/tx.c	2009-03-23 14:03:45.000000000 +0100
@@ -1024,13 +1024,8 @@ __ieee80211_tx_prepare(struct ieee80211_
 
 		spin_lock_irqsave(&tx->sta->lock, flags);
 		state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
-		if (*state == HT_AGG_STATE_OPERATIONAL) {
+		if (*state == HT_AGG_STATE_OPERATIONAL)
 			info->flags |= IEEE80211_TX_CTL_AMPDU;
-			if (local->hw.ampdu_queues)
-				skb_set_queue_mapping(
-					skb, tx->local->hw.queues +
-					     tx->sta->tid_to_tx_q[tid]);
-		}
 		spin_unlock_irqrestore(&tx->sta->lock, flags);
 	}
 
@@ -1103,10 +1098,29 @@ static int __ieee80211_tx(struct ieee802
 					    skb_get_queue_mapping(skb)))
 			return IEEE80211_TX_PENDING;
 
-		if (fragm) {
-			info = IEEE80211_SKB_CB(skb);
+		info = IEEE80211_SKB_CB(skb);
+
+		if (fragm)
 			info->flags &= ~(IEEE80211_TX_CTL_CLEAR_PS_FILT |
 					 IEEE80211_TX_CTL_FIRST_FRAGMENT);
+
+		/*
+		 * Internally, we need to have the queue mapping point to
+		 * the real AC queue, not the virtual A-MPDU queue. This
+		 * now finally sets the queue to what the driver wants.
+		 * We will later move this down into the only driver that
+		 * needs it, iwlwifi.
+		 */
+		if (tx->sta && local->hw.ampdu_queues &&
+		    info->flags & IEEE80211_TX_CTL_AMPDU) {
+			unsigned long flags;
+			u8 *qc = ieee80211_get_qos_ctl((void *) skb->data);
+			int tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
+
+			spin_lock_irqsave(&tx->sta->lock, flags);
+			skb_set_queue_mapping(skb, local->hw.queues +
+						   tx->sta->tid_to_tx_q[tid]);
+			spin_unlock_irqrestore(&tx->sta->lock, flags);
 		}
 
 		next = skb->next;
@@ -1817,9 +1831,11 @@ void ieee80211_tx_pending(unsigned long 
 	struct ieee80211_local *local = (struct ieee80211_local *)data;
 	struct net_device *dev = local->mdev;
 	struct ieee80211_tx_stored_packet *store;
+	struct ieee80211_hdr *hdr;
 	struct ieee80211_tx_data tx;
 	int i, ret;
 
+	rcu_read_lock();
 	netif_tx_lock_bh(dev);
 	for (i = 0; i < local->hw.queues; i++) {
 		/* Check that this queue is ok */
@@ -1839,6 +1855,8 @@ void ieee80211_tx_pending(unsigned long 
 		store = &local->pending_packet[i];
 		tx.flags = 0;
 		tx.skb = store->skb;
+		hdr = (struct ieee80211_hdr *)tx.skb->data;
+		tx.sta = sta_info_get(local, hdr->addr1);
 		ret = __ieee80211_tx(local, &tx);
 		store->skb = tx.skb;
 		if (!ret) {
@@ -1847,6 +1865,7 @@ void ieee80211_tx_pending(unsigned long 
 		}
 	}
 	netif_tx_unlock_bh(dev);
+	rcu_read_unlock();
 }
 
 /* functions for drivers to get certain frames */

-- 

Re: [PATCH 2/8] mac80211: fix A-MPDU queue assignment

[Luis R. Rodriguez]

On Mon, Mar 23, 2009 at 05:28:36PM +0100, Johannes Berg wrote:
> Internally, mac80211 requires the skb's queue mapping to be set
> to the AC queue, not the virtual A-MPDU queue. This is not done
> correctly currently, this patch moves the code down to directly
> before the driver is invoked and adds a comment that it will be
> moved into the driver later.
> 
> Since this requires __ieee80211_tx() to have the sta pointer,
> make sure to provide it in ieee80211_tx_pending().
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

  Luis

[PATCH 3/8] mac80211: rework the pending packets code

[Johannes Berg]

The pending packets code is quite incomprehensible, uses memory barriers
nobody really understands, etc. This patch reworks it entirely, using
the queue spinlock, proper stop bits and the skb queues themselves to
indicate whether packets are pending or not (rather than a separate
variable like before).

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Tested with the later aggregation patches, I see packets going onto
the pending queue and being sent out from there.

 net/mac80211/ieee80211_i.h |    9 --
 net/mac80211/main.c        |    2 
 net/mac80211/tx.c          |  142 +++++++++++++++++++++++++--------------------
 net/mac80211/util.c        |   22 ++++--
 4 files changed, 97 insertions(+), 78 deletions(-)

--- wireless-testing.orig/net/mac80211/ieee80211_i.h	2009-03-23 13:59:20.000000000 +0100
+++ wireless-testing/net/mac80211/ieee80211_i.h	2009-03-23 14:03:45.000000000 +0100
@@ -184,10 +184,6 @@ struct ieee80211_rx_data {
 	u16 tkip_iv16;
 };
 
-struct ieee80211_tx_stored_packet {
-	struct sk_buff *skb;
-};
-
 struct beacon_data {
 	u8 *head, *tail;
 	int head_len, tail_len;
@@ -581,6 +577,7 @@ enum queue_stop_reason {
 	IEEE80211_QUEUE_STOP_REASON_CSA,
 	IEEE80211_QUEUE_STOP_REASON_AGGREGATION,
 	IEEE80211_QUEUE_STOP_REASON_SUSPEND,
+	IEEE80211_QUEUE_STOP_REASON_PENDING,
 };
 
 struct ieee80211_master_priv {
@@ -637,9 +634,7 @@ struct ieee80211_local {
 	struct sta_info *sta_hash[STA_HASH_SIZE];
 	struct timer_list sta_cleanup;
 
-	unsigned long queues_pending[BITS_TO_LONGS(IEEE80211_MAX_QUEUES)];
-	unsigned long queues_pending_run[BITS_TO_LONGS(IEEE80211_MAX_QUEUES)];
-	struct ieee80211_tx_stored_packet pending_packet[IEEE80211_MAX_QUEUES];
+	struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
 	struct tasklet_struct tx_pending_tasklet;
 
 	/* number of interfaces with corresponding IFF_ flags */
--- wireless-testing.orig/net/mac80211/main.c	2009-03-23 13:59:20.000000000 +0100
+++ wireless-testing/net/mac80211/main.c	2009-03-23 14:03:45.000000000 +0100
@@ -781,6 +781,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(
 
 	sta_info_init(local);
 
+	for (i = 0; i < IEEE80211_MAX_QUEUES; i++)
+		skb_queue_head_init(&local->pending[i]);
 	tasklet_init(&local->tx_pending_tasklet, ieee80211_tx_pending,
 		     (unsigned long)local);
 	tasklet_disable(&local->tx_pending_tasklet);
--- wireless-testing.orig/net/mac80211/tx.c	2009-03-23 14:03:45.000000000 +0100
+++ wireless-testing/net/mac80211/tx.c	2009-03-23 14:03:45.000000000 +0100
@@ -1189,12 +1189,14 @@ static int ieee80211_tx(struct net_devic
 	struct ieee80211_tx_data tx;
 	ieee80211_tx_result res_prepare;
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-	int ret;
+	struct sk_buff *next;
+	unsigned long flags;
+	int ret, retries;
 	u16 queue;
 
 	queue = skb_get_queue_mapping(skb);
 
-	WARN_ON(test_bit(queue, local->queues_pending));
+	WARN_ON(!skb_queue_empty(&local->pending[queue]));
 
 	if (unlikely(skb->len < 10)) {
 		dev_kfree_skb(skb);
@@ -1219,40 +1221,52 @@ static int ieee80211_tx(struct net_devic
 	if (invoke_tx_handlers(&tx))
 		goto out;
 
-retry:
+	retries = 0;
+ retry:
 	ret = __ieee80211_tx(local, &tx);
-	if (ret) {
-		struct ieee80211_tx_stored_packet *store;
-
+	switch (ret) {
+	case IEEE80211_TX_OK:
+		break;
+	case IEEE80211_TX_AGAIN:
 		/*
 		 * Since there are no fragmented frames on A-MPDU
 		 * queues, there's no reason for a driver to reject
 		 * a frame there, warn and drop it.
 		 */
-		if (ret != IEEE80211_TX_PENDING)
-			if (WARN_ON(info->flags & IEEE80211_TX_CTL_AMPDU))
-				goto drop;
+		if (WARN_ON(info->flags & IEEE80211_TX_CTL_AMPDU))
+			goto drop;
+		/* fall through */
+	case IEEE80211_TX_PENDING:
+		skb = tx.skb;
+
+		spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+
+		if (__netif_subqueue_stopped(local->mdev, queue)) {
+			do {
+				next = skb->next;
+				skb->next = NULL;
+				skb_queue_tail(&local->pending[queue], skb);
+			} while ((skb = next));
 
-		store = &local->pending_packet[queue];
+			/*
+			 * Make sure nobody will enable the queue on us
+			 * (without going through the tasklet) nor disable the
+			 * netdev queue underneath the pending handling code.
+			 */
+			__set_bit(IEEE80211_QUEUE_STOP_REASON_PENDING,
+				  &local->queue_stop_reasons[queue]);
 
-		set_bit(queue, local->queues_pending);
-		smp_mb();
-		/*
-		 * When the driver gets out of buffers during sending of
-		 * fragments and calls ieee80211_stop_queue, the netif
-		 * subqueue is stopped. There is, however, a small window
-		 * in which the PENDING bit is not yet set. If a buffer
-		 * gets available in that window (i.e. driver calls
-		 * ieee80211_wake_queue), we would end up with ieee80211_tx
-		 * called with the PENDING bit still set. Prevent this by
-		 * continuing transmitting here when that situation is
-		 * possible to have happened.
-		 */
-		if (!__netif_subqueue_stopped(local->mdev, queue)) {
-			clear_bit(queue, local->queues_pending);
+			spin_unlock_irqrestore(&local->queue_stop_reason_lock,
+					       flags);
+		} else {
+			spin_unlock_irqrestore(&local->queue_stop_reason_lock,
+					       flags);
+
+			retries++;
+			if (WARN(retries > 10, "tx refused but queue active"))
+				goto drop;
 			goto retry;
 		}
-		store->skb = tx.skb;
 	}
  out:
 	rcu_read_unlock();
@@ -1263,8 +1277,6 @@ retry:
 
 	skb = tx.skb;
 	while (skb) {
-		struct sk_buff *next;
-
 		next = skb->next;
 		dev_kfree_skb(skb);
 		skb = next;
@@ -1803,23 +1815,10 @@ int ieee80211_subif_start_xmit(struct sk
  */
 void ieee80211_clear_tx_pending(struct ieee80211_local *local)
 {
-	struct sk_buff *skb;
 	int i;
 
-	for (i = 0; i < local->hw.queues; i++) {
-		if (!test_bit(i, local->queues_pending))
-			continue;
-
-		skb = local->pending_packet[i].skb;
-		while (skb) {
-			struct sk_buff *next;
-
-			next = skb->next;
-			dev_kfree_skb(skb);
-			skb = next;
-		}
-		clear_bit(i, local->queues_pending);
-	}
+	for (i = 0; i < local->hw.queues; i++)
+		skb_queue_purge(&local->pending[i]);
 }
 
 /*
@@ -1830,40 +1829,57 @@ void ieee80211_tx_pending(unsigned long 
 {
 	struct ieee80211_local *local = (struct ieee80211_local *)data;
 	struct net_device *dev = local->mdev;
-	struct ieee80211_tx_stored_packet *store;
 	struct ieee80211_hdr *hdr;
+	unsigned long flags;
 	struct ieee80211_tx_data tx;
 	int i, ret;
+	bool next;
 
 	rcu_read_lock();
 	netif_tx_lock_bh(dev);
+
 	for (i = 0; i < local->hw.queues; i++) {
-		/* Check that this queue is ok */
-		if (__netif_subqueue_stopped(local->mdev, i) &&
-		    !test_bit(i, local->queues_pending_run))
-			continue;
+		/*
+		 * If queue is stopped by something other than due to pending
+		 * frames, or we have no pending frames, proceed to next queue.
+		 */
+		spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+		next = false;
+		if (local->queue_stop_reasons[i] !=
+			BIT(IEEE80211_QUEUE_STOP_REASON_PENDING) ||
+		    skb_queue_empty(&local->pending[i]))
+			next = true;
+		spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
 
-		if (!test_bit(i, local->queues_pending)) {
-			clear_bit(i, local->queues_pending_run);
-			ieee80211_wake_queue(&local->hw, i);
+		if (next)
 			continue;
-		}
 
-		clear_bit(i, local->queues_pending_run);
+		/*
+		 * start the queue now to allow processing our packets,
+		 * we're under the tx lock here anyway so nothing will
+		 * happen as a result of this
+		 */
 		netif_start_subqueue(local->mdev, i);
 
-		store = &local->pending_packet[i];
-		tx.flags = 0;
-		tx.skb = store->skb;
-		hdr = (struct ieee80211_hdr *)tx.skb->data;
-		tx.sta = sta_info_get(local, hdr->addr1);
-		ret = __ieee80211_tx(local, &tx);
-		store->skb = tx.skb;
-		if (!ret) {
-			clear_bit(i, local->queues_pending);
-			ieee80211_wake_queue(&local->hw, i);
+		while (!skb_queue_empty(&local->pending[i])) {
+			tx.flags = 0;
+			tx.skb = skb_dequeue(&local->pending[i]);
+			hdr = (struct ieee80211_hdr *)tx.skb->data;
+			tx.sta = sta_info_get(local, hdr->addr1);
+
+			ret = __ieee80211_tx(local, &tx);
+			if (ret != IEEE80211_TX_OK) {
+				skb_queue_head(&local->pending[i], tx.skb);
+				break;
+			}
 		}
+
+		/* Start regular packet processing again. */
+		if (skb_queue_empty(&local->pending[i]))
+			ieee80211_wake_queue_by_reason(&local->hw, i,
+					IEEE80211_QUEUE_STOP_REASON_PENDING);
 	}
+
 	netif_tx_unlock_bh(dev);
 	rcu_read_unlock();
 }
--- wireless-testing.orig/net/mac80211/util.c	2009-03-23 13:59:20.000000000 +0100
+++ wireless-testing/net/mac80211/util.c	2009-03-23 14:03:45.000000000 +0100
@@ -365,16 +365,16 @@ static void __ieee80211_wake_queue(struc
 
 	__clear_bit(reason, &local->queue_stop_reasons[queue]);
 
+	if (!skb_queue_empty(&local->pending[queue]) &&
+	    local->queue_stop_reasons[queue] ==
+	    			BIT(IEEE80211_QUEUE_STOP_REASON_PENDING))
+		tasklet_schedule(&local->tx_pending_tasklet);
+
 	if (local->queue_stop_reasons[queue] != 0)
 		/* someone still has this queue stopped */
 		return;
 
-	if (test_bit(queue, local->queues_pending)) {
-		set_bit(queue, local->queues_pending_run);
-		tasklet_schedule(&local->tx_pending_tasklet);
-	} else {
-		netif_wake_subqueue(local->mdev, queue);
-	}
+	netif_wake_subqueue(local->mdev, queue);
 }
 
 void ieee80211_wake_queue_by_reason(struct ieee80211_hw *hw, int queue,
@@ -420,9 +420,15 @@ static void __ieee80211_stop_queue(struc
 		reason = IEEE80211_QUEUE_STOP_REASON_AGGREGATION;
 	}
 
-	__set_bit(reason, &local->queue_stop_reasons[queue]);
+	/*
+	 * Only stop if it was previously running, this is necessary
+	 * for correct pending packets handling because there we may
+	 * start (but not wake) the queue and rely on that.
+	 */
+	if (!local->queue_stop_reasons[queue])
+		netif_stop_subqueue(local->mdev, queue);
 
-	netif_stop_subqueue(local->mdev, queue);
+	__set_bit(reason, &local->queue_stop_reasons[queue]);
 }
 
 void ieee80211_stop_queue_by_reason(struct ieee80211_hw *hw, int queue,

-- 

Re: [PATCH 3/8] mac80211: rework the pending packets code

[Luis R. Rodriguez]

On Mon, Mar 23, 2009 at 05:28:37PM +0100, Johannes Berg wrote:
> The pending packets code is quite incomprehensible, uses memory barriers
> nobody really understands, etc. This patch reworks it entirely, using
> the queue spinlock, proper stop bits and the skb queues themselves to
> indicate whether packets are pending or not (rather than a separate
> variable like before).
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

<-- snip -->

>  /*
> @@ -1830,40 +1829,57 @@ void ieee80211_tx_pending(unsigned long 
>  {
>  	struct ieee80211_local *local = (struct ieee80211_local *)data;
>  	struct net_device *dev = local->mdev;
> -	struct ieee80211_tx_stored_packet *store;
>  	struct ieee80211_hdr *hdr;
> +	unsigned long flags;
>  	struct ieee80211_tx_data tx;
>  	int i, ret;
> +	bool next;
>  
>  	rcu_read_lock();
>  	netif_tx_lock_bh(dev);
> +
>  	for (i = 0; i < local->hw.queues; i++) {
> -		/* Check that this queue is ok */
> -		if (__netif_subqueue_stopped(local->mdev, i) &&
> -		    !test_bit(i, local->queues_pending_run))
> -			continue;
> +		/*
> +		 * If queue is stopped by something other than due to pending
> +		 * frames, or we have no pending frames, proceed to next queue.
> +		 */
> +		spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> +		next = false;
> +		if (local->queue_stop_reasons[i] !=
> +			BIT(IEEE80211_QUEUE_STOP_REASON_PENDING) ||
> +		    skb_queue_empty(&local->pending[i]))
> +			next = true;
> +		spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
>  
> -		if (!test_bit(i, local->queues_pending)) {
> -			clear_bit(i, local->queues_pending_run);
> -			ieee80211_wake_queue(&local->hw, i);
> +		if (next)
>  			continue;
> -		}
>  
> -		clear_bit(i, local->queues_pending_run);
> +		/*
> +		 * start the queue now to allow processing our packets,
> +		 * we're under the tx lock here anyway so nothing will
> +		 * happen as a result of this
> +		 */
>  		netif_start_subqueue(local->mdev, i);
>  
> -		store = &local->pending_packet[i];
> -		tx.flags = 0;
> -		tx.skb = store->skb;
> -		hdr = (struct ieee80211_hdr *)tx.skb->data;
> -		tx.sta = sta_info_get(local, hdr->addr1);
> -		ret = __ieee80211_tx(local, &tx);
> -		store->skb = tx.skb;
> -		if (!ret) {
> -			clear_bit(i, local->queues_pending);
> -			ieee80211_wake_queue(&local->hw, i);
> +		while (!skb_queue_empty(&local->pending[i])) {
> +			tx.flags = 0;
> +			tx.skb = skb_dequeue(&local->pending[i]);
> +			hdr = (struct ieee80211_hdr *)tx.skb->data;
> +			tx.sta = sta_info_get(local, hdr->addr1);
> +
> +			ret = __ieee80211_tx(local, &tx);
> +			if (ret != IEEE80211_TX_OK) {
> +				skb_queue_head(&local->pending[i], tx.skb);
> +				break;
> +			}
>  		}

So this is good functional change, might be worth mentioning in the
commit log, that is, we now requeue onto the pending queue the skb
when the driver's tx() didn't return NETDEV_TX_OK or when __ieee80211_tx()
returns IEEE80211_TX_PENDING (which happens when the queue is stopped).
Maybe even better as a seperate patch. Before this we were just dropping
the skbs in the pending queue under those same conditions.

Other than that:

Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

  Luis

Re: [PATCH 3/8] mac80211: rework the pending packets code

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

On Fri, 2009-03-27 at 18:22 -0400, Luis R. Rodriguez wrote:

> So this is good functional change, might be worth mentioning in the
> commit log, that is, we now requeue onto the pending queue the skb
> when the driver's tx() didn't return NETDEV_TX_OK or when __ieee80211_tx()
> returns IEEE80211_TX_PENDING (which happens when the queue is stopped).
> Maybe even better as a seperate patch. Before this we were just dropping
> the skbs in the pending queue under those same conditions.

Hmm? We always did that. Well, we didn't put it on there directly, we
kept it in extra_tx or whatever it was called and then later pushed it
over to the pending_frames thing.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 4/8] mac80211: clean up __ieee80211_tx args

[Johannes Berg]

__ieee80211_tx takes a struct ieee80211_tx_data argument, but only
uses a few of its members, namely 'skb' and 'sta'. Make that explicit,
so that less internal knowledge is required in ieee80211_tx_pending
and the possibility of introducing errors here is removed.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/tx.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

--- wireless-testing.orig/net/mac80211/tx.c	2009-03-23 14:03:45.000000000 +0100
+++ wireless-testing/net/mac80211/tx.c	2009-03-23 14:03:46.000000000 +0100
@@ -1084,9 +1084,10 @@ static int ieee80211_tx_prepare(struct i
 }
 
 static int __ieee80211_tx(struct ieee80211_local *local,
-			  struct ieee80211_tx_data *tx)
+			  struct sk_buff **skbp,
+			  struct sta_info *sta)
 {
-	struct sk_buff *skb = tx->skb, *next;
+	struct sk_buff *skb = *skbp, *next;
 	struct ieee80211_tx_info *info;
 	int ret;
 	bool fragm = false;
@@ -1111,23 +1112,23 @@ static int __ieee80211_tx(struct ieee802
 		 * We will later move this down into the only driver that
 		 * needs it, iwlwifi.
 		 */
-		if (tx->sta && local->hw.ampdu_queues &&
+		if (sta && local->hw.ampdu_queues &&
 		    info->flags & IEEE80211_TX_CTL_AMPDU) {
 			unsigned long flags;
 			u8 *qc = ieee80211_get_qos_ctl((void *) skb->data);
 			int tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
 
-			spin_lock_irqsave(&tx->sta->lock, flags);
+			spin_lock_irqsave(&sta->lock, flags);
 			skb_set_queue_mapping(skb, local->hw.queues +
-						   tx->sta->tid_to_tx_q[tid]);
-			spin_unlock_irqrestore(&tx->sta->lock, flags);
+						   sta->tid_to_tx_q[tid]);
+			spin_unlock_irqrestore(&sta->lock, flags);
 		}
 
 		next = skb->next;
 		ret = local->ops->tx(local_to_hw(local), skb);
 		if (ret != NETDEV_TX_OK)
 			return IEEE80211_TX_AGAIN;
-		tx->skb = skb = next;
+		*skbp = skb = next;
 		ieee80211_led_tx(local, 1);
 		fragm = true;
 	}
@@ -1223,7 +1224,7 @@ static int ieee80211_tx(struct net_devic
 
 	retries = 0;
  retry:
-	ret = __ieee80211_tx(local, &tx);
+	ret = __ieee80211_tx(local, &tx.skb, tx.sta);
 	switch (ret) {
 	case IEEE80211_TX_OK:
 		break;
@@ -1831,7 +1832,6 @@ void ieee80211_tx_pending(unsigned long 
 	struct net_device *dev = local->mdev;
 	struct ieee80211_hdr *hdr;
 	unsigned long flags;
-	struct ieee80211_tx_data tx;
 	int i, ret;
 	bool next;
 
@@ -1862,14 +1862,15 @@ void ieee80211_tx_pending(unsigned long 
 		netif_start_subqueue(local->mdev, i);
 
 		while (!skb_queue_empty(&local->pending[i])) {
-			tx.flags = 0;
-			tx.skb = skb_dequeue(&local->pending[i]);
-			hdr = (struct ieee80211_hdr *)tx.skb->data;
-			tx.sta = sta_info_get(local, hdr->addr1);
+			struct sk_buff *skb = skb_dequeue(&local->pending[i]);
+			struct sta_info *sta;
+
+			hdr = (struct ieee80211_hdr *)skb->data;
+			sta = sta_info_get(local, hdr->addr1);
 
-			ret = __ieee80211_tx(local, &tx);
+			ret = __ieee80211_tx(local, &skb, sta);
 			if (ret != IEEE80211_TX_OK) {
-				skb_queue_head(&local->pending[i], tx.skb);
+				skb_queue_head(&local->pending[i], skb);
 				break;
 			}
 		}

-- 

Re: [PATCH 4/8] mac80211: clean up __ieee80211_tx args

[Luis R. Rodriguez]

On Mon, Mar 23, 2009 at 05:28:38PM +0100, Johannes Berg wrote:
> __ieee80211_tx takes a struct ieee80211_tx_data argument, but only
> uses a few of its members, namely 'skb' and 'sta'. Make that explicit,
> so that less internal knowledge is required in ieee80211_tx_pending
> and the possibility of introducing errors here is removed.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

  Luis

[PATCH 5/8] mac80211: unify and fix TX aggregation start

[Johannes Berg]

When TX aggregation becomes operational, we do a number of steps:
 1) print a debug message
 2) wake the virtual queue
 3) notify the driver

Unfortunately, 1) and 3) are only done if the driver is first to
reply to the aggregation request, it is, however, possible that the
remote station replies before the driver! Thus, unify the code for
this and call the new function ieee80211_agg_tx_operational in both
places where TX aggregation can become operational.

Additionally, rename the driver notification from
IEEE80211_AMPDU_TX_RESUME to IEEE80211_AMPDU_TX_OPERATIONAL.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/wireless/ath9k/main.c |    2 -
 include/net/mac80211.h            |    4 +-
 net/mac80211/agg-tx.c             |   63 ++++++++++++++++----------------------
 3 files changed, 30 insertions(+), 39 deletions(-)

--- wireless-testing.orig/net/mac80211/agg-tx.c	2009-03-23 13:59:19.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c	2009-03-23 14:03:46.000000000 +0100
@@ -404,6 +404,27 @@ int ieee80211_start_tx_ba_session(struct
 }
 EXPORT_SYMBOL(ieee80211_start_tx_ba_session);
 
+static void ieee80211_agg_tx_operational(struct ieee80211_local *local,
+					 struct sta_info *sta, u16 tid)
+{
+#ifdef CONFIG_MAC80211_HT_DEBUG
+	printk(KERN_DEBUG "Aggregation is on for tid %d \n", tid);
+#endif
+
+	if (local->hw.ampdu_queues) {
+		/*
+		 * Wake up the A-MPDU queue, we stopped it earlier,
+		 * this will in turn wake the entire AC.
+		 */
+		ieee80211_wake_queue_by_reason(&local->hw,
+			local->hw.queues + sta->tid_to_tx_q[tid],
+			IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+	}
+
+	local->ops->ampdu_action(&local->hw, IEEE80211_AMPDU_TX_OPERATIONAL,
+				 &sta->sta, tid, NULL);
+}
+
 void ieee80211_start_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u16 tid)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
@@ -446,20 +467,8 @@ void ieee80211_start_tx_ba_cb(struct iee
 
 	*state |= HT_ADDBA_DRV_READY_MSK;
 
-	if (*state == HT_AGG_STATE_OPERATIONAL) {
-#ifdef CONFIG_MAC80211_HT_DEBUG
-		printk(KERN_DEBUG "Aggregation is on for tid %d \n", tid);
-#endif
-		if (hw->ampdu_queues) {
-			/*
-			 * Wake up this queue, we stopped it earlier,
-			 * this will in turn wake the entire AC.
-			 */
-			ieee80211_wake_queue_by_reason(hw,
-				hw->queues + sta->tid_to_tx_q[tid],
-				IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
-		}
-	}
+	if (*state == HT_AGG_STATE_OPERATIONAL)
+		ieee80211_agg_tx_operational(local, sta, tid);
 
  out:
 	spin_unlock_bh(&sta->lock);
@@ -646,9 +655,7 @@ void ieee80211_process_addba_resp(struct
 				  struct ieee80211_mgmt *mgmt,
 				  size_t len)
 {
-	struct ieee80211_hw *hw = &local->hw;
-	u16 capab;
-	u16 tid, start_seq_num;
+	u16 capab, tid;
 	u8 *state;
 
 	capab = le16_to_cpu(mgmt->u.action.u.addba_resp.capab);
@@ -682,26 +689,10 @@ void ieee80211_process_addba_resp(struct
 
 		*state |= HT_ADDBA_RECEIVED_MSK;
 
-		if (hw->ampdu_queues && *state != curstate &&
-		    *state == HT_AGG_STATE_OPERATIONAL) {
-			/*
-			 * Wake up this queue, we stopped it earlier,
-			 * this will in turn wake the entire AC.
-			 */
-			ieee80211_wake_queue_by_reason(hw,
-				hw->queues + sta->tid_to_tx_q[tid],
-				IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
-		}
-		sta->ampdu_mlme.addba_req_num[tid] = 0;
+		if (*state != curstate && *state == HT_AGG_STATE_OPERATIONAL)
+			ieee80211_agg_tx_operational(local, sta, tid);
 
-		if (local->ops->ampdu_action) {
-			(void)local->ops->ampdu_action(hw,
-					       IEEE80211_AMPDU_TX_RESUME,
-					       &sta->sta, tid, &start_seq_num);
-		}
-#ifdef CONFIG_MAC80211_HT_DEBUG
-		printk(KERN_DEBUG "Resuming TX aggregation for tid %d\n", tid);
-#endif /* CONFIG_MAC80211_HT_DEBUG */
+		sta->ampdu_mlme.addba_req_num[tid] = 0;
 	} else {
 		sta->ampdu_mlme.addba_req_num[tid]++;
 		___ieee80211_stop_tx_ba_session(sta, tid, WLAN_BACK_INITIATOR);
--- wireless-testing.orig/include/net/mac80211.h	2009-03-23 14:00:12.000000000 +0100
+++ wireless-testing/include/net/mac80211.h	2009-03-23 14:03:46.000000000 +0100
@@ -1213,14 +1213,14 @@ enum ieee80211_filter_flags {
  * @IEEE80211_AMPDU_RX_STOP: stop Rx aggregation
  * @IEEE80211_AMPDU_TX_START: start Tx aggregation
  * @IEEE80211_AMPDU_TX_STOP: stop Tx aggregation
- * @IEEE80211_AMPDU_TX_RESUME: resume TX aggregation
+ * @IEEE80211_AMPDU_TX_OPERATIONAL: TX aggregation has become operational
  */
 enum ieee80211_ampdu_mlme_action {
 	IEEE80211_AMPDU_RX_START,
 	IEEE80211_AMPDU_RX_STOP,
 	IEEE80211_AMPDU_TX_START,
 	IEEE80211_AMPDU_TX_STOP,
-	IEEE80211_AMPDU_TX_RESUME,
+	IEEE80211_AMPDU_TX_OPERATIONAL,
 };
 
 /**
--- wireless-testing.orig/drivers/net/wireless/ath9k/main.c	2009-03-23 13:59:19.000000000 +0100
+++ wireless-testing/drivers/net/wireless/ath9k/main.c	2009-03-23 14:03:46.000000000 +0100
@@ -2730,7 +2730,7 @@ static int ath9k_ampdu_action(struct iee
 
 		ieee80211_stop_tx_ba_cb_irqsafe(hw, sta->addr, tid);
 		break;
-	case IEEE80211_AMPDU_TX_RESUME:
+	case IEEE80211_AMPDU_TX_OPERATIONAL:
 		ath_tx_aggr_resume(sc, sta, tid);
 		break;
 	default:

-- 

Re: [PATCH 5/8] mac80211: unify and fix TX aggregation start

[Luis R. Rodriguez]

On Mon, Mar 23, 2009 at 05:28:39PM +0100, Johannes Berg wrote:
> When TX aggregation becomes operational, we do a number of steps:
>  1) print a debug message
>  2) wake the virtual queue
>  3) notify the driver
> 
> Unfortunately, 1) and 3) are only done if the driver is first to
> reply to the aggregation request, it is, however, possible that the
> remote station replies before the driver! Thus, unify the code for
> this and call the new function ieee80211_agg_tx_operational in both
> places where TX aggregation can become operational.
> 
> Additionally, rename the driver notification from
> IEEE80211_AMPDU_TX_RESUME to IEEE80211_AMPDU_TX_OPERATIONAL.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Nice catch man!

Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

  Luis

Re: [PATCH 5/8] mac80211: unify and fix TX aggregation start

[Luis R. Rodriguez]

On Fri, Mar 27, 2009 at 7:27 PM, Luis R. Rodriguez
<mcgrof@bombadil.infradead.org> wrote:
> On Mon, Mar 23, 2009 at 05:28:39PM +0100, Johannes Berg wrote:
>> When TX aggregation becomes operational, we do a number of steps:
>> =C2=A01) print a debug message
>> =C2=A02) wake the virtual queue
>> =C2=A03) notify the driver
>>
>> Unfortunately, 1) and 3) are only done if the driver is first to
>> reply to the aggregation request, it is, however, possible that the
>> remote station replies before the driver! Thus, unify the code for
>> this and call the new function ieee80211_agg_tx_operational in both
>> places where TX aggregation can become operational.
>>
>> Additionally, rename the driver notification from
>> IEEE80211_AMPDU_TX_RESUME to IEEE80211_AMPDU_TX_OPERATIONAL.
>>
>> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>
> Nice catch man!
>
> Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

Hm, maybe also a backported version could go to stable? Perhaps we can
trigger a panic on a UP box?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] mac80211: unify and fix TX aggregation start

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On Fri, 2009-03-27 at 20:01 -0700, Luis R. Rodriguez wrote:

> >> Unfortunately, 1) and 3) are only done if the driver is first to
> >> reply to the aggregation request, it is, however, possible that the
> >> remote station replies before the driver! Thus, unify the code for
> >> this and call the new function ieee80211_agg_tx_operational in both
> >> places where TX aggregation can become operational.
> >>
> >> Additionally, rename the driver notification from
> >> IEEE80211_AMPDU_TX_RESUME to IEEE80211_AMPDU_TX_OPERATIONAL.
> >>
> >> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> >
> > Nice catch man!
> >
> > Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> 
> Hm, maybe also a backported version could go to stable? Perhaps we can
> trigger a panic on a UP box?

No need -- ath9k always replies _instantly_, and iwlwifi doesn't support
aggregation in stable.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 6/8] mac80211: add skb length sanity checking

[Johannes Berg]

We just found a bug in zd1211rw where it would reject
packets in the ->tx() method but leave them modified,
which would cause retransmit attempts with completely
bogus skbs, eventually leading to a panic due to not
having enough headroom in those.

This patch adds a sanity check to mac80211 to catch
such driver mistakes; in this case we warn and drop
the skb.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/tx.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- wireless-testing.orig/net/mac80211/tx.c	2009-03-23 14:03:46.000000000 +0100
+++ wireless-testing/net/mac80211/tx.c	2009-03-23 14:03:47.000000000 +0100
@@ -1089,7 +1089,7 @@ static int __ieee80211_tx(struct ieee802
 {
 	struct sk_buff *skb = *skbp, *next;
 	struct ieee80211_tx_info *info;
-	int ret;
+	int ret, len;
 	bool fragm = false;
 
 	local->mdev->trans_start = jiffies;
@@ -1125,7 +1125,12 @@ static int __ieee80211_tx(struct ieee802
 		}
 
 		next = skb->next;
+		len = skb->len;
 		ret = local->ops->tx(local_to_hw(local), skb);
+		if (WARN_ON(ret != NETDEV_TX_OK && skb->len != len)) {
+			dev_kfree_skb(skb);
+			ret = NETDEV_TX_OK;
+		}
 		if (ret != NETDEV_TX_OK)
 			return IEEE80211_TX_AGAIN;
 		*skbp = skb = next;

-- 

Re: [PATCH 6/8] mac80211: add skb length sanity checking

[Luis R. Rodriguez]

On Mon, Mar 23, 2009 at 05:28:40PM +0100, Johannes Berg wrote:
> We just found a bug in zd1211rw where it would reject
> packets in the ->tx() method but leave them modified,
> which would cause retransmit attempts with completely
> bogus skbs, eventually leading to a panic due to not
> having enough headroom in those.
> 
> This patch adds a sanity check to mac80211 to catch
> such driver mistakes; in this case we warn and drop
> the skb.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Heh well this is obviously even a stable candidate and it got
merged already :D.

  Luis

Re: [PATCH 6/8] mac80211: add skb length sanity checking

[Luis R. Rodriguez]

On Fri, Mar 27, 2009 at 7:40 PM, Luis R. Rodriguez
<mcgrof@bombadil.infradead.org> wrote:
> On Mon, Mar 23, 2009 at 05:28:40PM +0100, Johannes Berg wrote:
>> We just found a bug in zd1211rw where it would reject
>> packets in the ->tx() method but leave them modified,
>> which would cause retransmit attempts with completely
>> bogus skbs, eventually leading to a panic due to not
>> having enough headroom in those.
>>
>> This patch adds a sanity check to mac80211 to catch
>> such driver mistakes; in this case we warn and drop
>> the skb.
>>
>> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>
> Heh well this is obviously even a stable candidate and it got
> merged already :D.

Nevermind, I was looking at .30, heh. So if zd1211rw also manged the
skb len in older kernels it seems this is stable candidate. Too lazy
to check right now though.

  Luis

Re: [PATCH 6/8] mac80211: add skb length sanity checking

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

On Fri, 2009-03-27 at 20:00 -0700, Luis R. Rodriguez wrote:

> >> This patch adds a sanity check to mac80211 to catch
> >> such driver mistakes; in this case we warn and drop
> >> the skb.
> >>
> >> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> >
> > Heh well this is obviously even a stable candidate and it got
> > merged already :D.
> 
> Nevermind, I was looking at .30, heh. So if zd1211rw also manged the
> skb len in older kernels it seems this is stable candidate. Too lazy
> to check right now though.

The fixes should go to stable, but this won't apply without serious
backporting.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Johannes Berg]

Instead of stopping the entire AC queue when enabling aggregation
(which was only done for hardware with aggregation queues) buffer
the packets for each station, and release them to the pending skb
queue once aggregation is turned on successfully.

We get a little more code, but it becomes conceptually simpler and
we can remove the entire virtual queue mechanism from mac80211 in
a follow-up patch.

This changes how mac80211 behaves towards drivers that support
aggregation but have no hardware queues -- those drivers will now
not be handed packets while the aggregation session is being
established, but only after it has been fully established.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 include/net/mac80211.h     |    4 +
 net/mac80211/agg-tx.c      |  136 ++++++++++++++++++++++++++-----------------
 net/mac80211/ieee80211_i.h |    8 ++
 net/mac80211/main.c        |    2 
 net/mac80211/sta_info.c    |    5 +
 net/mac80211/sta_info.h    |    2 
 net/mac80211/tx.c          |  142 ++++++++++++++++++++++++++++++++++++---------
 7 files changed, 221 insertions(+), 78 deletions(-)

--- wireless-testing.orig/net/mac80211/sta_info.h	2009-03-23 13:59:18.000000000 +0100
+++ wireless-testing/net/mac80211/sta_info.h	2009-03-23 14:04:30.000000000 +0100
@@ -73,11 +73,13 @@ enum ieee80211_sta_info_flags {
  * struct tid_ampdu_tx - TID aggregation information (Tx).
  *
  * @addba_resp_timer: timer for peer's response to addba request
+ * @pending: pending frames queue -- use sta's spinlock to protect
  * @ssn: Starting Sequence Number expected to be aggregated.
  * @dialog_token: dialog token for aggregation session
  */
 struct tid_ampdu_tx {
 	struct timer_list addba_resp_timer;
+	struct sk_buff_head pending;
 	u16 ssn;
 	u8 dialog_token;
 };
--- wireless-testing.orig/include/net/mac80211.h	2009-03-23 14:03:46.000000000 +0100
+++ wireless-testing/include/net/mac80211.h	2009-03-23 14:04:30.000000000 +0100
@@ -248,6 +248,9 @@ struct ieee80211_bss_conf {
  * @IEEE80211_TX_INTFL_RCALGO: mac80211 internal flag, do not test or
  *	set this flag in the driver; indicates that the rate control
  *	algorithm was used and should be notified of TX status
+ * @IEEE80211_TX_INTFL_NEED_TXPROCESSING: completely internal to mac80211,
+ *	used to indicate that a pending frame requires TX processing before
+ *	it can be sent out.
  */
 enum mac80211_tx_control_flags {
 	IEEE80211_TX_CTL_REQ_TX_STATUS		= BIT(0),
@@ -264,6 +267,7 @@ enum mac80211_tx_control_flags {
 	IEEE80211_TX_STAT_AMPDU_NO_BACK		= BIT(11),
 	IEEE80211_TX_CTL_RATE_CTRL_PROBE	= BIT(12),
 	IEEE80211_TX_INTFL_RCALGO		= BIT(13),
+	IEEE80211_TX_INTFL_NEED_TXPROCESSING	= BIT(14),
 };
 
 /**
--- wireless-testing.orig/net/mac80211/agg-tx.c	2009-03-23 14:03:46.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c	2009-03-23 14:04:30.000000000 +0100
@@ -132,16 +132,6 @@ static int ___ieee80211_stop_tx_ba_sessi
 	state = &sta->ampdu_mlme.tid_state_tx[tid];
 
 	if (local->hw.ampdu_queues) {
-		if (initiator) {
-			/*
-			 * Stop the AC queue to avoid issues where we send
-			 * unaggregated frames already before the delba.
-			 */
-			ieee80211_stop_queue_by_reason(&local->hw,
-				local->hw.queues + sta->tid_to_tx_q[tid],
-				IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
-		}
-
 		/*
 		 * Pretend the driver woke the queue, just in case
 		 * it disabled it before the session was stopped.
@@ -158,6 +148,10 @@ static int ___ieee80211_stop_tx_ba_sessi
 	/* HW shall not deny going back to legacy */
 	if (WARN_ON(ret)) {
 		*state = HT_AGG_STATE_OPERATIONAL;
+		/*
+		 * We may have pending packets get stuck in this case...
+		 * Not bothering with a workaround for now.
+		 */
 	}
 
 	return ret;
@@ -226,13 +220,6 @@ int ieee80211_start_tx_ba_session(struct
 	       ra, tid);
 #endif /* CONFIG_MAC80211_HT_DEBUG */
 
-	if (hw->ampdu_queues && ieee80211_ac_from_tid(tid) == 0) {
-#ifdef CONFIG_MAC80211_HT_DEBUG
-		printk(KERN_DEBUG "rejecting on voice AC\n");
-#endif
-		return -EINVAL;
-	}
-
 	rcu_read_lock();
 
 	sta = sta_info_get(local, ra);
@@ -267,6 +254,7 @@ int ieee80211_start_tx_ba_session(struct
 	}
 
 	spin_lock_bh(&sta->lock);
+	spin_lock(&local->ampdu_lock);
 
 	sdata = sta->sdata;
 
@@ -308,21 +296,19 @@ int ieee80211_start_tx_ba_session(struct
 			ret = -ENOSPC;
 			goto err_unlock_sta;
 		}
-
-		/*
-		 * If we successfully allocate the session, we can't have
-		 * anything going on on the queue this TID maps into, so
-		 * stop it for now. This is a "virtual" stop using the same
-		 * mechanism that drivers will use.
-		 *
-		 * XXX: queue up frames for this session in the sta_info
-		 *	struct instead to avoid hitting all other STAs.
-		 */
-		ieee80211_stop_queue_by_reason(
-			&local->hw, hw->queues + qn,
-			IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
 	}
 
+	/*
+	 * While we're asking the driver about the aggregation,
+	 * stop the AC queue so that we don't have to worry
+	 * about frames that came in while we were doing that,
+	 * which would require us to put them to the AC pending
+	 * afterwards which just makes the code more complex.
+	 */
+	ieee80211_stop_queue_by_reason(
+		&local->hw, ieee80211_ac_from_tid(tid),
+		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+
 	/* prepare A-MPDU MLME for Tx aggregation */
 	sta->ampdu_mlme.tid_tx[tid] =
 			kmalloc(sizeof(struct tid_ampdu_tx), GFP_ATOMIC);
@@ -336,6 +322,8 @@ int ieee80211_start_tx_ba_session(struct
 		goto err_return_queue;
 	}
 
+	skb_queue_head_init(&sta->ampdu_mlme.tid_tx[tid]->pending);
+
 	/* Tx timer */
 	sta->ampdu_mlme.tid_tx[tid]->addba_resp_timer.function =
 			sta_addba_resp_timer_expired;
@@ -362,6 +350,12 @@ int ieee80211_start_tx_ba_session(struct
 	}
 	sta->tid_to_tx_q[tid] = qn;
 
+	/* Driver vetoed or OKed, but we can take packets again now */
+	ieee80211_wake_queue_by_reason(
+		&local->hw, ieee80211_ac_from_tid(tid),
+		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+
+	spin_unlock(&local->ampdu_lock);
 	spin_unlock_bh(&sta->lock);
 
 	/* send an addBA request */
@@ -388,15 +382,16 @@ int ieee80211_start_tx_ba_session(struct
 	sta->ampdu_mlme.tid_tx[tid] = NULL;
  err_return_queue:
 	if (qn >= 0) {
-		/* We failed, so start queue again right away. */
-		ieee80211_wake_queue_by_reason(hw, hw->queues + qn,
-			IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
 		/* give queue back to pool */
 		spin_lock(&local->queue_stop_reason_lock);
 		local->ampdu_ac_queue[qn] = -1;
 		spin_unlock(&local->queue_stop_reason_lock);
 	}
+	ieee80211_wake_queue_by_reason(
+		&local->hw, ieee80211_ac_from_tid(tid),
+		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
  err_unlock_sta:
+	spin_unlock(&local->ampdu_lock);
 	spin_unlock_bh(&sta->lock);
  unlock:
 	rcu_read_unlock();
@@ -404,6 +399,45 @@ int ieee80211_start_tx_ba_session(struct
 }
 EXPORT_SYMBOL(ieee80211_start_tx_ba_session);
 
+/*
+ * splice packets from the STA's pending to the local pending,
+ * requires a call to ieee80211_agg_splice_finish and holding
+ * local->ampdu_lock across both calls.
+ */
+static void ieee80211_agg_splice_packets(struct ieee80211_local *local,
+					 struct sta_info *sta, u16 tid)
+{
+	unsigned long flags;
+	u16 queue = ieee80211_ac_from_tid(tid);
+
+	ieee80211_stop_queue_by_reason(
+		&local->hw, queue,
+		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+
+	if (!skb_queue_empty(&sta->ampdu_mlme.tid_tx[tid]->pending)) {
+		spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+		/* mark queue as pending, it is stopped already */
+		__set_bit(IEEE80211_QUEUE_STOP_REASON_PENDING,
+			  &local->queue_stop_reasons[queue]);
+		/* copy over remaining packets */
+		skb_queue_splice_tail_init(
+			&sta->ampdu_mlme.tid_tx[tid]->pending,
+			&local->pending[queue]);
+		spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
+	}
+}
+
+static void ieee80211_agg_splice_finish(struct ieee80211_local *local,
+					struct sta_info *sta, u16 tid)
+{
+	u16 queue = ieee80211_ac_from_tid(tid);
+
+	ieee80211_wake_queue_by_reason(
+		&local->hw, queue,
+		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+}
+
+/* caller must hold sta->lock */
 static void ieee80211_agg_tx_operational(struct ieee80211_local *local,
 					 struct sta_info *sta, u16 tid)
 {
@@ -411,15 +445,16 @@ static void ieee80211_agg_tx_operational
 	printk(KERN_DEBUG "Aggregation is on for tid %d \n", tid);
 #endif
 
-	if (local->hw.ampdu_queues) {
-		/*
-		 * Wake up the A-MPDU queue, we stopped it earlier,
-		 * this will in turn wake the entire AC.
-		 */
-		ieee80211_wake_queue_by_reason(&local->hw,
-			local->hw.queues + sta->tid_to_tx_q[tid],
-			IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
-	}
+	spin_lock(&local->ampdu_lock);
+	ieee80211_agg_splice_packets(local, sta, tid);
+	/*
+	 * NB: we rely on sta->lock being taken in the TX
+	 * processing here when adding to the pending queue,
+	 * otherwise we could only change the state of the
+	 * session to OPERATIONAL _here_.
+	 */
+	ieee80211_agg_splice_finish(local, sta, tid);
+	spin_unlock(&local->ampdu_lock);
 
 	local->ops->ampdu_action(&local->hw, IEEE80211_AMPDU_TX_OPERATIONAL,
 				 &sta->sta, tid, NULL);
@@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
 			WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
 
 	spin_lock_bh(&sta->lock);
+	spin_lock(&local->ampdu_lock);
 
-	if (*state & HT_AGG_STATE_INITIATOR_MSK &&
-	    hw->ampdu_queues) {
-		/*
-		 * Wake up this queue, we stopped it earlier,
-		 * this will in turn wake the entire AC.
-		 */
-		ieee80211_wake_queue_by_reason(hw,
-			hw->queues + sta->tid_to_tx_q[tid],
-			IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
-	}
+	ieee80211_agg_splice_packets(local, sta, tid);
 
 	*state = HT_AGG_STATE_IDLE;
+	/* from now on packets are no longer put onto sta->pending */
 	sta->ampdu_mlme.addba_req_num[tid] = 0;
 	kfree(sta->ampdu_mlme.tid_tx[tid]);
 	sta->ampdu_mlme.tid_tx[tid] = NULL;
+
+	ieee80211_agg_splice_finish(local, sta, tid);
+
+	spin_unlock(&local->ampdu_lock);
 	spin_unlock_bh(&sta->lock);
 
 	rcu_read_unlock();
--- wireless-testing.orig/net/mac80211/tx.c	2009-03-23 14:03:47.000000000 +0100
+++ wireless-testing/net/mac80211/tx.c	2009-03-23 14:04:30.000000000 +0100
@@ -984,9 +984,9 @@ __ieee80211_tx_prepare(struct ieee80211_
 	struct ieee80211_hdr *hdr;
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-
 	int hdrlen, tid;
 	u8 *qc, *state;
+	bool queued = false;
 
 	memset(tx, 0, sizeof(*tx));
 	tx->skb = skb;
@@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
 		 */
 	}
 
+	/*
+	 * If this flag is set to true anywhere, and we get here,
+	 * we are doing the needed processing, so remove the flag
+	 * now.
+	 */
+	info->flags &= ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
+
 	hdr = (struct ieee80211_hdr *) skb->data;
 
 	tx->sta = sta_info_get(local, hdr->addr1);
 
-	if (tx->sta && ieee80211_is_data_qos(hdr->frame_control)) {
+	if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
+	    (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
 		unsigned long flags;
+		struct tid_ampdu_tx *tid_tx;
+
 		qc = ieee80211_get_qos_ctl(hdr);
 		tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
 
 		spin_lock_irqsave(&tx->sta->lock, flags);
+		/*
+		 * XXX: This spinlock could be fairly expensive, but see the
+		 *	comment in agg-tx.c:ieee80211_agg_tx_operational().
+		 *	One way to solve this would be to do something RCU-like
+		 *	for managing the tid_tx struct and using atomic bitops
+		 *	for the actual state -- by introducing an actual
+		 *	'operational' bit that would be possible. It would
+		 *	require changing ieee80211_agg_tx_operational() to
+		 *	set that bit, and changing the way tid_tx is managed
+		 *	everywhere, including races between that bit and
+		 *	tid_tx going away (tid_tx being added can be easily
+		 *	committed to memory before the 'operational' bit).
+		 */
+		tid_tx = tx->sta->ampdu_mlme.tid_tx[tid];
 		state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
-		if (*state == HT_AGG_STATE_OPERATIONAL)
+		if (*state == HT_AGG_STATE_OPERATIONAL) {
 			info->flags |= IEEE80211_TX_CTL_AMPDU;
+		} else if (*state != HT_AGG_STATE_IDLE) {
+			/* in progress */
+			queued = true;
+			info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
+			__skb_queue_tail(&tid_tx->pending, skb);
+		}
 		spin_unlock_irqrestore(&tx->sta->lock, flags);
+
+		if (unlikely(queued))
+			return TX_QUEUED;
 	}
 
 	if (is_multicast_ether_addr(hdr->addr1)) {
@@ -1077,7 +1110,14 @@ static int ieee80211_tx_prepare(struct i
 	}
 	if (unlikely(!dev))
 		return -ENODEV;
-	/* initialises tx with control */
+	/*
+	 * initialises tx with control
+	 *
+	 * return value is safe to ignore here because this function
+	 * can only be invoked for multicast frames
+	 *
+	 * XXX: clean up
+	 */
 	__ieee80211_tx_prepare(tx, skb, dev);
 	dev_put(dev);
 	return 0;
@@ -1188,7 +1228,8 @@ static int invoke_tx_handlers(struct iee
 	return 0;
 }
 
-static int ieee80211_tx(struct net_device *dev, struct sk_buff *skb)
+static void ieee80211_tx(struct net_device *dev, struct sk_buff *skb,
+			 bool txpending)
 {
 	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
 	struct sta_info *sta;
@@ -1202,11 +1243,11 @@ static int ieee80211_tx(struct net_devic
 
 	queue = skb_get_queue_mapping(skb);
 
-	WARN_ON(!skb_queue_empty(&local->pending[queue]));
+	WARN_ON(!txpending && !skb_queue_empty(&local->pending[queue]));
 
 	if (unlikely(skb->len < 10)) {
 		dev_kfree_skb(skb);
-		return 0;
+		return;
 	}
 
 	rcu_read_lock();
@@ -1214,10 +1255,13 @@ static int ieee80211_tx(struct net_devic
 	/* initialises tx */
 	res_prepare = __ieee80211_tx_prepare(&tx, skb, dev);
 
-	if (res_prepare == TX_DROP) {
+	if (unlikely(res_prepare == TX_DROP)) {
 		dev_kfree_skb(skb);
 		rcu_read_unlock();
-		return 0;
+		return;
+	} else if (unlikely(res_prepare == TX_QUEUED)) {
+		rcu_read_unlock();
+		return;
 	}
 
 	sta = tx.sta;
@@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
 			do {
 				next = skb->next;
 				skb->next = NULL;
-				skb_queue_tail(&local->pending[queue], skb);
+				if (unlikely(txpending))
+					skb_queue_head(&local->pending[queue],
+						       skb);
+				else
+					skb_queue_tail(&local->pending[queue],
+						       skb);
 			} while ((skb = next));
 
 			/*
@@ -1276,7 +1325,7 @@ static int ieee80211_tx(struct net_devic
 	}
  out:
 	rcu_read_unlock();
-	return 0;
+	return;
 
  drop:
 	rcu_read_unlock();
@@ -1287,7 +1336,6 @@ static int ieee80211_tx(struct net_devic
 		dev_kfree_skb(skb);
 		skb = next;
 	}
-	return 0;
 }
 
 /* device xmit handlers */
@@ -1346,7 +1394,6 @@ int ieee80211_master_start_xmit(struct s
 		FOUND_SDATA,
 		UNKNOWN_ADDRESS,
 	} monitor_iface = NOT_MONITOR;
-	int ret;
 
 	if (skb->iif)
 		odev = dev_get_by_index(&init_net, skb->iif);
@@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
 		       "originating device\n", dev->name);
 #endif
 		dev_kfree_skb(skb);
-		return 0;
+		return NETDEV_TX_OK;
 	}
 
 	if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
@@ -1389,7 +1436,7 @@ int ieee80211_master_start_xmit(struct s
 		else
 			if (mesh_nexthop_lookup(skb, osdata)) {
 				dev_put(odev);
-				return 0;
+				return NETDEV_TX_OK;
 			}
 		if (memcmp(odev->dev_addr, hdr->addr4, ETH_ALEN) != 0)
 			IEEE80211_IFSTA_MESH_CTR_INC(&osdata->u.mesh,
@@ -1451,7 +1498,7 @@ int ieee80211_master_start_xmit(struct s
 	if (ieee80211_skb_resize(osdata->local, skb, headroom, may_encrypt)) {
 		dev_kfree_skb(skb);
 		dev_put(odev);
-		return 0;
+		return NETDEV_TX_OK;
 	}
 
 	if (osdata->vif.type == NL80211_IFTYPE_AP_VLAN)
@@ -1460,10 +1507,11 @@ int ieee80211_master_start_xmit(struct s
 				      u.ap);
 	if (likely(monitor_iface != UNKNOWN_ADDRESS))
 		info->control.vif = &osdata->vif;
-	ret = ieee80211_tx(odev, skb);
+
+	ieee80211_tx(odev, skb, false);
 	dev_put(odev);
 
-	return ret;
+	return NETDEV_TX_OK;
 }
 
 int ieee80211_monitor_start_xmit(struct sk_buff *skb,
@@ -1827,6 +1875,54 @@ void ieee80211_clear_tx_pending(struct i
 		skb_queue_purge(&local->pending[i]);
 }
 
+static bool ieee80211_tx_pending_skb(struct ieee80211_local *local,
+				     struct sk_buff *skb)
+{
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+	struct ieee80211_sub_if_data *sdata;
+	struct sta_info *sta;
+	struct ieee80211_hdr *hdr;
+	struct net_device *dev;
+	int ret;
+	bool result = true;
+
+	/* does interface still exist? */
+	dev = dev_get_by_index(&init_net, skb->iif);
+	if (!dev) {
+		dev_kfree_skb(skb);
+		return true;
+	}
+
+	/* validate info->control.vif against skb->iif */
+	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
+		sdata = container_of(sdata->bss,
+				     struct ieee80211_sub_if_data,
+				     u.ap);
+
+	if (unlikely(info->control.vif && info->control.vif != &sdata->vif)) {
+		dev_kfree_skb(skb);
+		result = true;
+		goto out;
+	}
+
+	if (info->flags & IEEE80211_TX_INTFL_NEED_TXPROCESSING) {
+		ieee80211_tx(dev, skb, true);
+	} else {
+		hdr = (struct ieee80211_hdr *)skb->data;
+		sta = sta_info_get(local, hdr->addr1);
+
+		ret = __ieee80211_tx(local, &skb, sta);
+		if (ret != IEEE80211_TX_OK)
+			result = false;
+	}
+
+ out:
+	dev_put(dev);
+
+	return result;
+}
+
 /*
  * Transmit all pending packets. Called from tasklet, locks master device
  * TX lock so that no new packets can come in.
@@ -1835,9 +1931,8 @@ void ieee80211_tx_pending(unsigned long 
 {
 	struct ieee80211_local *local = (struct ieee80211_local *)data;
 	struct net_device *dev = local->mdev;
-	struct ieee80211_hdr *hdr;
 	unsigned long flags;
-	int i, ret;
+	int i;
 	bool next;
 
 	rcu_read_lock();
@@ -1868,13 +1963,8 @@ void ieee80211_tx_pending(unsigned long 
 
 		while (!skb_queue_empty(&local->pending[i])) {
 			struct sk_buff *skb = skb_dequeue(&local->pending[i]);
-			struct sta_info *sta;
-
-			hdr = (struct ieee80211_hdr *)skb->data;
-			sta = sta_info_get(local, hdr->addr1);
 
-			ret = __ieee80211_tx(local, &skb, sta);
-			if (ret != IEEE80211_TX_OK) {
+			if (!ieee80211_tx_pending_skb(local, skb)) {
 				skb_queue_head(&local->pending[i], skb);
 				break;
 			}
--- wireless-testing.orig/net/mac80211/sta_info.c	2009-03-23 13:59:18.000000000 +0100
+++ wireless-testing/net/mac80211/sta_info.c	2009-03-23 14:04:30.000000000 +0100
@@ -239,6 +239,11 @@ void sta_info_destroy(struct sta_info *s
 		tid_tx = sta->ampdu_mlme.tid_tx[i];
 		if (tid_tx) {
 			del_timer_sync(&tid_tx->addba_resp_timer);
+			/*
+			 * STA removed while aggregation session being
+			 * started? Bit odd, but purge frames anyway.
+			 */
+			skb_queue_purge(&tid_tx->pending);
 			kfree(tid_tx);
 		}
 	}
--- wireless-testing.orig/net/mac80211/ieee80211_i.h	2009-03-23 14:03:45.000000000 +0100
+++ wireless-testing/net/mac80211/ieee80211_i.h	2009-03-23 14:04:30.000000000 +0100
@@ -637,6 +637,14 @@ struct ieee80211_local {
 	struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
 	struct tasklet_struct tx_pending_tasklet;
 
+	/*
+	 * This lock is used to prevent concurrent A-MPDU
+	 * session start/stop processing, this thus also
+	 * synchronises the ->ampdu_action() callback to
+	 * drivers and limits it to one at a time.
+	 */
+	spinlock_t ampdu_lock;
+
 	/* number of interfaces with corresponding IFF_ flags */
 	atomic_t iff_allmultis, iff_promiscs;
 
--- wireless-testing.orig/net/mac80211/main.c	2009-03-23 14:03:45.000000000 +0100
+++ wireless-testing/net/mac80211/main.c	2009-03-23 14:04:30.000000000 +0100
@@ -795,6 +795,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(
 	skb_queue_head_init(&local->skb_queue);
 	skb_queue_head_init(&local->skb_queue_unreliable);
 
+	spin_lock_init(&local->ampdu_lock);
+
 	return local_to_hw(local);
 }
 EXPORT_SYMBOL(ieee80211_alloc_hw);

-- 

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Luis R. Rodriguez]

On Mon, Mar 23, 2009 at 05:28:41PM +0100, Johannes Berg wrote:
> Instead of stopping the entire AC queue when enabling aggregation
> (which was only done for hardware with aggregation queues) buffer
> the packets for each station, and release them to the pending skb
> queue once aggregation is turned on successfully.

Thank you for the nice compromised solution.

> We get a little more code, but it becomes conceptually simpler and
> we can remove the entire virtual queue mechanism from mac80211 in
> a follow-up patch.
> 
> This changes how mac80211 behaves towards drivers that support
> aggregation but have no hardware queues -- those drivers will now
> not be handed packets while the aggregation session is being
> established, but only after it has been fully established.

That's fine from a logical point of view as compromise here as our
ampdu start should be relatively quick. Now while I can say this
from a logical point of view this patch obviously needed more
testing but lets see how it holds up and I'm glad you're the one
who wrote it. I'm confident there won't be any issues but that
is something I cannot gaurantee just based on the review. We now
need to go out and tests this. All other patches previous to this
make 100% perfect sense.

I'm particularly interested in seeing results with AP support.
Did you get to test that with ath9k by any chance?

Hauke -- just a heads up since you're quick with this now :)  :
skb_queue_splice_tail_init() needs some backporting to 2.6.27. I'd do
it but I'm beat and calling it a day now.

More comments below. I've tried to remove all the hunks I didn't have
comments on.

> @@ -308,21 +296,19 @@ int ieee80211_start_tx_ba_session(struct
>  			ret = -ENOSPC;
>  			goto err_unlock_sta;
>  		}
> -
> -		/*
> -		 * If we successfully allocate the session, we can't have
> -		 * anything going on on the queue this TID maps into, so
> -		 * stop it for now. This is a "virtual" stop using the same
> -		 * mechanism that drivers will use.
> -		 *
> -		 * XXX: queue up frames for this session in the sta_info
> -		 *	struct instead to avoid hitting all other STAs.
> -		 */
> -		ieee80211_stop_queue_by_reason(
> -			&local->hw, hw->queues + qn,
> -			IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
>  	}
>  
> +	/*
> +	 * While we're asking the driver about the aggregation,
> +	 * stop the AC queue so that we don't have to worry
> +	 * about frames that came in while we were doing that,
> +	 * which would require us to put them to the AC pending
> +	 * afterwards which just makes the code more complex.
> +	 */
> +	ieee80211_stop_queue_by_reason(
> +		&local->hw, ieee80211_ac_from_tid(tid),
> +		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> +

ath9k's ampdu start is pretty quick anyway, so this
won't be for long.

> @@ -404,6 +399,45 @@ int ieee80211_start_tx_ba_session(struct
>  }
>  EXPORT_SYMBOL(ieee80211_start_tx_ba_session);
>  
> +/*
> + * splice packets from the STA's pending to the local pending,
> + * requires a call to ieee80211_agg_splice_finish and holding
> + * local->ampdu_lock across both calls.
> + */
> +static void ieee80211_agg_splice_packets(struct ieee80211_local *local,
> +					 struct sta_info *sta, u16 tid)
> +{
> +	unsigned long flags;
> +	u16 queue = ieee80211_ac_from_tid(tid);
> +
> +	ieee80211_stop_queue_by_reason(
> +		&local->hw, queue,
> +		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);

No point in stopping the queue if the STA's tid queue is empty.

> @@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
>  			WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
>  
>  	spin_lock_bh(&sta->lock);
> +	spin_lock(&local->ampdu_lock);
>  
> -	if (*state & HT_AGG_STATE_INITIATOR_MSK &&
> -	    hw->ampdu_queues) {
> -		/*
> -		 * Wake up this queue, we stopped it earlier,
> -		 * this will in turn wake the entire AC.
> -		 */
> -		ieee80211_wake_queue_by_reason(hw,
> -			hw->queues + sta->tid_to_tx_q[tid],
> -			IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> -	}
> +	ieee80211_agg_splice_packets(local, sta, tid);

Is it possible for ieee80211_stop_tx_ba_cb() to be called while
state of the tid is not yet operational? from what I can tell
that's the only case when we would have added skbs to the STA's tid
pending queue.

>  
>  	*state = HT_AGG_STATE_IDLE;
> +	/* from now on packets are no longer put onto sta->pending */

I think it would help to refer to the pendign queue consitantly instead
as something like "STA's TID pending queue" as that is what it is. We also
now have the local->pending queue. STA's pending queue seems to imply
its also used for non-aggregation sessions.

See -- if the state is not operation nothing would have gone
been put into the STA's tid pending queue. Might also be worth
mentioning that in the comment.

> @@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
>  		 */
>  	}
>  
> +	/*
> +	 * If this flag is set to true anywhere, and we get here,
> +	 * we are doing the needed processing, so remove the flag
> +	 * now.
> +	 */
> +	info->flags &= ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> +
>  	hdr = (struct ieee80211_hdr *) skb->data;
>  
>  	tx->sta = sta_info_get(local, hdr->addr1);
>  
> -	if (tx->sta && ieee80211_is_data_qos(hdr->frame_control)) {
> +	if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
> +	    (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {

Hm, why were we not crashing here before? I figure we would have
with something like this:

state = &tx->sta->ampdu_mlme.tid_state_tx[tid];

That it itself should be separate patch, but too late now. Anyway
the new added check for IEEE80211_HW_AMPDU_AGGREGATION should go
to stable.

>  		unsigned long flags;
> +		struct tid_ampdu_tx *tid_tx;
> +
>  		qc = ieee80211_get_qos_ctl(hdr);
>  		tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
>  
>  		spin_lock_irqsave(&tx->sta->lock, flags);
> +		/*
> +		 * XXX: This spinlock could be fairly expensive, but see the
> +		 *	comment in agg-tx.c:ieee80211_agg_tx_operational().
> +		 *	One way to solve this would be to do something RCU-like
> +		 *	for managing the tid_tx struct and using atomic bitops
> +		 *	for the actual state -- by introducing an actual
> +		 *	'operational' bit that would be possible. It would
> +		 *	require changing ieee80211_agg_tx_operational() to
> +		 *	set that bit, and changing the way tid_tx is managed
> +		 *	everywhere, including races between that bit and
> +		 *	tid_tx going away (tid_tx being added can be easily
> +		 *	committed to memory before the 'operational' bit).
> +		 */
> +		tid_tx = tx->sta->ampdu_mlme.tid_tx[tid];
>  		state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
> -		if (*state == HT_AGG_STATE_OPERATIONAL)
> +		if (*state == HT_AGG_STATE_OPERATIONAL) {
>  			info->flags |= IEEE80211_TX_CTL_AMPDU;

See, when its operational we don't add stuff to the STA's TID pending 
queue.

This piece:

> +		} else if (*state != HT_AGG_STATE_IDLE) {
> +			/* in progress */
> +			queued = true;
> +			info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> +			__skb_queue_tail(&tid_tx->pending, skb);
> +		}

Can probably be ported to stable too, to just TX_DROP.

> @@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
>  			do {
>  				next = skb->next;
>  				skb->next = NULL;
> -				skb_queue_tail(&local->pending[queue], skb);
> +				if (unlikely(txpending))
> +					skb_queue_head(&local->pending[queue],
> +						       skb);
> +				else
> +					skb_queue_tail(&local->pending[queue],
> +						       skb);

For someone who hasn't read all the pending queue code stuff the above would be
a little brain twister. It might be good to leave a comment explaining why
txpendign would be set and why we add it to the head (i.e. because the "skb" sent
at a previous time was put into a pending queue). Maybe even rename txpending to
something like skb_was_pending.

> @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
>  		       "originating device\n", dev->name);
>  #endif
>  		dev_kfree_skb(skb);
> -		return 0;
> +		return NETDEV_TX_OK;

All these NETDEV_TX_OK could've gone into a separate patch.

  Luis

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 7909 bytes --]

On Sat, 2009-03-28 at 00:55 -0400, Luis R. Rodriguez wrote:


> > This changes how mac80211 behaves towards drivers that support
> > aggregation but have no hardware queues -- those drivers will now
> > not be handed packets while the aggregation session is being
> > established, but only after it has been fully established.
> 
> That's fine from a logical point of view as compromise here as our
> ampdu start should be relatively quick. Now while I can say this
> from a logical point of view this patch obviously needed more
> testing but lets see how it holds up and I'm glad you're the one
> who wrote it. I'm confident there won't be any issues but that
> is something I cannot gaurantee just based on the review. We now
> need to go out and tests this. All other patches previous to this
> make 100% perfect sense.

:)
I think it also makes more _sense_ to not ask the driver to handle these
frames, because we won't set the AMPDU flag correctly if the session
start is declined.

> > +	/*
> > +	 * While we're asking the driver about the aggregation,
> > +	 * stop the AC queue so that we don't have to worry
> > +	 * about frames that came in while we were doing that,
> > +	 * which would require us to put them to the AC pending
> > +	 * afterwards which just makes the code more complex.
> > +	 */
> > +	ieee80211_stop_queue_by_reason(
> > +		&local->hw, ieee80211_ac_from_tid(tid),
> > +		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> > +
> 
> ath9k's ampdu start is pretty quick anyway, so this
> won't be for long.

Yup.

> > +/*
> > + * splice packets from the STA's pending to the local pending,
> > + * requires a call to ieee80211_agg_splice_finish and holding
> > + * local->ampdu_lock across both calls.
> > + */
> > +static void ieee80211_agg_splice_packets(struct ieee80211_local *local,
> > +					 struct sta_info *sta, u16 tid)
> > +{
> > +	unsigned long flags;
> > +	u16 queue = ieee80211_ac_from_tid(tid);
> > +
> > +	ieee80211_stop_queue_by_reason(
> > +		&local->hw, queue,
> > +		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> 
> No point in stopping the queue if the STA's tid queue is empty.

Hmm, true I guess.

> > @@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
> >  			WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
> >  
> >  	spin_lock_bh(&sta->lock);
> > +	spin_lock(&local->ampdu_lock);
> >  
> > -	if (*state & HT_AGG_STATE_INITIATOR_MSK &&
> > -	    hw->ampdu_queues) {
> > -		/*
> > -		 * Wake up this queue, we stopped it earlier,
> > -		 * this will in turn wake the entire AC.
> > -		 */
> > -		ieee80211_wake_queue_by_reason(hw,
> > -			hw->queues + sta->tid_to_tx_q[tid],
> > -			IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> > -	}
> > +	ieee80211_agg_splice_packets(local, sta, tid);
> 
> Is it possible for ieee80211_stop_tx_ba_cb() to be called while
> state of the tid is not yet operational? from what I can tell
> that's the only case when we would have added skbs to the STA's tid
> pending queue.

This happens when the session is denied by the peer.

> >  	*state = HT_AGG_STATE_IDLE;
> > +	/* from now on packets are no longer put onto sta->pending */
> 
> I think it would help to refer to the pendign queue consitantly instead
> as something like "STA's TID pending queue" as that is what it is. We also
> now have the local->pending queue. STA's pending queue seems to imply
> its also used for non-aggregation sessions.
> 
> See -- if the state is not operation nothing would have gone
> been put into the STA's tid pending queue. Might also be worth
> mentioning that in the comment.

Hmm, yeah, might rewrite the comments a bit. Mind if I do a separate
patch later?

> > @@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
> >  		 */
> >  	}
> >  
> > +	/*
> > +	 * If this flag is set to true anywhere, and we get here,
> > +	 * we are doing the needed processing, so remove the flag
> > +	 * now.
> > +	 */
> > +	info->flags &= ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> > +
> >  	hdr = (struct ieee80211_hdr *) skb->data;
> >  
> >  	tx->sta = sta_info_get(local, hdr->addr1);
> >  
> > -	if (tx->sta && ieee80211_is_data_qos(hdr->frame_control)) {
> > +	if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
> > +	    (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
> 
> Hm, why were we not crashing here before? I figure we would have
> with something like this:
> 
> state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
> 
> That it itself should be separate patch, but too late now. Anyway
> the new added check for IEEE80211_HW_AMPDU_AGGREGATION should go
> to stable.

Nah, state_tx always exists (and will be IDLE) but this was just an
added check to make us not need the spinlock for non-ampdu hw.

> >  		unsigned long flags;
> > +		struct tid_ampdu_tx *tid_tx;
> > +
> >  		qc = ieee80211_get_qos_ctl(hdr);
> >  		tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
> >  
> >  		spin_lock_irqsave(&tx->sta->lock, flags);
> > +		/*
> > +		 * XXX: This spinlock could be fairly expensive, but see the
> > +		 *	comment in agg-tx.c:ieee80211_agg_tx_operational().
> > +		 *	One way to solve this would be to do something RCU-like
> > +		 *	for managing the tid_tx struct and using atomic bitops
> > +		 *	for the actual state -- by introducing an actual
> > +		 *	'operational' bit that would be possible. It would
> > +		 *	require changing ieee80211_agg_tx_operational() to
> > +		 *	set that bit, and changing the way tid_tx is managed
> > +		 *	everywhere, including races between that bit and
> > +		 *	tid_tx going away (tid_tx being added can be easily
> > +		 *	committed to memory before the 'operational' bit).
> > +		 */
> > +		tid_tx = tx->sta->ampdu_mlme.tid_tx[tid];
> >  		state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
> > -		if (*state == HT_AGG_STATE_OPERATIONAL)
> > +		if (*state == HT_AGG_STATE_OPERATIONAL) {
> >  			info->flags |= IEEE80211_TX_CTL_AMPDU;
> 
> See, when its operational we don't add stuff to the STA's TID pending 
> queue.
> 
> This piece:
> 
> > +		} else if (*state != HT_AGG_STATE_IDLE) {
> > +			/* in progress */
> > +			queued = true;
> > +			info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> > +			__skb_queue_tail(&tid_tx->pending, skb);
> > +		}
> 
> Can probably be ported to stable too, to just TX_DROP.

No, why? We're handing the frames to the driver. It wants to handle
those frames before agg session is started correctly. That's what I was
referring to before with this making more sense now.

> > @@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
> >  			do {
> >  				next = skb->next;
> >  				skb->next = NULL;
> > -				skb_queue_tail(&local->pending[queue], skb);
> > +				if (unlikely(txpending))
> > +					skb_queue_head(&local->pending[queue],
> > +						       skb);
> > +				else
> > +					skb_queue_tail(&local->pending[queue],
> > +						       skb);
> 
> For someone who hasn't read all the pending queue code stuff the above would be
> a little brain twister. It might be good to leave a comment explaining why
> txpendign would be set and why we add it to the head (i.e. because the "skb" sent
> at a previous time was put into a pending queue). Maybe even rename txpending to
> something like skb_was_pending.

Ok that might make some sense, though as a parameter you can easily see
where it's coming from, I think :) I agree that the entire code is a
little brain twister...

> > @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
> >  		       "originating device\n", dev->name);
> >  #endif
> >  		dev_kfree_skb(skb);
> > -		return 0;
> > +		return NETDEV_TX_OK;
> 
> All these NETDEV_TX_OK could've gone into a separate patch.

I guess, they're also not strictly necessary since 0 == TX_OK :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Luis R. Rodriguez]

On Sat, Mar 28, 2009 at 10:41 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sat, 2009-03-28 at 00:55 -0400, Luis R. Rodriguez wrote:
>
>
>> > This changes how mac80211 behaves towards drivers that support
>> > aggregation but have no hardware queues -- those drivers will now
>> > not be handed packets while the aggregation session is being
>> > established, but only after it has been fully established.
>>
>> That's fine from a logical point of view as compromise here as our
>> ampdu start should be relatively quick. Now while I can say this
>> from a logical point of view this patch obviously needed more
>> testing but lets see how it holds up and I'm glad you're the one
>> who wrote it. I'm confident there won't be any issues but that
>> is something I cannot gaurantee just based on the review. We now
>> need to go out and tests this. All other patches previous to this
>> make 100% perfect sense.
>
> :)
> I think it also makes more _sense_ to not ask the driver to handle th=
ese
> frames, because we won't set the AMPDU flag correctly if the session
> start is declined.

Hm, wouldn't we send them as normal frames if its declined here eventua=
lly?

>> > @@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
>> >
>> > =C2=A0 =C2=A0 spin_lock_bh(&sta->lock);
>> > + =C2=A0 spin_lock(&local->ampdu_lock);
>> >
>> > - =C2=A0 if (*state & HT_AGG_STATE_INITIATOR_MSK &&
>> > - =C2=A0 =C2=A0 =C2=A0 hw->ampdu_queues) {
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Wake up this queue, w=
e stopped it earlier,
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* this will in turn wak=
e the entire AC.
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ieee80211_wake_queue_by_reaso=
n(hw,
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 h=
w->queues + sta->tid_to_tx_q[tid],
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 I=
EEE80211_QUEUE_STOP_REASON_AGGREGATION);
>> > - =C2=A0 }
>> > + =C2=A0 ieee80211_agg_splice_packets(local, sta, tid);
>>
>> Is it possible for ieee80211_stop_tx_ba_cb() to be called while
>> state of the tid is not yet operational? from what I can tell
>> that's the only case when we would have added skbs to the STA's tid
>> pending queue.
>
> This happens when the session is denied by the peer.

Ah, got it, thanks.

>> > =C2=A0 =C2=A0 *state =3D HT_AGG_STATE_IDLE;
>> > + =C2=A0 /* from now on packets are no longer put onto sta->pendin=
g */
>>
>> I think it would help to refer to the pendign queue consitantly inst=
ead
>> as something like "STA's TID pending queue" as that is what it is. W=
e also
>> now have the local->pending queue. STA's pending queue seems to impl=
y
>> its also used for non-aggregation sessions.
>>
>> See -- if the state is not operation nothing would have gone
>> been put into the STA's tid pending queue. Might also be worth
>> mentioning that in the comment.
>
> Hmm, yeah, might rewrite the comments a bit. Mind if I do a separate
> patch later?

Nope.

>> > @@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> > =C2=A0 =C2=A0 }
>> >
>> > + =C2=A0 /*
>> > + =C2=A0 =C2=A0* If this flag is set to true anywhere, and we get =
here,
>> > + =C2=A0 =C2=A0* we are doing the needed processing, so remove the=
 flag
>> > + =C2=A0 =C2=A0* now.
>> > + =C2=A0 =C2=A0*/
>> > + =C2=A0 info->flags &=3D ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> > +
>> > =C2=A0 =C2=A0 hdr =3D (struct ieee80211_hdr *) skb->data;
>> >
>> > =C2=A0 =C2=A0 tx->sta =3D sta_info_get(local, hdr->addr1);
>> >
>> > - =C2=A0 if (tx->sta && ieee80211_is_data_qos(hdr->frame_control))=
 {
>> > + =C2=A0 if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) =
&&
>> > + =C2=A0 =C2=A0 =C2=A0 (local->hw.flags & IEEE80211_HW_AMPDU_AGGRE=
GATION)) {
>>
>> Hm, why were we not crashing here before? I figure we would have
>> with something like this:
>>
>> state =3D &tx->sta->ampdu_mlme.tid_state_tx[tid];
>>
>> That it itself should be separate patch, but too late now. Anyway
>> the new added check for IEEE80211_HW_AMPDU_AGGREGATION should go
>> to stable.
>
> Nah, state_tx always exists (and will be IDLE) but this was just an
> added check to make us not need the spinlock for non-ampdu hw.

Ah yes, its the ampdu_mlme.tid_tx that gets kmalloc'd only on aggr
session start.

>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned long flags;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct tid_ampdu_tx *tid_tx;
>> > +
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qc =3D ieee80211_get_qos=
_ctl(hdr);
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tid =3D *qc & IEEE80211_=
QOS_CTL_TID_MASK;
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 spin_lock_irqsave(&tx->s=
ta->lock, flags);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* XXX: This spinlock co=
uld be fairly expensive, but see the
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0c=
omment in agg-tx.c:ieee80211_agg_tx_operational().
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0O=
ne way to solve this would be to do something RCU-like
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0f=
or managing the tid_tx struct and using atomic bitops
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0f=
or the actual state -- by introducing an actual
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0'=
operational' bit that would be possible. It would
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0r=
equire changing ieee80211_agg_tx_operational() to
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0s=
et that bit, and changing the way tid_tx is managed
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0e=
verywhere, including races between that bit and
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0t=
id_tx going away (tid_tx being added can be easily
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0c=
ommitted to memory before the 'operational' bit).
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tid_tx =3D tx->sta->ampdu_mlm=
e.tid_tx[tid];
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 state =3D &tx->sta->ampd=
u_mlme.tid_state_tx[tid];
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (*state =3D=3D HT_AGG_STAT=
E_OPERATIONAL)
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (*state =3D=3D HT_AGG_STAT=
E_OPERATIONAL) {
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 info->flags |=3D IEEE80211_TX_CTL_AMPDU;
>>
>> See, when its operational we don't add stuff to the STA's TID pendin=
g
>> queue.
>>
>> This piece:
>>
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else if (*state !=3D HT_AGG=
_STATE_IDLE) {
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /=
* in progress */
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 q=
ueued =3D true;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 i=
nfo->flags |=3D IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 _=
_skb_queue_tail(&tid_tx->pending, skb);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>>
>> Can probably be ported to stable too, to just TX_DROP.
>
> No, why? We're handing the frames to the driver. It wants to handle
> those frames before agg session is started correctly. That's what I w=
as
> referring to before with this making more sense now.

OK I see that but I am not sure of what's done to the skb in the old
case if the session is note complete yet, nor now if the session gets
declined. I can check later, right now I'm feeling lazy.

>> > @@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 do {
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 next =3D skb->next;
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb->next =3D NULL;
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 skb_queue_tail(&local->pending[queue], skb);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 if (unlikely(txpending))
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb_queue_head(&lo=
cal->pending[queue],
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0skb);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 else
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb_queue_tail(&lo=
cal->pending[queue],
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0skb);
>>
>> For someone who hasn't read all the pending queue code stuff the abo=
ve would be
>> a little brain twister. It might be good to leave a comment explaini=
ng why
>> txpendign would be set and why we add it to the head (i.e. because t=
he "skb" sent
>> at a previous time was put into a pending queue). Maybe even rename =
txpending to
>> something like skb_was_pending.
>
> Ok that might make some sense, though as a parameter you can easily s=
ee
> where it's coming from, I think :) I agree that the entire code is a
> little brain twister...

Yeah.. I had to re-read aggregation code again to get the hang of it.
Anything we can do to make
it easier for people to pick would be nice. I think I'll go patch up
the aggr-tx kdoc too now, unless we
plan on nuking some stuff soon again. I suspect the next big change
will be to move software based
aggregation queue handling and mapping to ACs within mac80211 for that
type of hardware which
we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
and our next generation 11n USB
driver.

>> > @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0"originating device\n", dev->name);
>> > =C2=A0#endif
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_kfree_skb(skb);
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return NETDEV_TX_OK;
>>
>> All these NETDEV_TX_OK could've gone into a separate patch.
>
> I guess, they're also not strictly necessary since 0 =3D=3D TX_OK :)

Sure, what I'm trying to say is these patches are pretty hard to
review because of the topic,
it just helps to trim them down as much as possible -- but I realize
even that is painful.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

On Sat, 2009-03-28 at 12:18 -0700, Luis R. Rodriguez wrote:

> >> That's fine from a logical point of view as compromise here as our
> >> ampdu start should be relatively quick. Now while I can say this
> >> from a logical point of view this patch obviously needed more
> >> testing but lets see how it holds up and I'm glad you're the one
> >> who wrote it. I'm confident there won't be any issues but that
> >> is something I cannot gaurantee just based on the review. We now
> >> need to go out and tests this. All other patches previous to this
> >> make 100% perfect sense.
> >
> > :)
> > I think it also makes more _sense_ to not ask the driver to handle these
> > frames, because we won't set the AMPDU flag correctly if the session
> > start is declined.
> 
> Hm, wouldn't we send them as normal frames if its declined here eventually?

Yes, we take them off the sta->pending, put them through TX processing
and send them as normal frames.


> >> This piece:
> >>
> >> > +           } else if (*state != HT_AGG_STATE_IDLE) {
> >> > +                   /* in progress */
> >> > +                   queued = true;
> >> > +                   info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> >> > +                   __skb_queue_tail(&tid_tx->pending, skb);
> >> > +           }
> >>
> >> Can probably be ported to stable too, to just TX_DROP.
> >
> > No, why? We're handing the frames to the driver. It wants to handle
> > those frames before agg session is started correctly. That's what I was
> > referring to before with this making more sense now.
> 
> OK I see that but I am not sure of what's done to the skb in the old
> case if the session is note complete yet, nor now if the session gets
> declined. I can check later, right now I'm feeling lazy.

Before my patch the skb is still passed to the driver, which would need
to be able to handle it (it == session being stopped). I'm not entirely
sure ath9k handles it properly but I suspect it does.

> > Ok that might make some sense, though as a parameter you can easily see
> > where it's coming from, I think :) I agree that the entire code is a
> > little brain twister...
> 
> Yeah.. I had to re-read aggregation code again to get the hang of it.
> Anything we can do to make
> it easier for people to pick would be nice. I think I'll go patch up
> the aggr-tx kdoc too now, unless we
> plan on nuking some stuff soon again. I suspect the next big change
> will be to move software based
> aggregation queue handling and mapping to ACs within mac80211 for that
> type of hardware which
> we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
> and our next generation 11n USB
> driver.

Right -- but I don't expect to be working on it any time soon myself.

> >> > @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
> >> >                    "originating device\n", dev->name);
> >> >  #endif
> >> >             dev_kfree_skb(skb);
> >> > -           return 0;
> >> > +           return NETDEV_TX_OK;
> >>
> >> All these NETDEV_TX_OK could've gone into a separate patch.
> >
> > I guess, they're also not strictly necessary since 0 == TX_OK :)
> 
> Sure, what I'm trying to say is these patches are pretty hard to
> review because of the topic,
> it just helps to trim them down as much as possible -- but I realize
> even that is painful.

:)

Thanks man, I really appreciate you giving them a close look!

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Luis R. Rodriguez]

On Sat, Mar 28, 2009 at 12:52 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sat, 2009-03-28 at 12:18 -0700, Luis R. Rodriguez wrote:
>
>> >> That's fine from a logical point of view as compromise here as ou=
r
>> >> ampdu start should be relatively quick. Now while I can say this
>> >> from a logical point of view this patch obviously needed more
>> >> testing but lets see how it holds up and I'm glad you're the one
>> >> who wrote it. I'm confident there won't be any issues but that
>> >> is something I cannot gaurantee just based on the review. We now
>> >> need to go out and tests this. All other patches previous to this
>> >> make 100% perfect sense.
>> >
>> > :)
>> > I think it also makes more _sense_ to not ask the driver to handle=
 these
>> > frames, because we won't set the AMPDU flag correctly if the sessi=
on
>> > start is declined.
>>
>> Hm, wouldn't we send them as normal frames if its declined here even=
tually?
>
> Yes, we take them off the sta->pending, put them through TX processin=
g
> and send them as normal frames.

OK so you're not suggesting we change this more, just stating that
this makes sense.

>> >> This piece:
>> >>
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else if (*state !=3D HT_=
AGG_STATE_IDLE) {
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 /* in progress */
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 queued =3D true;
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 info->flags |=3D IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 __skb_queue_tail(&tid_tx->pending, skb);
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> >>
>> >> Can probably be ported to stable too, to just TX_DROP.
>> >
>> > No, why? We're handing the frames to the driver. It wants to handl=
e
>> > those frames before agg session is started correctly. That's what =
I was
>> > referring to before with this making more sense now.
>>
>> OK I see that but I am not sure of what's done to the skb in the old
>> case if the session is note complete yet, nor now if the session get=
s
>> declined. I can check later, right now I'm feeling lazy.
>
> Before my patch the skb is still passed to the driver, which would ne=
ed
> to be able to handle it (it =3D=3D session being stopped). I'm not en=
tirely
> sure ath9k handles it properly but I suspect it does.

Not sure, this begs the question when you were seeing the received
addba response come up first. SInce both iwlagn and ath9k use the irq
safe aggregation cb I was suspecting this would happen in general on
UP boxen and probably even less likely to happen on ath9k as we don't
have to talk to the firmware during the ampdu start action.

>> > Ok that might make some sense, though as a parameter you can easil=
y see
>> > where it's coming from, I think :) I agree that the entire code is=
 a
>> > little brain twister...
>>
>> Yeah.. I had to re-read aggregation code again to get the hang of it=
=2E
>> Anything we can do to make
>> it easier for people to pick would be nice. I think I'll go patch up
>> the aggr-tx kdoc too now, unless we
>> plan on nuking some stuff soon again. I suspect the next big change
>> will be to move software based
>> aggregation queue handling and mapping to ACs within mac80211 for th=
at
>> type of hardware which
>> we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
>> and our next generation 11n USB
>> driver.
>
> Right -- but I don't expect to be working on it any time soon myself.

I didn't expect you to. We may have the need first unless chr gets to
ar9170 stuff first.

   Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

On Sat, 2009-03-28 at 13:26 -0700, Luis R. Rodriguez wrote:

> >> > I think it also makes more _sense_ to not ask the driver to handle these
> >> > frames, because we won't set the AMPDU flag correctly if the session
> >> > start is declined.
> >>
> >> Hm, wouldn't we send them as normal frames if its declined here eventually?
> >
> > Yes, we take them off the sta->pending, put them through TX processing
> > and send them as normal frames.
> 
> OK so you're not suggesting we change this more, just stating that
> this makes sense.

Yes; I don't see why we should change it. Otherwise the driver needs to
ignore the TXCTL_AMPDU flag and do its own processing, would would seem
odd. By the way ath9k does it, it probably does it automatically though,
but I don't see why we should require it.

> >> OK I see that but I am not sure of what's done to the skb in the old
> >> case if the session is note complete yet, nor now if the session gets
> >> declined. I can check later, right now I'm feeling lazy.
> >
> > Before my patch the skb is still passed to the driver, which would need
> > to be able to handle it (it == session being stopped). I'm not entirely
> > sure ath9k handles it properly but I suspect it does.
> 
> Not sure, this begs the question when you were seeing the received
> addba response come up first. SInce both iwlagn and ath9k use the irq
> safe aggregation cb I was suspecting this would happen in general on
> UP boxen and probably even less likely to happen on ath9k as we don't
> have to talk to the firmware during the ampdu start action.

No, it can only happen when the remote station is faster to reply than
the driver.

> >> > Ok that might make some sense, though as a parameter you can easily see
> >> > where it's coming from, I think :) I agree that the entire code is a
> >> > little brain twister...
> >>
> >> Yeah.. I had to re-read aggregation code again to get the hang of it.
> >> Anything we can do to make
> >> it easier for people to pick would be nice. I think I'll go patch up
> >> the aggr-tx kdoc too now, unless we
> >> plan on nuking some stuff soon again. I suspect the next big change
> >> will be to move software based
> >> aggregation queue handling and mapping to ACs within mac80211 for that
> >> type of hardware which
> >> we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
> >> and our next generation 11n USB
> >> driver.
> >
> > Right -- but I don't expect to be working on it any time soon myself.
> 
> I didn't expect you to. We may have the need first unless chr gets to
> ar9170 stuff first.

Ok, makes sense.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Luis R. Rodriguez]

On Sat, Mar 28, 2009 at 1:42 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sat, 2009-03-28 at 13:26 -0700, Luis R. Rodriguez wrote:

>> >> OK I see that but I am not sure of what's done to the skb in the old
>> >> case if the session is note complete yet, nor now if the session gets
>> >> declined. I can check later, right now I'm feeling lazy.
>> >
>> > Before my patch the skb is still passed to the driver, which would need
>> > to be able to handle it (it == session being stopped). I'm not entirely
>> > sure ath9k handles it properly but I suspect it does.
>>
>> Not sure, this begs the question when you were seeing the received
>> addba response come up first. SInce both iwlagn and ath9k use the irq
>> safe aggregation cb I was suspecting this would happen in general on
>> UP boxen and probably even less likely to happen on ath9k as we don't
>> have to talk to the firmware during the ampdu start action.
>
> No, it can only happen when the remote station is faster to reply than
> the driver.

My point was that if you have the tasklet handled by a separate CPU
the DRV flag would most likely be set by the time the addba response
comes back. On a UP box though you have no option but to wait for wait
for it to be done after we send off the addba request so then it is a
race between the scheduler to schedule the tasklet and the remote
station's speed + the IRQ processing of the received response.

If we would somehow be able to ensure pending BHs complete before we
process the addba response this wouldn't be needed in both places.

Anyway I'm also stating that its likely that the ampdu start action is
slower with iwlagn as interaction with the firmware is required so I
would suspect one can see this behavior more likely on UP iwlagn
boxen.

Where did you see it?

  Luis

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On Sat, 2009-03-28 at 14:06 -0700, Luis R. Rodriguez wrote:

> My point was that if you have the tasklet handled by a separate CPU
> the DRV flag would most likely be set by the time the addba response
> comes back. On a UP box though you have no option but to wait for wait
> for it to be done after we send off the addba request so then it is a
> race between the scheduler to schedule the tasklet and the remote
> station's speed + the IRQ processing of the received response.

Ah, I see what you mean. But this goes off the same tasklet :) so ath9k
doesn't have a problem in any case.

> If we would somehow be able to ensure pending BHs complete before we
> process the addba response this wouldn't be needed in both places.

No, we can't ensure that -- the driver might very well take longer and
we don't have a way to only process the received frame later.

> Anyway I'm also stating that its likely that the ampdu start action is
> slower with iwlagn as interaction with the firmware is required so I
> would suspect one can see this behavior more likely on UP iwlagn
> boxen.
> 
> Where did you see it?

I didn't actually see it :) But iwlagn can possibly flush the queue
before calling back to the cb.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/8] mac80211: fix aggregation to not require queue stop

[Luis R. Rodriguez]

On Sat, Mar 28, 2009 at 2:17 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sat, 2009-03-28 at 14:06 -0700, Luis R. Rodriguez wrote:
>
>> My point was that if you have the tasklet handled by a separate CPU
>> the DRV flag would most likely be set by the time the addba response
>> comes back. On a UP box though you have no option but to wait for wait
>> for it to be done after we send off the addba request so then it is a
>> race between the scheduler to schedule the tasklet and the remote
>> station's speed + the IRQ processing of the received response.
>
> Ah, I see what you mean. But this goes off the same tasklet :) so ath9k
> doesn't have a problem in any case.

:D

>> If we would somehow be able to ensure pending BHs complete before we
>> process the addba response this wouldn't be needed in both places.
>
> No, we can't ensure that -- the driver might very well take longer and
> we don't have a way to only process the received frame later.

Well as you pointed out we already do since its on the same tasklet --
the delay will happen within the ampdu start action in the driver, not
the irqsafe callback itself.

>> Anyway I'm also stating that its likely that the ampdu start action is
>> slower with iwlagn as interaction with the firmware is required so I
>> would suspect one can see this behavior more likely on UP iwlagn
>> boxen.
>>
>> Where did you see it?
>
> I didn't actually see it :) But iwlagn can possibly flush the queue
> before calling back to the cb.

Heh, flush what queue? A virtual ampdu queue thing? Or something else,
I'm trying to understand exactly where this would happen, I don't
think I get the picture yet.

  Luis

[PATCH 8/8] mac80211/iwlwifi: move virtual A-MDPU queue bookkeeping to iwlwifi

[Johannes Berg]

This patch removes all the virtual A-MPDU-queue bookkeeping from
mac80211. Curiously, iwlwifi already does its own bookkeeping, so
it doesn't require much changes except where it needs to handle
starting and stopping the queues in mac80211.

To handle the queue stop/wake properly, we rewrite the software
queue number for aggregation frames and internally to iwlwifi keep
track of the queues that map into the same AC queue, and only talk
to mac80211 about the AC queue. The implementation requires calling
two new functions, iwl_stop_queue and iwl_wake_queue instead of the
mac80211 counterparts.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Reinette Chattre <reinette.chatre@intel.com>
---
 drivers/net/wireless/iwlwifi/iwl-3945.c     |    2 
 drivers/net/wireless/iwlwifi/iwl-4965.c     |    7 +--
 drivers/net/wireless/iwlwifi/iwl-5000.c     |    7 +--
 drivers/net/wireless/iwlwifi/iwl-core.c     |    3 -
 drivers/net/wireless/iwlwifi/iwl-dev.h      |    6 ++
 drivers/net/wireless/iwlwifi/iwl-helpers.h  |   52 +++++++++++++++++++++++++
 drivers/net/wireless/iwlwifi/iwl-tx.c       |    8 ++-
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    2 
 drivers/net/wireless/mac80211_hwsim.c       |    1 
 include/net/mac80211.h                      |   14 ------
 net/mac80211/agg-tx.c                       |   44 +--------------------
 net/mac80211/ieee80211_i.h                  |    7 ---
 net/mac80211/main.c                         |    9 ----
 net/mac80211/sta_info.c                     |   12 -----
 net/mac80211/sta_info.h                     |    2 
 net/mac80211/tx.c                           |   19 ---------
 net/mac80211/util.c                         |   58 +++-------------------------
 17 files changed, 84 insertions(+), 169 deletions(-)

--- wireless-testing.orig/net/mac80211/ieee80211_i.h	2009-03-23 14:04:30.000000000 +0100
+++ wireless-testing/net/mac80211/ieee80211_i.h	2009-03-23 14:04:33.000000000 +0100
@@ -592,12 +592,7 @@ struct ieee80211_local {
 
 	const struct ieee80211_ops *ops;
 
-	/* AC queue corresponding to each AMPDU queue */
-	s8 ampdu_ac_queue[IEEE80211_MAX_AMPDU_QUEUES];
-	unsigned int amdpu_ac_stop_refcnt[IEEE80211_MAX_AMPDU_QUEUES];
-
-	unsigned long queue_stop_reasons[IEEE80211_MAX_QUEUES +
-					 IEEE80211_MAX_AMPDU_QUEUES];
+	unsigned long queue_stop_reasons[IEEE80211_MAX_QUEUES];
 	/* also used to protect ampdu_ac_queue and amdpu_ac_stop_refcnt */
 	spinlock_t queue_stop_reason_lock;
 
--- wireless-testing.orig/net/mac80211/util.c	2009-03-23 14:03:45.000000000 +0100
+++ wireless-testing/net/mac80211/util.c	2009-03-23 14:04:33.000000000 +0100
@@ -339,29 +339,8 @@ static void __ieee80211_wake_queue(struc
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 
-	if (queue >= hw->queues) {
-		if (local->ampdu_ac_queue[queue - hw->queues] < 0)
-			return;
-
-		/*
-		 * for virtual aggregation queues, we need to refcount the
-		 * internal mac80211 disable (multiple times!), keep track of
-		 * driver disable _and_ make sure the regular queue is
-		 * actually enabled.
-		 */
-		if (reason == IEEE80211_QUEUE_STOP_REASON_AGGREGATION)
-			local->amdpu_ac_stop_refcnt[queue - hw->queues]--;
-		else
-			__clear_bit(reason, &local->queue_stop_reasons[queue]);
-
-		if (local->queue_stop_reasons[queue] ||
-		    local->amdpu_ac_stop_refcnt[queue - hw->queues])
-			return;
-
-		/* now go on to treat the corresponding regular queue */
-		queue = local->ampdu_ac_queue[queue - hw->queues];
-		reason = IEEE80211_QUEUE_STOP_REASON_AGGREGATION;
-	}
+	if (WARN_ON(queue >= hw->queues))
+		return;
 
 	__clear_bit(reason, &local->queue_stop_reasons[queue]);
 
@@ -400,25 +379,8 @@ static void __ieee80211_stop_queue(struc
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 
-	if (queue >= hw->queues) {
-		if (local->ampdu_ac_queue[queue - hw->queues] < 0)
-			return;
-
-		/*
-		 * for virtual aggregation queues, we need to refcount the
-		 * internal mac80211 disable (multiple times!), keep track of
-		 * driver disable _and_ make sure the regular queue is
-		 * actually enabled.
-		 */
-		if (reason == IEEE80211_QUEUE_STOP_REASON_AGGREGATION)
-			local->amdpu_ac_stop_refcnt[queue - hw->queues]++;
-		else
-			__set_bit(reason, &local->queue_stop_reasons[queue]);
-
-		/* now go on to treat the corresponding regular queue */
-		queue = local->ampdu_ac_queue[queue - hw->queues];
-		reason = IEEE80211_QUEUE_STOP_REASON_AGGREGATION;
-	}
+	if (WARN_ON(queue >= hw->queues))
+		return;
 
 	/*
 	 * Only stop if it was previously running, this is necessary
@@ -474,15 +436,9 @@ EXPORT_SYMBOL(ieee80211_stop_queues);
 int ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
-	unsigned long flags;
 
-	if (queue >= hw->queues) {
-		spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
-		queue = local->ampdu_ac_queue[queue - hw->queues];
-		spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
-		if (queue < 0)
-			return true;
-	}
+	if (WARN_ON(queue >= hw->queues))
+		return true;
 
 	return __netif_subqueue_stopped(local->mdev, queue);
 }
@@ -497,7 +453,7 @@ void ieee80211_wake_queues_by_reason(str
 
 	spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
 
-	for (i = 0; i < hw->queues + hw->ampdu_queues; i++)
+	for (i = 0; i < hw->queues; i++)
 		__ieee80211_wake_queue(hw, i, reason);
 
 	spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
--- wireless-testing.orig/net/mac80211/agg-tx.c	2009-03-23 14:04:30.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c	2009-03-23 14:04:33.000000000 +0100
@@ -131,14 +131,6 @@ static int ___ieee80211_stop_tx_ba_sessi
 
 	state = &sta->ampdu_mlme.tid_state_tx[tid];
 
-	if (local->hw.ampdu_queues) {
-		/*
-		 * Pretend the driver woke the queue, just in case
-		 * it disabled it before the session was stopped.
-		 */
-		ieee80211_wake_queue(
-			&local->hw, local->hw.queues + sta->tid_to_tx_q[tid]);
-	}
 	*state = HT_AGG_STATE_REQ_STOP_BA_MSK |
 		(initiator << HT_AGG_STATE_INITIATOR_SHIFT);
 
@@ -206,7 +198,7 @@ int ieee80211_start_tx_ba_session(struct
 	struct sta_info *sta;
 	struct ieee80211_sub_if_data *sdata;
 	u8 *state;
-	int i, qn = -1, ret = 0;
+	int ret = 0;
 	u16 start_seq_num;
 
 	if (WARN_ON(!local->ops->ampdu_action))
@@ -275,29 +267,6 @@ int ieee80211_start_tx_ba_session(struct
 		goto err_unlock_sta;
 	}
 
-	if (hw->ampdu_queues) {
-		spin_lock(&local->queue_stop_reason_lock);
-		/* reserve a new queue for this session */
-		for (i = 0; i < local->hw.ampdu_queues; i++) {
-			if (local->ampdu_ac_queue[i] < 0) {
-				qn = i;
-				local->ampdu_ac_queue[qn] =
-					ieee80211_ac_from_tid(tid);
-				break;
-			}
-		}
-		spin_unlock(&local->queue_stop_reason_lock);
-
-		if (qn < 0) {
-#ifdef CONFIG_MAC80211_HT_DEBUG
-			printk(KERN_DEBUG "BA request denied - "
-			       "queue unavailable for tid %d\n", tid);
-#endif /* CONFIG_MAC80211_HT_DEBUG */
-			ret = -ENOSPC;
-			goto err_unlock_sta;
-		}
-	}
-
 	/*
 	 * While we're asking the driver about the aggregation,
 	 * stop the AC queue so that we don't have to worry
@@ -319,7 +288,7 @@ int ieee80211_start_tx_ba_session(struct
 					tid);
 #endif
 		ret = -ENOMEM;
-		goto err_return_queue;
+		goto err_wake_queue;
 	}
 
 	skb_queue_head_init(&sta->ampdu_mlme.tid_tx[tid]->pending);
@@ -348,7 +317,6 @@ int ieee80211_start_tx_ba_session(struct
 		*state = HT_AGG_STATE_IDLE;
 		goto err_free;
 	}
-	sta->tid_to_tx_q[tid] = qn;
 
 	/* Driver vetoed or OKed, but we can take packets again now */
 	ieee80211_wake_queue_by_reason(
@@ -380,13 +348,7 @@ int ieee80211_start_tx_ba_session(struct
  err_free:
 	kfree(sta->ampdu_mlme.tid_tx[tid]);
 	sta->ampdu_mlme.tid_tx[tid] = NULL;
- err_return_queue:
-	if (qn >= 0) {
-		/* give queue back to pool */
-		spin_lock(&local->queue_stop_reason_lock);
-		local->ampdu_ac_queue[qn] = -1;
-		spin_unlock(&local->queue_stop_reason_lock);
-	}
+ err_wake_queue:
 	ieee80211_wake_queue_by_reason(
 		&local->hw, ieee80211_ac_from_tid(tid),
 		IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
--- wireless-testing.orig/net/mac80211/main.c	2009-03-23 14:04:30.000000000 +0100
+++ wireless-testing/net/mac80211/main.c	2009-03-23 14:04:33.000000000 +0100
@@ -774,11 +774,6 @@ struct ieee80211_hw *ieee80211_alloc_hw(
 	setup_timer(&local->dynamic_ps_timer,
 		    ieee80211_dynamic_ps_timer, (unsigned long) local);
 
-	for (i = 0; i < IEEE80211_MAX_AMPDU_QUEUES; i++)
-		local->ampdu_ac_queue[i] = -1;
-	/* using an s8 won't work with more than that */
-	BUILD_BUG_ON(IEEE80211_MAX_AMPDU_QUEUES > 127);
-
 	sta_info_init(local);
 
 	for (i = 0; i < IEEE80211_MAX_QUEUES; i++)
@@ -874,10 +869,6 @@ int ieee80211_register_hw(struct ieee802
 	 */
 	if (hw->queues > IEEE80211_MAX_QUEUES)
 		hw->queues = IEEE80211_MAX_QUEUES;
-	if (hw->ampdu_queues > IEEE80211_MAX_AMPDU_QUEUES)
-		hw->ampdu_queues = IEEE80211_MAX_AMPDU_QUEUES;
-	if (hw->queues < 4)
-		hw->ampdu_queues = 0;
 
 	mdev = alloc_netdev_mq(sizeof(struct ieee80211_master_priv),
 			       "wmaster%d", ieee80211_master_setup,
--- wireless-testing.orig/include/net/mac80211.h	2009-03-23 14:04:30.000000000 +0100
+++ wireless-testing/include/net/mac80211.h	2009-03-23 14:04:33.000000000 +0100
@@ -93,12 +93,9 @@ struct ieee80211_ht_bss_info {
  * enum ieee80211_max_queues - maximum number of queues
  *
  * @IEEE80211_MAX_QUEUES: Maximum number of regular device queues.
- * @IEEE80211_MAX_AMPDU_QUEUES: Maximum number of queues usable
- *	for A-MPDU operation.
  */
 enum ieee80211_max_queues {
 	IEEE80211_MAX_QUEUES =		4,
-	IEEE80211_MAX_AMPDU_QUEUES =	16,
 };
 
 /**
@@ -947,12 +944,6 @@ enum ieee80211_hw_flags {
  *	data packets. WMM/QoS requires at least four, these
  *	queues need to have configurable access parameters.
  *
- * @ampdu_queues: number of available hardware transmit queues
- *	for A-MPDU packets, these have no access parameters
- *	because they're used only for A-MPDU frames. Note that
- *	mac80211 will not currently use any of the regular queues
- *	for aggregation.
- *
  * @rate_control_algorithm: rate control algorithm for this hardware.
  *	If unset (NULL), the default algorithm will be used. Must be
  *	set before calling ieee80211_register_hw().
@@ -977,7 +968,6 @@ struct ieee80211_hw {
 	int vif_data_size;
 	int sta_data_size;
 	u16 queues;
-	u16 ampdu_queues;
 	u16 max_listen_interval;
 	s8 max_signal;
 	u8 max_rates;
@@ -1347,8 +1337,8 @@ enum ieee80211_ampdu_mlme_action {
  * @get_tx_stats: Get statistics of the current TX queue status. This is used
  *	to get number of currently queued packets (queue length), maximum queue
  *	size (limit), and total number of packets sent using each TX queue
- *	(count). The 'stats' pointer points to an array that has hw->queues +
- *	hw->ampdu_queues items.
+ *	(count). The 'stats' pointer points to an array that has hw->queues
+ *	items.
  *
  * @get_tsf: Get the current TSF timer value from firmware/hardware. Currently,
  *	this is only used for IBSS mode BSSID merging and debugging. Is not a
--- wireless-testing.orig/net/mac80211/sta_info.c	2009-03-23 14:04:30.000000000 +0100
+++ wireless-testing/net/mac80211/sta_info.c	2009-03-23 14:04:33.000000000 +0100
@@ -203,17 +203,6 @@ void sta_info_destroy(struct sta_info *s
 		if (tid_rx)
 			tid_rx->shutdown = true;
 
-		/*
-		 * The stop callback cannot find this station any more, but
-		 * it didn't complete its work -- start the queue if necessary
-		 */
-		if (sta->ampdu_mlme.tid_state_tx[i] & HT_AGG_STATE_INITIATOR_MSK &&
-		    sta->ampdu_mlme.tid_state_tx[i] & HT_AGG_STATE_REQ_STOP_BA_MSK &&
-		    local->hw.ampdu_queues)
-			ieee80211_wake_queue_by_reason(&local->hw,
-				local->hw.queues + sta->tid_to_tx_q[i],
-				IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
-
 		spin_unlock_bh(&sta->lock);
 
 		/*
@@ -292,7 +281,6 @@ struct sta_info *sta_info_alloc(struct i
 		 * enable session_timer's data differentiation. refer to
 		 * sta_rx_agg_session_timer_expired for useage */
 		sta->timer_to_tid[i] = i;
-		sta->tid_to_tx_q[i] = -1;
 		/* rx */
 		sta->ampdu_mlme.tid_state_rx[i] = HT_AGG_STATE_IDLE;
 		sta->ampdu_mlme.tid_rx[i] = NULL;
--- wireless-testing.orig/net/mac80211/sta_info.h	2009-03-23 14:04:30.000000000 +0100
+++ wireless-testing/net/mac80211/sta_info.h	2009-03-23 14:04:33.000000000 +0100
@@ -206,7 +206,6 @@ struct sta_ampdu_mlme {
  * @tid_seq: per-TID sequence numbers for sending to this STA
  * @ampdu_mlme: A-MPDU state machine state
  * @timer_to_tid: identity mapping to ID timers
- * @tid_to_tx_q: map tid to tx queue (invalid == negative values)
  * @llid: Local link ID
  * @plid: Peer link ID
  * @reason: Cancel reason on PLINK_HOLDING state
@@ -281,7 +280,6 @@ struct sta_info {
 	 */
 	struct sta_ampdu_mlme ampdu_mlme;
 	u8 timer_to_tid[STA_TID_NUM];
-	s8 tid_to_tx_q[STA_TID_NUM];
 
 #ifdef CONFIG_MAC80211_MESH
 	/*
--- wireless-testing.orig/net/mac80211/tx.c	2009-03-23 14:04:30.000000000 +0100
+++ wireless-testing/net/mac80211/tx.c	2009-03-23 14:04:33.000000000 +0100
@@ -1145,25 +1145,6 @@ static int __ieee80211_tx(struct ieee802
 			info->flags &= ~(IEEE80211_TX_CTL_CLEAR_PS_FILT |
 					 IEEE80211_TX_CTL_FIRST_FRAGMENT);
 
-		/*
-		 * Internally, we need to have the queue mapping point to
-		 * the real AC queue, not the virtual A-MPDU queue. This
-		 * now finally sets the queue to what the driver wants.
-		 * We will later move this down into the only driver that
-		 * needs it, iwlwifi.
-		 */
-		if (sta && local->hw.ampdu_queues &&
-		    info->flags & IEEE80211_TX_CTL_AMPDU) {
-			unsigned long flags;
-			u8 *qc = ieee80211_get_qos_ctl((void *) skb->data);
-			int tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
-
-			spin_lock_irqsave(&sta->lock, flags);
-			skb_set_queue_mapping(skb, local->hw.queues +
-						   sta->tid_to_tx_q[tid]);
-			spin_unlock_irqrestore(&sta->lock, flags);
-		}
-
 		next = skb->next;
 		len = skb->len;
 		ret = local->ops->tx(local_to_hw(local), skb);
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-core.c	2009-03-23 13:24:03.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-core.c	2009-03-23 14:04:33.000000000 +0100
@@ -1309,9 +1309,6 @@ int iwl_setup_mac(struct iwl_priv *priv)
 
 	/* Default value; 4 EDCA QOS priorities */
 	hw->queues = 4;
-	/* queues to support 11n aggregation */
-	if (priv->cfg->sku & IWL_SKU_N)
-		hw->ampdu_queues = priv->cfg->mod_params->num_of_ampdu_queues;
 
 	hw->conf.beacon_int = 100;
 	hw->max_listen_interval = IWL_CONN_MAX_LISTEN_INTERVAL;
--- wireless-testing.orig/drivers/net/wireless/mac80211_hwsim.c	2009-03-23 13:24:03.000000000 +0100
+++ wireless-testing/drivers/net/wireless/mac80211_hwsim.c	2009-03-23 14:04:33.000000000 +0100
@@ -927,7 +927,6 @@ static int __init init_mac80211_hwsim(vo
 			BIT(NL80211_IFTYPE_STATION) |
 			BIT(NL80211_IFTYPE_AP) |
 			BIT(NL80211_IFTYPE_MESH_POINT);
-		hw->ampdu_queues = 1;
 
 		hw->flags = IEEE80211_HW_MFP_CAPABLE;
 
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-3945.c	2009-03-23 13:24:03.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-3945.c	2009-03-23 14:04:33.000000000 +0100
@@ -293,7 +293,7 @@ static void iwl3945_tx_queue_reclaim(str
 	if (iwl_queue_space(q) > q->low_mark && (txq_id >= 0) &&
 			(txq_id != IWL_CMD_QUEUE_NUM) &&
 			priv->mac80211_registered)
-		ieee80211_wake_queue(priv->hw, txq_id);
+		iwl_wake_queue(priv, txq_id);
 }
 
 /**
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965.c	2009-03-23 13:24:03.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965.c	2009-03-23 14:04:33.000000000 +0100
@@ -2178,10 +2178,9 @@ static void iwl4965_rx_reply_tx(struct i
 			    (iwl_queue_space(&txq->q) > txq->q.low_mark) &&
 			    (agg->state != IWL_EMPTYING_HW_QUEUE_DELBA)) {
 				if (agg->state == IWL_AGG_OFF)
-					ieee80211_wake_queue(priv->hw, txq_id);
+					iwl_wake_queue(priv, txq_id);
 				else
-					ieee80211_wake_queue(priv->hw,
-							     txq->swq_id);
+					iwl_wake_queue(priv, txq->swq_id);
 			}
 		}
 	} else {
@@ -2205,7 +2204,7 @@ static void iwl4965_rx_reply_tx(struct i
 
 		if (priv->mac80211_registered &&
 		    (iwl_queue_space(&txq->q) > txq->q.low_mark))
-			ieee80211_wake_queue(priv->hw, txq_id);
+			iwl_wake_queue(priv, txq_id);
 	}
 
 	if (qc && likely(sta_id != IWL_INVALID_STATION))
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-5000.c	2009-03-23 13:24:02.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-5000.c	2009-03-23 14:04:33.000000000 +0100
@@ -1295,10 +1295,9 @@ static void iwl5000_rx_reply_tx(struct i
 			    (iwl_queue_space(&txq->q) > txq->q.low_mark) &&
 			    (agg->state != IWL_EMPTYING_HW_QUEUE_DELBA)) {
 				if (agg->state == IWL_AGG_OFF)
-					ieee80211_wake_queue(priv->hw, txq_id);
+					iwl_wake_queue(priv, txq_id);
 				else
-					ieee80211_wake_queue(priv->hw,
-							     txq->swq_id);
+					iwl_wake_queue(priv, txq->swq_id);
 			}
 		}
 	} else {
@@ -1324,7 +1323,7 @@ static void iwl5000_rx_reply_tx(struct i
 
 		if (priv->mac80211_registered &&
 		    (iwl_queue_space(&txq->q) > txq->q.low_mark))
-			ieee80211_wake_queue(priv->hw, txq_id);
+			iwl_wake_queue(priv, txq_id);
 	}
 
 	if (ieee80211_is_data_qos(tx_resp->frame_ctrl))
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-dev.h	2009-03-23 13:24:03.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-dev.h	2009-03-23 14:04:33.000000000 +0100
@@ -996,6 +996,12 @@ struct iwl_priv {
 	u8 key_mapping_key;
 	unsigned long ucode_key_table;
 
+	/* queue refcounts */
+#define IWL_MAX_HW_QUEUES	32
+	unsigned long queue_stopped[BITS_TO_LONGS(IWL_MAX_HW_QUEUES)];
+	/* for each AC */
+	atomic_t queue_stop_count[4];
+
 	/* Indication if ieee80211_ops->open has been called */
 	u8 is_open;
 
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-helpers.h	2009-03-23 13:24:02.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-helpers.h	2009-03-23 14:04:33.000000000 +0100
@@ -93,4 +93,56 @@ static inline int iwl_alloc_fw_desc(stru
 	return (desc->v_addr != NULL) ? 0 : -ENOMEM;
 }
 
+/*
+ * we have 8 bits used like this:
+ *
+ * 7 6 5 4 3 2 1 0
+ * | | | | | | | |
+ * | | | | | | +-+-------- AC queue (0-3)
+ * | | | | | |
+ * | +-+-+-+-+------------ HW A-MPDU queue
+ * |
+ * +---------------------- indicates agg queue
+ */
+static inline u8 iwl_virtual_agg_queue_num(u8 ac, u8 hwq)
+{
+	BUG_ON(ac > 3);   /* only have 2 bits */
+	BUG_ON(hwq > 31); /* only have 5 bits */
+
+	return 0x80 | (hwq << 2) | ac;
+}
+
+static inline void iwl_wake_queue(struct iwl_priv *priv, u8 queue)
+{
+	u8 ac = queue;
+	u8 hwq = queue;
+
+	if (queue & 0x80) {
+		ac = queue & 3;
+		hwq = (queue >> 2) & 0x1f;
+	}
+
+	if (test_and_clear_bit(hwq, priv->queue_stopped))
+		if (atomic_dec_return(&priv->queue_stop_count[ac]) <= 0)
+			ieee80211_wake_queue(priv->hw, ac);
+}
+
+static inline void iwl_stop_queue(struct iwl_priv *priv, u8 queue)
+{
+	u8 ac = queue;
+	u8 hwq = queue;
+
+	if (queue & 0x80) {
+		ac = queue & 3;
+		hwq = (queue >> 2) & 0x1f;
+	}
+
+	if (!test_and_set_bit(hwq, priv->queue_stopped))
+		if (atomic_inc_return(&priv->queue_stop_count[ac]) > 0)
+			ieee80211_stop_queue(priv->hw, ac);
+}
+
+#define ieee80211_stop_queue DO_NOT_USE_ieee80211_stop_queue
+#define ieee80211_wake_queue DO_NOT_USE_ieee80211_wake_queue
+
 #endif				/* __iwl_helpers_h__ */
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-tx.c	2009-03-23 13:24:03.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-tx.c	2009-03-23 14:04:33.000000000 +0100
@@ -763,8 +763,10 @@ int iwl_tx_skb(struct iwl_priv *priv, st
 		hdr->seq_ctrl |= cpu_to_le16(seq_number);
 		seq_number += 0x10;
 		/* aggregation is on for this <sta,tid> */
-		if (info->flags & IEEE80211_TX_CTL_AMPDU)
+		if (info->flags & IEEE80211_TX_CTL_AMPDU) {
 			txq_id = priv->stations[sta_id].tid[tid].agg.txq_id;
+			swq_id = iwl_virtual_agg_queue_num(swq_id, txq_id);
+		}
 		priv->stations[sta_id].tid[tid].tfds_in_queue++;
 	}
 
@@ -895,7 +897,7 @@ int iwl_tx_skb(struct iwl_priv *priv, st
 			iwl_txq_update_write_ptr(priv, txq);
 			spin_unlock_irqrestore(&priv->lock, flags);
 		} else {
-			ieee80211_stop_queue(priv->hw, txq->swq_id);
+			iwl_stop_queue(priv, txq->swq_id);
 		}
 	}
 
@@ -1433,7 +1435,7 @@ void iwl_rx_reply_compressed_ba(struct i
 		if ((iwl_queue_space(&txq->q) > txq->q.low_mark) &&
 		    priv->mac80211_registered &&
 		    (agg->state != IWL_EMPTYING_HW_QUEUE_DELBA))
-			ieee80211_wake_queue(priv->hw, txq->swq_id);
+			iwl_wake_queue(priv, txq->swq_id);
 
 		iwl_txq_check_empty(priv, sta_id, tid, scd_flow);
 	}
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl3945-base.c	2009-03-23 13:24:02.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl3945-base.c	2009-03-23 14:04:33.000000000 +0100
@@ -1168,7 +1168,7 @@ static int iwl3945_tx_skb(struct iwl_pri
 			spin_unlock_irqrestore(&priv->lock, flags);
 		}
 
-		ieee80211_stop_queue(priv->hw, skb_get_queue_mapping(skb));
+		iwl_stop_queue(priv, skb_get_queue_mapping(skb));
 	}
 
 	return 0;

-- 

Re: [PATCH 8/8] mac80211/iwlwifi: move virtual A-MDPU queue bookkeeping to iwlwifi

[Luis R. Rodriguez]

On Mon, Mar 23, 2009 at 05:28:42PM +0100, Johannes Berg wrote:
> This patch removes all the virtual A-MPDU-queue bookkeeping from
> mac80211. Curiously, iwlwifi already does its own bookkeeping, so
> it doesn't require much changes except where it needs to handle
> starting and stopping the queues in mac80211.
> 
> To handle the queue stop/wake properly, we rewrite the software
> queue number for aggregation frames and internally to iwlwifi keep
> track of the queues that map into the same AC queue, and only talk
> to mac80211 about the AC queue. The implementation requires calling
> two new functions, iwl_stop_queue and iwl_wake_queue instead of the
> mac80211 counterparts.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> Cc: Reinette Chattre <reinette.chatre@intel.com>

This one I enjoyed the most. Fucking awesome.

Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

  Luis

Re: [PATCH 0/8] mac80211 aggregation improvements

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Mon, 2009-03-23 at 17:28 +0100, Johannes Berg wrote:
   
> Here's my patch set for improving aggregation

John, can I convince you to put this on track for .30 despite being
posted just hours before the merge window officially opened? The reason
I'm asking is that otherwise I'll have to support two code bases with
rather different ways to have aggregation working for iwlwifi again,
with different sets of bugs and different ways to fix it. This patch set
has also been tested fairly extensively already [1] inside Intel for
iwlwifi hardware and by Sujith for ath9k (and myself for both).

The decision isn't mine to make, obviously, and I'll put up with
whatever you decide is best, even if that means supporting both code
bases.

johannes

[1] unlike that patch to remove the superfluous code, sorry about that

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/8] mac80211 aggregation improvements

[Luis R. Rodriguez]

On Tue, Mar 24, 2009 at 1:28 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-03-23 at 17:28 +0100, Johannes Berg wrote:
>
>> Here's my patch set for improving aggregation
>
> John, can I convince you to put this on track for .30 despite being
> posted just hours before the merge window officially opened? The reason
> I'm asking is that otherwise I'll have to support two code bases with
> rather different ways to have aggregation working for iwlwifi again,
> with different sets of bugs and different ways to fix it. This patch set
> has also been tested fairly extensively already [1] inside Intel for
> iwlwifi hardware and by Sujith for ath9k (and myself for both).

It is not accurate to say Sujith has extensively tested this for
ath9k, he probably just ran it a few times. So I cannot say we are as
comfortable with the patches as you or Intel seem to be already.

  Luis

Re: [PATCH 0/8] mac80211 aggregation improvements

[John W. Linville]

On Tue, Mar 24, 2009 at 09:13:21AM -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 24, 2009 at 1:28 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Mon, 2009-03-23 at 17:28 +0100, Johannes Berg wrote:
> >
> >> Here's my patch set for improving aggregation
> >
> > John, can I convince you to put this on track for .30 despite being
> > posted just hours before the merge window officially opened? The reason
> > I'm asking is that otherwise I'll have to support two code bases with
> > rather different ways to have aggregation working for iwlwifi again,
> > with different sets of bugs and different ways to fix it. This patch set
> > has also been tested fairly extensively already [1] inside Intel for
> > iwlwifi hardware and by Sujith for ath9k (and myself for both).
> 
> It is not accurate to say Sujith has extensively tested this for
> ath9k, he probably just ran it a few times. So I cannot say we are as
> comfortable with the patches as you or Intel seem to be already.

Johannes,

Given Luis's objection I'm inclined to delay.  I have asked
him to try to "get comfortable" with this patch series over the next
few days.  If that happens, then I'll try to get this merged during
the window.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 0/8] mac80211 aggregation improvements

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On Tue, 2009-03-24 at 15:48 -0400, John W. Linville wrote:

> > It is not accurate to say Sujith has extensively tested this for
> > ath9k, he probably just ran it a few times. So I cannot say we are as
> > comfortable with the patches as you or Intel seem to be already.

Indeed, both Sujith and I just gave it a few test runs. Given that ath9k
enables aggregation on the first packet always though, the code was
definitely exercised.

> Given Luis's objection I'm inclined to delay.  I have asked
> him to try to "get comfortable" with this patch series over the next
> few days.  If that happens, then I'll try to get this merged during
> the window.

Fair enough.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]