[PATCH 0/8] mac80211/iwlwifi: integrate IEEE802.11n A-MPDU Rx MLME

[PATCH 0/8] mac80211/iwlwifi: integrate IEEE802.11n A-MPDU Rx MLME

[Ron Rindjunsky]

This series of patches contains implementation for the IEEE802.11n 
aggregated MPDUs (A-MPDU) MLME. The series handles Rx A-MPDU (recipient party).

Please notice:
===============
Patch "mac80211: A-MPDU Rx adding BAR handling capability" depends on patch 
"mac80211: pass in PS_POLL frames" that was already sent as a stand alone patch.
Further more, the latter patch is needed for this series to function well.

Notes to this series:
======================
This series of patches splits into:
 - patches 1-7 add 802.11n Rx A-MPDU MLME support to mac80211.
 - patch 8 demonstrates the use of the above mac80211's MLME through iwl4965 low level driver

The patches in the series should not be treated as stand alone patches,
but as a complete MLME according to IEEE802.11n spec, although separation
to patches by subjects was made for ease of handling.

The patches do _not_ break any existing driver.
Patches were made (and tested to work) with wireless branch 2.6.24-rc3 (#everything)

The MLME framework includes the following:
- A-MPDU MLME (for aggregated Rx per STA/TID)
- addBA request support.
- delBA request support (recipient party).
- Block Ack Request (BAR) support.
- A-MPDU reordering.


---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 1/8] mac80211: A-MPDU Rx add low level driver API

[Ron Rindjunsky]

This patch adds the API to perform A-MPDU actions between mac80211 and low
level driver.

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 include/net/mac80211.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 0d67b33..f50c28a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -915,6 +915,18 @@ enum ieee80211_erp_change_flags {
 	IEEE80211_ERP_CHANGE_PREAMBLE	= 1<<1,
 };
 
+/**
+ * enum ieee80211_ampdu_mlme_flags - A-MPDU action flags
+ *
+ * These flags are used with the ampdu_action() callback in
+ * &struct ieee80211_ops to indicate which action is needed.
+ * @IEEE80211_AMPDU_RX_START: start Rx aggregation
+ * @IEEE80211_AMPDU_RX_STOP: stop Rx aggregation
+ */
+enum ieee80211_ampdu_mlme_flags {
+	IEEE80211_AMPDU_RX_START = 1<<0,
+	IEEE80211_AMPDU_RX_STOP  = 1<<1,
+};
 
 /**
  * struct ieee80211_ops - callbacks from mac80211 to the driver
@@ -1043,6 +1055,12 @@ enum ieee80211_erp_change_flags {
  *	used to determine whether to reply to Probe Requests.
  *
  * @conf_ht: Configures low level driver with 802.11n HT data. Must be atomic.
+ *
+ * @ampdu_action: Ask low-level driver to perform a certain A-MPDU action
+ * 	The RA/TID combination determines the destination and TID we want
+ * 	the ampdu action to be perfoemed for. The action is defined through
+ * 	ieee80211_ampdu_mlme_flags. starting sequence number (ssn)
+ * 	is the first frame we expect to perform the action on.
  */
 struct ieee80211_ops {
 	int (*tx)(struct ieee80211_hw *hw, struct sk_buff *skb,
@@ -1089,6 +1107,8 @@ struct ieee80211_ops {
 			     struct ieee80211_tx_control *control);
 	int (*tx_last_beacon)(struct ieee80211_hw *hw);
 	int (*conf_ht)(struct ieee80211_hw *hw, struct ieee80211_conf *conf);
+	int (*ampdu_action)(struct ieee80211_hw *hw, u8 action, const u8 *ra,
+				u16 tid, u16 ssn);
 };
 
 /**
-- 
1.5.3.3

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/8] mac80211: A-MPDU Rx add low level driver API

[Johannes Berg]

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

Hi,

Cool stuff. Minor (really) comments below.

> +/**
> + * enum ieee80211_ampdu_mlme_flags - A-MPDU action flags
> + *
> + * These flags are used with the ampdu_action() callback in
> + * &struct ieee80211_ops to indicate which action is needed.
> + * @IEEE80211_AMPDU_RX_START: start Rx aggregation
> + * @IEEE80211_AMPDU_RX_STOP: stop Rx aggregation
> + */
> +enum ieee80211_ampdu_mlme_flags {
> +	IEEE80211_AMPDU_RX_START = 1<<0,
> +	IEEE80211_AMPDU_RX_STOP  = 1<<1,
> +};

Can both really happen at the same time? Or why are they flags rather
than just an action code?
 
> + * @ampdu_action: Ask low-level driver to perform a certain A-MPDU action

I think we usually just say "Perform a certain A-MPDU action" because
these comments are more for driver writers than mac80211 people and they
like to be told what to do ;)

> + * 	The RA/TID combination determines the destination and TID we want
> + * 	the ampdu action to be perfoemed for. The action is defined through

small typo: "performed"

> + * 	ieee80211_ampdu_mlme_flags. starting sequence number (ssn)

small typo: "Starting"

@ssn will highlight the variable name afaik. I need to get my docbook
stuff up to be mergeable.

