[patch 3/5] A-MSDU Rx aggregation support

[patch 3/5] A-MSDU Rx aggregation support

[mohamed]

Add A-MSDU Rx aggregation support.

To support IEEE 802.11n, we need to be able to process A-MSDU frames. 
The present of the HT control field indicates it is A-MSDU frame. 
This patch adds support to discover and process A-MSDU frames.

Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>

diff -Nupr wireless-dev/net/mac80211/ieee80211.c
wireless-dev-new/net/mac80211/ieee80211.c
--- wireless-dev/net/mac80211/ieee80211.c	2007-03-27 00:36:24.000000000
-0700
+++ wireless-dev-new/net/mac80211/ieee80211.c	2007-03-27
01:54:48.000000000 -0700
@@ -2446,6 +2446,143 @@ static inline int ieee80211_bssid_match(
 }
 
 
+inline static unsigned int calc_pad_len(unsigned int len) 
+{ 
+	return (4 - len % 4) % 4; 
+}
+
+inline static int is_agg_frame(struct sk_buff *skb, u16 fc)
+{
+	u8 *p = skb->data + ieee80211_get_hdrlen(fc) - 2;
+
+	if (!(fc & IEEE80211_STYPE_QOS_DATA));
+		return 0;
+
+	if (*p & QOS_CONTROL_A_MSDU_PRESENT)
+		return 1;
+	else
+		return 0;
+}
+
+static ieee80211_txrx_result
+ieee80211_rx_h_data_agg(struct ieee80211_txrx_data *rx)
+{
+	struct net_device *dev = rx->dev;
+	struct ieee80211_local *local = rx->local;
+	u16 fc, hdrlen, ethertype;
+	u8 *payload;
+	struct sk_buff *skb = rx->skb, *skb2, *frame;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	const struct ethhdr* eth;
+
+	fc = rx->fc;
+	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
+		return TXRX_CONTINUE;
+
+	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
+		return TXRX_DROP;
+
+	if (!is_agg_frame(skb, fc))
+		return TXRX_CONTINUE;
+
+	hdrlen = ieee80211_get_hdrlen(fc);
+
+	payload = skb->data + hdrlen;
+
+	if (unlikely(skb->len - hdrlen < 8)) {
+		if (net_ratelimit()) {
+			printk(KERN_DEBUG "%s: RX too short data frame "
+				"payload\n", dev->name);
+		}
+		return TXRX_DROP;
+	}
+
+	ethertype = (payload[6] << 8) | payload[7];
+
+	if (likely((memcmp(payload, rfc1042_header, 6) == 0 &&
+		ethertype != ETH_P_AARP && ethertype != ETH_P_IPX) ||
+		memcmp(payload, bridge_tunnel_header, 6) == 0)) {
+		/* remove RFC1042 or Bridge-Tunnel encapsulation and
+		* replace EtherType */	
+		skb_pull(skb, hdrlen + 6);
+	} else {
+		skb_pull(skb, hdrlen);
+	}
+
+	eth = (struct ethhdr*)skb->data;
+
+	while ((u8*)eth < skb->data + skb->len) {
+        	unsigned int eth_len = sizeof(struct ethhdr) + 
+						ntohs(eth->h_proto);
+        
+		frame = dev_alloc_skb(3000);
+
+		if (frame == NULL) 
+			return TXRX_DROP;
+
+		frame->mac.raw = frame->data;
+		memcpy(skb_put(frame, eth_len), (u8*)eth, eth_len);
+
+		frame->dev = dev;
+
+		skb2 = NULL;
+
+		sdata->stats.rx_packets++;
+		sdata->stats.rx_bytes += frame->len;
+
+		if (local->bridge_packets && 
+		    (sdata->type == IEEE80211_IF_TYPE_AP ||
+		     sdata->type == IEEE80211_IF_TYPE_VLAN) && 
+                    rx->u.rx.ra_match) {
+			if (is_multicast_ether_addr(frame->data)) {
+				/* send multicast frames both to higher layers
+				* in local net stack and back to the wireless 
+				* media */
+				skb2 = skb_copy(frame, GFP_ATOMIC);
+				if (!skb2)
+					printk(KERN_DEBUG "%s: failed to clone "
+					"multicast frame\n", dev->name);
+			} else {
+				struct sta_info *dsta;
+				dsta = sta_info_get(local, frame->data);
+				if (dsta && !dsta->dev) {
+					printk(KERN_DEBUG "Station with null "
+					"dev structure!\n");
+                        	} else if (dsta && dsta->dev == dev) {
+                                	/* Destination station is associated 
+					* to this AP, so send the frame
+					* directly to it and do not pass 
+					* the frame to local net stack.
+					*/
+					skb2 = skb;
+					skb = NULL;
+				}
+				if (dsta)
+					sta_info_put(dsta);
+			}
+		}
+		if (frame) {
+			/* deliver to local stack */
+			frame->protocol = eth_type_trans(frame, dev);
+			memset(frame->cb, 0, sizeof(frame->cb));
+			netif_rx(frame);
+		}
+
+		if (skb2) {
+			/* send to wireless media */
+			skb2->protocol = __constant_htons(ETH_P_802_3);
+			skb2->mac.raw = skb2->nh.raw = skb2->data;
+			dev_queue_xmit(skb2);
+		}
+
+		eth = (struct ethhdr*)((u8*)eth + eth_len 
+					+ calc_pad_len(eth_len));
+	}
+
+	dev_kfree_skb(skb);
+        return TXRX_QUEUED;
+}
+
 static ieee80211_txrx_result
 ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 {
@@ -4373,6 +4510,7 @@ static ieee80211_rx_handler ieee80211_rx
 	ieee80211_rx_h_remove_qos_control,
 	ieee80211_rx_h_802_1x_pae,
 	ieee80211_rx_h_drop_unencrypted,
+	ieee80211_rx_h_data_agg,
 	ieee80211_rx_h_data,
 	ieee80211_rx_h_mgmt,
 	NULL
diff -Nupr wireless-dev/net/mac80211/wme.h
wireless-dev-new/net/mac80211/wme.h
--- wireless-dev/net/mac80211/wme.h	2007-03-22 11:40:17.000000000 -0700
+++ wireless-dev-new/net/mac80211/wme.h	2007-03-27 01:48:34.000000000
-0700
@@ -24,6 +24,8 @@
 
 #define QOS_CONTROL_TAG1D_MASK 0x07
 
+#define QOS_CONTROL_A_MSDU_PRESENT 0x80
+
 ieee80211_txrx_result
 ieee80211_rx_h_parse_qos(struct ieee80211_txrx_data *rx);
 

Re: [patch 3/5] A-MSDU Rx aggregation support

[Randy Dunlap]

On Mon, 26 Mar 2007 04:40:00 -0700 mohamed wrote:

> Add A-MSDU Rx aggregation support.
> 
> To support IEEE 802.11n, we need to be able to process A-MSDU frames. 
> The present of the HT control field indicates it is A-MSDU frame. 
> This patch adds support to discover and process A-MSDU frames.
> 
> Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>
> 
> diff -Nupr wireless-dev/net/mac80211/ieee80211.c
> wireless-dev-new/net/mac80211/ieee80211.c
> --- wireless-dev/net/mac80211/ieee80211.c	2007-03-27 00:36:24.000000000
> -0700
> +++ wireless-dev-new/net/mac80211/ieee80211.c	2007-03-27
> 01:54:48.000000000 -0700
> @@ -2446,6 +2446,143 @@ static inline int ieee80211_bssid_match(
>  }
>  
>  
> +inline static unsigned int calc_pad_len(unsigned int len) 
> +{ 
> +	return (4 - len % 4) % 4; 

Oops, line ends with a space.  Please, no lines that end with
space or tab.

Is gcc doing shifts for these % operations?  Hope so.

> +}
> +
> +inline static int is_agg_frame(struct sk_buff *skb, u16 fc)
> +{
> +	u8 *p = skb->data + ieee80211_get_hdrlen(fc) - 2;
> +
> +	if (!(fc & IEEE80211_STYPE_QOS_DATA));

line ends with ';' ???

> +		return 0;
> +
> +	if (*p & QOS_CONTROL_A_MSDU_PRESENT)
> +		return 1;
> +	else

'else' not needed (just a style thing)

> +		return 0;
> +}
> +
> +static ieee80211_txrx_result
> +ieee80211_rx_h_data_agg(struct ieee80211_txrx_data *rx)
> +{
> +	struct net_device *dev = rx->dev;
> +	struct ieee80211_local *local = rx->local;
> +	u16 fc, hdrlen, ethertype;
> +	u8 *payload;
> +	struct sk_buff *skb = rx->skb, *skb2, *frame;
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +	const struct ethhdr* eth;
> +
> +	fc = rx->fc;
> +	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
> +		return TXRX_CONTINUE;
> +
> +	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
> +		return TXRX_DROP;
> +
> +	if (!is_agg_frame(skb, fc))
> +		return TXRX_CONTINUE;
> +
> +	hdrlen = ieee80211_get_hdrlen(fc);
> +
> +	payload = skb->data + hdrlen;
> +
> +	if (unlikely(skb->len - hdrlen < 8)) {
> +		if (net_ratelimit()) {
> +			printk(KERN_DEBUG "%s: RX too short data frame "
> +				"payload\n", dev->name);
> +		}
> +		return TXRX_DROP;
> +	}
> +
> +	ethertype = (payload[6] << 8) | payload[7];
> +
> +	if (likely((memcmp(payload, rfc1042_header, 6) == 0 &&
> +		ethertype != ETH_P_AARP && ethertype != ETH_P_IPX) ||
> +		memcmp(payload, bridge_tunnel_header, 6) == 0)) {
> +		/* remove RFC1042 or Bridge-Tunnel encapsulation and
> +		* replace EtherType */	
> +		skb_pull(skb, hdrlen + 6);
> +	} else {
> +		skb_pull(skb, hdrlen);
> +	}

No braces on single-statement "blocks".  (happens elsewhere also)

> +
> +	eth = (struct ethhdr*)skb->data;
> +
> +	while ((u8*)eth < skb->data + skb->len) {
> +        	unsigned int eth_len = sizeof(struct ethhdr) + 
> +						ntohs(eth->h_proto);
> +        
> +		frame = dev_alloc_skb(3000);

Why 3000?  can it be a well-defined #define??

> +
> +		if (frame == NULL) 
> +			return TXRX_DROP;
> +
> +		frame->mac.raw = frame->data;
> +		memcpy(skb_put(frame, eth_len), (u8*)eth, eth_len);
> +
> +		frame->dev = dev;
> +
> +		skb2 = NULL;
> +
> +		sdata->stats.rx_packets++;
> +		sdata->stats.rx_bytes += frame->len;
> +
> +		if (local->bridge_packets && 
> +		    (sdata->type == IEEE80211_IF_TYPE_AP ||
> +		     sdata->type == IEEE80211_IF_TYPE_VLAN) && 
> +                    rx->u.rx.ra_match) {
> +			if (is_multicast_ether_addr(frame->data)) {
> +				/* send multicast frames both to higher layers
> +				* in local net stack and back to the wireless 
> +				* media */
> +				skb2 = skb_copy(frame, GFP_ATOMIC);
> +				if (!skb2)
> +					printk(KERN_DEBUG "%s: failed to clone "
> +					"multicast frame\n", dev->name);

more indentation on last part.

> +			} else {
> +				struct sta_info *dsta;
> +				dsta = sta_info_get(local, frame->data);
> +				if (dsta && !dsta->dev) {
> +					printk(KERN_DEBUG "Station with null "
> +					"dev structure!\n");

Indentation.  no braces.

> +                        	} else if (dsta && dsta->dev == dev) {
> +                                	/* Destination station is associated 
> +					* to this AP, so send the frame
> +					* directly to it and do not pass 
> +					* the frame to local net stack.
> +					*/
> +					skb2 = skb;
> +					skb = NULL;
> +				}
> +				if (dsta)
> +					sta_info_put(dsta);
> +			}
> +		}
> +		if (frame) {
> +			/* deliver to local stack */
> +			frame->protocol = eth_type_trans(frame, dev);
> +			memset(frame->cb, 0, sizeof(frame->cb));
> +			netif_rx(frame);
> +		}
> +
> +		if (skb2) {
> +			/* send to wireless media */
> +			skb2->protocol = __constant_htons(ETH_P_802_3);
> +			skb2->mac.raw = skb2->nh.raw = skb2->data;
> +			dev_queue_xmit(skb2);
> +		}
> +
> +		eth = (struct ethhdr*)((u8*)eth + eth_len 
> +					+ calc_pad_len(eth_len));
> +	}
> +
> +	dev_kfree_skb(skb);
> +        return TXRX_QUEUED;

Inconsisten indentation above.  'return' line is using spaces
instead of a tab.

> +}
> +
>  static ieee80211_txrx_result
>  ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  {


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch 3/5] A-MSDU Rx aggregation support

[Randy Dunlap]

mohamed wrote:
> On Mon, 2007-03-26 at 16:36 -0700, Randy Dunlap wrote:
>> On Mon, 26 Mar 2007 04:40:00 -0700 mohamed wrote:
>>
>>> Add A-MSDU Rx aggregation support.
>>>
>>> To support IEEE 802.11n, we need to be able to process A-MSDU frames. 
>>> The present of the HT control field indicates it is A-MSDU frame. 
>>> This patch adds support to discover and process A-MSDU frames.
>>>
>>> Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>

>>> +	while ((u8*)eth < skb->data + skb->len) {
>>> +        	unsigned int eth_len = sizeof(struct ethhdr) + 
>>> +						ntohs(eth->h_proto);
>>> +        
>>> +		frame = dev_alloc_skb(3000);
>> Why 3000?  can it be a well-defined #define??
> 3000 just a safe number I picked up. I will change to 
> dev_alloc_skb(local->hw.extra_tx_headroom + eth_len);
> Are any more space needed?

Nope, that looks fine.  Thanks.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

On Mon, 2007-03-26 at 04:40 -0700, mohamed wrote:

> +	ethertype = (payload[6] << 8) | payload[7];
> +
> +	if (likely((memcmp(payload, rfc1042_header, 6) == 0 &&
> +		ethertype != ETH_P_AARP && ethertype != ETH_P_IPX) ||
> +		memcmp(payload, bridge_tunnel_header, 6) == 0)) {
> +		/* remove RFC1042 or Bridge-Tunnel encapsulation and
> +		* replace EtherType */	
> +		skb_pull(skb, hdrlen + 6);
> +	} else {
> +		skb_pull(skb, hdrlen);
> +	}
> +
> +	eth = (struct ethhdr*)skb->data;

If you put the eth = stuff before ethertype = you can make this code
something like ethertype = be16_to_cpu(eth->h_proto) instead of the
hardcoded endianness conversion. And the memcmp() could be
compare_ether_addr(eth->h_dest, rfc1042_header) instead.

> +	while ((u8*)eth < skb->data + skb->len) {

I don't understand this cast of eth to u8*. That seems wrong given that
there's an ethhdr there where the first byte isn't a length byte.

> +        	unsigned int eth_len = sizeof(struct ethhdr) + 
> +						ntohs(eth->h_proto);
> +        
> +		frame = dev_alloc_skb(3000);

Maybe there's a way to get an skb that actually points into the payload?
Not sure that is desirable though we really should optimise the receive
path.. it does far too many copies already.


I'll look some things later again, have to go now.

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

On Mon, 2007-03-26 at 04:40 -0700, mohamed wrote:

Scratch my previous comments. Sorry about that, I should've looked
better.

> +inline static int is_agg_frame(struct sk_buff *skb, u16 fc)
> +{
> +       u8 *p = skb->data + ieee80211_get_hdrlen(fc) - 2;

See below, but isn't this broken if somebody includes an HT field?

> +       if (!(fc & IEEE80211_STYPE_QOS_DATA));
> +               return 0;
> +
> +       if (*p & QOS_CONTROL_A_MSDU_PRESENT)
> +               return 1;
> +       else
> +               return 0;
> +}

Hmm. Can we add the appropriate stuff to struct ieee80211_hdr in
include/linux/ieee80211.h and use that? Like 

struct ieee80211_hdr *hdr = skb->data;

if (!(fc & ...))
	return 0;
if (le16_to_cpu(hdr->qos_control) & QOS_CONTROL_A_MSDU_PRESENT)
	return 1;
return 0;

Then again, this is a problem with the addr4 format, we'll probably have
to split up ieee80211_hdr into a 3-addr and a 4-addr format, or use a
union like this:

struct ieee80211_hdr {
        __le16 frame_control;
        __le16 duration_id;
        __u8 addr1[6];
        __u8 addr2[6];
        __u8 addr3[6];
        __le16 seq_ctrl;
	union {
	        __u8 addr4[6];	/* keep for easier access */
		struct {
			__u8 addr4[6];
			__le16 qos;
			__le32 ht;
		} _4addr;
		struct {
			__le16 qos;
			__le32 ht;
		} _3addr;
	}
} __attribute__ ((packed));

Or something like that. Can you get frames w/o qos information but with
ht info? If so, we'll need even more cases here :/

Then again, how about we simply add a few inlines like
__le16 ieee80211_get_qos() that gives you the qos field etc, and then
use those. This seems dangerous anyway considering the possible presence
of the HT field.

> +	hdrlen = ieee80211_get_hdrlen(fc);

I haven't seen a patch to ieee80211_get_hdrlen to add the HT field.
Isn't that missing?

> +	payload = skb->data + hdrlen;

Ok so payload now points to the subframes.

> +	if (unlikely(skb->len - hdrlen < 8)) {
> +		if (net_ratelimit()) {
> +			printk(KERN_DEBUG "%s: RX too short data frame "
> +				"payload\n", dev->name);

Nit: this should increase the counter we have for too short frames
somewhere.

> +		}
> +		return TXRX_DROP;
> +	}
> +
> +	ethertype = (payload[6] << 8) | payload[7];

Is that really correct?

> +	if (likely((memcmp(payload, rfc1042_header, 6) == 0 &&
> +		ethertype != ETH_P_AARP && ethertype != ETH_P_IPX) ||
> +		memcmp(payload, bridge_tunnel_header, 6) == 0)) {
> +		/* remove RFC1042 or Bridge-Tunnel encapsulation and
> +		* replace EtherType */	
> +		skb_pull(skb, hdrlen + 6);
> +	} else {
> +		skb_pull(skb, hdrlen);
> +	}
> +
> +	eth = (struct ethhdr*)skb->data;

So that now points to the first actual subframe ethhdr. I misread that
in my first comment. Why do you actually skb_pull on this skb? It would
probably be more efficient to just assign "eth" in the different cases
since that needs no memory write (eth will probably be kept in a
register). And you throw it away anyway...

> +	while ((u8*)eth < skb->data + skb->len) {

And that compares the pointer, not the value at it as I thought. Sorry.

> +        	unsigned int eth_len = sizeof(struct ethhdr) + 
> +						ntohs(eth->h_proto);

rename to subframe_len maybe?

> +		frame = dev_alloc_skb(3000);
> +
> +		if (frame == NULL) 
> +			return TXRX_DROP;
> +
> +		frame->mac.raw = frame->data;
> +		memcpy(skb_put(frame, eth_len), (u8*)eth, eth_len);

Nope, not nice to do this. You really need to check if eth_len doesn't
exceed the original skb's length or people can crash this by sending you
short frames with bogus huge eth len embedded in it. And if you move the
check up a bit then you can reuse "skb" for the last subframe in the
case where people don't send us garbage:

	int remaining = subframe_len - ((u8*)eth - skb->data);
	if (remaining < 0)
		/* idiots. should blacklist their mac address */
		return TXRX_DROP;
	if (remaining < 4 /* padding */) {
		frame = skb;
		skb = NULL;
		skb_pull(blabla)
	} else {
		frame = dev_alloc_skb(...)
		if (!frame)
			return TXRX_DROP;
	}

or something like that.

Don't you also need to set ((ethhdr*)frame->data)->h_proto to something
other than the length? Not exactly sure though, it seems eth_type_trans
can handle that and I don't have the 802.3 specs handy.

> +		frame->dev = dev;

I think it'd be good to move that closer to the netif_rx.

> +
> +		skb2 = NULL;
> +
> +		sdata->stats.rx_packets++;
> +		sdata->stats.rx_bytes += frame->len;
> +
> +		if (local->bridge_packets && 
> +		    (sdata->type == IEEE80211_IF_TYPE_AP ||
> +		     sdata->type == IEEE80211_IF_TYPE_VLAN) && 
> +                    rx->u.rx.ra_match) {

> +			if (is_multicast_ether_addr(frame->data)) {

Use a new ethhdr* variable for that, like subframe_hdr, and then use
subframe_hdr->h_dest. Makes it much clearer what's going on with these
subframes.

> +				/* send multicast frames both to higher layers
> +				* in local net stack and back to the wireless 
> +				* media */
> +				skb2 = skb_copy(frame, GFP_ATOMIC);
> +				if (!skb2)
> +					printk(KERN_DEBUG "%s: failed to clone "
> +					"multicast frame\n", dev->name);
> +			} else {
> +				struct sta_info *dsta;
> +				dsta = sta_info_get(local, frame->data);
> +				if (dsta && !dsta->dev) {
> +					printk(KERN_DEBUG "Station with null "
> +					"dev structure!\n");
> +                        	} else if (dsta && dsta->dev == dev) {
> +                                	/* Destination station is associated 
> +					* to this AP, so send the frame
> +					* directly to it and do not pass 
> +					* the frame to local net stack.
> +					*/
> +					skb2 = skb;
> +					skb = NULL;

I'm pretty sure you mean frame here instead of skb in both cases and
this is a copy/paste error from the other data rx handler.

> +				}
> +				if (dsta)
> +					sta_info_put(dsta);
> +			}
> +		}
> +		if (frame) {
> +			/* deliver to local stack */
> +			frame->protocol = eth_type_trans(frame, dev);
> +			memset(frame->cb, 0, sizeof(frame->cb));

That memset is useless on a newly allocated frame.

> +			netif_rx(frame);
> +		}
> +
> +		if (skb2) {
> +			/* send to wireless media */
> +			skb2->protocol = __constant_htons(ETH_P_802_3);
> +			skb2->mac.raw = skb2->nh.raw = skb2->data;
> +			dev_queue_xmit(skb2);
> +		}
> +
> +		eth = (struct ethhdr*)((u8*)eth + eth_len 
> +					+ calc_pad_len(eth_len));
> +	}
> +
> +	dev_kfree_skb(skb);
> +        return TXRX_QUEUED;
> +}
> +
>  static ieee80211_txrx_result
>  ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  {
> @@ -4373,6 +4510,7 @@ static ieee80211_rx_handler ieee80211_rx
>  	ieee80211_rx_h_remove_qos_control,
>  	ieee80211_rx_h_802_1x_pae,
>  	ieee80211_rx_h_drop_unencrypted,
> +	ieee80211_rx_h_data_agg,
>  	ieee80211_rx_h_data,
>  	ieee80211_rx_h_mgmt,
>  	NULL
> diff -Nupr wireless-dev/net/mac80211/wme.h
> wireless-dev-new/net/mac80211/wme.h
> --- wireless-dev/net/mac80211/wme.h	2007-03-22 11:40:17.000000000 -0700
> +++ wireless-dev-new/net/mac80211/wme.h	2007-03-27 01:48:34.000000000
> -0700
> @@ -24,6 +24,8 @@
>  
>  #define QOS_CONTROL_TAG1D_MASK 0x07
>  
> +#define QOS_CONTROL_A_MSDU_PRESENT 0x80

Hm. Can we put all these things into <linux/ieee80211.h> instead? I'm ok
with you putting it here for now, we'll just move them later. Or you can
add a patch to your series before this one that already moves the
defines and adds this one.

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[Michael Buesch]

On Wednesday 28 March 2007 20:23, Johannes Berg wrote:
> > +	while ((u8*)eth < skb->data + skb->len) {
> 
> I don't understand this cast of eth to u8*. That seems wrong given that
> there's an ethhdr there where the first byte isn't a length byte.

We're not comparing the data eth points to here, but the eth pointer
itself. It's pretty weird code. Maybe this can be solved in a
more clear way. But it might be correct as-is.

-- 
Greetings Michael.

Re: [patch 3/5] A-MSDU Rx aggregation support

[mohamed]

On Mon, 2007-03-26 at 16:36 -0700, Randy Dunlap wrote:
> On Mon, 26 Mar 2007 04:40:00 -0700 mohamed wrote:
> 
> > Add A-MSDU Rx aggregation support.
> > 
> > To support IEEE 802.11n, we need to be able to process A-MSDU frames. 
> > The present of the HT control field indicates it is A-MSDU frame. 
> > This patch adds support to discover and process A-MSDU frames.
> > 
> > Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>
> > 
> > diff -Nupr wireless-dev/net/mac80211/ieee80211.c
> > wireless-dev-new/net/mac80211/ieee80211.c
> > --- wireless-dev/net/mac80211/ieee80211.c	2007-03-27 00:36:24.000000000
> > -0700
> > +++ wireless-dev-new/net/mac80211/ieee80211.c	2007-03-27
> > 01:54:48.000000000 -0700
> > @@ -2446,6 +2446,143 @@ static inline int ieee80211_bssid_match(
> >  }
> >  
> >  
> > +inline static unsigned int calc_pad_len(unsigned int len) 
> > +{ 
> > +	return (4 - len % 4) % 4; 
> 
> Oops, line ends with a space.  Please, no lines that end with
> space or tab.
I will clean all style issues. 
> 
> Is gcc doing shifts for these % operations?  Hope so.
> 
> > +}
> > +
> > +inline static int is_agg_frame(struct sk_buff *skb, u16 fc)
> > +{
> > +	u8 *p = skb->data + ieee80211_get_hdrlen(fc) - 2;
> > +
> > +	if (!(fc & IEEE80211_STYPE_QOS_DATA));
> 
> line ends with ';' ???
> 
> > +		return 0;
> > +
> > +	if (*p & QOS_CONTROL_A_MSDU_PRESENT)
> > +		return 1;
> > +	else
> 
> 'else' not needed (just a style thing)
> 
> > +		return 0;
> > +}
> > +
> > +static ieee80211_txrx_result
> > +ieee80211_rx_h_data_agg(struct ieee80211_txrx_data *rx)
> > +{
> > +	struct net_device *dev = rx->dev;
> > +	struct ieee80211_local *local = rx->local;
> > +	u16 fc, hdrlen, ethertype;
> > +	u8 *payload;
> > +	struct sk_buff *skb = rx->skb, *skb2, *frame;
> > +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > +	const struct ethhdr* eth;
> > +
> > +	fc = rx->fc;
> > +	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
> > +		return TXRX_CONTINUE;
> > +
> > +	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
> > +		return TXRX_DROP;
> > +
> > +	if (!is_agg_frame(skb, fc))
> > +		return TXRX_CONTINUE;
> > +
> > +	hdrlen = ieee80211_get_hdrlen(fc);
> > +
> > +	payload = skb->data + hdrlen;
> > +
> > +	if (unlikely(skb->len - hdrlen < 8)) {
> > +		if (net_ratelimit()) {
> > +			printk(KERN_DEBUG "%s: RX too short data frame "
> > +				"payload\n", dev->name);
> > +		}
> > +		return TXRX_DROP;
> > +	}
> > +
> > +	ethertype = (payload[6] << 8) | payload[7];
> > +
> > +	if (likely((memcmp(payload, rfc1042_header, 6) == 0 &&
> > +		ethertype != ETH_P_AARP && ethertype != ETH_P_IPX) ||
> > +		memcmp(payload, bridge_tunnel_header, 6) == 0)) {
> > +		/* remove RFC1042 or Bridge-Tunnel encapsulation and
> > +		* replace EtherType */	
> > +		skb_pull(skb, hdrlen + 6);
> > +	} else {
> > +		skb_pull(skb, hdrlen);
> > +	}
> 
> No braces on single-statement "blocks".  (happens elsewhere also)
> 
> > +
> > +	eth = (struct ethhdr*)skb->data;
> > +
> > +	while ((u8*)eth < skb->data + skb->len) {
> > +        	unsigned int eth_len = sizeof(struct ethhdr) + 
> > +						ntohs(eth->h_proto);
> > +        
> > +		frame = dev_alloc_skb(3000);
> 
> Why 3000?  can it be a well-defined #define??
3000 just a safe number I picked up. I will change to 
dev_alloc_skb(local->hw.extra_tx_headroom + eth_len);
Are any more space needed?

> 
> > +
> > +		if (frame == NULL) 
> > +			return TXRX_DROP;
> > +
> > +		frame->mac.raw = frame->data;
> > +		memcpy(skb_put(frame, eth_len), (u8*)eth, eth_len);
> > +
> > +		frame->dev = dev;
> > +
> > +		skb2 = NULL;
> > +
> > +		sdata->stats.rx_packets++;
> > +		sdata->stats.rx_bytes += frame->len;
> > +
> > +		if (local->bridge_packets && 
> > +		    (sdata->type == IEEE80211_IF_TYPE_AP ||
> > +		     sdata->type == IEEE80211_IF_TYPE_VLAN) && 
> > +                    rx->u.rx.ra_match) {
> > +			if (is_multicast_ether_addr(frame->data)) {
> > +				/* send multicast frames both to higher layers
> > +				* in local net stack and back to the wireless 
> > +				* media */
> > +				skb2 = skb_copy(frame, GFP_ATOMIC);
> > +				if (!skb2)
> > +					printk(KERN_DEBUG "%s: failed to clone "
> > +					"multicast frame\n", dev->name);
> 
> more indentation on last part.
> 
> > +			} else {
> > +				struct sta_info *dsta;
> > +				dsta = sta_info_get(local, frame->data);
> > +				if (dsta && !dsta->dev) {
> > +					printk(KERN_DEBUG "Station with null "
> > +					"dev structure!\n");
> 
> Indentation.  no braces.
> 
> > +                        	} else if (dsta && dsta->dev == dev) {
> > +                                	/* Destination station is associated 
> > +					* to this AP, so send the frame
> > +					* directly to it and do not pass 
> > +					* the frame to local net stack.
> > +					*/
> > +					skb2 = skb;
> > +					skb = NULL;
> > +				}
> > +				if (dsta)
> > +					sta_info_put(dsta);
> > +			}
> > +		}
> > +		if (frame) {
> > +			/* deliver to local stack */
> > +			frame->protocol = eth_type_trans(frame, dev);
> > +			memset(frame->cb, 0, sizeof(frame->cb));
> > +			netif_rx(frame);
> > +		}
> > +
> > +		if (skb2) {
> > +			/* send to wireless media */
> > +			skb2->protocol = __constant_htons(ETH_P_802_3);
> > +			skb2->mac.raw = skb2->nh.raw = skb2->data;
> > +			dev_queue_xmit(skb2);
> > +		}
> > +
> > +		eth = (struct ethhdr*)((u8*)eth + eth_len 
> > +					+ calc_pad_len(eth_len));
> > +	}
> > +
> > +	dev_kfree_skb(skb);
> > +        return TXRX_QUEUED;
> 
> Inconsisten indentation above.  'return' line is using spaces
> instead of a tab.
> 
> > +}
> > +
> >  static ieee80211_txrx_result
> >  ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
> >  {
> 
> 
> ---
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

On Wed, 2007-03-28 at 23:08 +0200, Michael Buesch wrote:
> On Wednesday 28 March 2007 20:23, Johannes Berg wrote:
> > > +	while ((u8*)eth < skb->data + skb->len) {
> > 
> > I don't understand this cast of eth to u8*. That seems wrong given that
> > there's an ethhdr there where the first byte isn't a length byte.
> 
> We're not comparing the data eth points to here, but the eth pointer
> itself. It's pretty weird code. Maybe this can be solved in a
> more clear way. But it might be correct as-is.

Yeah, I read it wrong. It's fine, see my new mail.

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

Could you please refrain from using HTML mail? It's making my life
unnecessarily complex.

>         Or something like that. Can you get frames w/o qos information
>         but with
>         ht info? If so, we'll need even more cases here :/
> 
> No,  HT is QoS derived.

I thought so but wasn't sure if it was really always necessary to
include both.


> Yes this has to be put  into  ieee80211.h  -  and QoS is also 2 bytes
> so make it 0x0080 to stress it.
> HT It's defined be IEEE and not by WiFi (WME)

De facto standard...

Please trim your quotes.

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

On Mon, 2007-04-02 at 23:30 -0700, mabbas wrote:

> I am calling ieee80211_rx_h_data_agg after ieee80211_rx_h_remove_qos_control which
> clears all QoS data which include QOS_CONTROL_A_MSDU_PRESENT bit. We can solve this by
> adding new field is_frame_agg to ieee80211_txrx_data.u.rx.is_frame_agg which will be set in
> ieee80211_rx_h_parse_qos or we can add new function to ieee80211_rx_pre_handlers just to do that. the other 
> option is to shuffle calling sequence here if that possible to call ieee80211_rx_h_data_agg
> before ieee80211_rx_h_remove_qos_control. any opinion?

Since it's part of the QoS stuff that .11n adds I think we should stick
it into the qos parser.

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[Tomas Winkler]

11n is QoS derived but it's not WME.
I think we should extract qos control filed and ht control fields from
header in pre-handlers
one of them is qos parser,  just it needs to save the qos fields on
the txrx data
This will be needed in both rx and tx path. With 11n there are few
other points in the code  that these fields need to be accessed.
You can query fc  whether this fields are set.

ieee80211_txrx_data
      u16 fc, ethertype;
+    u16 qos_ctrl; /* cpu  order */
+    u16 ht_ctrl;

The other option is to introduce again all the types of headers
24,26...32.  I don' t know real reason why they are not present in
mac80211 but it looks intentional.

Next option to avoid the blow up of header types we can use offset functions
int ieee80211_get_qosctl_offset(u16 fc)
int ieee80211_get_htctl_offset(u16 fc)
This will give consistent offset of these control fields but it still
won't work when you use this remove qos field handler.

Other thing is to change ieee80211_get_hdrlen function as the header
is longer now by other 2 octets (ht control field)

On 4/2/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2007-04-02 at 23:30 -0700, mabbas wrote:
>
> > I am calling ieee80211_rx_h_data_agg after ieee80211_rx_h_remove_qos_control which
> > clears all QoS data which include QOS_CONTROL_A_MSDU_PRESENT bit. We can solve this by
> > adding new field is_frame_agg to ieee80211_txrx_data.u.rx.is_frame_agg which will be set in
> > ieee80211_rx_h_parse_qos or we can add new function to ieee80211_rx_pre_handlers just to do that. the other
> > option is to shuffle calling sequence here if that possible to call ieee80211_rx_h_data_agg
> > before ieee80211_rx_h_remove_qos_control. any opinion?
>
> Since it's part of the QoS stuff that .11n adds I think we should stick
> it into the qos parser.
>
> johannes
>
>

Re: [patch 3/5] A-MSDU Rx aggregation support

[mabbas]

>
>>  static ieee80211_txrx_result
>>  ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>>  {
>> @@ -4373,6 +4510,7 @@ static ieee80211_rx_handler ieee80211_rx
>>  	ieee80211_rx_h_remove_qos_control,
>>  	ieee80211_rx_h_802_1x_pae,
>>  	ieee80211_rx_h_drop_unencrypted,
>> +	ieee80211_rx_h_data_agg,
>>  	ieee80211_rx_h_data,
>>  	ieee80211_rx_h_mgmt,
>>  	NULL
>> diff -Nupr wireless-dev/net/mac80211/wme.h
>> wireless-dev-new/net/mac80211/wme.h
>> --- wireless-dev/net/mac80211/wme.h	2007-03-22 11:40:17.000000000 -0700
>> +++ wireless-dev-new/net/mac80211/wme.h	2007-03-27 01:48:34.000000000
>> -0700
>> @@ -24,6 +24,8 @@
>>  
>>  #define QOS_CONTROL_TAG1D_MASK 0x07
>>  
>> +#define QOS_CONTROL_A_MSDU_PRESENT 0x80

I am calling ieee80211_rx_h_data_agg after ieee80211_rx_h_remove_qos_control which
clears all QoS data which include QOS_CONTROL_A_MSDU_PRESENT bit. We can solve this by
adding new field is_frame_agg to ieee80211_txrx_data.u.rx.is_frame_agg which will be set in
ieee80211_rx_h_parse_qos or we can add new function to ieee80211_rx_pre_handlers just to do that. the other 
option is to shuffle calling sequence here if that possible to call ieee80211_rx_h_data_agg
before ieee80211_rx_h_remove_qos_control. any opinion?
Mohamed
 

Re: [patch 3/5] A-MSDU Rx aggregation support

[mabbas]

modified patch at the end
Johannes Berg wrote:
> On Mon, 2007-03-26 at 04:40 -0700, mohamed wrote:
>
> Scratch my previous comments. Sorry about that, I should've looked
> better.
>
>   
>> +inline static int is_agg_frame(struct sk_buff *skb, u16 fc)
>> +{
>> +       u8 *p = skb->data + ieee80211_get_hdrlen(fc) - 2;
>>     
>
> See below, but isn't this broken if somebody includes an HT field?
>
>   
>> +       if (!(fc & IEEE80211_STYPE_QOS_DATA));
>> +               return 0;
>> +
>> +       if (*p & QOS_CONTROL_A_MSDU_PRESENT)
>> +               return 1;
>> +       else
>> +               return 0;
>> +}
>>     
>
>   
> +	payload = skb->data + hdrlen;
>   
>
> Ok so payload now points to the subframes.
>
>   
>> +	if (unlikely(skb->len - hdrlen < 8)) {
>> +		if (net_ratelimit()) {
>> +			printk(KERN_DEBUG "%s: RX too short data frame "
>> +				"payload\n", dev->name);
>>     
>
> Nit: this should increase the counter we have for too short frames
> somewhere.
>
>   
>> +		}
>> +		return TXRX_DROP;
>> +	}
>> +
>> +	ethertype = (payload[6] << 8) | payload[7];
>>     
>
> Is that really correct?
>
>   
>> +	if (likely((memcmp(payload, rfc1042_header, 6) == 0 &&
>> +		ethertype != ETH_P_AARP && ethertype != ETH_P_IPX) ||
>> +		memcmp(payload, bridge_tunnel_header, 6) == 0)) {
>> +		/* remove RFC1042 or Bridge-Tunnel encapsulation and
>> +		* replace EtherType */	
>> +		skb_pull(skb, hdrlen + 6);
>> +	} else {
>> +		skb_pull(skb, hdrlen);
>> +	}
>> +
>> +	eth = (struct ethhdr*)skb->data;
>>     
>
> So that now points to the first actual subframe ethhdr. I misread that
> in my first comment. Why do you actually skb_pull on this skb? It would
> probably be more efficient to just assign "eth" in the different cases
> since that needs no memory write (eth will probably be kept in a
> register). And you throw it away anyway...
>
>   
>> +	while ((u8*)eth < skb->data + skb->len) {
>>     
>
> And that compares the pointer, not the value at it as I thought. Sorry.
>
>   
>> +        	unsigned int eth_len = sizeof(struct ethhdr) + 
>> +						ntohs(eth->h_proto);
>>     
>
> rename to subframe_len maybe?
>
>   
>> +		frame = dev_alloc_skb(3000);
>> +
>> +		if (frame == NULL) 
>> +			return TXRX_DROP;
>> +
>> +		frame->mac.raw = frame->data;
>> +		memcpy(skb_put(frame, eth_len), (u8*)eth, eth_len);
>>     
>
> Nope, not nice to do this. You really need to check if eth_len doesn't
> exceed the original skb's length or people can crash this by sending you
> short frames with bogus huge eth len embedded in it. And if you move the
> check up a bit then you can reuse "skb" for the last subframe in the
> case where people don't send us garbage:
>
> 	int remaining = subframe_len - ((u8*)eth - skb->data);
> 	if (remaining < 0)
> 		/* idiots. should blacklist their mac address */
> 		return TXRX_DROP;
> 	if (remaining < 4 /* padding */) {
> 		frame = skb;
> 		skb = NULL;
> 		skb_pull(blabla)
> 	} else {
> 		frame = dev_alloc_skb(...)
> 		if (!frame)
> 			return TXRX_DROP;
> 	}
>
> or something like that.
>
> Don't you also need to set ((ethhdr*)frame->data)->h_proto to something
> other than the length? Not exactly sure though, it seems eth_type_trans
> can handle that and I don't have the 802.3 specs handy.
>
>   
>> +		frame->dev = dev;
>>     
>
> I think it'd be good to move that closer to the netif_rx.
>
>   
>> +					* directly to it and do not pass 
>> +					* the frame to local net stack.
>> +					*/
>> +					skb2 = skb;
>> +					skb = NULL;
>>     
>
> I'm pretty sure you mean frame here instead of skb in both cases and
> this is a copy/paste error from the other data rx handler.
>
>   
>> +				}
>> +				if (dsta)
>> +					sta_info_put(dsta);
>> +			}
>> +		}
>> +		if (frame) {
>> +			/* deliver to local stack */
>> +			frame->protocol = eth_type_trans(frame, dev);
>> +			memset(frame->cb, 0, sizeof(frame->cb));
>>     
>
> That memset is useless on a newly allocated frame.
>
>   
>
>
> johannes
>   
Add A-MSDU Rx aggregation support.

To support IEEE 802.11n, we need to be able to process A-MSDU frames.
The present of the HT control field indicates it is A-MSDU frame.
This patch adds support to discover and process A-MSDU frames.

Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>

diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 3bf0be4..31cf5b8 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -2460,6 +2460,139 @@ static inline int ieee80211_bssid_match(
 }
 
 
+inline static unsigned int calc_pad_len(unsigned int len)
+{
+	return ((4 - len) & 0x3);
+}
+
+static ieee80211_txrx_result
+ieee80211_rx_h_data_agg(struct ieee80211_txrx_data *rx)
+{
+	struct net_device *dev = rx->dev;
+	struct ieee80211_local *local = rx->local;
+	u16 fc, hdrlen, ethertype;
+	u8 *payload;
+	struct sk_buff *skb = rx->skb, *skb2, *frame;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	const struct ethhdr* eth;
+	int remaining;
+
+	fc = rx->fc;
+	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
+		return TXRX_CONTINUE;
+
+	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
+		return TXRX_DROP;
+
+	if (!rx->u.rx.is_agg_frame)
+		return TXRX_CONTINUE;
+
+	hdrlen = ieee80211_get_hdrlen(fc);
+
+	payload = skb->data + hdrlen;
+
+	if (unlikely((skb->len - hdrlen) < 8)) {
+		if (net_ratelimit())
+			printk(KERN_DEBUG "%s: RX too short data frame "
+			       "payload\n", dev->name);
+		return TXRX_DROP;
+	}
+
+	ethertype = (payload[6] << 8) | payload[7];
+
+	if (likely((compare_ether_addr(payload, rfc1042_header) == 0 &&
+		    ethertype != ETH_P_AARP && ethertype != ETH_P_IPX) ||
+	    	   compare_ether_addr(payload, bridge_tunnel_header) == 0)) {
+		/* remove RFC1042 or Bridge-Tunnel encapsulation and
+		* replace EtherType */	
+		eth = (struct ethhdr*) (skb->data + hdrlen + 6);
+		remaining = skb->len - (hdrlen + 6);
+	} else {
+		eth = (struct ethhdr*) (skb->data + hdrlen);
+		remaining = skb->len - hdrlen;
+	}
+
+	while ((u8*)eth < skb->data + skb->len) {
+		u8 padding;
+        	unsigned int subframe_len = sizeof(struct ethhdr) +
+						   ntohs(eth->h_proto);
+
+		padding = calc_pad_len(subframe_len);
+		/* the last MSDU has no padding */
+		if (subframe_len > remaining)
+			return TXRX_DROP;
+
+		frame = dev_alloc_skb(local->hw.extra_tx_headroom + 
+				      subframe_len);
+
+		if (frame == NULL) 
+			return TXRX_DROP;
+
+		memcpy(skb_put(frame, subframe_len), (u8*)eth, subframe_len);
+		frame->mac.raw = frame->data;
+		skb2 = NULL;
+
+		sdata->stats.rx_packets++;
+		sdata->stats.rx_bytes += frame->len;
+
+		if (local->bridge_packets && 
+		    (sdata->type == IEEE80211_IF_TYPE_AP ||
+		     sdata->type == IEEE80211_IF_TYPE_VLAN) && 
+		     rx->u.rx.ra_match) {
+			if (is_multicast_ether_addr(frame->data)) {
+				/* send multicast frames both to higher layers
+				* in local net stack and back to the wireless 
+				* media */
+				skb2 = skb_copy(frame, GFP_ATOMIC);
+				if (!skb2)
+					printk(KERN_DEBUG "%s: failed to clone"
+					       " multicast frame\n", dev->name);
+			} else {
+				struct sta_info *dsta;
+
+				dsta = sta_info_get(local, frame->data);
+				if (dsta && !dsta->dev) 
+					printk(KERN_DEBUG "Station with null "
+					       "dev structure!\n");
+				else if (dsta && dsta->dev == dev) {
+                                	/* Destination station is associated 
+					* to this AP, so send the frame
+					* directly to it and do not pass 
+					* the frame to local net stack.
+					*/
+					skb2 = frame;
+					frame = NULL;
+				}
+				if (dsta)
+					sta_info_put(dsta);
+			}
+		}
+		if (frame) {
+			/* deliver to local stack */
+			frame->protocol = eth_type_trans(frame, dev);
+			frame->priority = skb->priority;
+			frame->dev = dev;
+			netif_rx(frame);
+		}
+
+		if (skb2) {
+			/* send to wireless media */
+			skb2->protocol = __constant_htons(ETH_P_802_3);
+			skb2->mac.raw = skb2->nh.raw = skb2->data;
+			skb2->priority = skb->priority;
+			skb2->dev = dev;
+			dev_queue_xmit(skb2);
+		}
+
+		eth = (struct ethhdr*)((u8*)eth + subframe_len + padding);
+
+		remaining -= (subframe_len + padding);
+	}
+
+	dev_kfree_skb(skb);
+	return TXRX_QUEUED;
+}
+
 static ieee80211_txrx_result
 ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 {
@@ -4389,6 +4522,7 @@ static ieee80211_rx_handler ieee80211_rx
 	ieee80211_rx_h_remove_qos_control,
 	ieee80211_rx_h_802_1x_pae,
 	ieee80211_rx_h_drop_unencrypted,
+	ieee80211_rx_h_data_agg,
 	ieee80211_rx_h_data,
 	ieee80211_rx_h_mgmt,
 	NULL
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index dd1d031..1cf8e82 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -142,10 +142,12 @@ struct ieee80211_txrx_data {
 			int sent_ps_buffered;
 			int queue;
 			int load;
+			u16 qos_control;
 			unsigned int in_scan:1;
 			/* frame is destined to interface currently processed
 			 * (including multicast frames) */
 			unsigned int ra_match:1;
+			unsigned int is_agg_frame:1;
 		} rx;
 	} u;
 #ifdef CONFIG_HOSTAPD_WPA_TESTING
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index d57be24..63b503c 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -31,12 +31,18 @@ ieee80211_rx_h_parse_qos(struct ieee8021
 {
 	u8 *data = rx->skb->data;
 	int tid;
+	unsigned int is_agg_frame = 0;
 
 	/* does the frame have a qos control field? */
 	if (WLAN_FC_IS_QOS_DATA(rx->fc)) {
 		u8 *qc = data + ieee80211_get_hdrlen(rx->fc) - QOS_CONTROL_LEN;
+		
 		/* frame has qos control */
-		tid = qc[0] & QOS_CONTROL_TID_MASK;
+		rx->u.rx.qos_control = le16_to_cpu(*((__le16*)qc));
+		tid = rx->u.rx.qos_control & QOS_CONTROL_TID_MASK;
+		if (rx->u.rx.qos_control & 
+		    IEEE80211_QOS_CONTROL_A_MSDU_PRESENT)
+			is_agg_frame = 1;
 	} else {
 		if (unlikely((rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_MGMT)) {
 			/* Separate TID for management frames */
@@ -45,6 +51,7 @@ ieee80211_rx_h_parse_qos(struct ieee8021
 			/* no qos control present */
 			tid = 0; /* 802.1d - Best Effort */
 		}
+		rx->u.rx.qos_control = 0;
 	}
 #ifdef CONFIG_MAC80211_DEBUG_COUNTERS
 	I802_DEBUG_INC(rx->local->wme_rx_queue[tid]);
@@ -54,6 +61,7 @@ #ifdef CONFIG_MAC80211_DEBUG_COUNTERS
 #endif /* CONFIG_MAC80211_DEBUG_COUNTERS */
 
 	rx->u.rx.queue = tid;
+	rx->u.rx.is_agg_frame = is_agg_frame;
 	/* Set skb->priority to 1d tag if highest order bit of TID is not set.
 	 * For now, set skb->priority to 0 for other cases. */
 	rx->skb->priority = (tid > 7) ? 0 : tid;

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

Mohamed,

> +		padding = calc_pad_len(subframe_len);
> +		/* the last MSDU has no padding */
> +		if (subframe_len > remaining)
> +			return TXRX_DROP;
> +
> +		frame = dev_alloc_skb(local->hw.extra_tx_headroom + 
> +				      subframe_len);
> +
> +		if (frame == NULL) 
> +			return TXRX_DROP;
> +
> +		memcpy(skb_put(frame, subframe_len), (u8*)eth, subframe_len);
> +		frame->mac.raw = frame->data;
> +		skb2 = NULL;

Here you allocate a new frame which can be sent back to the device when
one of the aggregated frames was a multicast frame and we're an AP
device. You also correctly add extra_tx_headroom, but it seems that this
is missing some skb_reserve(extra_tx_headroom), no? Could you make a
patch adding it? I'm not sure I fully understand the code there.

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[John W. Linville]

On Wed, Jun 20, 2007 at 11:13:39PM +0200, Johannes Berg wrote:
> Mohamed,
> 
> > +		padding = calc_pad_len(subframe_len);
> > +		/* the last MSDU has no padding */
> > +		if (subframe_len > remaining)
> > +			return TXRX_DROP;
> > +
> > +		frame = dev_alloc_skb(local->hw.extra_tx_headroom + 
> > +				      subframe_len);
> > +
> > +		if (frame == NULL) 
> > +			return TXRX_DROP;
> > +
> > +		memcpy(skb_put(frame, subframe_len), (u8*)eth, subframe_len);
> > +		frame->mac.raw = frame->data;
> > +		skb2 = NULL;
> 
> Here you allocate a new frame which can be sent back to the device when
> one of the aggregated frames was a multicast frame and we're an AP
> device. You also correctly add extra_tx_headroom, but it seems that this
> is missing some skb_reserve(extra_tx_headroom), no? Could you make a
> patch adding it? I'm not sure I fully understand the code there.

Was there ever any reply to this?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

On Tue, 2007-07-17 at 20:01 -0400, John W. Linville wrote:

> > Here you allocate a new frame which can be sent back to the device when
> > one of the aggregated frames was a multicast frame and we're an AP
> > device. You also correctly add extra_tx_headroom, but it seems that this
> > is missing some skb_reserve(extra_tx_headroom), no? Could you make a
> > patch adding it? I'm not sure I fully understand the code there.
> 
> Was there ever any reply to this?

I didn't get one but never double-checked the code either, the relevant
patches I had touching this have fallen off the list anyway :)

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[Tomas Winkler]

On 7/18/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2007-07-17 at 20:01 -0400, John W. Linville wrote:
>
> > > Here you allocate a new frame which can be sent back to the device when
> > > one of the aggregated frames was a multicast frame and we're an AP
> > > device.
HT aggregtion is extension of QoS. Multicast and broadcast frames are
not QoS frames and therefore such frames  want be incorporated into an
A-MSDU.

You also correctly add extra_tx_headroom, but it seems that this
> > > is missing some skb_reserve(extra_tx_headroom), no? Could you make a
> > > patch adding it? I'm not sure I fully understand the code there.
> >
> > Was there ever any reply to this?
>
> I didn't get one but never double-checked the code either, the relevant
> patches I had touching this have fallen off the list anyway :)
>
> johannes
>
>

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

On Wed, 2007-07-18 at 16:22 +0300, Tomas Winkler wrote:

> HT aggregtion is extension of QoS. Multicast and broadcast frames are
> not QoS frames and therefore such frames  want be incorporated into an
> A-MSDU.

Right. I do know what it is :)


> You also correctly add extra_tx_headroom, but it seems that this

Hm?

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[Tomas Winkler]

On 7/18/07, Tomas Winkler <tomasw@gmail.com> wrote:
> On 7/18/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Tue, 2007-07-17 at 20:01 -0400, John W. Linville wrote:
> >
> > > > Here you allocate a new frame which can be sent back to the device when
> > > > one of the aggregated frames was a multicast frame and we're an AP
> > > > device.
> HT aggregtion is extension of QoS. Multicast and broadcast frames are
> not QoS frames and therefore such frames  want be incorporated into an
> A-MSDU.
Oops
s/want/won't/

> You also correctly add extra_tx_headroom, but it seems that this
> > > > is missing some skb_reserve(extra_tx_headroom), no? Could you make a
> > > > patch adding it? I'm not sure I fully understand the code there.
> > >
> > > Was there ever any reply to this?
> >
> > I didn't get one but never double-checked the code either, the relevant
> > patches I had touching this have fallen off the list anyway :)
> >
> > johannes
> >
> >
>

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

On Wed, 2007-07-18 at 18:51 +0300, Tomas Winkler wrote:

> > HT aggregtion is extension of QoS. Multicast and broadcast frames are
> > not QoS frames and therefore such frames  want be incorporated into an
> > A-MSDU.
> Oops
> s/want/won't/

Ah, ok, I didn't bother to re-check. Mind doing a patch then that rips
out this code?

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[Tomas Winkler]

On 7/18/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2007-07-18 at 18:51 +0300, Tomas Winkler wrote:
>
> > > HT aggregtion is extension of QoS. Multicast and broadcast frames are
> > > not QoS frames and therefore such frames  want be incorporated into an
> > > A-MSDU.
> > Oops
> > s/want/won't/
>
> Ah, ok, I didn't bother to re-check. Mind doing a patch then that rips
> out this code?
>
It's on my list, thanks
Tomas
> johannes
>
>

Re: [patch 3/5] A-MSDU Rx aggregation support

[Tomas Winkler]

On 7/18/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2007-07-18 at 18:51 +0300, Tomas Winkler wrote:
>
> > > HT aggregtion is extension of QoS. Multicast and broadcast frames are
> > > not QoS frames and therefore such frames  want be incorporated into an
> > > A-MSDU.
> > Oops
> > s/want/won't/
>
> Ah, ok, I didn't bother to re-check. Mind doing a patch then that rips
> out this code?
>
> johannes
>
>
I've checked the Spec again and I was wrong. Theoretically,  A-MSDU
packet can be a multicast packet so we get back to the problem you've
pointed first.

Re: [patch 3/5] A-MSDU Rx aggregation support

[Johannes Berg]

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

On Wed, 2007-07-25 at 11:06 +0300, Tomas Winkler wrote:

> I've checked the Spec again and I was wrong. Theoretically,  A-MSDU
> packet can be a multicast packet so we get back to the problem you've
> pointed first.

You seem to know this much better than me, think you could fix it?

johannes

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

Re: [patch 3/5] A-MSDU Rx aggregation support

[mohamed salim abbas]

I thought the Destinations Address (DA) and Senders Address (SA)
parameter values in the subframe header must map to the
same Receiver Address (RA) and Transmitter Address (TA)
in the MAC header. Thus, broadcasting or multicasting is not allowed.


On 7/18/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2007-07-17 at 20:01 -0400, John W. Linville wrote:
>
> > > Here you allocate a new frame which can be sent back to the device when
> > > one of the aggregated frames was a multicast frame and we're an AP
> > > device. You also correctly add extra_tx_headroom, but it seems that this
> > > is missing some skb_reserve(extra_tx_headroom), no? Could you make a
> > > patch adding it? I'm not sure I fully understand the code there.
> >
> > Was there ever any reply to this?
>
> I didn't get one but never double-checked the code either, the relevant
> patches I had touching this have fallen off the list anyway :)
>
> johannes
>
>

Re: [patch 3/5] A-MSDU Rx aggregation support

[Tomas Winkler]

On 7/25/07, mohamed salim abbas <mabbaswireless@gmail.com> wrote:
> I thought the Destinations Address (DA) and Senders Address (SA)
> parameter values in the subframe header must map to the
> same Receiver Address (RA) and Transmitter Address (TA)
> in the MAC header. Thus, broadcasting or multicasting is not allowed.
>
The packet as whole can be a multicast packet
>
> On 7/18/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Tue, 2007-07-17 at 20:01 -0400, John W. Linville wrote:
> >
> > > > Here you allocate a new frame which can be sent back to the device when
> > > > one of the aggregated frames was a multicast frame and we're an AP
> > > > device. You also correctly add extra_tx_headroom, but it seems that this
> > > > is missing some skb_reserve(extra_tx_headroom), no? Could you make a
> > > > patch adding it? I'm not sure I fully understand the code there.
> > >
> > > Was there ever any reply to this?
> >
> > I didn't get one but never double-checked the code either, the relevant
> > patches I had touching this have fallen off the list anyway :)
> >
> > johannes
> >
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>