> + * 	is the first frame we expect to perform the action on.
>   */
>  struct ieee80211_ops {
>  	int (*tx)(struct ieee80211_hw *hw, struct sk_buff *skb,
> @@ -1089,6 +1107,8 @@ struct ieee80211_ops {
>  			     struct ieee80211_tx_control *control);
>  	int (*tx_last_beacon)(struct ieee80211_hw *hw);
>  	int (*conf_ht)(struct ieee80211_hw *hw, struct ieee80211_conf *conf);
> +	int (*ampdu_action)(struct ieee80211_hw *hw, u8 action, const u8 *ra,
> +				u16 tid, u16 ssn);

I personally prefer indentation to right after the opening parenthesis
and that's the style over the whole struct.

johannes

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

Re: [PATCH 1/8] mac80211: A-MPDU Rx add low level driver API

[Johannes Berg]

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


> > +enum ieee80211_ampdu_mlme_flags {
> > +	IEEE80211_AMPDU_RX_START = 1<<0,
> > +	IEEE80211_AMPDU_RX_STOP  = 1<<1,
> > +};
> 
> Can both really happen at the same time? Or why are they flags rather
> than just an action code?

Ok, looking at the rest of the code, they do not happen together. But
thanks for splitting up the patches anyway, it does make things more
readable. Please just remove the numbers completely, rename it to enum
ieee80211_apmdu_mlme_action and use "enum ieee80211_ampdu_mlme_action
action" instead of "u8 action" in the apmdu_action callback.
 
johannes

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

Re: [PATCH 1/8] mac80211: A-MPDU Rx add low level driver API

[Ron Rindjunsky]

> > +enum ieee80211_ampdu_mlme_flags {
> > +     IEEE80211_AMPDU_RX_START = 1<<0,
> > +     IEEE80211_AMPDU_RX_STOP  = 1<<1,
> > +};
>
> Can both really happen at the same time? Or why are they flags rather
> than just an action code?
>
i agree, it is confusing. i will change this enum to what you suggested.

thanks for typos and comments catches, i will fix them.

>
> @ssn will highlight the variable name afaik. I need to get my docbook
> stuff up to be mergeable.
>

didn't get this one. i didn't do @ssn as no other comment in
ieee80211_ops does it, it was just my extra info for drivers writers.

Re: [PATCH 1/8] mac80211: A-MPDU Rx add low level driver API

[Johannes Berg]

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


> > @ssn will highlight the variable name afaik. I need to get my docbook
> > stuff up to be mergeable.
> >
> 
> didn't get this one. i didn't do @ssn as no other comment in
> ieee80211_ops does it, it was just my extra info for drivers writers.

Sorry, that really was a bit confusing. What I meant was that I think
you should write

+ * @ampdu_action: Ask low-level driver to perform a certain A-MPDU action
+ *     The RA/TID combination determines the destination and TID we want
+ *     the ampdu action to be perfoemed for. The action is defined through
+ *     ieee80211_ampdu_mlme_flags. starting sequence number (@ssn)
                                                              ^^^^
+ *     is the first frame we expect to perform the action on.

so that the "ssn" text will be printed in bold in the generated
documentation.

johannes

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

[PATCH 2/8] mac80211: A-MPDU Rx add MLME structures

[Ron Rindjunsky]

This patch adds the needed structures to describe the Rx aggregation MLME per STA
new:
 - struct tid_ampdu_rx: TID aggregation information (Rx)
 - struct sta_ampdu_mlme: MLME aggregation information for STA
changed:
 - struct sta_info: ampdu_mlme added to describe A-MPDU MLME per STA,
		    and timer_to_tid added to map timer id into TID

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 net/mac80211/sta_info.h |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index e1a4ac1..451b761 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -31,6 +31,51 @@
 #define WLAN_STA_WME BIT(9)
 #define WLAN_STA_WDS BIT(27)
 
+#define STA_TID_NUM 16
+#define ADDBA_RESP_INTERVAL HZ
+
+#define HT_AGG_STATE_INITIATOR_SHIFT	(4)
+
+#define HT_AGG_STATE_REQ_STOP_BA_MSK	BIT(3)
+
+#define HT_AGG_STATE_IDLE		(0x0)
+#define HT_AGG_STATE_OPERATIONAL	(0x7)
+
+/**
+ * struct tid_ampdu_rx - TID aggregation information (Rx).
+ *
+ * @state: TID's state in session state machine.
+ * @dialog_token: dialog token for aggregation session
+ * @ssn: Starting Sequence Number expected to be aggregated.
+ * @buf_size: buffer size for incoming A-MPDUs
+ * @timeout: reset timer value.
+ * @head_seq_num: head sequence number in reordering buffer.
+ * @stored_mpdu_num: number of MPDUs in reordering buffer
+ * @reorder_buf: buffer to reorder incoming aggregated MPDUs
+ * @session_timer: check if peer keeps Tx-ing on the TID (by timeout value)
+ */
+struct tid_ampdu_rx {
+	u8 state;
+	u8 dialog_token;
+	u16 ssn;
+	u16 buf_size;
+	u16 timeout;
+	u16 head_seq_num;
+	u16 stored_mpdu_num;
+	struct sk_buff **reorder_buf;
+	struct timer_list session_timer;
+};
+
+/**
+ * struct sta_ampdu_mlme - STA aggregation information.
+ *
+ * @tid_agg_info_rx: aggregation info for Rx per TID
+ * @ampdu_rx: for locking sections in aggregation Rx flow
+ */
+struct sta_ampdu_mlme {
+	struct tid_ampdu_rx tid_rx[STA_TID_NUM];
+	spinlock_t ampdu_rx;
+};
 
 struct sta_info {
 	struct kref kref;
@@ -101,6 +146,8 @@ struct sta_info {
 
 	struct ieee80211_ht_info ht_info; /* 802.11n HT capabilities
 					     of this STA */
+	struct sta_ampdu_mlme ampdu_mlme;
+	int timer_to_tid[STA_TID_NUM];  /* convert timer id to tid */
 
 #ifdef CONFIG_MAC80211_DEBUGFS
 	struct sta_info_debugfsdentries {
-- 
1.5.3.3

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 2/8] mac80211: A-MPDU Rx add MLME structures

[Johannes Berg]

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


> +	struct sta_ampdu_mlme ampdu_mlme;
> +	int timer_to_tid[STA_TID_NUM];  /* convert timer id to tid */

Since you only use that as an identity mapping it can well be a u8
array. Not that I like the trick used, but I don't see any good way to
avoid it either.

johannes

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

Re: [PATCH 2/8] mac80211: A-MPDU Rx add MLME structures

[Ron Rindjunsky]

>
> > +     struct sta_ampdu_mlme ampdu_mlme;
> > +     int timer_to_tid[STA_TID_NUM];  /* convert timer id to tid */
>
> Since you only use that as an identity mapping it can well be a u8
> array. Not that I like the trick used, but I don't see any good way to
> avoid it either.
>

yes... that was quite an irritating issue, had to use this kind of
mechanism here.
i will check changing type to u8 (need to exmine if it will cause any
addresses problems)

Re: [PATCH 2/8] mac80211: A-MPDU Rx add MLME structures

[Johannes Berg]

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


> yes... that was quite an irritating issue, had to use this kind of
> mechanism here.
> i will check changing type to u8 (need to exmine if it will cause any
> addresses problems)

Hmm, you may be right, some things internally use the lowest two bits of
an address. Not sure if timers do though. It'd still be interesting
since we can potentially have many sta_info structures.

johannes

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

Re: [PATCH 2/8] mac80211: A-MPDU Rx add MLME structures

[Johannes Berg]

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


On Tue, 2007-12-18 at 15:41 +0200, Ron Rindjunsky wrote:
> >
> > > +     struct sta_ampdu_mlme ampdu_mlme;
> > > +     int timer_to_tid[STA_TID_NUM];  /* convert timer id to tid */
> >
> > Since you only use that as an identity mapping it can well be a u8
> > array. Not that I like the trick used, but I don't see any good way to
> > avoid it either.
> >
> 
> yes... that was quite an irritating issue, had to use this kind of
> mechanism here.
> i will check changing type to u8 (need to exmine if it will cause any
> addresses problems)

I just checked, it's an unsigned long and not an address so you can
really pass anything to it. Therefore, I think using a u8 array would be
appropriate.

johannes

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

RE: [PATCH 2/8] mac80211: A-MPDU Rx add MLME structures

[Rindjunsky, Ron]

> > > +     struct sta_ampdu_mlme ampdu_mlme;
> > > +     int timer_to_tid[STA_TID_NUM];  /* convert timer id to tid
*/
> >
> > Since you only use that as an identity mapping it can well be a u8
> > array. Not that I like the trick used, but I don't see any good way
to
> > avoid it either.
> >
> 
> yes... that was quite an irritating issue, had to use this kind of
> mechanism here.
> i will check changing type to u8 (need to exmine if it will cause any
> addresses problems)

> I just checked, it's an unsigned long and not an address so you can
> really pass anything to it. Therefore, I think using a u8 array would
be
> appropriate.

Yes, I also checked and it seems ok.

> johannes
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 3/8] mac80211: A-MPDU Rx adding basic functionality

[Ron Rindjunsky]

This patch adds the basic needed abilities and functions for A-MPDU Rx session
changed functions:
 - ieee80211_sta_process_addba_request - Rx A-MPDU initialization enabled
added functions:
 - ieee80211_send_delba - used for sending out Del BA in A-MPDU sessions
 - ieee80211_sta_stop_rx_BA_session - stopping Rx A-MPDU session
 - sta_rx_agg_session_timer_expired - stops A-MPDU Rx use if load is too low

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 include/linux/ieee80211.h    |    7 ++
 net/mac80211/ieee80211_i.h   |    3 +
 net/mac80211/ieee80211_sta.c |  223 ++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 223 insertions(+), 10 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 3e64159..c0edd5a 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -472,6 +472,13 @@ enum ieee80211_back_actioncode {
 	WLAN_ACTION_DELBA = 2,
 };
 
+/* BACK parties */
+enum ieee80211_back_parties {
+	WLAN_BACK_RECIPIENT = 0,
+	WLAN_BACK_INITIATOR = 1,
+	WLAN_BACK_TIMER = 2,
+};
+
 /* A-MSDU 802.11n */
 #define IEEE80211_QOS_CONTROL_A_MSDU_PRESENT 0x0080
 
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index b54ed5f..cf2256b 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -770,6 +770,9 @@ int ieee80211_ht_cap_ie_to_ht_info(struct ieee80211_ht_cap *ht_cap_ie,
 int ieee80211_ht_addt_info_ie_to_ht_bss_info(
 			struct ieee80211_ht_addt_info *ht_add_info_ie,
 			struct ieee80211_ht_bss_info *bss_info);
+void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *da,
+				u16 tid, u16 initiator, u16 reason);
+void sta_rx_agg_session_timer_expired(unsigned long data);
 /* ieee80211_iface.c */
 int ieee80211_if_add(struct net_device *dev, const char *name,
 		     struct net_device **new_dev, int type);
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 2e0f861..ca98d06 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -64,6 +64,9 @@
 #define IEEE80211_ADDBA_PARAM_TID_MASK 0x003C
 #define IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK 0xFFA0
 
+#define IEEE80211_MIN_AMPDU_BUF 0x8
+#define IEEE80211_MAX_AMPDU_BUF 0x40
+
 static void ieee80211_send_probe_req(struct net_device *dev, u8 *dst,
 				     u8 *ssid, size_t ssid_len);
 static struct ieee80211_sta_bss *
@@ -1051,9 +1054,14 @@ static void ieee80211_sta_process_addba_request(struct net_device *dev,
 						size_t len)
 {
 	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+	struct ieee80211_hw *hw = &local->hw;
+	struct ieee80211_conf *conf = &hw->conf;
 	struct sta_info *sta;
-	u16 capab, tid, timeout, ba_policy, buf_size, status;
+	struct tid_ampdu_rx *tid_agg_rx;
+	u16 capab, tid, timeout, ba_policy, buf_size, start_seq_num, status;
 	u8 dialog_token;
+	int ret = -EOPNOTSUPP;
+	DECLARE_MAC_BUF(mac);
 
 	sta = sta_info_get(local, mgmt->sa);
 	if (!sta)
@@ -1062,28 +1070,223 @@ static void ieee80211_sta_process_addba_request(struct net_device *dev,
 	/* extract session parameters from addba request frame */
 	dialog_token = mgmt->u.action.u.addba_req.dialog_token;
 	timeout = le16_to_cpu(mgmt->u.action.u.addba_req.timeout);
+	start_seq_num =
+		le16_to_cpu(mgmt->u.action.u.addba_req.start_seq_num) >> 4;
 
 	capab = le16_to_cpu(mgmt->u.action.u.addba_req.capab);
 	ba_policy = (capab & IEEE80211_ADDBA_PARAM_POLICY_MASK) >> 1;
 	tid = (capab & IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
 	buf_size = (capab & IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK) >> 6;
 
-	/* TODO - currently aggregation is declined (A-MPDU add BA request
-	* acceptance is not obligatory by 802.11n draft), but here is
-	* the entry point for dealing with it */
-#ifdef MAC80211_HT_DEBUG
-	if (net_ratelimit())
-		printk(KERN_DEBUG "Add Block Ack request arrived,"
-				   " currently denying it\n");
-#endif /* MAC80211_HT_DEBUG */
-
 	status = WLAN_STATUS_REQUEST_DECLINED;
 
+
+	/* sanity check for incoming parameters:
+	 * check if configuration can support the BA policy
+	 * and if buffer size does not exceeds max value */
+	if (((ba_policy != 1)
+		&& (!(conf->ht_conf.cap & IEEE80211_HT_CAP_DELAY_BA)))
+		|| (buf_size > IEEE80211_MAX_AMPDU_BUF)) {
+		status = WLAN_STATUS_INVALID_QOS_PARAM;
+#ifdef CONFIG_MAC80211_HT_DEBUG
+		if (net_ratelimit())
+			printk(KERN_DEBUG "Block Ack Req with bad params from "
+				"%s on tid %u. policy %d, buffer size %d\n",
+				print_mac(mac, mgmt->sa), tid, ba_policy,
+				buf_size);
+#endif /* CONFIG_MAC80211_HT_DEBUG */
+		goto end_no_lock;
+	}
+	/* determine default buffer size */
+	if (buf_size == 0) {
+		struct ieee80211_hw_mode *mode = conf->mode;
+		buf_size = IEEE80211_MIN_AMPDU_BUF;
+		buf_size = buf_size << mode->ht_info.ampdu_factor;
+	}
+
+	tid_agg_rx = &sta->ampdu_mlme.tid_rx[tid];
+
+	/* examine state machine */
+	spin_lock_bh(&sta->ampdu_mlme.ampdu_rx);
+
+	if (tid_agg_rx->state != HT_AGG_STATE_IDLE) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+		if (net_ratelimit())
+			printk(KERN_DEBUG "unexpected Block Ack Req from "
+				"%s on tid %u\n",
+				print_mac(mac, mgmt->sa), tid);
+#endif /* CONFIG_MAC80211_HT_DEBUG */
+		goto end;
+	}
+
+	/* prepare reordering buffer */
+	tid_agg_rx->reorder_buf =
+		kmalloc(buf_size * sizeof(struct sk_buf *), GFP_ATOMIC);
+	if (!tid_agg_rx->reorder_buf) {
+		printk(KERN_ERR "can not allocate reordering buffer "
+						"to tid %d\n", tid);
+		tid_agg_rx->state = HT_AGG_STATE_IDLE;
+		goto end;
+	}
+	memset(tid_agg_rx->reorder_buf, 0,
+		buf_size * sizeof(struct sk_buf *));
+
+	if (local->ops->ampdu_action)
+		ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_RX_START,
+					       sta->addr, tid, start_seq_num);
+	if (ret) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+		if (net_ratelimit())
+			printk(KERN_DEBUG "A-MPDU on tid %d result: %s", tid,
+			(ret == -EOPNOTSUPP)? "Not Supported": "Driver Error");
+#endif /* CONFIG_MAC80211_HT_DEBUG */
+		kfree(tid_agg_rx->reorder_buf);
+		goto end;
+	}
+
+	/* change state and send addba resp */
+	tid_agg_rx->state = HT_AGG_STATE_OPERATIONAL;
+	tid_agg_rx->dialog_token = dialog_token;
+	tid_agg_rx->ssn = start_seq_num;
+	tid_agg_rx->head_seq_num = start_seq_num;
+	tid_agg_rx->buf_size = buf_size;
+	tid_agg_rx->timeout = timeout;
+	tid_agg_rx->stored_mpdu_num = 0;
+	status = WLAN_STATUS_SUCCESS;
+end:
+	spin_unlock_bh(&sta->ampdu_mlme.ampdu_rx);
+
+end_no_lock:
 	ieee80211_send_addba_resp(sta->dev, sta->addr, tid, dialog_token,
 				status, 1, buf_size, timeout);
 	sta_info_put(sta);
 }
 
+void ieee80211_send_delba(struct net_device *dev, const u8 *da, u16 tid,
+				u16 initiator, u16 reason_code)
+{
+	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct ieee80211_if_sta *ifsta = &sdata->u.sta;
+	struct sk_buff *skb;
+	struct ieee80211_mgmt *mgmt;
+	u16 params;
+
+	skb = dev_alloc_skb(sizeof(*mgmt) + local->hw.extra_tx_headroom + 1 +
+					sizeof(mgmt->u.action.u.delba));
+
+	if (!skb) {
+		printk(KERN_ERR "%s: failed to allocate buffer "
+					"for delba frame\n", dev->name);
+		return;
+	}
+
+	skb_reserve(skb, local->hw.extra_tx_headroom);
+	mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
+	memset(mgmt, 0, 24);
+	memcpy(mgmt->da, da, ETH_ALEN);
+	memcpy(mgmt->sa, dev->dev_addr, ETH_ALEN);
+	if (sdata->type == IEEE80211_IF_TYPE_AP)
+		memcpy(mgmt->bssid, dev->dev_addr, ETH_ALEN);
+	else
+		memcpy(mgmt->bssid, ifsta->bssid, ETH_ALEN);
+	mgmt->frame_control = IEEE80211_FC(IEEE80211_FTYPE_MGMT,
+					IEEE80211_STYPE_ACTION);
+
+	skb_put(skb, 1 + sizeof(mgmt->u.action.u.delba));
+
+	mgmt->u.action.category = WLAN_CATEGORY_BACK;
+	mgmt->u.action.u.delba.action_code = WLAN_ACTION_DELBA;
+	params = (u16)(initiator << 11); 	/* bit 11 initiator */
+	params |= (u16)(tid << 12); 		/* bit 15:12 TID number */
+
+	mgmt->u.action.u.delba.params = cpu_to_le16(params);
+	mgmt->u.action.u.delba.reason_code = cpu_to_le16(reason_code);
+
+	ieee80211_sta_tx(dev, skb, 0);
+}
+
+void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
+					u16 initiator, u16 reason)
+{
+	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+	struct ieee80211_hw *hw = &local->hw;
+	struct sta_info *sta;
+
+	sta = sta_info_get(local, ra);
+	if (!sta)
+		return;
+
+	/* check if the TID is in opertional state */
+	spin_lock_bh(&sta->ampdu_mlme.ampdu_rx);
+	if (sta->ampdu_mlme.tid_rx[tid].state
+				!= HT_AGG_STATE_OPERATIONAL) {
+		spin_unlock_bh(&sta->ampdu_mlme.ampdu_rx);
+		if (net_ratelimit())
+			printk(KERN_DEBUG "rx BA session requested to stop on "
+				"unactive tid %d\n", tid);
+		sta_info_put(sta);
+		return;
+	}
+	sta->ampdu_mlme.tid_rx[tid].state =
+		HT_AGG_STATE_REQ_STOP_BA_MSK |
+		(initiator << HT_AGG_STATE_INITIATOR_SHIFT);
+		spin_unlock_bh(&sta->ampdu_mlme.ampdu_rx);
+
+	/* stop HW Rx aggregation */
+	if (local->ops->ampdu_action) {
+		int ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_RX_STOP,
+						   ra, tid, EINVAL);
+		if (ret)
+			printk(KERN_DEBUG "HW problem - can not stop rx "
+				"aggergation for tid %d\n", tid);
+	}
+
+	/* shutdown timer has not expired */
+	if (initiator != WLAN_BACK_TIMER)
+		del_timer_sync(&sta->ampdu_mlme.tid_rx[tid].
+					session_timer);
+
+	/* check if this is a self generated aggregation halt */
+	if (initiator == WLAN_BACK_RECIPIENT || initiator == WLAN_BACK_TIMER)
+		ieee80211_send_delba(dev, ra, tid, 0, reason);
+
+	/* free the reordering buffer */
+	kfree(sta->ampdu_mlme.tid_rx[tid].reorder_buf);
+
+	sta->ampdu_mlme.tid_rx[tid].state = HT_AGG_STATE_IDLE;
+	sta_info_put(sta);
+}
+
+/*
+ * After receiving Block Ack Request (BAR) we activated a
+ * timer after each frame arrives from the originator.
+ * if this timer expires ieee80211_sta_stop_rx_BA_session will be executed.
+ */
+void sta_rx_agg_session_timer_expired(unsigned long data)
+{
+	/* not an elegant detour, but there is no choice as the timer passes
+	* only one argument, and ieee80211_local is needed here */
+	int *ptid = (int *)data;
+	int *timer_to_id = ptid - *ptid;
+	struct sta_info *temp_sta = container_of(timer_to_id, struct sta_info,
+					 timer_to_tid[0]);
+	struct ieee80211_local *local = temp_sta->local;
+	struct sta_info *sta;
+	u16 tid = (u16)*ptid;
+	sta = sta_info_get(local, temp_sta->addr);
+
+	if (!sta)
+		return;
+
+	printk(KERN_DEBUG "rx session timer expired on tid %d\n", tid);
+	ieee80211_sta_stop_rx_BA_session(sta->dev, sta->addr, tid,
+					 WLAN_BACK_TIMER,
+					 WLAN_REASON_QSTA_TIMEOUT);
+	sta_info_put(sta);
+}
+
+
 static void ieee80211_rx_mgmt_auth(struct net_device *dev,
 				   struct ieee80211_if_sta *ifsta,
 				   struct ieee80211_mgmt *mgmt,
-- 
1.5.3.3

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 3/8] mac80211: A-MPDU Rx adding basic functionality

[Johannes Berg]

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


> +/* BACK parties */

Please write out "block-ack" or "BACK (block-ack)" :)
  
> +#define IEEE80211_MIN_AMPDU_BUF 0x8
> +#define IEEE80211_MAX_AMPDU_BUF 0x40

Any comments on what these represent? Should they be tunable?

> +	if (tid_agg_rx->state != HT_AGG_STATE_IDLE) {
> +#ifdef CONFIG_MAC80211_HT_DEBUG
> +		if (net_ratelimit())
> +			printk(KERN_DEBUG "unexpected Block Ack Req from "
> +				"%s on tid %u\n",
> +				print_mac(mac, mgmt->sa), tid);
> +#endif /* CONFIG_MAC80211_HT_DEBUG */
> +		goto end;
> +	}
> +
> +	/* prepare reordering buffer */
> +	tid_agg_rx->reorder_buf =
> +		kmalloc(buf_size * sizeof(struct sk_buf *), GFP_ATOMIC);
> +	if (!tid_agg_rx->reorder_buf) {
> +		printk(KERN_ERR "can not allocate reordering buffer "
> +						"to tid %d\n", tid);
> +		tid_agg_rx->state = HT_AGG_STATE_IDLE;

That's unnecessary, you can't get here w/ state != IDLE afaict.

> +		goto end;
> +	}
> +	memset(tid_agg_rx->reorder_buf, 0,
> +		buf_size * sizeof(struct sk_buf *));
> +
> +	if (local->ops->ampdu_action)
> +		ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_RX_START,
> +					       sta->addr, tid, start_seq_num);
> +	if (ret) {
> +#ifdef CONFIG_MAC80211_HT_DEBUG
> +		if (net_ratelimit())
> +			printk(KERN_DEBUG "A-MPDU on tid %d result: %s", tid,
> +			(ret == -EOPNOTSUPP)? "Not Supported": "Driver Error");

I'm not sure we need to ratelimit debug messages, and I think we should
print out the error code. People who are writing a driver will need to
decipher the number but it's an error they return so I think we should
give them a chance to see which error path was hit.

> +void ieee80211_send_delba(struct net_device *dev, const u8 *da, u16 tid,
> +				u16 initiator, u16 reason_code)
> +{
> +	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +	struct ieee80211_if_sta *ifsta = &sdata->u.sta;
> +	struct sk_buff *skb;
> +	struct ieee80211_mgmt *mgmt;
> +	u16 params;
> +
> +	skb = dev_alloc_skb(sizeof(*mgmt) + local->hw.extra_tx_headroom + 1 +
> +					sizeof(mgmt->u.action.u.delba));
> +
> +	if (!skb) {
> +		printk(KERN_ERR "%s: failed to allocate buffer "
> +					"for delba frame\n", dev->name);

Do we send this as a reply to anything, ie. should it be rate-limited
too?

> +void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
> +					u16 initiator, u16 reason)
> +{

> +	/* check if the TID is in opertional state */

small typo

> +	/* stop HW Rx aggregation */
> +	if (local->ops->ampdu_action) {

Can we get into that path with ops->ampdu_action == NULL? I'd think not
because state is required to be active... I'd rather stick in a
	BUG_ON(!local->ops->ampdu_action)
because it seems to me that'd be a mac80211 bug.

> + * After receiving Block Ack Request (BAR) we activated a
> + * timer after each frame arrives from the originator.
> + * if this timer expires ieee80211_sta_stop_rx_BA_session will be executed.
> + */
> +void sta_rx_agg_session_timer_expired(unsigned long data)
> +{
> +	/* not an elegant detour, but there is no choice as the timer passes
> +	* only one argument, and ieee80211_local is needed here */
> +	int *ptid = (int *)data;
> +	int *timer_to_id = ptid - *ptid;
> +	struct sta_info *temp_sta = container_of(timer_to_id, struct sta_info,
> +					 timer_to_tid[0]);

I think this needs more comments. I can, after a while, see what it
does, but I'm not even sure it's correct. The whole timer_to_id thing is
only for this code?

> +	struct ieee80211_local *local = temp_sta->local;
> +	struct sta_info *sta;
> +	u16 tid = (u16)*ptid;
> +	sta = sta_info_get(local, temp_sta->addr);

Missing newline.

> +	if (!sta)
> +		return;

Why do you even do a sta_info_get() on the temp_sta's addr? Either you
already have a good pointer or the whole thing will access invalid
memory.

johannes

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

Re: [PATCH 3/8] mac80211: A-MPDU Rx adding basic functionality

[Ron Rindjunsky]

> > +/* BACK parties */
>
> Please write out "block-ack" or "BACK (block-ack)" :)

will do

>
> > +#define IEEE80211_MIN_AMPDU_BUF 0x8
> > +#define IEEE80211_MAX_AMPDU_BUF 0x40
>
> Any comments on what these represent? Should they be tunable?
>

no, they are not tuneable, but were determined in the 802.11n spec. I
will add comments here.

> > +     if (tid_agg_rx->state != HT_AGG_STATE_IDLE) {
> > +#ifdef CONFIG_MAC80211_HT_DEBUG
> > +             if (net_ratelimit())
> > +                     printk(KERN_DEBUG "unexpected Block Ack Req from "
> > +                             "%s on tid %u\n",
> > +                             print_mac(mac, mgmt->sa), tid);
> > +#endif /* CONFIG_MAC80211_HT_DEBUG */
> > +             goto end;
> > +     }
> > +
> > +     /* prepare reordering buffer */
> > +     tid_agg_rx->reorder_buf =
> > +             kmalloc(buf_size * sizeof(struct sk_buf *), GFP_ATOMIC);
> > +     if (!tid_agg_rx->reorder_buf) {
> > +             printk(KERN_ERR "can not allocate reordering buffer "
> > +                                             "to tid %d\n", tid);
> > +             tid_agg_rx->state = HT_AGG_STATE_IDLE;
>
> That's unnecessary, you can't get here w/ state != IDLE afaict.

right, thanks.

> > +#ifdef CONFIG_MAC80211_HT_DEBUG
> > +             if (net_ratelimit())
> > +                     printk(KERN_DEBUG "A-MPDU on tid %d result: %s", tid,
> > +                     (ret == -EOPNOTSUPP)? "Not Supported": "Driver Error");
>
> I'm not sure we need to ratelimit debug messages, and I think we should
> print out the error code. People who are writing a driver will need to
> decipher the number but it's an error they return so I think we should
> give them a chance to see which error path was hit.

agree, will change.

>
> > +void ieee80211_send_delba(struct net_device *dev, const u8 *da, u16 tid,
> > +                             u16 initiator, u16 reason_code)
> > +     if (!skb) {
> > +             printk(KERN_ERR "%s: failed to allocate buffer "
> > +                                     "for delba frame\n", dev->name);
>
> Do we send this as a reply to anything, ie. should it be rate-limited
> too?

no, this Tx is due to internal events only, so it will occur at most
once per session.

>
> > +     /* check if the TID is in opertional state */
>
> small typo

thanks, will change

>
> > +     /* stop HW Rx aggregation */
> > +     if (local->ops->ampdu_action) {
>
> Can we get into that path with ops->ampdu_action == NULL? I'd think not
> because state is required to be active... I'd rather stick in a
>        BUG_ON(!local->ops->ampdu_action)
> because it seems to me that'd be a mac80211 bug.

i agree, will do.

>
> > + * After receiving Block Ack Request (BAR) we activated a
> > + * timer after each frame arrives from the originator.
> > + * if this timer expires ieee80211_sta_stop_rx_BA_session will be executed.
> > + */
> > +void sta_rx_agg_session_timer_expired(unsigned long data)
> > +{
> > +     /* not an elegant detour, but there is no choice as the timer passes
> > +     * only one argument, and ieee80211_local is needed here */
> > +     int *ptid = (int *)data;
> > +     int *timer_to_id = ptid - *ptid;
> > +     struct sta_info *temp_sta = container_of(timer_to_id, struct sta_info,
> > +                                      timer_to_tid[0]);
>
> I think this needs more comments. I can, after a while, see what it
> does, but I'm not even sure it's correct. The whole timer_to_id thing is
> only for this code?

i will add comments.
currently we use it only here, but as Tx will suffer from the same
problem (MLME there requires to limit the time for addBA response per
TID) it will be soon used elsewhere.

> > +     u16 tid = (u16)*ptid;
> > +     sta = sta_info_get(local, temp_sta->addr);
>
> Missing newline.
>

certainly, thanks

> > +     if (!sta)
> > +             return;
>
> Why do you even do a sta_info_get() on the temp_sta's addr? Either you
> already have a good pointer or the whole thing will access invalid
> memory.
>

excellent and many thanks.
I forgot to add my del_timer_sync in sta_info_release to prevent this
problem, will add it now.

[PATCH 4/8] mac80211: A-MPDU Rx MLME data initialization

[Ron Rindjunsky]

This patch initialize A-MPDU MLME data for Rx sessions.

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 net/mac80211/sta_info.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index e849155..37381f3 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -132,6 +132,7 @@ struct sta_info * sta_info_add(struct ieee80211_local *local,
 			       struct net_device *dev, u8 *addr, gfp_t gfp)
 {
 	struct sta_info *sta;
+	int i;
 	DECLARE_MAC_BUF(mac);
 
 	sta = kzalloc(sizeof(*sta), gfp);
@@ -151,6 +152,16 @@ struct sta_info * sta_info_add(struct ieee80211_local *local,
 	memcpy(sta->addr, addr, ETH_ALEN);
 	sta->local = local;
 	sta->dev = dev;
+	spin_lock_init(&sta->ampdu_mlme.ampdu_rx);
+	for (i = 0; i < STA_TID_NUM; i++) {
+		sta->timer_to_tid[i] = i;
+		/* rx timers */
+		sta->ampdu_mlme.tid_rx[i].session_timer.function =
+			sta_rx_agg_session_timer_expired;
+		sta->ampdu_mlme.tid_rx[i].session_timer.data =
+			(unsigned long)&sta->timer_to_tid[i];
+		init_timer(&sta->ampdu_mlme.tid_rx[i].session_timer);
+	}
 	skb_queue_head_init(&sta->ps_tx_buf);
 	skb_queue_head_init(&sta->tx_filtered);
 	__sta_info_get(sta);	/* sta used by caller, decremented by
-- 
1.5.3.3

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 4/8] mac80211: A-MPDU Rx MLME data initialization

[Johannes Berg]

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


> +	spin_lock_init(&sta->ampdu_mlme.ampdu_rx);
> +	for (i = 0; i < STA_TID_NUM; i++) {
> +		sta->timer_to_tid[i] = i;

Please add a comment here as well that this *must* be initialised to the
identity mapping and have the comment mention the user(s).

johannes

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

Re: [PATCH 4/8] mac80211: A-MPDU Rx MLME data initialization

[Ron Rindjunsky]

>
> > +     spin_lock_init(&sta->ampdu_mlme.ampdu_rx);
> > +     for (i = 0; i < STA_TID_NUM; i++) {
> > +             sta->timer_to_tid[i] = i;
>
> Please add a comment here as well that this *must* be initialised to the
> identity mapping and have the comment mention the user(s).
>

will add a comment

[PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Ron Rindjunsky]

This patch handles the reordering of the Rx A-MPDU.
This issue occurs when the sequence of the internal MPDUs is not in the
right order. such a case can be encountered for example when some MPDUs from
previous aggregations were recieved, while others failed, so current A-MPDU
will contain a mix of re-transmited MPDUs and new ones.

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 include/net/mac80211.h       |    2 +
 net/mac80211/ieee80211_sta.c |   16 ++++
 net/mac80211/rx.c            |  172 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 189 insertions(+), 1 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f50c28a..27a15f0 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -376,6 +376,7 @@ enum mac80211_rx_flags {
  * @noise: PHY noise when receiving this frame
  * @antenna: antenna used
  * @rate: data rate
+ * @ordered: used in A-MPSDU reordering scheme
  * @flag: %RX_FLAG_*
  */
 struct ieee80211_rx_status {
@@ -388,6 +389,7 @@ struct ieee80211_rx_status {
 	int noise;
 	int antenna;
 	int rate;
+	int ordered;
 	int flag;
 };
 
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index ca98d06..4fa0301 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -1212,6 +1212,8 @@ void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
 	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
 	struct ieee80211_hw *hw = &local->hw;
 	struct sta_info *sta;
+	struct ieee80211_rx_status status;
+	int i;
 
 	sta = sta_info_get(local, ra);
 	if (!sta)
@@ -1252,6 +1254,20 @@ void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
 		ieee80211_send_delba(dev, ra, tid, 0, reason);
 
 	/* free the reordering buffer */
+	for (i = 0; i < sta->ampdu_mlme.tid_rx[tid].buf_size; i++) {
+		if (sta->ampdu_mlme.tid_rx[tid].reorder_buf[i]) {
+			/* release the reordered frames to stack,
+			 * but drop them there */
+			memcpy(&status, sta->ampdu_mlme.tid_rx[tid].
+				reorder_buf[i]->cb, sizeof(status));
+			status.ordered = TXRX_DROP;
+			__ieee80211_rx(hw, sta->ampdu_mlme.
+				       tid_rx[tid].reorder_buf[i],
+				       &status);
+			sta->ampdu_mlme.tid_rx[tid].stored_mpdu_num--;
+			sta->ampdu_mlme.tid_rx[tid].reorder_buf[i] = NULL;
+		}
+	}
 	kfree(sta->ampdu_mlme.tid_rx[tid].reorder_buf);
 
 	sta->ampdu_mlme.tid_rx[tid].state = HT_AGG_STATE_IDLE;
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5dbf5d6..7a705eb 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -226,6 +226,114 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb,
 	return origskb;
 }
 
+#define SEQ_MODULO 0x1000
+#define SEQ_MASK   0xfff
+
+static inline int seq_less(u16 sq1, u16 sq2)
+{
+	return (((sq1 - sq2) & SEQ_MASK) > (SEQ_MODULO >> 1));
+}
+
+static inline u16 seq_inc(u16 sq)
+{
+	return ((sq + 1) & SEQ_MASK);
+}
+
+static inline u16 seq_sub(u16 sq1, u16 sq2)
+{
+	return ((sq1 - sq2) & SEQ_MASK);
+}
+
+ieee80211_txrx_result
+ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
+				struct ieee80211_txrx_data *rx,
+				struct tid_ampdu_rx *tid_agg_rx,
+				struct sk_buff *skb, u16 mpdu_seq_num,
+				int bar_req)
+{
+	struct ieee80211_rx_status status;
+	u16 head_seq_num, buf_size;
+	int index;
+
+	buf_size = tid_agg_rx->buf_size;
+	head_seq_num = tid_agg_rx->head_seq_num;
+
+	/* frame with out of date sequence number */
+	if (seq_less(mpdu_seq_num, head_seq_num))
+		return TXRX_DROP;
+
+	/* if frame sequence number exceeds our buffering window size or
+	 * block Ack Request arrived - release stored frames */
+	if ((!seq_less(mpdu_seq_num, head_seq_num + buf_size)) || (bar_req)) {
+		/* new head to the ordering buffer */
+		if (bar_req)
+			head_seq_num = mpdu_seq_num;
+		else
+			head_seq_num =
+				seq_inc(seq_sub(mpdu_seq_num, buf_size));
+		/* release stored frames up to new head to stack */
+		while (seq_less(tid_agg_rx->head_seq_num, head_seq_num)) {
+			index = seq_sub(tid_agg_rx->head_seq_num,
+				tid_agg_rx->ssn)
+				% tid_agg_rx->buf_size;
+
+			if (tid_agg_rx->reorder_buf[index]) {
+				/* release the reordered frames to stack */
+				memcpy(&status,
+					tid_agg_rx->reorder_buf[index]->cb,
+					sizeof(status));
+
+				status.ordered = TXRX_QUEUED;
+				__ieee80211_rx(hw,
+					tid_agg_rx->reorder_buf[index],
+					&status);
+				tid_agg_rx->stored_mpdu_num--;
+				tid_agg_rx->reorder_buf[index] = NULL;
+			}
+			tid_agg_rx->head_seq_num =
+				seq_inc(tid_agg_rx->head_seq_num);
+		}
+		if (bar_req)
+			return TXRX_DROP;
+	}
+
+	/* now the new frame is always in the range of the reordering */
+	/* buffer window */
+	index = seq_sub(mpdu_seq_num, tid_agg_rx->ssn)
+				% tid_agg_rx->buf_size;
+	/* check if we already stored this frame */
+	if (tid_agg_rx->reorder_buf[index])
+		return TXRX_DROP;
+
+	/* if arrived mpdu is in the right order and nothing else stored */
+	/* release it immediately */
+	if (mpdu_seq_num == tid_agg_rx->head_seq_num &&
+			tid_agg_rx->stored_mpdu_num == 0) {
+		tid_agg_rx->head_seq_num =
+			seq_inc(tid_agg_rx->head_seq_num);
+		return TXRX_CONTINUE;
+	}
+
+	/* put the frame in the reordering buffer */
+	tid_agg_rx->reorder_buf[index] = skb;
+	tid_agg_rx->stored_mpdu_num++;
+	/* release the buffer until next missing frame */
+	index = seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn)
+						% tid_agg_rx->buf_size;
+	while (tid_agg_rx->reorder_buf[index]) {
+		/* release the reordered frame back to stack */
+		memcpy(&status, tid_agg_rx->reorder_buf[index]->cb,
+			sizeof(status));
+		status.ordered = TXRX_QUEUED;
+		__ieee80211_rx(hw, tid_agg_rx->reorder_buf[index], &status);
+		tid_agg_rx->stored_mpdu_num--;
+		tid_agg_rx->reorder_buf[index] = NULL;
+		tid_agg_rx->head_seq_num = seq_inc(tid_agg_rx->head_seq_num);
+		index =	seq_sub(tid_agg_rx->head_seq_num,
+			tid_agg_rx->ssn) % tid_agg_rx->buf_size;
+	}
+	return TXRX_QUEUED;
+}
 
 /* pre-rx handlers
  *
@@ -285,7 +393,7 @@ ieee80211_rx_h_load_stats(struct ieee80211_txrx_data *rx)
 
 	/* Estimate total channel use caused by this frame */
 
-	if (unlikely(mode->num_rates < 0))
+	if (unlikely(mode->num_rates < 0) || (rx->u.rx.status->ordered))
 		return TXRX_CONTINUE;
 
 	rate = &mode->rates[0];
@@ -320,10 +428,72 @@ ieee80211_rx_h_load_stats(struct ieee80211_txrx_data *rx)
 	return TXRX_CONTINUE;
 }
 
+static ieee80211_txrx_result
+ieee80211_rx_h_reorder_ampdu(struct ieee80211_txrx_data *rx)
+{
+	struct ieee80211_local *local = rx->local;
+	struct ieee80211_hw *hw = &local->hw;
+	struct sk_buff *skb = rx->skb;
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+	struct tid_ampdu_rx *tid_agg_rx;
+	u16 sc;
+	u16 mpdu_seq_num;
+
+	if (!rx->sta)
+		return TXRX_CONTINUE;
+
+	tid_agg_rx = &(rx->sta->ampdu_mlme.tid_rx[rx->u.rx.queue]);
+
+	/* filter the QoS data rx stream according to
+	 * STA/TID and check if this STA/TID is on aggregation */
+	if ((tid_agg_rx->state != HT_AGG_STATE_OPERATIONAL) ||
+	    (!WLAN_FC_IS_QOS_DATA(rx->fc)))
+		return TXRX_CONTINUE;
+
+	/* null data frames are excluded */
+	if (rx->fc & IEEE80211_STYPE_QOS_NULLFUNC)
+		return TXRX_CONTINUE;
+
+	/* check if this frame has already been queued and ordered */
+	if (rx->u.rx.status->ordered == TXRX_QUEUED)
+		return TXRX_CONTINUE;
+
+	/* check if this frame has already been queued and ordered but
+	 * should be deleted */
+	if (rx->u.rx.status->ordered == TXRX_DROP) {
+		if (skb)
+			dev_kfree_skb(skb);
+		return TXRX_DROP;
+	}
+
+	/* new un-ordered ampdu frame - process it */
+
+	/* reset session timer */
+	if (tid_agg_rx->timeout) {
+		unsigned long expires =
+			jiffies + (tid_agg_rx->timeout / 1000) * HZ;
+		mod_timer(&tid_agg_rx->session_timer, expires);
+	}
+
+	/* if this mpdu is fragmented - terminate rx aggregation session */
+	sc = le16_to_cpu(hdr->seq_ctrl);
+	if (sc & IEEE80211_SCTL_FRAG) {
+		ieee80211_sta_stop_rx_BA_session(rx->dev, rx->sta->addr,
+			rx->u.rx.queue, 0, WLAN_REASON_QSTA_REQUIRE_SETUP);
+		return TXRX_CONTINUE;
+	}
+
+	/* according to mpdu sequence number deal with reordering buffer */
+	mpdu_seq_num = (sc & IEEE80211_SCTL_SEQ) >> 4;
+	return ieee80211_sta_manage_reorder_buf(hw, rx, tid_agg_rx, skb,
+			mpdu_seq_num, 0);
+}
+
 ieee80211_rx_handler ieee80211_rx_pre_handlers[] =
 {
 	ieee80211_rx_h_parse_qos,
 	ieee80211_rx_h_load_stats,
+	ieee80211_rx_h_reorder_ampdu,
 	NULL
 };
 
-- 
1.5.3.3

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg]

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


>  struct ieee80211_rx_status {
> @@ -388,6 +389,7 @@ struct ieee80211_rx_status {
>  	int noise;
>  	int antenna;
>  	int rate;
> +	int ordered;

That's not an int, and it's only used internally. Can we somehow avoid
putting it into the rx status, have it in the txrx status structure
instead and give it the proper type (txrx result)? More of a question
than a suggestion, I'd like to have that but I'm not sure it's doable.
 
> @@ -1252,6 +1254,20 @@ void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
>  		ieee80211_send_delba(dev, ra, tid, 0, reason);
>  
>  	/* free the reordering buffer */
> +	for (i = 0; i < sta->ampdu_mlme.tid_rx[tid].buf_size; i++) {
> +		if (sta->ampdu_mlme.tid_rx[tid].reorder_buf[i]) {
> +			/* release the reordered frames to stack,
> +			 * but drop them there */
> +			memcpy(&status, sta->ampdu_mlme.tid_rx[tid].
> +				reorder_buf[i]->cb, sizeof(status));
> +			status.ordered = TXRX_DROP;
> +			__ieee80211_rx(hw, sta->ampdu_mlme.
> +				       tid_rx[tid].reorder_buf[i],
> +				       &status);

This is strange. Why are they even passed to __ieee80211_rx?

> +#define SEQ_MODULO 0x1000
> +#define SEQ_MASK   0xfff
> +
> +static inline int seq_less(u16 sq1, u16 sq2)
> +{
> +	return (((sq1 - sq2) & SEQ_MASK) > (SEQ_MODULO >> 1));

Is it really correct to subtract before masking?

> +static inline u16 seq_sub(u16 sq1, u16 sq2)
> +{
> +	return ((sq1 - sq2) & SEQ_MASK);
> +}

Same here. And maybe seq_less should use seq_sub and the SEQ_MODULO
define should be (SEQ_MASK+1)?

I think I need to take a second look at this patch to even understand
the problem it solves.

johannes

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

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Ron Rindjunsky]

>
> >  struct ieee80211_rx_status {
> > @@ -388,6 +389,7 @@ struct ieee80211_rx_status {
> >       int noise;
> >       int antenna;
> >       int rate;
> > +     int ordered;
>
> That's not an int, and it's only used internally. Can we somehow avoid
> putting it into the rx status, have it in the txrx status structure
> instead and give it the proper type (txrx result)? More of a question
> than a suggestion, I'd like to have that but I'm not sure it's doable.

well, i was asking myself this same question, and went to
ieee80211_rx_status as ieee80211_txrx_data derives its data from
ieee80211_rx_status in __ieee80211_rx, so i have no other way to pass
info per sk_buff.
type int was chosen as it is not a bool type (can be
drop/continue/queue) so it can't be fit to flags. i can put it in u8,
but i don't think it will change much, especially if alignment is
involved.

>
> > @@ -1252,6 +1254,20 @@ void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
> >               ieee80211_send_delba(dev, ra, tid, 0, reason);
> >
> >       /* free the reordering buffer */
> > +     for (i = 0; i < sta->ampdu_mlme.tid_rx[tid].buf_size; i++) {
> > +             if (sta->ampdu_mlme.tid_rx[tid].reorder_buf[i]) {
> > +                     /* release the reordered frames to stack,
> > +                      * but drop them there */
> > +                     memcpy(&status, sta->ampdu_mlme.tid_rx[tid].
> > +                             reorder_buf[i]->cb, sizeof(status));
> > +                     status.ordered = TXRX_DROP;
> > +                     __ieee80211_rx(hw, sta->ampdu_mlme.
> > +                                    tid_rx[tid].reorder_buf[i],
> > +                                    &status);
>
> This is strange. Why are they even passed to __ieee80211_rx?

of course i can get rid from there here, but thought it will be more
systematic and clearer to pass them back to __ieee80211_rx. if you
have any objection to this (efficiency considerations or like) i will
reconsider it.

>
> > +#define SEQ_MODULO 0x1000
> > +#define SEQ_MASK   0xfff
> > +
> > +static inline int seq_less(u16 sq1, u16 sq2)
> > +{
> > +     return (((sq1 - sq2) & SEQ_MASK) > (SEQ_MODULO >> 1));
>
> Is it really correct to subtract before masking?

as these values are already "masked" as sequence numbers the answer is yes.
if this is not the case, and somehow we deal internally with sequence
numbers > 4095 this function should be the least to worry about  ;-)

>
> > +static inline u16 seq_sub(u16 sq1, u16 sq2)
> > +{
> > +     return ((sq1 - sq2) & SEQ_MASK);
> > +}
>
> Same here.

this is modulo arithmetic. looks fine to me.

> And maybe seq_less should use seq_sub and the SEQ_MODULO
> define should be (SEQ_MASK+1)?
>

the fact these 2 #defines are 1 number apart shouldn't confuse. it is
just a more
convenient way for the 802.11 reader (used to sequence number limit)
to look at it. i could have, if i would like, decide that pass
criteria for seq_less is not 2048, but only the closest 200 frames, in
that case i would have give it the value of 200.

> I think I need to take a second look at this patch to even understand
> the problem it solves.
>

no one guarantees you get the frames in the right order... please see
the patch description for such a case.

> johannes
>
>

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg]

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


> > > +     int ordered;
> >
> > That's not an int, and it's only used internally. Can we somehow avoid
> > putting it into the rx status, have it in the txrx status structure
> > instead and give it the proper type (txrx result)? More of a question
> > than a suggestion, I'd like to have that but I'm not sure it's doable.
> 
> well, i was asking myself this same question, and went to
> ieee80211_rx_status as ieee80211_txrx_data derives its data from
> ieee80211_rx_status in __ieee80211_rx, so i have no other way to pass
> info per sk_buff.

Ok.

> type int was chosen as it is not a bool type (can be
> drop/continue/queue) so it can't be fit to flags.

No, I was thinking that it's enum txrx_result or whatever that is named.
But you can't use that type in mac80211.h since it's only defined in
ieee80211_i.h... Maybe you should put the declaration into mac80211.h
and use it here so that the compiler can error check this ordered field
better...

> > > @@ -1252,6 +1254,20 @@ void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
> > >               ieee80211_send_delba(dev, ra, tid, 0, reason);
> > >
> > >       /* free the reordering buffer */
> > > +     for (i = 0; i < sta->ampdu_mlme.tid_rx[tid].buf_size; i++) {
> > > +             if (sta->ampdu_mlme.tid_rx[tid].reorder_buf[i]) {
> > > +                     /* release the reordered frames to stack,
> > > +                      * but drop them there */
> > > +                     memcpy(&status, sta->ampdu_mlme.tid_rx[tid].
> > > +                             reorder_buf[i]->cb, sizeof(status));
> > > +                     status.ordered = TXRX_DROP;
> > > +                     __ieee80211_rx(hw, sta->ampdu_mlme.
> > > +                                    tid_rx[tid].reorder_buf[i],
> > > +                                    &status);
> >
> > This is strange. Why are they even passed to __ieee80211_rx?
> 
> of course i can get rid from there here, but thought it will be more
> systematic and clearer to pass them back to __ieee80211_rx. if you
> have any objection to this (efficiency considerations or like) i will
> reconsider it.

I'm just not sure why you'd want to. As far as I can the frames already
passed __ieee80211_rx(), no? Maybe only as part of the aggregation?

> > > +static inline int seq_less(u16 sq1, u16 sq2)
> > > +{
> > > +     return (((sq1 - sq2) & SEQ_MASK) > (SEQ_MODULO >> 1));
> >
> > Is it really correct to subtract before masking?
> 
> as these values are already "masked" as sequence numbers the answer is yes.
> if this is not the case, and somehow we deal internally with sequence
> numbers > 4095 this function should be the least to worry about  ;-)

Heh, ok, I didn't really look where it was used.

> the fact these 2 #defines are 1 number apart shouldn't confuse. it is
> just a more
> convenient way for the 802.11 reader (used to sequence number limit)
> to look at it. i could have, if i would like, decide that pass
> criteria for seq_less is not 2048, but only the closest 200 frames, in
> that case i would have give it the value of 200.

Ah, ok. Makes sense, can you add a comment?

> > I think I need to take a second look at this patch to even understand
> > the problem it solves.
> >
> 
> no one guarantees you get the frames in the right order... please see
> the patch description for such a case.

Ok, right. Still not quite sure I've understand the solution, I'll take
another look.

johannes

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

RE: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Rindjunsky, Ron]

>>> +     int ordered;
>>
>> type int was chosen as it is not a bool type (can be
>> drop/continue/queue) so it can't be fit to flags.

> No, I was thinking that it's enum txrx_result or whatever that is
named.
> But you can't use that type in mac80211.h since it's only defined in
> ieee80211_i.h... Maybe you should put the declaration into mac80211.h
> and use it here so that the compiler can error check this ordered
field
> better...

I thought about doing it. The txrx_result enum did look to me
as a very useful one for low level drivers, as the same set
of actions of drop/continue/queue is relevant there as well.
As you have the same opinion I'll go ahead with it.

>>> @@ -1252,6 +1254,20 @@ void ieee80211_sta_stop_rx_BA_session(struct 
>>>               ieee80211_send_delba(dev, ra, tid, 0, reason);
>>>
>>>       /* free the reordering buffer */
>>> +     for (i = 0; i < sta->ampdu_mlme.tid_rx[tid].buf_size; i++) {
>>> +             if (sta->ampdu_mlme.tid_rx[tid].reorder_buf[i]) {
>>> +                     /* release the reordered frames to stack,
>>> +                      * but drop them there */
>>> +                     memcpy(&status, sta->ampdu_mlme.tid_rx[tid].
>>> +                             reorder_buf[i]->cb, sizeof(status));
>>> +                     status.ordered = TXRX_DROP;
>>> +                     __ieee80211_rx(hw, sta->ampdu_mlme.
>>> +                                    tid_rx[tid].reorder_buf[i],
>>> +                                    &status);
>>>
>>> This is strange. Why are they even passed to __ieee80211_rx?
>> 
>> of course i can get rid from there here, but thought it will be more
>> systematic and clearer to pass them back to __ieee80211_rx. if you
>> have any objection to this (efficiency considerations or like) i will
>> reconsider it.

> I'm just not sure why you'd want to. As far as I can the frames
already
> passed __ieee80211_rx(), no? Maybe only as part of the aggregation?

No, not passed it yet, they are still queued for reordering, but as
aggregation is being tear down I am expected to drop the frames
that are still not in order.
Again, if you see any performance issue with it, although this may
occur only once per session, do tell, if not, I would prefer to
leave it that way, as this will 1 - give more accurate statistics
of dropped frames, 2 - I don't want to drop frames in places
people will have trouble finding, other then the in the handlers.
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg]

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


> > I'm just not sure why you'd want to. As far as I can the frames
> already
> > passed __ieee80211_rx(), no? Maybe only as part of the aggregation?
> 
> No, not passed it yet, they are still queued for reordering, but as
> aggregation is being tear down I am expected to drop the frames
> that are still not in order.

But haven't they passed through __ieee80211_rx() as the aggregate frame?

> Again, if you see any performance issue with it, although this may
> occur only once per session, do tell, if not, I would prefer to
> leave it that way, as this will 1 - give more accurate statistics
> of dropped frames, 2 - I don't want to drop frames in places
> people will have trouble finding, other then the in the handlers.

No I guess it's ok I'm just trying to understand it better.

johannes

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

RE: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Winkler, Tomas]

>-----Original Message-----
>From: Johannes Berg [mailto:johannes@sipsolutions.net]
>Sent: Wednesday, December 19, 2007 5:47 PM
>To: Rindjunsky, Ron
>Cc: linville@tuxdriver.com; linux-wireless@vger.kernel.org;
>flamingice@sourmilk.net; Winkler, Tomas; Zhu, Yi
>Subject: RE: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation
>reordering
>
>
>> > I'm just not sure why you'd want to. As far as I can the frames
>> already
>> > passed __ieee80211_rx(), no? Maybe only as part of the aggregation?
>>
>> No, not passed it yet, they are still queued for reordering, but as
>> aggregation is being tear down I am expected to drop the frames
>> that are still not in order.
>
>But haven't they passed through __ieee80211_rx() as the aggregate
frame?

AMsDU aggregation is different from AMpDU aggregation. For AMPDU the
disaggregating is happening let say in PHY level. First one is many
packets in packed into one. Second one is packets in a row without ACK
between them. (You answer with Block Ack to them). They appear as
regular frames to mac80211 but they are not in order. Only ordered
sequence might be passed to the network stack. So we might drop the
frames are result of non order.
So what Ron was concerning about is whether we it's not waste of time to
pass packets through rx handler before we know the sequence is
completed.

Tomas
>johannes
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg]

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


> AMsDU aggregation is different from AMpDU aggregation. 

Ohhh. I knew that. But thanks for pointing me to it again, I was indeed
thinking of A-MSDU all the time. Sorry.

johannes

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

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg]

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

Hi Ron,

> This patch handles the reordering of the Rx A-MPDU.
> This issue occurs when the sequence of the internal MPDUs is not in the
> right order. such a case can be encountered for example when some MPDUs from
> previous aggregations were recieved, while others failed, so current A-MPDU
> will contain a mix of re-transmited MPDUs and new ones.

I thought about this a bit more, here's a counter-proposal. :)

Your current patch means that when a monitor mode interface is up, it
will receive frames twice because the A-MPDU frames have already passed
__ieee80211_rx() when they were sent up by the hardware, and that copies
the frame to the monitor first thing. Also, what you're doing means that
the frame is accounted for channel load twice (yeah, I know, the stats
there suck... but still...)

What we'd really want I think is to
 (1) put the frame to monitor regularly [indicating A-MPDU in the
     radiotap header, this will need radiotap standard work, so not now]
 (2) "keep" the frame in the ieee80211_rx_h_reorder_ampdu rx-pre-handler
 (3) "release" the frame with an appropriate status later

Hence, I think it'd be best to first reorganise the current code:
 (1) split __ieee80211_rx() right after the pre handler invocation and
     call e.g. __ieee80211_rx_handlers() which contains all the code
     from "if (sta && !(sta->flags & (WLAN_STA_WDS | WLAN_STA_ASSOC_AP"
     to "} else dev_kfree_skb(skb);"
     The function would have certain requirements like a filled
     ieee80211_txrx_data and running under rcu_read_lock().
 (2) then, you don't need status.ordered but can drop the frame right
     away or pass it to __ieee80211_rx_handlers().

That way, we guarantee that each frame only passes through the complete
RX path once and your dropping the unordered frames is equivalent to the
reorder_ampdu handler having an oracle that tells it which frames to
drop and which to keep :)

Comments?

johannes

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

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Ron Rindjunsky]

> > This patch handles the reordering of the Rx A-MPDU.
>
> I thought about this a bit more, here's a counter-proposal. :)
>
> Your current patch means that when a monitor mode interface is up, it
> will receive frames twice because the A-MPDU frames have already passed
> __ieee80211_rx() when they were sent up by the hardware, and that copies
> the frame to the monitor first thing.

true, i forgot monitor is no longer a handle like it used to be, and i
do pass there twice. this can be fixed by checking .ordered field
there

> Also, what you're doing means that
> the frame is accounted for channel load twice (yeah, I know, the stats
> there suck... but still...)

here i am already checking the .ordered field (like i proposed for
monitor), and ignore the in-order frames.

>
> What we'd really want I think is to
>  (1) put the frame to monitor regularly [indicating A-MPDU in the
>     radiotap header, this will need radiotap standard work, so not now]
>  (2) "keep" the frame in the ieee80211_rx_h_reorder_ampdu rx-pre-handler
>  (3) "release" the frame with an appropriate status later
>
> Hence, I think it'd be best to first reorganise the current code:
>  (1) split __ieee80211_rx() right after the pre handler invocation and
>     call e.g. __ieee80211_rx_handlers() which contains all the code
>     from "if (sta && !(sta->flags & (WLAN_STA_WDS | WLAN_STA_ASSOC_AP"
>     to "} else dev_kfree_skb(skb);"
>     The function would have certain requirements like a filled
>     ieee80211_txrx_data and running under rcu_read_lock().
>  (2) then, you don't need status.ordered but can drop the frame right
>     away or pass it to __ieee80211_rx_handlers().
>
> That way, we guarantee that each frame only passes through the complete
> RX path once and your dropping the unordered frames is equivalent to the
> reorder_ampdu handler having an oracle that tells it which frames to

so you suggest to remove ieee80211_rx_h_reorder_ampdu in favor of a
function that will call ieee80211_sta_manage_reorder_buf?
in any case, function or handler, i think it is better to leave this
separation - filtering and reordering -  in 2 different place.

> drop and which to keep :)
>
> Comments?
>

I looked at your proposal, generally it can be done, yet there are
pros and cons we need to consider:
1 - (con) by inserting the .ordered check to ieee80211_rx_monitor or
before it i can avoid changing the code - just 2 more lines and
problem is solved
2 - (pro) yet, i'll end up checking the .ordered field in 3 places -
monitor,statistics and reordering buf, which lowers the encapsulation
of work inside ieee80211_rx_h_reorder_ampdu
3 - (con) i will have to free un-needed sk_buffs in
ieee80211_sta_stop_rx_BA_session without going back to __ieee80211_rx,
 which may confuse othere.
4 - (pro) i'll use less variables (with a little more code, but a clear one)

> johannes
>
>

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg]

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


> > Also, what you're doing means that
> > the frame is accounted for channel load twice (yeah, I know, the stats
> > there suck... but still...)
> 
> here i am already checking the .ordered field (like i proposed for
> monitor), and ignore the in-order frames.

Ah. You'd need to ignore the out-of-order frames too since they too have
gone through __ieee80211_rx already, no? Or do you by out-of-order just
mean those that have been queued?

> so you suggest to remove ieee80211_rx_h_reorder_ampdu in favor of a
> function that will call ieee80211_sta_manage_reorder_buf?

I guess the rx_h_reorder_ampdu pre-handler can stay, but as far as I
understood the flow it will cause some frames to be "buffered". And I
think frames shouldn't go through the same path twice even if you've
annotated that path all over skipping when the frame was injected.

Basically, it seems to me that you're injecting frames down the same
__ieee80211_rx() path when they have already gone through that, and then
after the fact annotate pretty much all of that path before the actual
rx handlers (after pre-handlers) with "skip if reordered frame". That I
don't really like, it means we need to special-case everything we ever
put there.

> 2 - (pro) yet, i'll end up checking the .ordered field in 3 places -
> monitor,statistics and reordering buf, which lowers the encapsulation
> of work inside ieee80211_rx_h_reorder_ampdu
> 3 - (con) i will have to free un-needed sk_buffs in
> ieee80211_sta_stop_rx_BA_session without going back to __ieee80211_rx,
>  which may confuse othere.

I don't think it's that confusing because __ieee80211_rx would do the
same thing, it would free the frame if requested by the pre-handlers
instead of passing it to the rx handlers. What your code does would
simply be a diversion through another queue that has the same effect.
(If you keep the rx-pre-handler for that you should add a comment that
it must be last)

johannes

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

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Ron Rindjunsky]

ok, I agree.
I'll make one patch for seperation only inside __ieee80211_rx, and
then build the reordering on top of it.

> > > Also, what you're doing means that
> > > the frame is accounted for channel load twice (yeah, I know, the stats
> > > there suck... but still...)
> >
> > here i am already checking the .ordered field (like i proposed for
> > monitor), and ignore the in-order frames.
>
> Ah. You'd need to ignore the out-of-order frames too since they too have
> gone through __ieee80211_rx already, no? Or do you by out-of-order just
> mean those that have been queued?
>
> > so you suggest to remove ieee80211_rx_h_reorder_ampdu in favor of a
> > function that will call ieee80211_sta_manage_reorder_buf?
>
> I guess the rx_h_reorder_ampdu pre-handler can stay, but as far as I
> understood the flow it will cause some frames to be "buffered". And I
> think frames shouldn't go through the same path twice even if you've
> annotated that path all over skipping when the frame was injected.
>
> Basically, it seems to me that you're injecting frames down the same
> __ieee80211_rx() path when they have already gone through that, and then
> after the fact annotate pretty much all of that path before the actual
> rx handlers (after pre-handlers) with "skip if reordered frame". That I
> don't really like, it means we need to special-case everything we ever
> put there.
>
> > 2 - (pro) yet, i'll end up checking the .ordered field in 3 places -
> > monitor,statistics and reordering buf, which lowers the encapsulation
> > of work inside ieee80211_rx_h_reorder_ampdu
> > 3 - (con) i will have to free un-needed sk_buffs in
> > ieee80211_sta_stop_rx_BA_session without going back to __ieee80211_rx,
> >  which may confuse othere.
>
> I don't think it's that confusing because __ieee80211_rx would do the
> same thing, it would free the frame if requested by the pre-handlers
> instead of passing it to the rx handlers. What your code does would
> simply be a diversion through another queue that has the same effect.
> (If you keep the rx-pre-handler for that you should add a comment that
> it must be last)
>
> johannes
>
>

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Ron Rindjunsky]

>
> Basically, it seems to me that you're injecting frames down the same
> __ieee80211_rx() path when they have already gone through that, and then
> after the fact annotate pretty much all of that path before the actual
> rx handlers (after pre-handlers) with "skip if reordered frame". That I
> don't really like, it means we need to special-case everything we ever
> put there.
>
> Hence, I think it'd be best to first reorganise the current code:
>  (1) split __ieee80211_rx() right after the pre handler invocation and
>     call e.g. __ieee80211_rx_handlers() which contains all the code
>     from "if (sta && !(sta->flags & (WLAN_STA_WDS | WLAN_STA_ASSOC_AP"
>     to "} else dev_kfree_skb(skb);"
>     The function would have certain requirements like a filled
>     ieee80211_txrx_data and running under rcu_read_lock().
>  (2) then, you don't need status.ordered but can drop the frame right
>     away or pass it to __ieee80211_rx_handlers().
>
> That way, we guarantee that each frame only passes through the complete
> RX path once and your dropping the unordered frames is equivalent to

Johannes, i re-inspected the code to implement your solution, and it
is not straight forward as you have seen it.
what we do now in __ieee80211_rx is:
- invoke monitor
- fill in txrx_data
- invoke michael_mic_report (if needed)
- call pre-handlers of QoS that change txrx_data as well.
- call pre-handlers of statistics that change txrx_data
- call reorder buffer prehandler
    - saving sk_buff to reordering buffer.
    - if we can, send sk_buffs back to __ieee80211_rx, that will
properly fill txrx_data

now, in order to proceed according to your implementation, as i do not
call again  __ieee80211, and do not get the txrx_data restored,  i
will either:
1 - have to fill in txrx_data inside reordering function, which will
end in massive code duplication, and i don't like this solution
2 - keep the whole txrx_data and not only the sk_buff inside
reordering buffer, which will end up in allocation of the txrx_data
struct for every incoming frame, or in a big maximum possible size of
64 pre-allocated array of txrx_data per tid - this is either waste of
cpu or memory
3 - stick to my current implementation, but assure that no function
will be called for ordered frames on their second call in __ieee80211.
we already mentioned it may lead to low encapsulation of reordering
mechanism.

so, what do you think about next solution:

1 - move the rxtx_data indifferent functionality out of __ieee80211_rx
(monitor, load and reordering) to, lets say __ieee80211_rx_pre_handle.
2 - __ieee80211_rx_pre_handle will be called for each frame. either
directly or through tasklet.
3 - in regular state, proceed to __ieee80211_rx immediately, in
reorder state loop over __ieee80211_rx for each already ordered frame.
4 - I thought to eliminate __ieee80211_invoke_rx_handlers(local,
local->rx_pre_handlers), as QoS will be the only one to inhabit it
now, so it may move to ieee80211_invoke_rx_handlers(local,
local->rx_handlers)

sorry for the long mail, but i wanted to write my full opinion about this issue.

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg]

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

Hi Ron,

> Johannes, i re-inspected the code to implement your solution, and it
> is not straight forward as you have seen it.

Ok.

> what we do now in __ieee80211_rx is:
> - invoke monitor
> - fill in txrx_data
> - invoke michael_mic_report (if needed)
> - call pre-handlers of QoS that change txrx_data as well.
> - call pre-handlers of statistics that change txrx_data
> - call reorder buffer prehandler
>     - saving sk_buff to reordering buffer.
>     - if we can, send sk_buffs back to __ieee80211_rx, that will
> properly fill txrx_data

Yeah, sounds like it. Some things like monitor may discard the frame.

> now, in order to proceed according to your implementation, as i do not
> call again  __ieee80211, and do not get the txrx_data restored,  i
> will either:
> 1 - have to fill in txrx_data inside reordering function, which will
> end in massive code duplication, and i don't like this solution
> 2 - keep the whole txrx_data and not only the sk_buff inside
> reordering buffer, which will end up in allocation of the txrx_data
> struct for every incoming frame, or in a big maximum possible size of
> 64 pre-allocated array of txrx_data per tid - this is either waste of
> cpu or memory

Hmm, good points. Didn't really think about it but yeah, txrx data is on
the stack there.

> so, what do you think about next solution:
> 
> 1 - move the rxtx_data indifferent functionality out of __ieee80211_rx
> (monitor, load and reordering) to, lets say __ieee80211_rx_pre_handle.

Sounds fine.

> 2 - __ieee80211_rx_pre_handle will be called for each frame. either
> directly or through tasklet.

Hm, ok, we should keep __ieee80211_rx() as the entry point though so we
don't have to change drivers.

> 3 - in regular state, proceed to __ieee80211_rx immediately, in
> reorder state loop over __ieee80211_rx for each already ordered frame.

Ah ok.

> 4 - I thought to eliminate __ieee80211_invoke_rx_handlers(local,
> local->rx_pre_handlers), as QoS will be the only one to inhabit it
> now, so it may move to ieee80211_invoke_rx_handlers(local,
> local->rx_handlers)

I don't think it can move there because the rx handlers can potentially
be invoked multiple times. Will that work? In any case, I'm not against
totally dissolving the pre-handlers into function calls. Makes for
better code generation anyway since gcc will be able to optimise across
the functions when inlining them (since they'll all have only one user)

> sorry for the long mail, but i wanted to write my full opinion about this issue.

Not at all, thanks for doing that.

I think we should probably keep __iee80211_rx() as the entry point for
the tasklet or the drivers.

So I'd see the flow as:

tasklet/driver
__ieee80211_rx()
	__rx_pre_monitor()
	__rx_pre_load()
	__rx_pre_qos()

	if (agg)
		__rx_handle_reorder()
	else
		__rx_handle_packet()

and __rx_handle_reorder() calls __rx_handle_packet() which does all the
rest, I guess.

Does that sound sane to you? You're in a better position to judge. I
think this is basically what you wanted, just keeping __ieee80211_rx()
as the main entry point.

Or maybe I'm totally off. Can you explain again what we need to do for
such a reordered frame?

johannes

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

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Ron Rindjunsky]

> So I'd see the flow as:
>
> tasklet/driver
> __ieee80211_rx()
>        __rx_pre_monitor()
>        __rx_pre_load()
>        __rx_pre_qos()
>
>        if (agg)
>                __rx_handle_reorder()
>        else
>                __rx_handle_packet()
>
> and __rx_handle_reorder() calls __rx_handle_packet() which does all the
> rest, I guess.
>

that's the general idea, but __rx_pre_qos() won't be able to be in
__ieee80211_rx(), as it handles txrx_data. if it will be there i am
back in the original problem - the need to restore txrx_data per
frame. i will have to put it inside __rx_handle_packet().

Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg]

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


> > tasklet/driver
> > __ieee80211_rx()
> >        __rx_pre_monitor()
> >        __rx_pre_load()
> >        __rx_pre_qos()
> >
> >        if (agg)
> >                __rx_handle_reorder()
> >        else
> >                __rx_handle_packet()
> >
> > and __rx_handle_reorder() calls __rx_handle_packet() which does all the
> > rest, I guess.
> >
> 
> that's the general idea, but __rx_pre_qos() won't be able to be in
> __ieee80211_rx(), as it handles txrx_data. if it will be there i am
> back in the original problem - the need to restore txrx_data per
> frame. i will have to put it inside __rx_handle_packet().

Ah, ok, I see, sounds good. It can well be there but I guess it should
be before invoking the RX handlers though; then I'll go and see if
anything else only needs to be done once per interface.

johannes

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

[PATCH 6/8] mac80211: A-MPDU Rx adding BAR handling capability

[Ron Rindjunsky]

This patch adds the ability to handle Block Ack Request

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 net/mac80211/ieee80211_i.h |    3 +-
 net/mac80211/rx.c          |   53 ++++++++++++++++++++++++++++++++++++++++---
 net/mac80211/util.c        |   12 +++++++++-
 3 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index cf2256b..ed7ae6f 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -804,7 +804,8 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb, struct net_device *dev);
 extern void *mac80211_wiphy_privid; /* for wiphy privid */
 extern const unsigned char rfc1042_header[6];
 extern const unsigned char bridge_tunnel_header[6];
-u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len);
+u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len,
+			unsigned int type);
 int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
 int ieee80211_frame_duration(struct ieee80211_local *local, size_t len,
 			     int rate, int erp, int short_preamble);
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 7a705eb..ca0e89f 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -64,7 +64,9 @@ static inline int should_drop_frame(struct ieee80211_rx_status *status,
 	if (((hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_FTYPE)) ==
 			cpu_to_le16(IEEE80211_FTYPE_CTL)) &&
 	    ((hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_STYPE)) !=
-			cpu_to_le16(IEEE80211_STYPE_PSPOLL)))
+			cpu_to_le16(IEEE80211_STYPE_PSPOLL)) &&
+	    ((hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_STYPE)) !=
+			cpu_to_le16(IEEE80211_STYPE_BACK_REQ)))
 		return 1;
 	return 0;
 }
@@ -803,7 +805,8 @@ ieee80211_rx_h_sta_process(struct ieee80211_txrx_data *rx)
 	 * BSSID to avoid keeping the current IBSS network alive in cases where
 	 * other STAs are using different BSSID. */
 	if (rx->sdata->type == IEEE80211_IF_TYPE_IBSS) {
-		u8 *bssid = ieee80211_get_bssid(hdr, rx->skb->len);
+		u8 *bssid = ieee80211_get_bssid(hdr, rx->skb->len,
+						IEEE80211_IF_TYPE_IBSS);
 		if (compare_ether_addr(bssid, rx->sdata->u.sta.bssid) == 0)
 			sta->last_rx = jiffies;
 	} else
@@ -1530,6 +1533,48 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 }
 
 static ieee80211_txrx_result
+ieee80211_rx_h_ctrl(struct ieee80211_txrx_data *rx)
+{
+	struct ieee80211_local *local = rx->local;
+	struct ieee80211_hw *hw = &local->hw;
+	struct sk_buff *skb = rx->skb;
+	struct ieee80211_bar *bar = (struct ieee80211_bar *) skb->data;
+	struct tid_ampdu_rx *tid_agg_rx;
+	u16 start_seq_num;
+	u16 tid;
+
+	if (likely((rx->fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_CTL))
+		return TXRX_CONTINUE;
+
+	if ((rx->fc & IEEE80211_FCTL_STYPE) == IEEE80211_STYPE_BACK_REQ) {
+		if (!rx->sta)
+			return TXRX_CONTINUE;
+		tid = le16_to_cpu(bar->control) >> 12;
+		tid_agg_rx = &(rx->sta->ampdu_mlme.tid_rx[tid]);
+		if (tid_agg_rx->state != HT_AGG_STATE_OPERATIONAL)
+			return TXRX_CONTINUE;
+
+		start_seq_num = le16_to_cpu(bar->start_seq_num) >> 4;
+
+		/* reset session timer */
+		if (tid_agg_rx->timeout) {
+			unsigned long expires =
+				jiffies + (tid_agg_rx->timeout / 1000) * HZ;
+			mod_timer(&tid_agg_rx->session_timer, expires);
+		}
+
+		/* manage reordering buffer according to requested */
+		/* sequence number */
+		ieee80211_sta_manage_reorder_buf(hw, rx, tid_agg_rx,
+						NULL, start_seq_num, 1);
+
+		return TXRX_DROP;
+	}
+
+	return TXRX_CONTINUE;
+}
+
+static ieee80211_txrx_result
 ieee80211_rx_h_mgmt(struct ieee80211_txrx_data *rx)
 {
 	struct ieee80211_sub_if_data *sdata;
@@ -1680,6 +1725,7 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
 	ieee80211_rx_h_remove_qos_control,
 	ieee80211_rx_h_amsdu,
 	ieee80211_rx_h_data,
+	ieee80211_rx_h_ctrl,
 	ieee80211_rx_h_mgmt,
 	NULL
 };
@@ -1841,8 +1887,6 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
 		return;
 	}
 
-	bssid = ieee80211_get_bssid(hdr, skb->len);
-
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 		if (!netif_running(sdata->dev))
 			continue;
@@ -1850,6 +1894,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
 		if (sdata->type == IEEE80211_IF_TYPE_MNTR)
 			continue;
 
+		bssid = ieee80211_get_bssid(hdr, skb->len, sdata->type);
 		rx.flags |= IEEE80211_TXRXD_RXRA_MATCH;
 		prepres = prepare_for_handlers(sdata, bssid, &rx, hdr);
 		/* prepare_for_handlers can change sta */
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index fb7fd89..d1a6138 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -131,7 +131,8 @@ void ieee80211_prepare_rates(struct ieee80211_local *local,
 	}
 }
 
-u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len)
+u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len,
+			unsigned int type)
 {
 	u16 fc;
 
@@ -163,6 +164,15 @@ u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len)
 	case IEEE80211_FTYPE_CTL:
 		if ((fc & IEEE80211_FCTL_STYPE) == IEEE80211_STYPE_PSPOLL)
 			return hdr->addr1;
+		else if ((fc & IEEE80211_FCTL_STYPE) ==
+						IEEE80211_STYPE_BACK_REQ) {
+			switch (type) {
+			case IEEE80211_IF_TYPE_STA:
+				return hdr->addr2;
+			case IEEE80211_IF_TYPE_AP:
+				return hdr->addr1;
+			}
+		}
 		else
 			return NULL;
 	}
-- 
1.5.3.3

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 6/8] mac80211: A-MPDU Rx adding BAR handling capability

[Johannes Berg]

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


> -u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len);
> +u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len,
> +			unsigned int type);

Please use the proper enum.

> @@ -1530,6 +1533,48 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  }
>  
>  static ieee80211_txrx_result
> +ieee80211_rx_h_ctrl(struct ieee80211_txrx_data *rx)
> +{
> +	struct ieee80211_local *local = rx->local;
> +	struct ieee80211_hw *hw = &local->hw;
> +	struct sk_buff *skb = rx->skb;
> +	struct ieee80211_bar *bar = (struct ieee80211_bar *) skb->data;
> +	struct tid_ampdu_rx *tid_agg_rx;
> +	u16 start_seq_num;
> +	u16 tid;
> +
> +	if (likely((rx->fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_CTL))
> +		return TXRX_CONTINUE;
> +
> +	if ((rx->fc & IEEE80211_FCTL_STYPE) == IEEE80211_STYPE_BACK_REQ) {
> +		if (!rx->sta)
> +			return TXRX_CONTINUE;
> +		tid = le16_to_cpu(bar->control) >> 12;
> +		tid_agg_rx = &(rx->sta->ampdu_mlme.tid_rx[tid]);
> +		if (tid_agg_rx->state != HT_AGG_STATE_OPERATIONAL)
> +			return TXRX_CONTINUE;
> +
> +		start_seq_num = le16_to_cpu(bar->start_seq_num) >> 4;
> +
> +		/* reset session timer */
> +		if (tid_agg_rx->timeout) {
> +			unsigned long expires =
> +				jiffies + (tid_agg_rx->timeout / 1000) * HZ;
> +			mod_timer(&tid_agg_rx->session_timer, expires);
> +		}
> +
> +		/* manage reordering buffer according to requested */
> +		/* sequence number */
> +		ieee80211_sta_manage_reorder_buf(hw, rx, tid_agg_rx,
> +						NULL, start_seq_num, 1);
> +
> +		return TXRX_DROP;
> +	}
> +
> +	return TXRX_CONTINUE;
> +}
> +
> +static ieee80211_txrx_result
>  ieee80211_rx_h_mgmt(struct ieee80211_txrx_data *rx)
>  {
>  	struct ieee80211_sub_if_data *sdata;
> @@ -1680,6 +1725,7 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
>  	ieee80211_rx_h_remove_qos_control,
>  	ieee80211_rx_h_amsdu,
>  	ieee80211_rx_h_data,
> +	ieee80211_rx_h_ctrl,
>  	ieee80211_rx_h_mgmt,

Maybe we should have a single rx_h_frame function that passes it to
three control/data/mgmt functions depending on the frame type? Not sure
though since data comes first and that's the hot path.


> @@ -163,6 +164,15 @@ u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len)
>  	case IEEE80211_FTYPE_CTL:
>  		if ((fc & IEEE80211_FCTL_STYPE) == IEEE80211_STYPE_PSPOLL)
>  			return hdr->addr1;
> +		else if ((fc & IEEE80211_FCTL_STYPE) ==
> +						IEEE80211_STYPE_BACK_REQ) {
> +			switch (type) {
> +			case IEEE80211_IF_TYPE_STA:
> +				return hdr->addr2;
> +			case IEEE80211_IF_TYPE_AP:
> +				return hdr->addr1;
> +			}

Doesn't that result in a compiler warning? And what does happen if we
receive a BACK_REQ when we're an IBSS/MESH?

johannes

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

Re: [PATCH 6/8] mac80211: A-MPDU Rx adding BAR handling capability

[Ron Rindjunsky]

>
> > -u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len);
> > +u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len,
> > +                     unsigned int type);
>
> Please use the proper enum.

will do.

> > @@ -1680,6 +1725,7 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
> >       ieee80211_rx_h_remove_qos_control,
> >       ieee80211_rx_h_amsdu,
> >       ieee80211_rx_h_data,
> > +     ieee80211_rx_h_ctrl,
> >       ieee80211_rx_h_mgmt,
>
> Maybe we should have a single rx_h_frame function that passes it to
> three control/data/mgmt functions depending on the frame type? Not sure
> though since data comes first and that's the hot path.

especially in A-MPDU, data is the most traffic you'll see in these
handlers (lets put aside the AP that bombed me with 749 BARs over 30
seconds ;-) i really think it was a buggy functionality of the AP
there)

>
>
> > @@ -163,6 +164,15 @@ u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len)
> >       case IEEE80211_FTYPE_CTL:
> >               if ((fc & IEEE80211_FCTL_STYPE) == IEEE80211_STYPE_PSPOLL)
> >                       return hdr->addr1;
> > +             else if ((fc & IEEE80211_FCTL_STYPE) ==
> > +                                             IEEE80211_STYPE_BACK_REQ) {
> > +                     switch (type) {
> > +                     case IEEE80211_IF_TYPE_STA:
> > +                             return hdr->addr2;
> > +                     case IEEE80211_IF_TYPE_AP:
> > +                             return hdr->addr1;
> > +                     }
>
> Doesn't that result in a compiler warning? And what does happen if we
> receive a BACK_REQ when we're an IBSS/MESH?
>

IBSS is not supporting 802.11n currently (spec decision)
MESH i am not familier enough yet to answer. are you sure it supports 11n?

Re: [PATCH 6/8] mac80211: A-MPDU Rx adding BAR handling capability

[Johannes Berg]

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


> (lets put aside the AP that bombed me with 749 BARs over 30
> seconds ;-) i really think it was a buggy functionality of the AP
> there)

Heh. Fun.

> > > +             else if ((fc & IEEE80211_FCTL_STYPE) ==
> > > +                                             IEEE80211_STYPE_BACK_REQ) {
> > > +                     switch (type) {
> > > +                     case IEEE80211_IF_TYPE_STA:
> > > +                             return hdr->addr2;
> > > +                     case IEEE80211_IF_TYPE_AP:
> > > +                             return hdr->addr1;
> > > +                     }
> >
> > Doesn't that result in a compiler warning? And what does happen if we
> > receive a BACK_REQ when we're an IBSS/MESH?

I was obviously wrong here, it doesn't result in a compiler warning
since you didn't use the enum type. It will once you use the enum type.

> IBSS is not supporting 802.11n currently (spec decision)

Ok.

> MESH i am not familier enough yet to answer. are you sure it supports 11n?

I guess we should just return NULL in all other cases here. But please
enumerate the cases explicitly so that people who add new interface
types get warnings, i.e. don't use a default: statement in a switch
(interface type)

johannes

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

RE: [PATCH 6/8] mac80211: A-MPDU Rx adding BAR handling capability

[Rindjunsky, Ron]

>>> +                     switch (type) {
>>> +                     case IEEE80211_IF_TYPE_STA:
>>> +                             return hdr->addr2;
>>> +                     case IEEE80211_IF_TYPE_AP:
>>> +                             return hdr->addr1;
>>> +                     }
>>
>> Doesn't that result in a compiler warning? And what does happen if we
>> receive a BACK_REQ when we're an IBSS/MESH?

> I guess we should just return NULL in all other cases here. But please
> enumerate the cases explicitly so that people who add new interface
> types get warnings, i.e. don't use a default: statement in a switch
> (interface type)

you sure you want these ugly warnings? I do prefer to put default here.

> johannes
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH 6/8] mac80211: A-MPDU Rx adding BAR handling capability

[Johannes Berg]

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


> > I guess we should just return NULL in all other cases here. But please
> > enumerate the cases explicitly so that people who add new interface
> > types get warnings, i.e. don't use a default: statement in a switch
> > (interface type)
> 
> you sure you want these ugly warnings? I do prefer to put default here.

No, I want to put all currently possible values there instead of the
default so that when the mesh folks add their type they get the warning
there.

johannes

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

RE: [PATCH 6/8] mac80211: A-MPDU Rx adding BAR handling capability

[Rindjunsky, Ron]

> > I guess we should just return NULL in all other cases here. But
please
> > enumerate the cases explicitly so that people who add new interface
> > types get warnings, i.e. don't use a default: statement in a switch
> > (interface type)
> 
> you sure you want these ugly warnings? I do prefer to put default
here.

Ok, will do. I will rebase all the patches and resend them.
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 7/8] mac80211: A-MPDU Rx handling DELBA requests

[Ron Rindjunsky]

This patch opens the flow to DELBA management frames, and handles end
of A-MPDU session produced by this event.

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 net/mac80211/ieee80211_sta.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 39 insertions(+), 1 deletions(-)

Index: wl2_6_24_rc3_Rx_Agg/net/mac80211/ieee80211_sta.c
===================================================================
--- wl2_6_24_rc3_Rx_Agg.orig/net/mac80211/ieee80211_sta.c
+++ wl2_6_24_rc3_Rx_Agg/net/mac80211/ieee80211_sta.c
@@ -63,6 +63,8 @@
 #define IEEE80211_ADDBA_PARAM_POLICY_MASK 0x0002
 #define IEEE80211_ADDBA_PARAM_TID_MASK 0x003C
 #define IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK 0xFFA0
+#define IEEE80211_DELBA_PARAM_TID_MASK 0xF000
+#define IEEE80211_DELBA_PARAM_INITIATOR_MASK 0x0800
 
 #define IEEE80211_MIN_AMPDU_BUF 0x8
 #define IEEE80211_MAX_AMPDU_BUF 0x40
@@ -1274,6 +1276,36 @@ void ieee80211_sta_stop_rx_BA_session(st
 	sta_info_put(sta);
 }
 
+static void ieee80211_sta_process_delba(struct net_device *dev,
+			struct ieee80211_mgmt *mgmt, size_t len)
+{
+	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+	struct sta_info *sta;
+	u16 tid, params;
+	u16 initiator;
+	DECLARE_MAC_BUF(mac);
+
+	sta = sta_info_get(local, mgmt->sa);
+	if (!sta)
+		return;
+
+	params = le16_to_cpu(mgmt->u.action.u.delba.params);
+	tid = (params & IEEE80211_DELBA_PARAM_TID_MASK) >> 12;
+	initiator = (params & IEEE80211_DELBA_PARAM_INITIATOR_MASK) >> 11;
+
+#ifdef CONFIG_MAC80211_HT_DEBUG
+	if (net_ratelimit())
+		printk(KERN_DEBUG "delba from %s on tid %d reason code %d\n",
+			print_mac(mac, mgmt->sa), tid,
+			mgmt->u.action.u.delba.reason_code);
+#endif /* CONFIG_MAC80211_HT_DEBUG */
+
+	if (initiator == WLAN_BACK_INITIATOR)
+		ieee80211_sta_stop_rx_BA_session(dev, sta->addr, tid,
+						 WLAN_BACK_INITIATOR, 0);
+	sta_info_put(sta);
+}
+
 /*
  * After receiving Block Ack Request (BAR) we activated a
  * timer after each frame arrives from the originator.
@@ -2234,9 +2266,15 @@ void ieee80211_rx_mgmt_action(struct net
 				break;
 			ieee80211_sta_process_addba_request(dev, mgmt, len);
 			break;
+		case WLAN_ACTION_DELBA:
+			if (len < (IEEE80211_MIN_ACTION_SIZE +
+				   sizeof(mgmt->u.action.u.delba)))
+				break;
+			ieee80211_sta_process_delba(dev, mgmt, len);
+			break;
 		default:
 			if (net_ratelimit())
-			   printk(KERN_DEBUG "%s: received unsupported BACK\n",
+			   printk(KERN_DEBUG "%s: Rx unknown A-MPDU action\n",
 					dev->name);
 			break;
 		}
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 7/8] mac80211: A-MPDU Rx handling DELBA requests

[Johannes Berg]

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


> +	params = le16_to_cpu(mgmt->u.action.u.delba.params);
> +	tid = (params & IEEE80211_DELBA_PARAM_TID_MASK) >> 12;
> +	initiator = (params & IEEE80211_DELBA_PARAM_INITIATOR_MASK) >> 11;
> +
> +#ifdef CONFIG_MAC80211_HT_DEBUG
> +	if (net_ratelimit())
> +		printk(KERN_DEBUG "delba from %s on tid %d reason code %d\n",
> +			print_mac(mac, mgmt->sa), tid,
> +			mgmt->u.action.u.delba.reason_code);
> +#endif /* CONFIG_MAC80211_HT_DEBUG */
> +
> +	if (initiator == WLAN_BACK_INITIATOR)
> +		ieee80211_sta_stop_rx_BA_session(dev, sta->addr, tid,
> +						 WLAN_BACK_INITIATOR, 0);

Does this intentionally ignore the other cases, and if so, why?

johannes

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

Re: [PATCH 7/8] mac80211: A-MPDU Rx handling DELBA requests

[Ron Rindjunsky]

> > +
> > +     if (initiator == WLAN_BACK_INITIATOR)
> > +             ieee80211_sta_stop_rx_BA_session(dev, sta->addr, tid,
> > +                                              WLAN_BACK_INITIATOR, 0);
>
> Does this intentionally ignore the other cases, and if so, why?
>

yes, i am handling only Rx A-MPDU, so only initiator of the session
can send us delBA

Re: [PATCH 7/8] mac80211: A-MPDU Rx handling DELBA requests

[Johannes Berg]

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


> yes, i am handling only Rx A-MPDU, so only initiator of the session
> can send us delBA

Ok, thanks for the clarification. Should we log an error if somebody
else sends us one?

johannes

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

RE: [PATCH 7/8] mac80211: A-MPDU Rx handling DELBA requests

[Rindjunsky, Ron]

>> yes, i am handling only Rx A-MPDU, so only initiator of the session
>> can send us delBA

> Ok, thanks for the clarification. Should we log an error if somebody
> else sends us one?

I don't think so. I have a debug print already there, and unless we deal
with an evil peer it won't happen. If we do deal with an evil opponent I
think it falls into TGw area.

> johannes
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 8/8] iwlwifi: A-MPDU Rx flow enabled

[Ron Rindjunsky]

This patch enables the A-MPDU Rx flow. it contains several
adjustments to new mac80211 A-MPDU Rx flow.

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 drivers/net/wireless/iwlwifi/iwl-4965.c     |   63 +++++++++++++--------------
 drivers/net/wireless/iwlwifi/iwl-4965.h     |    6 +--
 drivers/net/wireless/iwlwifi/iwl4965-base.c |    3 +-
 3 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index f65fd6e..38826c7 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -3868,9 +3868,7 @@ static void iwl4965_rx_reply_rx(struct iwl4965_priv *priv,
 		.antenna = 0,
 		.rate = iwl4965_hw_get_rate(rx_start->rate_n_flags),
 		.flag = 0,
-#ifdef CONFIG_IWL4965_HT_AGG
 		.ordered = 0
-#endif /* CONFIG_IWL4965_HT_AGG */
 	};
 	u8 network_packet;
 
@@ -4065,7 +4063,7 @@ static void iwl4965_rx_reply_rx(struct iwl4965_priv *priv,
 		break;
 
 	case IEEE80211_FTYPE_CTL:
-#ifdef CONFIG_IWL4965_HT_AGG
+#ifdef CONFIG_IWL4965_HT
 		switch (fc & IEEE80211_FCTL_STYPE) {
 		case IEEE80211_STYPE_BACK_REQ:
 			IWL_DEBUG_HT("IEEE80211_STYPE_BACK_REQ arrived\n");
@@ -4076,7 +4074,6 @@ static void iwl4965_rx_reply_rx(struct iwl4965_priv *priv,
 			break;
 		}
 #endif
-
 		break;
 
 	case IEEE80211_FTYPE_DATA: {
@@ -4663,8 +4660,6 @@ void iwl4965_set_ht_add_station(struct iwl4965_priv *priv, u8 index,
 	return;
 }
 
-#ifdef CONFIG_IWL4965_HT_AGG
-
 static void iwl4965_sta_modify_add_ba_tid(struct iwl4965_priv *priv,
 					  int sta_id, int tid, u16 ssn)
 {
@@ -4696,6 +4691,35 @@ static void iwl4965_sta_modify_del_ba_tid(struct iwl4965_priv *priv,
 	iwl4965_send_add_station(priv, &priv->stations[sta_id].sta, CMD_ASYNC);
 }
 
+int iwl4965_mac_ampdu_action(struct ieee80211_hw *hw, u8 action,
+			     const u8 *addr, u16 tid, u16 ssn)
+{
+	struct iwl4965_priv *priv = hw->priv;
+	int sta_id;
+	DECLARE_MAC_BUF(mac);
+
+	IWL_DEBUG_HT("A-MPDU action on da=%s tid=%d ",
+			print_mac(mac, addr), tid);
+	sta_id = iwl4965_hw_find_station(priv, addr);
+	switch (action) {
+	case IEEE80211_AMPDU_RX_START:
+		IWL_DEBUG_HT("start Rx\n");
+		iwl4965_sta_modify_add_ba_tid(priv, sta_id, tid, ssn);
+		break;
+	case IEEE80211_AMPDU_RX_STOP:
+		IWL_DEBUG_HT("stop Rx\n");
+		iwl4965_sta_modify_del_ba_tid(priv, sta_id, tid);
+		break;
+	default:
+		IWL_DEBUG_HT("unknown\n");
+		return -EINVAL;
+		break;
+	}
+	return 0;
+}
+
+#ifdef CONFIG_IWL4965_HT_AGG
+
 static const u16 default_tid_to_tx_fifo[] = {
 	IWL_TX_FIFO_AC1,
 	IWL_TX_FIFO_AC0,
@@ -4825,33 +4849,6 @@ int iwl4965_mac_ht_tx_agg_stop(struct ieee80211_hw *hw, u8 *da, u16 tid,
 	return 0;
 }
 
-int iwl4965_mac_ht_rx_agg_start(struct ieee80211_hw *hw, u8 *da,
-			    u16 tid, u16 start_seq_num)
-{
-	struct iwl4965_priv *priv = hw->priv;
-	int sta_id;
-	DECLARE_MAC_BUF(mac);
-
-	IWL_WARNING("iwl-AGG iwl4965_mac_ht_rx_agg_start on da=%s"
-		    " tid=%d\n", print_mac(mac, da), tid);
-	sta_id = iwl4965_hw_find_station(priv, da);
-	iwl4965_sta_modify_add_ba_tid(priv, sta_id, tid, start_seq_num);
-	return 0;
-}
-
-int iwl4965_mac_ht_rx_agg_stop(struct ieee80211_hw *hw, u8 *da,
-			   u16 tid, int generator)
-{
-	struct iwl4965_priv *priv = hw->priv;
-	int sta_id;
-	DECLARE_MAC_BUF(mac);
-
-	IWL_WARNING("iwl-AGG iwl4965_mac_ht_rx_agg_stop on da=%s tid=%d\n",
-		    print_mac(mac, da), tid);
-	sta_id = iwl4965_hw_find_station(priv, da);
-	iwl4965_sta_modify_del_ba_tid(priv, sta_id, tid);
-	return 0;
-}
 
 #endif /* CONFIG_IWL4965_HT_AGG */
 #endif /* CONFIG_IWL4965_HT */
diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.h b/drivers/net/wireless/iwlwifi/iwl-4965.h
index 267ae75..b8fd5d0 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.h
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.h
@@ -802,13 +802,11 @@ extern void iwl4965_set_rxon_ht(struct iwl4965_priv *priv,
 				struct iwl_ht_info *ht_info);
 extern void iwl4965_set_ht_add_station(struct iwl4965_priv *priv, u8 index,
 				struct ieee80211_ht_info *sta_ht_inf);
+extern int iwl4965_mac_ampdu_action(struct ieee80211_hw *hw, u8 action,
+				    const u8 *addr, u16 tid, u16 ssn);
 #ifdef CONFIG_IWL4965_HT_AGG
 extern int iwl4965_mac_ht_tx_agg_start(struct ieee80211_hw *hw, u8 *da,
 				   u16 tid, u16 *start_seq_num);
-extern int iwl4965_mac_ht_rx_agg_start(struct ieee80211_hw *hw, u8 *da,
-				   u16 tid, u16 start_seq_num);
-extern int iwl4965_mac_ht_rx_agg_stop(struct ieee80211_hw *hw, u8 *da,
-				  u16 tid, int generator);
 extern int iwl4965_mac_ht_tx_agg_stop(struct ieee80211_hw *hw, u8 *da,
 				  u16 tid, int generator);
 extern void iwl4965_turn_off_agg(struct iwl4965_priv *priv, u8 tid);
diff --git a/drivers/net/wireless/iwlwifi/iwl4965-base.c b/drivers/net/wireless/iwlwifi/iwl4965-base.c
index f14c44a..18c19e5 100644
--- a/drivers/net/wireless/iwlwifi/iwl4965-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl4965-base.c
@@ -9051,11 +9051,10 @@ static struct ieee80211_ops iwl4965_hw_ops = {
 	.erp_ie_changed = iwl4965_mac_erp_ie_changed,
 #ifdef CONFIG_IWL4965_HT
 	.conf_ht = iwl4965_mac_conf_ht,
+	.ampdu_action = iwl4965_mac_ampdu_action,
 #ifdef CONFIG_IWL4965_HT_AGG
 	.ht_tx_agg_start = iwl4965_mac_ht_tx_agg_start,
 	.ht_tx_agg_stop = iwl4965_mac_ht_tx_agg_stop,
-	.ht_rx_agg_start = iwl4965_mac_ht_rx_agg_start,
-	.ht_rx_agg_stop = iwl4965_mac_ht_rx_agg_stop,
 #endif  /* CONFIG_IWL4965_HT_AGG */
 #endif  /* CONFIG_IWL4965_HT */
 	.hw_scan = iwl4965_mac_hw_scan
-- 
1.5.3.3

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.