[patch 2/5] Add basic support for IEEE 802.11n discovery and association

[patch 2/5] Add basic support for IEEE 802.11n discovery and association

[mohamed]

Add basic support for IEEE 802.11n discovery and association.

This patch adds support to discover IEEE 802.11n AP and enable 
association to 802.11n Network. It parses beacon to discover 802.11n 
IE and include HT capability information element in Association Request
Frame. 
It also call low level driver with the HT capability available during 
association. 

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

diff -Nupr wireless-dev/include/net/mac80211.h
wireless-dev-new/include/net/mac80211.h
--- wireless-dev/include/net/mac80211.h	2007-03-27 00:36:28.000000000
-0700
+++ wireless-dev-new/include/net/mac80211.h	2007-03-27
00:59:40.000000000 -0700
@@ -526,6 +526,9 @@ struct ieee80211_hw {
 	 * per-packet RC4 key with each TX frame when doing hwcrypto */
 #define IEEE80211_HW_TKIP_REQ_PHASE2_KEY (1<<14)
 
+	/* The device capable of supporting 11n */
+#define IEEE80211_HW_SUPPORT_HT_MODE (1<<15)
+
 	u32 flags;			/* hardware flags defined above */
 
 	/* Set to the size of a needed device specific skb headroom for TX
skbs. */
@@ -720,6 +723,17 @@ struct ieee80211_ops {
 	/* Get the current TSF timer value from firmware/hardware. Currently,
 	 * this is only used for IBSS mode debugging and, as such, is not a
 	 * required function. */
+
+	/* Configure ht parameters 
+	*/
+	int (*conf_ht)(struct ieee80211_hw *hw, 
+		       struct ieee80211_ht_capability *ht_cap_param,
+		       struct ieee80211_ht_additional_info *ht_extra_param);
+
+	/* Get ht capabilities from the device */
+	int (*get_ht_capab)(struct ieee80211_hw *hw,
+			    struct ieee80211_ht_capability *ht_cap_param);
+
 	u64 (*get_tsf)(struct ieee80211_hw *hw);
 
 	/* Reset the TSF timer and allow firmware/hardware to synchronize with
diff -Nupr wireless-dev/net/mac80211/ieee80211_i.h
wireless-dev-new/net/mac80211/ieee80211_i.h
--- wireless-dev/net/mac80211/ieee80211_i.h	2007-03-27
00:36:28.000000000 -0700
+++ wireless-dev-new/net/mac80211/ieee80211_i.h	2007-03-27
00:59:40.000000000 -0700
@@ -91,6 +91,8 @@ struct ieee80211_sta_bss {
 	size_t rsn_ie_len;
 	u8 *wmm_ie;
 	size_t wmm_ie_len;
+	u8 *ht_ie;
+	size_t ht_ie_len;
 #define IEEE80211_MAX_SUPP_RATES 32
 	u8 supp_rates[IEEE80211_MAX_SUPP_RATES];
 	size_t supp_rates_len;
@@ -269,6 +271,7 @@ struct ieee80211_if_sta {
 	unsigned int create_ibss:1;
 	unsigned int mixed_cell:1;
 	unsigned int wmm_enabled:1;
+	unsigned int ht_enabled:1;
 	unsigned int auto_ssid_sel:1;
 	unsigned int auto_bssid_sel:1;
 	unsigned int auto_channel_sel:1;
diff -Nupr wireless-dev/net/mac80211/ieee80211_iface.c
wireless-dev-new/net/mac80211/ieee80211_iface.c
--- wireless-dev/net/mac80211/ieee80211_iface.c	2007-03-27
00:36:28.000000000 -0700
+++ wireless-dev-new/net/mac80211/ieee80211_iface.c	2007-03-27
00:59:40.000000000 -0700
@@ -191,6 +191,7 @@ void ieee80211_if_set_type(struct net_de
 			IEEE80211_AUTH_ALG_SHARED_KEY;
 		ifsta->create_ibss = 1;
 		ifsta->wmm_enabled = 1;
+		ifsta->ht_enabled = 1;
 		ifsta->auto_channel_sel = 1;
 		ifsta->auto_bssid_sel = 1;
 
diff -Nupr wireless-dev/net/mac80211/ieee80211_sta.c
wireless-dev-new/net/mac80211/ieee80211_sta.c
--- wireless-dev/net/mac80211/ieee80211_sta.c	2007-03-27
00:36:28.000000000 -0700
+++ wireless-dev-new/net/mac80211/ieee80211_sta.c	2007-03-27
01:26:06.000000000 -0700
@@ -96,6 +96,10 @@ struct ieee802_11_elems {
 	u8 rsn_len;
 	u8 *erp_info;
 	u8 erp_info_len;
+	u8 *ht_cap_param;
+	u8 ht_cap_param_len;
+	u8 *ht_extra_param;
+	u8 ht_extra_param_len;
 	u8 *ext_supp_rates;
 	u8 ext_supp_rates_len;
 	u8 *wmm_info;
@@ -197,6 +201,14 @@ static ParseRes ieee802_11_parse_elems(u
 			elems->ext_supp_rates = pos;
 			elems->ext_supp_rates_len = elen;
 			break;
+		case WLAN_EID_HT_CAPABILITY:
+			elems->ht_cap_param = pos;
+			elems->ht_cap_param_len = elen;
+			break;
+		case WLAN_EID_HT_EXTRA_INFO:		
+			elems->ht_extra_param = pos;
+			elems->ht_extra_param_len = elen;
+			break;
 		default:
 #if 0
 			printk(KERN_DEBUG "IEEE 802.11 element parse ignored "
@@ -230,7 +242,55 @@ static int ecw2cw(int ecw)
 	return cw - 1;
 }
 
+/* call low level driver with 11n params as it was recieved
+   from the AP
+*/   
+static void ieee80211_sta_ht_params(struct net_device *dev, 
+			struct sta_info *sta,
+			struct ieee80211_ht_additional_info *ht_extra_param,
+			struct ieee80211_ht_capability *ht_cap_param)
+{
+	int rc;
+	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+
+	if (local->ops->conf_ht) {
+		rc = local->ops->conf_ht(local_to_hw(local), ht_cap_param, 
+					 ht_extra_param);
+
+		if (rc)
+			sta->flags &= ~WLAN_STA_HT;
+	} else 
+		sta->flags &= ~WLAN_STA_HT;
+
+	return;
+}
+
+/* Get 11n capabilties from low level driver */
+static void ieee80211_fill_ht_ie(struct net_device *dev,
+				 struct ieee80211_ht_capability *ht_capab)
+{
+	int rc;
+	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+
+	if (!local->ops->get_ht_capab){
+		memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
+		return;
+	}
+
+	rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
+	if (!rc) {
+		memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
+		return;
+	}
 
+	ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
+					ht_capab->capabilitiesInfo);
+	ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
+					ht_capab->extended_ht_capability_info);
+	ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
+					ht_capab->tx_BF_capability_info);
+}
+ 
 static void ieee80211_sta_wmm_params(struct net_device *dev,
 				     struct ieee80211_if_sta *ifsta,
 				     u8 *wmm_param, size_t wmm_param_len)
@@ -487,6 +547,7 @@ static void ieee80211_send_assoc(struct 
 	u16 capab;
 	struct ieee80211_sta_bss *bss;
 	int wmm = 0;
+	int ht_enabled = 0;
 
 	skb = dev_alloc_skb(local->hw.extra_tx_headroom +
 			    sizeof(*mgmt) + 200 + ifsta->extra_ie_len +
@@ -509,6 +570,9 @@ static void ieee80211_send_assoc(struct 
 			capab |= WLAN_CAPABILITY_PRIVACY;
 		if (bss->wmm_ie) {
 			wmm = 1;
+		
+			if (bss->ht_ie)
+				ht_enabled = 1;
 		}
 		ieee80211_rx_bss_put(dev, bss);
 	}
@@ -584,6 +648,15 @@ static void ieee80211_send_assoc(struct 
 		*pos++ = 0;
 	}
 
+	/* if low level driver support 11n fill in 11n IE */
+	if (ht_enabled && ifsta->ht_enabled && local->ops->get_ht_capab) {
+		pos = skb_put(skb, sizeof(struct ieee80211_ht_capability)+2);
+		*pos++ = WLAN_EID_HT_CAPABILITY;
+		*pos++ = sizeof(struct ieee80211_ht_capability);
+		ieee80211_fill_ht_ie(dev, 
+				     (struct ieee80211_ht_capability *)pos);
+	}
+		
 	kfree(ifsta->assocreq_ies);
 	ifsta->assocreq_ies_len = (skb->data + skb->len) - ies;
 	ifsta->assocreq_ies = kmalloc(ifsta->assocreq_ies_len, GFP_ATOMIC);
@@ -1215,6 +1288,16 @@ static void ieee80211_rx_mgmt_assoc_resp
 	}
 	sta->supp_rates = rates;
 
+	if (elems.ht_extra_param && elems.ht_cap_param && elems.wmm_param &&
+	    ifsta->ht_enabled  ) {
+		sta->flags |= WLAN_STA_HT;
+		ieee80211_sta_ht_params(dev, sta, 
+					(struct ieee80211_ht_additional_info *) 
+					elems.ht_extra_param, 
+					(struct ieee80211_ht_capability *)
+					elems.ht_cap_param);
+	}
+
 	rate_control_rate_init(sta, local);
 
 	if (elems.wmm_param && ifsta->wmm_enabled) {
@@ -1310,6 +1393,7 @@ static void ieee80211_rx_bss_free(struct
 	kfree(bss->wpa_ie);
 	kfree(bss->rsn_ie);
 	kfree(bss->wmm_ie);
+	kfree(bss->ht_ie);
 	kfree(bss);
 }
 
@@ -1556,6 +1640,22 @@ static void ieee80211_rx_bss_info(struct
 		bss->wmm_ie_len = 0;
 	}
 
+	if (elems.ht_cap_param &&
+	    (!bss->ht_ie || bss->ht_ie_len != elems.ht_cap_param_len ||
+	     memcmp(bss->ht_ie, elems.ht_cap_param, elems.ht_cap_param_len)))
{
+		kfree(bss->ht_ie);	
+		bss->ht_ie = kmalloc(elems.ht_cap_param_len + 2, GFP_ATOMIC);
+		if (bss->ht_ie) {
+			memcpy(bss->ht_ie, elems.ht_cap_param - 2,
+				elems.ht_cap_param_len + 2);
+			bss->ht_ie_len = elems.ht_cap_param_len + 2;
+		} else
+                        bss->ht_ie_len = 0;
+	} else if (!elems.ht_cap_param && bss->ht_ie) {
+		kfree(bss->ht_ie);
+		bss->ht_ie = NULL;
+		bss->ht_ie_len = 0;
+	}
 
 	bss->hw_mode = rx_status->phymode;
 	bss->channel = channel;
diff -Nupr wireless-dev/net/mac80211/ieee80211_sysfs_sta.c
wireless-dev-new/net/mac80211/ieee80211_sysfs_sta.c
--- wireless-dev/net/mac80211/ieee80211_sysfs_sta.c	2007-03-27
00:36:28.000000000 -0700
+++ wireless-dev-new/net/mac80211/ieee80211_sysfs_sta.c	2007-03-27
00:59:40.000000000 -0700
@@ -79,7 +79,7 @@ STA_ATTR(wep_weak_iv_count, wep_weak_iv_
 
 static ssize_t show_sta_flags(const struct sta_info *sta, char *buf)
 {
-	return sprintf(buf, "%s%s%s%s%s%s%s%s%s",
+	return sprintf(buf, "%s%s%s%s%s%s%s%s%s%s",
 		       sta->flags & WLAN_STA_AUTH ? "AUTH\n" : "",
 		       sta->flags & WLAN_STA_ASSOC ? "ASSOC\n" : "",
 		       sta->flags & WLAN_STA_PS ? "PS\n" : "",
@@ -89,7 +89,8 @@ static ssize_t show_sta_flags(const stru
 		       sta->flags & WLAN_STA_SHORT_PREAMBLE ?
 		       "SHORT PREAMBLE\n" : "",
 		       sta->flags & WLAN_STA_WME ? "WME\n" : "",
-		       sta->flags & WLAN_STA_WDS ? "WDS\n" : "");
+		       sta->flags & WLAN_STA_WDS ? "WDS\n" : "",
+		       sta->flags & WLAN_STA_HT ? "HT\n" : "");
 }
 __STA_ATTR(flags);
 
diff -Nupr wireless-dev/net/mac80211/sta_info.h
wireless-dev-new/net/mac80211/sta_info.h
--- wireless-dev/net/mac80211/sta_info.h	2007-03-27 00:36:28.000000000
-0700
+++ wireless-dev-new/net/mac80211/sta_info.h	2007-03-27
00:59:40.000000000 -0700
@@ -26,6 +26,7 @@
 				    */
 #define WLAN_STA_SHORT_PREAMBLE BIT(7)
 #define WLAN_STA_WME BIT(9)
+#define WLAN_STA_HT  BIT(10)
 #define WLAN_STA_WDS BIT(27)
 
 

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Randy Dunlap]

On Mon, 26 Mar 2007 04:38:25 -0700 mohamed wrote:

> Add basic support for IEEE 802.11n discovery and association.
> 
> This patch adds support to discover IEEE 802.11n AP and enable 
> association to 802.11n Network. It parses beacon to discover 802.11n 
> IE and include HT capability information element in Association Request
> Frame. 
> It also call low level driver with the HT capability available during 
> association. 
> 
> Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>
> 
> diff -Nupr wireless-dev/include/net/mac80211.h
> wireless-dev-new/include/net/mac80211.h
> --- wireless-dev/include/net/mac80211.h	2007-03-27 00:36:28.000000000
> -0700
> +++ wireless-dev-new/include/net/mac80211.h	2007-03-27
> 00:59:40.000000000 -0700

I'm seeing an email/patch problem here.  The lines above should be
3 lines only, not 6 that I'm seeing.  Maybe it's my mail client,
but it usually works for me, so possibly yours is splitting lines
when it should not.

> @@ -526,6 +526,9 @@ struct ieee80211_hw {
>  	 * per-packet RC4 key with each TX frame when doing hwcrypto */
>  #define IEEE80211_HW_TKIP_REQ_PHASE2_KEY (1<<14)
>  
> +	/* The device capable of supporting 11n */
> +#define IEEE80211_HW_SUPPORT_HT_MODE (1<<15)
> +
>  	u32 flags;			/* hardware flags defined above */
>  
>  	/* Set to the size of a needed device specific skb headroom for TX
> skbs. */
> @@ -720,6 +723,17 @@ struct ieee80211_ops {
>  	/* Get the current TSF timer value from firmware/hardware. Currently,
>  	 * this is only used for IBSS mode debugging and, as such, is not a
>  	 * required function. */
> +
> +	/* Configure ht parameters 

Line above ends with space.  :(

What is "ht"?

> +	*/
> +	int (*conf_ht)(struct ieee80211_hw *hw, 
> +		       struct ieee80211_ht_capability *ht_cap_param,
> +		       struct ieee80211_ht_additional_info *ht_extra_param);
> +
> +	/* Get ht capabilities from the device */
> +	int (*get_ht_capab)(struct ieee80211_hw *hw,
> +			    struct ieee80211_ht_capability *ht_cap_param);
> +
>  	u64 (*get_tsf)(struct ieee80211_hw *hw);
>  
>  	/* Reset the TSF timer and allow firmware/hardware to synchronize with
> diff -Nupr wireless-dev/net/mac80211/ieee80211_sta.c
> wireless-dev-new/net/mac80211/ieee80211_sta.c
> --- wireless-dev/net/mac80211/ieee80211_sta.c	2007-03-27
> 00:36:28.000000000 -0700
> +++ wireless-dev-new/net/mac80211/ieee80211_sta.c	2007-03-27
> 01:26:06.000000000 -0700
> @@ -230,7 +242,55 @@ static int ecw2cw(int ecw)
>  	return cw - 1;
>  }
>  
> +/* call low level driver with 11n params as it was recieved

                                                      received
> +   from the AP
> +*/   

Line above ends with spaces.

Kernel long comment style is:

/*
 * call low-level driver with 11n params as they were received
 * from the AP
 */

> +static void ieee80211_sta_ht_params(struct net_device *dev, 
> +			struct sta_info *sta,
> +			struct ieee80211_ht_additional_info *ht_extra_param,
> +			struct ieee80211_ht_capability *ht_cap_param)
> +{
> +	int rc;
> +	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> +
> +	if (local->ops->conf_ht) {
> +		rc = local->ops->conf_ht(local_to_hw(local), ht_cap_param, 
> +					 ht_extra_param);
> +
> +		if (rc)
> +			sta->flags &= ~WLAN_STA_HT;
> +	} else 
> +		sta->flags &= ~WLAN_STA_HT;
> +
> +	return;
> +}
> +
> +/* Get 11n capabilties from low level driver */
> +static void ieee80211_fill_ht_ie(struct net_device *dev,
> +				 struct ieee80211_ht_capability *ht_capab)
> +{
> +	int rc;
> +	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> +
> +	if (!local->ops->get_ht_capab){
> +		memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> +		return;
> +	}
> +
> +	rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
> +	if (!rc) {
> +		memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> +		return;
> +	}
>  
> +	ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> +					ht_capab->capabilitiesInfo);
> +	ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> +					ht_capab->extended_ht_capability_info);
> +	ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> +					ht_capab->tx_BF_capability_info);
> +}
> + 
>  static void ieee80211_sta_wmm_params(struct net_device *dev,
>  				     struct ieee80211_if_sta *ifsta,
>  				     u8 *wmm_param, size_t wmm_param_len)
> @@ -584,6 +648,15 @@ static void ieee80211_send_assoc(struct 
>  		*pos++ = 0;
>  	}
>  
> +	/* if low level driver support 11n fill in 11n IE */

                               supports 11n,

> +	if (ht_enabled && ifsta->ht_enabled && local->ops->get_ht_capab) {
> +		pos = skb_put(skb, sizeof(struct ieee80211_ht_capability)+2);
> +		*pos++ = WLAN_EID_HT_CAPABILITY;
> +		*pos++ = sizeof(struct ieee80211_ht_capability);
> +		ieee80211_fill_ht_ie(dev, 
> +				     (struct ieee80211_ht_capability *)pos);
> +	}
> +		
>  	kfree(ifsta->assocreq_ies);
>  	ifsta->assocreq_ies_len = (skb->data + skb->len) - ies;
>  	ifsta->assocreq_ies = kmalloc(ifsta->assocreq_ies_len, GFP_ATOMIC);


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

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Luis R. Rodriguez]

On 3/26/07, mohamed <mabbas@linux.intel.com> wrote:

> +/* Get 11n capabilties from low level driver */
> +static void ieee80211_fill_ht_ie(struct net_device *dev,
> +                                struct ieee80211_ht_capability *ht_capab)
> +{
> +       int rc;
> +       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> +
> +       if (!local->ops->get_ht_capab){
> +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> +               return;
> +       }
> +
> +       rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
> +       if (!rc) {
> +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> +               return;
> +       }
>
> +       ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> +                                       ht_capab->capabilitiesInfo);
> +       ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> +                                       ht_capab->extended_ht_capability_info);
> +       ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> +                                       ht_capab->tx_BF_capability_info);
> +}
> +

We should memset to 0 the entire ht_capab to regardless as its coming
from skb_put() otherwise we'll get random data there if we don't set
it and we sure as hell don't want to just transmit that :) Also
ieee80211_send_assoc() is already checking for
local->ops->get_ht_capab so lets just do a BUG_ON here to capture
cases where someone else didn't do the proper checking. So how about
instead something like:

/* Get 11n capabilties from low level driver */
static void ieee80211_fill_ht_ie(struct net_device *dev,
                                struct ieee80211_ht_capability *ht_capab)
{
       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
       BUG_ON(!local->ops->get_ht_capab);
       memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
       if (!local->ops->get_ht_capab(local_to_hw(local), ht_capab))
               return;
       ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
                                       ht_capab->capabilitiesInfo);
       ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
                                       ht_capab->extended_ht_capability_info);
       ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
                                       ht_capab->tx_BF_capability_info);
}

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Luis R. Rodriguez]

On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On 3/26/07, mohamed <mabbas@linux.intel.com> wrote:
>
> > +/* Get 11n capabilties from low level driver */
> > +static void ieee80211_fill_ht_ie(struct net_device *dev,
> > +                                struct ieee80211_ht_capability *ht_capab)
> > +{
> > +       int rc;
> > +       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > +
> > +       if (!local->ops->get_ht_capab){
> > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > +               return;
> > +       }
> > +
> > +       rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
> > +       if (!rc) {
> > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > +               return;
> > +       }
> >
> > +       ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> > +                                       ht_capab->capabilitiesInfo);
> > +       ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> > +                                       ht_capab->extended_ht_capability_info);
> > +       ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> > +                                       ht_capab->tx_BF_capability_info);
> > +}
> > +
>
> We should memset to 0 the entire ht_capab to regardless as its coming
> from skb_put() otherwise we'll get random data there if we don't set
> it and we sure as hell don't want to just transmit that :) Also
> ieee80211_send_assoc() is already checking for
> local->ops->get_ht_capab so lets just do a BUG_ON here to capture
> cases where someone else didn't do the proper checking. So how about
> instead something like:
>
> /* Get 11n capabilties from low level driver */
> static void ieee80211_fill_ht_ie(struct net_device *dev,
>                                 struct ieee80211_ht_capability *ht_capab)
> {
>        struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
>        BUG_ON(!local->ops->get_ht_capab);
>        memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
>        if (!local->ops->get_ht_capab(local_to_hw(local), ht_capab))
>                return;
>        ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
>                                        ht_capab->capabilitiesInfo);
>        ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
>                                        ht_capab->extended_ht_capability_info);
>        ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
>                                        ht_capab->tx_BF_capability_info);
> }
>

On second thought just memset to 0 on ieee80211_send_assoc() right
after the skb_put and remove that from above.

  Luis

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Luis R. Rodriguez]

On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> > On 3/26/07, mohamed <mabbas@linux.intel.com> wrote:
> >
> > > +/* Get 11n capabilties from low level driver */
> > > +static void ieee80211_fill_ht_ie(struct net_device *dev,
> > > +                                struct ieee80211_ht_capability *ht_capab)
> > > +{
> > > +       int rc;
> > > +       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > > +
> > > +       if (!local->ops->get_ht_capab){
> > > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > +               return;
> > > +       }
> > > +
> > > +       rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
> > > +       if (!rc) {
> > > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > +               return;
> > > +       }
> > >
> > > +       ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> > > +                                       ht_capab->capabilitiesInfo);
> > > +       ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> > > +                                       ht_capab->extended_ht_capability_info);
> > > +       ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> > > +                                       ht_capab->tx_BF_capability_info);
> > > +}
> > > +
> >
> > We should memset to 0 the entire ht_capab to regardless as its coming
> > from skb_put() otherwise we'll get random data there if we don't set
> > it and we sure as hell don't want to just transmit that :) Also
> > ieee80211_send_assoc() is already checking for
> > local->ops->get_ht_capab so lets just do a BUG_ON here to capture
> > cases where someone else didn't do the proper checking. So how about
> > instead something like:
> >
> > /* Get 11n capabilties from low level driver */
> > static void ieee80211_fill_ht_ie(struct net_device *dev,
> >                                 struct ieee80211_ht_capability *ht_capab)
> > {
> >        struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> >        BUG_ON(!local->ops->get_ht_capab);
> >        memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> >        if (!local->ops->get_ht_capab(local_to_hw(local), ht_capab))
> >                return;
> >        ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> >                                        ht_capab->capabilitiesInfo);
> >        ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> >                                        ht_capab->extended_ht_capability_info);
> >        ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> >                                        ht_capab->tx_BF_capability_info);
> > }
> >
>
> On second thought just memset to 0 on ieee80211_send_assoc() right
> after the skb_put and remove that from above.
>
>   Luis
>

Last few comments:

I. In struct ieee80211_ops you added your hunk after the TSF comment,
but before  u64 (*get_tsf)(struct ieee80211_hw *hw) was listed. Please
move the hunk after get_tsf or before the comment.

II. In ieee80211_fill_ht_ie() you do cpu_to_le* on the same fields.. I
take it the driver's get_ht_capab() is supposed to fill these. Since
you already defined them as little endian in the struct
ieee80211_ht_capability, how about just leaving that up to the driver
developer to make sure they do this? That's would be 6 lines less of
code in ieee80211_fill_ht_ie().

  Luis

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Johannes Berg]

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

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

> +	/* Get ht capabilities from the device */
> +	int (*get_ht_capab)(struct ieee80211_hw *hw,
> +			    struct ieee80211_ht_capability *ht_cap_param);
> +

Does this really need to return an error? And if so, what happens? You
currently send a blank HT capabilities IE but I think in that case it
should be left out completely. But I don't think it will ever return an
error, it doesn't even need to allocate memory. Just change this to
void?

johannes

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

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Luis R. Rodriguez]

On 3/29/07, mohamed <mabbas@linux.intel.com> wrote:
> On Tue, 2007-03-27 at 13:29 -0400, Luis R. Rodriguez wrote:
> > On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> > > On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> > > > On 3/26/07, mohamed <mabbas@linux.intel.com> wrote:
> > > >
> > > > > +/* Get 11n capabilties from low level driver */
> > > > > +static void ieee80211_fill_ht_ie(struct net_device *dev,
> > > > > +                                struct ieee80211_ht_capability *ht_capab)
> > > > > +{
> > > > > +       int rc;
> > > > > +       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > > > > +
> > > > > +       if (!local->ops->get_ht_capab){
> > > > > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > > +       rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
> > > > > +       if (!rc) {
> > > > > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > > > +               return;
> > > > > +       }
> > > > >
> > > > > +       ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> > > > > +                                       ht_capab->capabilitiesInfo);
> > > > > +       ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> > > > > +                                       ht_capab->extended_ht_capability_info);
> > > > > +       ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> > > > > +                                       ht_capab->tx_BF_capability_info);
> > > > > +}
> > > > > +
> > > >
> > > > We should memset to 0 the entire ht_capab to regardless as its coming
> > > > from skb_put() otherwise we'll get random data there if we don't set
> > > > it and we sure as hell don't want to just transmit that :)
> I will memset to 0 before using the structure. I will change the patch
> to reflect this.

Thanks

> > Also
> > > > ieee80211_send_assoc() is already checking for
> > > > local->ops->get_ht_capab so lets just do a BUG_ON here to capture
> > > > cases where someone else didn't do the proper checking. So how about
> > > > instead something like:
> make sense.

Thanks

> > > > /* Get 11n capabilties from low level driver */
> > > > static void ieee80211_fill_ht_ie(struct net_device *dev,
> > > >                                 struct ieee80211_ht_capability *ht_capab)
> > > > {
> > > >        struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > > >        BUG_ON(!local->ops->get_ht_capab);
> > > >        memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > >        if (!local->ops->get_ht_capab(local_to_hw(local), ht_capab))
> > > >                return;
> > > >        ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> > > >                                        ht_capab->capabilitiesInfo);
> > > >        ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> > > >                                        ht_capab->extended_ht_capability_info);
> > > >        ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> > > >                                        ht_capab->tx_BF_capability_info);
> > > > }
> > > >
> > >
> > > On second thought just memset to 0 on ieee80211_send_assoc() right
> > > after the skb_put and remove that from above.
> > >
> > >   Luis
> > >
> >
> > Last few comments:
> >
> > I. In struct ieee80211_ops you added your hunk after the TSF comment,
> > but before  u64 (*get_tsf)(struct ieee80211_hw *hw) was listed. Please
> > move the hunk after get_tsf or before the comment.
> >
> > II. In ieee80211_fill_ht_ie() you do cpu_to_le* on the same fields.. I
> > take it the driver's get_ht_capab() is supposed to fill these. Since
> > you already defined them as little endian in the struct
> > ieee80211_ht_capability, how about just leaving that up to the driver
> > developer to make sure they do this? That's would be 6 lines less of
> > code in ieee80211_fill_ht_ie().

And how about regarding the above comment (II)? We can just expect
driver developers to follow proper endianness. That cleans that
routine a bit more too.

  Luis

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Johannes Berg]

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

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

> @@ -96,6 +96,10 @@ struct ieee802_11_elems {
>  	u8 rsn_len;
>  	u8 *erp_info;
>  	u8 erp_info_len;
> +	u8 *ht_cap_param;
> +	u8 ht_cap_param_len;
> +	u8 *ht_extra_param;
> +	u8 ht_extra_param_len;
>  	u8 *ext_supp_rates;
>  	u8 ext_supp_rates_len;
>  	u8 *wmm_info;

Random note not applicable to your patch: This is quite a large struct,
especially on 64-bit. Maybe we should be thinking about making those
pointers there u16 offsets instead. And we *definitely* should change
the order of these fields, having u8 and u8* alternate just sucks.

> +static void ieee80211_sta_ht_params(struct net_device *dev, 
> +			struct sta_info *sta,
> +			struct ieee80211_ht_additional_info *ht_extra_param,
> +			struct ieee80211_ht_capability *ht_cap_param)
> +{
> +	int rc;
> +	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> +
> +	if (local->ops->conf_ht) {
> +		rc = local->ops->conf_ht(local_to_hw(local), ht_cap_param, 
> +					 ht_extra_param);
> +
> +		if (rc)
> +			sta->flags &= ~WLAN_STA_HT;
> +	} else 
> +		sta->flags &= ~WLAN_STA_HT;

Shouldn't this also set the STA_HT flag in the case where conf_ht
returns without error?

> +/* Get 11n capabilties from low level driver */
> +static void ieee80211_fill_ht_ie(struct net_device *dev,

[filling in how it looks like after Luis's and my proposed changes]

> +				 struct ieee80211_ht_capability *ht_capab)
> +{
> +       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> +       BUG_ON(!local->ops->get_ht_capab);
> +       memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> +       local->ops->get_ht_capab(local_to_hw(local), ht_capab);

some blank lines would be good ;)

Maybe we should be setting some fields to default values here that
aren't zero? Haven't checked.
 
> +	/* if low level driver support 11n fill in 11n IE */
> +	if (ht_enabled && ifsta->ht_enabled && local->ops->get_ht_capab) {
> +		pos = skb_put(skb, sizeof(struct ieee80211_ht_capability)+2);
> +		*pos++ = WLAN_EID_HT_CAPABILITY;
> +		*pos++ = sizeof(struct ieee80211_ht_capability);
> +		ieee80211_fill_ht_ie(dev, 
> +				     (struct ieee80211_ht_capability *)pos);

Now that fill_ht_ie is so short maybe it should just be rolled into this
code.

johannes

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

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[mohamed]

On Wed, 2007-03-28 at 11:14 +0200, Johannes Berg wrote:
> On Mon, 2007-03-26 at 04:38 -0700, mohamed wrote:
> 
> > +	/* Get ht capabilities from the device */
> > +	int (*get_ht_capab)(struct ieee80211_hw *hw,
> > +			    struct ieee80211_ht_capability *ht_cap_param);
> > +
> 
> Does this really need to return an error? And if so, what happens? You
> currently send a blank HT capabilities IE but I think in that case it
> should be left out completely. But I don't think it will ever return an
> error, it doesn't even need to allocate memory. Just change this to
> void?
> 
> johannes
Since we are setting the structure to all 0 before calling the function
I guess it is safe to change the function to return void. I will
resubmit the patch with that fix
Mohamed

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[mohamed]

On Tue, 2007-03-27 at 13:29 -0400, Luis R. Rodriguez wrote:
> On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> > On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> > > On 3/26/07, mohamed <mabbas@linux.intel.com> wrote:
> > >
> > > > +/* Get 11n capabilties from low level driver */
> > > > +static void ieee80211_fill_ht_ie(struct net_device *dev,
> > > > +                                struct ieee80211_ht_capability *ht_capab)
> > > > +{
> > > > +       int rc;
> > > > +       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > > > +
> > > > +       if (!local->ops->get_ht_capab){
> > > > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > > +               return;
> > > > +       }
> > > > +
> > > > +       rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
> > > > +       if (!rc) {
> > > > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > > +               return;
> > > > +       }
> > > >
> > > > +       ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> > > > +                                       ht_capab->capabilitiesInfo);
> > > > +       ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> > > > +                                       ht_capab->extended_ht_capability_info);
> > > > +       ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> > > > +                                       ht_capab->tx_BF_capability_info);
> > > > +}
> > > > +
> > >
> > > We should memset to 0 the entire ht_capab to regardless as its coming
> > > from skb_put() otherwise we'll get random data there if we don't set
> > > it and we sure as hell don't want to just transmit that :) 
I will memset to 0 before using the structure. I will change the patch
to reflect this.
> Also
> > > ieee80211_send_assoc() is already checking for
> > > local->ops->get_ht_capab so lets just do a BUG_ON here to capture
> > > cases where someone else didn't do the proper checking. So how about
> > > instead something like:
make sense.
> > >
> > > /* Get 11n capabilties from low level driver */
> > > static void ieee80211_fill_ht_ie(struct net_device *dev,
> > >                                 struct ieee80211_ht_capability *ht_capab)
> > > {
> > >        struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > >        BUG_ON(!local->ops->get_ht_capab);
> > >        memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > >        if (!local->ops->get_ht_capab(local_to_hw(local), ht_capab))
> > >                return;
> > >        ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> > >                                        ht_capab->capabilitiesInfo);
> > >        ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> > >                                        ht_capab->extended_ht_capability_info);
> > >        ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> > >                                        ht_capab->tx_BF_capability_info);
> > > }
> > >
> >
> > On second thought just memset to 0 on ieee80211_send_assoc() right
> > after the skb_put and remove that from above.
> >
> >   Luis
> >
> 
> Last few comments:
> 
> I. In struct ieee80211_ops you added your hunk after the TSF comment,
> but before  u64 (*get_tsf)(struct ieee80211_hw *hw) was listed. Please
> move the hunk after get_tsf or before the comment.
> 
> II. In ieee80211_fill_ht_ie() you do cpu_to_le* on the same fields.. I
> take it the driver's get_ht_capab() is supposed to fill these. Since
> you already defined them as little endian in the struct
> ieee80211_ht_capability, how about just leaving that up to the driver
> developer to make sure they do this? That's would be 6 lines less of
> code in ieee80211_fill_ht_ie().
> 
>   Luis

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[mohamed]

On Wed, 2007-03-28 at 15:11 -0400, Luis R. Rodriguez wrote:
> On 3/29/07, mohamed <mabbas@linux.intel.com> wrote:
> > On Tue, 2007-03-27 at 13:29 -0400, Luis R. Rodriguez wrote:
> > > On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> > > > On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> > > > > On 3/26/07, mohamed <mabbas@linux.intel.com> wrote:
> > > > >
> > > > > > +/* Get 11n capabilties from low level driver */
> > > > > > +static void ieee80211_fill_ht_ie(struct net_device *dev,
> > > > > > +                                struct ieee80211_ht_capability *ht_capab)
> > > > > > +{
> > > > > > +       int rc;
> > > > > > +       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > > > > > +
> > > > > > +       if (!local->ops->get_ht_capab){
> > > > > > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > > > > +               return;
> > > > > > +       }
> > > > > > +
> > > > > > +       rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
> > > > > > +       if (!rc) {
> > > > > > +               memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > > > > +               return;
> > > > > > +       }
> > > > > >
> > > > > > +       ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> > > > > > +                                       ht_capab->capabilitiesInfo);
> > > > > > +       ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> > > > > > +                                       ht_capab->extended_ht_capability_info);
> > > > > > +       ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> > > > > > +                                       ht_capab->tx_BF_capability_info);
> > > > > > +}
> > > > > > +
> > > > >
> > > > > We should memset to 0 the entire ht_capab to regardless as its coming
> > > > > from skb_put() otherwise we'll get random data there if we don't set
> > > > > it and we sure as hell don't want to just transmit that :)
> > I will memset to 0 before using the structure. I will change the patch
> > to reflect this.
> 
> Thanks
> 
> > > Also
> > > > > ieee80211_send_assoc() is already checking for
> > > > > local->ops->get_ht_capab so lets just do a BUG_ON here to capture
> > > > > cases where someone else didn't do the proper checking. So how about
> > > > > instead something like:
> > make sense.
> 
> Thanks
> 
> > > > > /* Get 11n capabilties from low level driver */
> > > > > static void ieee80211_fill_ht_ie(struct net_device *dev,
> > > > >                                 struct ieee80211_ht_capability *ht_capab)
> > > > > {
> > > > >        struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > > > >        BUG_ON(!local->ops->get_ht_capab);
> > > > >        memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
> > > > >        if (!local->ops->get_ht_capab(local_to_hw(local), ht_capab))
> > > > >                return;
> > > > >        ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
> > > > >                                        ht_capab->capabilitiesInfo);
> > > > >        ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
> > > > >                                        ht_capab->extended_ht_capability_info);
> > > > >        ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
> > > > >                                        ht_capab->tx_BF_capability_info);
> > > > > }
> > > > >
> > > >
> > > > On second thought just memset to 0 on ieee80211_send_assoc() right
> > > > after the skb_put and remove that from above.
> > > >
> > > >   Luis
> > > >
> > >
> > > Last few comments:
> > >
> > > I. In struct ieee80211_ops you added your hunk after the TSF comment,
> > > but before  u64 (*get_tsf)(struct ieee80211_hw *hw) was listed. Please
> > > move the hunk after get_tsf or before the comment.
> > >
> > > II. In ieee80211_fill_ht_ie() you do cpu_to_le* on the same fields.. I
> > > take it the driver's get_ht_capab() is supposed to fill these. Since
> > > you already defined them as little endian in the struct
> > > ieee80211_ht_capability, how about just leaving that up to the driver
> > > developer to make sure they do this? That's would be 6 lines less of
> > > code in ieee80211_fill_ht_ie().
> 
> And how about regarding the above comment (II)? We can just expect
> driver developers to follow proper endianness. That cleans that
> routine a bit more too.
> 
Yes I will do this as well
Mohamed
>   Luis

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Luis R. Rodriguez]

On 3/28/07, Johannes Berg <johannes@sipsolutions.net> wrote:

> > +     /* if low level driver support 11n fill in 11n IE */
> > +     if (ht_enabled && ifsta->ht_enabled && local->ops->get_ht_capab) {
> > +             pos = skb_put(skb, sizeof(struct ieee80211_ht_capability)+2);
> > +             *pos++ = WLAN_EID_HT_CAPABILITY;
> > +             *pos++ = sizeof(struct ieee80211_ht_capability);
> > +             ieee80211_fill_ht_ie(dev,
> > +                                  (struct ieee80211_ht_capability *)pos);
>
> Now that fill_ht_ie is so short maybe it should just be rolled into this
> code.

True, might as well.

  Luis

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Johannes Berg]

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

On Fri, 2007-03-30 at 01:48 +0200, Tomas Winkler wrote:

> Can someone  explain to me what is rational to choose between  making
> information element (IE) a struct  or 'rolling it into the code' . I'm
> finding it much more readable to use struct and when it  changes for
> example because 11n draft changes. And the change is reflected in
> compilation time. 

I wasn't talking about making it a struct or not, I was merely saying
that there isn't much point in a three-line function that does
essentially nothing related to the actual task.

johannes


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

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Johannes Berg]

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

Text email is great. But the useless attachment isn't.

> 
> Anyhow I still would like to repeat my question unrelated to the
> discussion above since I don't see IE structs defined in headers
> unlike the old wireless stack. (net/ieee80211.h)

I don't understand you. What's wrong with linux/ieee80211.h and
Mohamed's patch 1/5?

johannes


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

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Michael Wu]

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

On Monday 26 March 2007 07:38, mohamed wrote:
> Add basic support for IEEE 802.11n discovery and association.
>
> This patch adds support to discover IEEE 802.11n AP and enable
> association to 802.11n Network. It parses beacon to discover 802.11n
> IE and include HT capability information element in Association Request
> Frame.
> It also call low level driver with the HT capability available during
> association.
>
So.. what happens when the user is using a userspace mlme?

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[mabbas]

modified patch at the end

Randy Dunlap wrote:
>
>
> I'm seeing an email/patch problem here.  The lines above should be
> 3 lines only, not 6 that I'm seeing.  Maybe it's my mail client,
> but it usually works for me, so possibly yours is splitting lines
> when it should not.
>
>   
>> @TSF timer value from firmware/hardware. Currently,
>>  	 * this is only used for IBSS mode debugging and, as such, is not a
>>  	 * required function. */
>> +
>> +	/* Configure ht parameters 
>>     
>
> Line above ends with space.  :(
>
> What is "ht"?
>
>   
>>  	return cw - 1;
>>  }
>>  
>> +/* call low level driver with 11n params as it was recieved
>>     
>
>                                                       received
>   
>> +   from the AP
>> +*/   
>>     
>
> Line above ends with spaces.
>
> Kernel long comment style is:
>
> /*
>  * call low-level driver with 11n params as they were received
>  * from the AP
>  */
>
>   
>>  				     struct ieee80211_if_sta *ifsta,
>>  				     u8 *wmm_param, size_t wmm_param_len)
>> @@ -584,6 +648,15 @@ static void ieee80211_send_assoc(struct 
>>  		*pos++ = 0;
>>  	}
>>  
>> +	/* if low level driver support 11n fill in 11n IE */
>>     
>
>                                supports 11n,
>
>   
>> +	if (ht_enabled && ifsta->ht_enabled && local->ops->get_ht_capab) {
>> +		pos = skb_put(skb, sizeof(struct ieee80211_ht_capability)+2);
>> +		*pos++ = WLAN_EID_HT_CAPABILITY;
>> +		*pos++ = sizeof(struct ieee80211_ht_capability);
>> +		ieee80211_fill_ht_ie(dev, 
>> +				     (struct ieee80211_ht_capability *)pos);
>> +	}
>> +		
>>  	kfree(ifsta->assocreq_ies);
>>  	ifsta->assocreq_ies_len = (skb->data + skb->len) - ies;
>>  	ifsta->assocreq_ies = kmalloc(ifsta->assocreq_ies_len, GFP_ATOMIC);
>>     
>
>   
On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
On 3/27/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
 > On 3/26/07, mohamed <mabbas@linux.intel.com> wrote:
 >
 > > +/* Get 11n capabilties from low level driver */
 > > +static void ieee80211_fill_ht_ie(struct net_device *dev,
 > > + struct ieee80211_ht_capability *ht_capab)
 > > +{
 > > + int rc;
 > > + struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
 > > +
 > > + if (!local->ops->get_ht_capab){
 > > + memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
 > > + return;
 > > + }
 > > +
 > > + rc = local->ops->get_ht_capab(local_to_hw(local), ht_capab);
 > > + if (!rc) {
 > > + memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
 > > + return;
 > > + }
 > >
 > > + ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
 > > + ht_capab->capabilitiesInfo);
 > > + ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
 > > + ht_capab->extended_ht_capability_info);
 > > + ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
 > > + ht_capab->tx_BF_capability_info);
 > > +}
 > > +
 >
 > We should memset to 0 the entire ht_capab to regardless as its coming
 > from skb_put() otherwise we'll get random data there if we don't set
 > it and we sure as hell don't want to just transmit that :) Also
 > ieee80211_send_assoc() is already checking for
 > local->ops->get_ht_capab so lets just do a BUG_ON here to capture
 > cases where someone else didn't do the proper checking. So how about
 > instead something like:
 >
 > /* Get 11n capabilties from low level driver */
 > static void ieee80211_fill_ht_ie(struct net_device *dev,
 > struct ieee80211_ht_capability *ht_capab)
 > {
 > struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
 > BUG_ON(!local->ops->get_ht_capab);
 > memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
 > if (!local->ops->get_ht_capab(local_to_hw(local), ht_capab))
 > return;
 > ht_capab->capabilitiesInfo = (__le16) cpu_to_le16(
 > ht_capab->capabilitiesInfo);
 > ht_capab->extended_ht_capability_info = (__le16) cpu_to_le16(
 > ht_capab->extended_ht_capability_info);
 > ht_capab->tx_BF_capability_info = (__le32) cpu_to_le32(
 > ht_capab->tx_BF_capability_info);
 > }
 >

 > On second thought just memset to 0 on ieee80211_send_assoc() right
 > after the skb_put and remove that from above.

Johannes Berg wrote:
> On Mon, 2007-03-26 at 04:38 -0700, mohamed wrote:
>
>   
>> @@ -96,6 +96,10 @@ struct ieee802_11_elems {
>>  	u8 rsn_len;
>>  	u8 *erp_info;
>>  	u8 erp_info_len;
>> +	u8 *ht_cap_param;
>> +	u8 ht_cap_param_len;
>> +	u8 *ht_extra_param;
>> +	u8 ht_extra_param_len;
>>  	u8 *ext_supp_rates;
>>  	u8 ext_supp_rates_len;
>>  	u8 *wmm_info;
>>     
>
> Random note not applicable to your patch: This is quite a large struct,
> especially on 64-bit. Maybe we should be thinking about making those
> pointers there u16 offsets instead. And we *definitely* should change
> the order of these fields, having u8 and u8* alternate just sucks.
>
>   
>> +static void ieee80211_sta_ht_params(struct net_device *dev, 
>> +			struct sta_info *sta,
>> +			struct ieee80211_ht_additional_info *ht_extra_param,
>> +			struct ieee80211_ht_capability *ht_cap_param)
>> +{
>> +	int rc;
>> +	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
>> +
>> +	if (local->ops->conf_ht) {
>> +		rc = local->ops->conf_ht(local_to_hw(local), ht_cap_param, 
>> +					 ht_extra_param);
>> +
>> +		if (rc)
>> +			sta->flags &= ~WLAN_STA_HT;
>> +	} else 
>> +		sta->flags &= ~WLAN_STA_HT;
>>     
>
> Shouldn't this also set the STA_HT flag in the case where conf_ht
> returns without error?
>
>   
>> +/* Get 11n capabilties from low level driver */
>> +static void ieee80211_fill_ht_ie(struct net_device *dev,
>>     
>
> [filling in how it looks like after Luis's and my proposed changes]
>
>   
>> +				 struct ieee80211_ht_capability *ht_capab)
>> +{
>> +       struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
>> +       BUG_ON(!local->ops->get_ht_capab);
>> +       memset(ht_capab, 0, sizeof(struct ieee80211_ht_capability));
>> +       local->ops->get_ht_capab(local_to_hw(local), ht_capab);
>>     
>
> some blank lines would be good ;)
>
> Maybe we should be setting some fields to default values here that
> aren't zero? Haven't checked.
>  
>   
>> +	/* if low level driver support 11n fill in 11n IE */
>> +	if (ht_enabled && ifsta->ht_enabled && local->ops->get_ht_capab) {
>> +		pos = skb_put(skb, sizeof(struct ieee80211_ht_capability)+2);
>> +		*pos++ = WLAN_EID_HT_CAPABILITY;
>> +		*pos++ = sizeof(struct ieee80211_ht_capability);
>> +		ieee80211_fill_ht_ie(dev, 
>> +				     (struct ieee80211_ht_capability *)pos);
>> Now that fill_ht_ie is so short maybe it should just be rolled into this
>>     
Add basic support for IEEE 802.11n discovery and association.

This patch adds support to discover IEEE 802.11n AP and enable
association to 802.11n Network. It parses beacon to discover 802.11n
IE and include HT capability information element in Association Request Frame.
It also call low level driver with the HT capability available during
association.
    Tomas:

    1. Removed trailing spaces
    2. tsf handlers declarations not splited by ht hanlders
    2. fill_ht and ht_params functions removed
    3. Moved code from sta_sysfs.c to debug_sta.c

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

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 916b21b..b1bbc3d 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -529,6 +529,9 @@ #define IEEE80211_HW_TKIP_REQ_PHASE1_KEY
 	 * per-packet RC4 key with each TX frame when doing hwcrypto */
 #define IEEE80211_HW_TKIP_REQ_PHASE2_KEY (1<<14)
 
+	/* The device capable of supporting 11n */
+#define IEEE80211_HW_SUPPORT_HT_MODE (1<<15)
+
 	u32 flags;			/* hardware flags defined above */
 
 	/* Set to the size of a needed device specific skb headroom for TX skbs. */
@@ -731,6 +734,15 @@ struct ieee80211_ops {
 	 * TSF synchronization. */
 	void (*reset_tsf)(struct ieee80211_hw *hw);
 
+	/* Configure ht parameters. */
+	int (*conf_ht)(struct ieee80211_hw *hw,
+		       struct ieee80211_ht_capability *ht_cap_param,
+		       struct ieee80211_ht_additional_info *ht_extra_param);
+
+	/* Get ht capabilities from the device */
+	void (*get_ht_capab)(struct ieee80211_hw *hw,
+			     struct ieee80211_ht_capability *ht_cap_param);
+
 	/* Setup beacon data for IBSS beacons. Unlike access point (Master),
 	 * IBSS uses a fixed beacon frame which is configured using this
 	 * function. This handler is required only for IBSS mode. */
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 3012b9e..aa3035b 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -87,7 +87,7 @@ static ssize_t sta_flags_read(struct fil
 {
 	char buf[100];
 	struct sta_info *sta = file->private_data;
-	int res = scnprintf(buf, sizeof(buf), "%s%s%s%s%s%s%s%s%s",
+	int res = scnprintf(buf, sizeof(buf), "%s%s%s%s%s%s%s%s%s%s",
 		sta->flags & WLAN_STA_AUTH ? "AUTH\n" : "",
 		sta->flags & WLAN_STA_ASSOC ? "ASSOC\n" : "",
 		sta->flags & WLAN_STA_PS ? "PS\n" : "",
@@ -96,6 +96,7 @@ static ssize_t sta_flags_read(struct fil
 		sta->flags & WLAN_STA_AUTHORIZED ? "AUTHORIZED\n" : "",
 		sta->flags & WLAN_STA_SHORT_PREAMBLE ? "SHORT PREAMBLE\n" : "",
 		sta->flags & WLAN_STA_WME ? "WME\n" : "",
+		sta->flags & WLAN_STA_HT ? "HT\n" : "",
 		sta->flags & WLAN_STA_WDS ? "WDS\n" : "");
 	return simple_read_from_buffer(userbuf, count, ppos, buf, res);
 }
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index e8eea4d..dd1d031 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -89,6 +89,8 @@ struct ieee80211_sta_bss {
 	size_t rsn_ie_len;
 	u8 *wmm_ie;
 	size_t wmm_ie_len;
+	u8 *ht_ie;
+	size_t ht_ie_len;
 #define IEEE80211_MAX_SUPP_RATES 32
 	u8 supp_rates[IEEE80211_MAX_SUPP_RATES];
 	size_t supp_rates_len;
@@ -268,6 +270,7 @@ struct ieee80211_if_sta {
 	unsigned int create_ibss:1;
 	unsigned int mixed_cell:1;
 	unsigned int wmm_enabled:1;
+	unsigned int ht_enabled:1;
 	unsigned int auto_ssid_sel:1;
 	unsigned int auto_bssid_sel:1;
 	unsigned int auto_channel_sel:1;
diff --git a/net/mac80211/ieee80211_iface.c b/net/mac80211/ieee80211_iface.c
index b1b20ba..ebea50d 100644
--- a/net/mac80211/ieee80211_iface.c
+++ b/net/mac80211/ieee80211_iface.c
@@ -184,6 +184,7 @@ void ieee80211_if_set_type(struct net_de
 			IEEE80211_AUTH_ALG_SHARED_KEY;
 		ifsta->create_ibss = 1;
 		ifsta->wmm_enabled = 1;
+		ifsta->ht_enabled = 1;
 		ifsta->auto_channel_sel = 1;
 		ifsta->auto_bssid_sel = 1;
 
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 087f176..2ae0a56 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -96,6 +96,10 @@ struct ieee802_11_elems {
 	u8 rsn_len;
 	u8 *erp_info;
 	u8 erp_info_len;
+	u8 *ht_cap_param;
+	u8 ht_cap_param_len;
+	u8 *ht_extra_param;
+	u8 ht_extra_param_len;
 	u8 *ext_supp_rates;
 	u8 ext_supp_rates_len;
 	u8 *wmm_info;
@@ -197,6 +201,14 @@ #endif
 			elems->ext_supp_rates = pos;
 			elems->ext_supp_rates_len = elen;
 			break;
+		case WLAN_EID_HT_CAPABILITY:
+			elems->ht_cap_param = pos;
+			elems->ht_cap_param_len = elen;
+			break;
+		case WLAN_EID_HT_EXTRA_INFO:
+			elems->ht_extra_param = pos;
+			elems->ht_extra_param_len = elen;
+			break;
 		default:
 #if 0
 			printk(KERN_DEBUG "IEEE 802.11 element parse ignored "
@@ -230,7 +242,6 @@ static int ecw2cw(int ecw)
 	return cw - 1;
 }
 
-
 static void ieee80211_sta_wmm_params(struct net_device *dev,
 				     struct ieee80211_if_sta *ifsta,
 				     u8 *wmm_param, size_t wmm_param_len)
@@ -490,6 +501,7 @@ static void ieee80211_send_assoc(struct 
 	u16 capab;
 	struct ieee80211_sta_bss *bss;
 	int wmm = 0;
+	int ht_enabled = 0;
 
 	skb = dev_alloc_skb(local->hw.extra_tx_headroom +
 			    sizeof(*mgmt) + 200 + ifsta->extra_ie_len +
@@ -513,6 +525,8 @@ static void ieee80211_send_assoc(struct 
 			capab |= WLAN_CAPABILITY_PRIVACY;
 		if (bss->wmm_ie) {
 			wmm = 1;
+
+				ht_enabled = 1;
 		}
 		ieee80211_rx_bss_put(dev, bss);
 	}
@@ -588,6 +602,16 @@ static void ieee80211_send_assoc(struct 
 		*pos++ = 0;
 	}
 
+	/* if low level driver supports 11n, fill in 11n IE */
+	if (ht_enabled && ifsta->ht_enabled && local->ops->get_ht_capab) {
+		pos = skb_put(skb, sizeof(struct ieee80211_ht_capability)+2);
+		*pos++ = WLAN_EID_HT_CAPABILITY;
+		*pos++ = sizeof(struct ieee80211_ht_capability);
+		memset(pos, 0, sizeof(struct ieee80211_ht_capability));
+		local->ops->get_ht_capab(local_to_hw(local),
+			(struct ieee80211_ht_capability *)pos);
+	}
+
 	kfree(ifsta->assocreq_ies);
 	ifsta->assocreq_ies_len = (skb->data + skb->len) - ies;
 	ifsta->assocreq_ies = kmalloc(ifsta->assocreq_ies_len, GFP_ATOMIC);
@@ -1223,6 +1247,20 @@ static void ieee80211_rx_mgmt_assoc_resp
 	}
 	sta->supp_rates = rates;
 
+	if (elems.ht_extra_param && elems.ht_cap_param && elems.wmm_param &&
+	    ifsta->ht_enabled && local->ops->conf_ht){
+		int rc;
+		
+		rc = local->ops->conf_ht(local_to_hw(local),
+					 (struct ieee80211_ht_capability *)
+					 elems.ht_cap_param,
+					 (struct ieee80211_ht_additional_info *)
+					 elems.ht_extra_param);
+		if (!rc)
+			sta->flags |= WLAN_STA_HT;
+	}
+
+
 	rate_control_rate_init(sta, local);
 
 	if (elems.wmm_param && ifsta->wmm_enabled) {
@@ -1318,6 +1356,7 @@ static void ieee80211_rx_bss_free(struct
 	kfree(bss->wpa_ie);
 	kfree(bss->rsn_ie);
 	kfree(bss->wmm_ie);
+	kfree(bss->ht_ie);
 	kfree(bss);
 }
 
@@ -1564,6 +1603,23 @@ #endif
 		bss->wmm_ie_len = 0;
 	}
 
+	if (elems.ht_cap_param &&
+	    (!bss->ht_ie || bss->ht_ie_len != elems.ht_cap_param_len ||
+	     memcmp(bss->ht_ie, elems.ht_cap_param, elems.ht_cap_param_len))) {
+		if (bss->ht_ie)
+			kfree(bss->ht_ie);
+		bss->ht_ie = kmalloc(elems.ht_cap_param_len + 2, GFP_ATOMIC);
+		if (bss->ht_ie) {
+			memcpy(bss->ht_ie, elems.ht_cap_param - 2,
+			       elems.ht_cap_param_len + 2);
+			bss->ht_ie_len = elems.ht_cap_param_len + 2;
+		} else
+			bss->ht_ie_len = 0;
+	} else if (!elems.ht_cap_param && bss->ht_ie) {
+		kfree(bss->ht_ie);
+		bss->ht_ie = NULL;
+		bss->ht_ie_len = 0;
+	}
 
 	bss->hw_mode = rx_status->phymode;
 	bss->channel = channel;
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index ed7ca59..0010fde 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -27,6 +27,7 @@ #define WLAN_STA_AUTHORIZED BIT(5) /* If
 				    */
 #define WLAN_STA_SHORT_PREAMBLE BIT(7)
 #define WLAN_STA_WME BIT(9)
+#define WLAN_STA_HT  BIT(10)
 #define WLAN_STA_WDS BIT(27)
 
 

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[John W. Linville]

On Sat, Mar 31, 2007 at 11:37:19AM -0400, Michael Wu wrote:
> On Monday 26 March 2007 07:38, mohamed wrote:
> > Add basic support for IEEE 802.11n discovery and association.
> >
> > This patch adds support to discover IEEE 802.11n AP and enable
> > association to 802.11n Network. It parses beacon to discover 802.11n
> > IE and include HT capability information element in Association Request
> > Frame.
> > It also call low level driver with the HT capability available during
> > association.
> >
> So.. what happens when the user is using a userspace mlme?

So...Michael is unhappy that I merged this one (and maybe unhappy
about the whole series).

Mohamed, could you address his concern that your patch preclude
userland MLME functionality?  Let's make sure we aren't closing-off
that capability.

Thanks,

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

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[mabbas]

John W. Linville wrote:
> On Sat, Mar 31, 2007 at 11:37:19AM -0400, Michael Wu wrote:
>   
>> On Monday 26 March 2007 07:38, mohamed wrote:
>>     
>>> Add basic support for IEEE 802.11n discovery and association.
>>>
>>> This patch adds support to discover IEEE 802.11n AP and enable
>>> association to 802.11n Network. It parses beacon to discover 802.11n
>>> IE and include HT capability information element in Association Request
>>> Frame.
>>> It also call low level driver with the HT capability available during
>>> association.
>>>
>>>       
>> So.. what happens when the user is using a userspace mlme?
>>     
I am not familiar with userspace mlme what needed to be done?
Mohamed
>
> So...Michael is unhappy that I merged this one (and maybe unhappy
> about the whole series).
>
> Mohamed, could you address his concern that your patch preclude
> userland MLME functionality?  Let's make sure we aren't closing-off
> that capability.
>
> Thanks,
>
> John
>   

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Michael Wu]

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

On Wednesday 11 April 2007 16:04, mabbas wrote:
> I am not familiar with userspace mlme what needed to be done?
mac80211 has a userspace mlme mode which defers all things dealing with 
management frames to userspace (wpa_supplicant). It basically disables all 
the code in ieee80211_sta.c and routes management frames to a special 
interface where wpa_supplicant (or hostap) handles things. This is preferred 
and the ieee80211_sta.c code is there for backwards compatibility.

No major features should be added to the in-kernel MLME (ieee80211_sta.c) but 
even more importantly, your patch series does not provide the appropriate 
hooks for a userspace MLME to support a driver using this 802.11n API.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Tomas Winkler]

On 4/11/07, Michael Wu <flamingice@sourmilk.net> wrote:
> On Wednesday 11 April 2007 16:04, mabbas wrote:
> > I am not familiar with userspace mlme what needed to be done?
> mac80211 has a userspace mlme mode which defers all things dealing with
> management frames to userspace (wpa_supplicant). It basically disables all
> the code in ieee80211_sta.c and routes management frames to a special
> interface where wpa_supplicant (or hostap) handles things. This is preferred
> and the ieee80211_sta.c code is there for backwards compatibility.
>
> No major features should be added to the in-kernel MLME (ieee80211_sta.c) but
> even more importantly, your patch series does not provide the appropriate
> hooks for a userspace MLME to support a driver using this 802.11n API.
>
> -Michael Wu
>
>
Meanwhile I don't see feasible user space implementation for this as
data packet classification is done in kernel. BACK streams are
dynamically  opened and teared down per TID according current traffic
shape.
This is new feature and the user space handling is not well understand
yet. I suggest that we proceed with patches and let the evolution
making its steps.

If someone has concrete suggestion on implementing it as user space
MLME I will be glad to hear.
Thanks


Tomas

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[mabbas]

Michael Wu wrote:
> On Wednesday 11 April 2007 16:04, mabbas wrote:
>   
>> I am not familiar with userspace mlme what needed to be done?
>>     
> mac80211 has a userspace mlme mode which defers all things dealing with 
> management frames to userspace (wpa_supplicant). It basically disables all 
> the code in ieee80211_sta.c and routes management frames to a special 
> interface where wpa_supplicant (or hostap) handles things. This is preferred 
> and the ieee80211_sta.c code is there for backwards compatibility.
>
> No major features should be added to the in-kernel MLME (ieee80211_sta.c) but 
> even more importantly, your patch series does not provide the appropriate 
> hooks for a userspace MLME to support a driver using this 802.11n API.
>
> -Michael Wu
>   
where i can get mac80211 userspace mlme?
Mohamed

RE: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Simon Barber]

Setup and tear down of BA should be done by userspace - actual
processing of the BA frames themselves should be done in mac80211. This
allows maximum flexibility from userspace to control the policy of when
to setup and teardown BA streams.

Simon

-----Original Message-----
From: Tomas Winkler [mailto:tomasw@gmail.com] 
Sent: Wednesday, April 11, 2007 3:04 PM
To: Michael Wu
Cc: mabbas; John W. Linville; linux-wireless@vger.kernel.org; Jouni
Malinen; Simon Barber
Subject: Re: [patch 2/5] Add basic support for IEEE 802.11n discovery
and association

On 4/11/07, Michael Wu <flamingice@sourmilk.net> wrote:
> On Wednesday 11 April 2007 16:04, mabbas wrote:
> > I am not familiar with userspace mlme what needed to be done?
> mac80211 has a userspace mlme mode which defers all things dealing 
> with management frames to userspace (wpa_supplicant). It basically 
> disables all the code in ieee80211_sta.c and routes management frames 
> to a special interface where wpa_supplicant (or hostap) handles 
> things. This is preferred and the ieee80211_sta.c code is there for
backwards compatibility.
>
> No major features should be added to the in-kernel MLME 
> (ieee80211_sta.c) but even more importantly, your patch series does 
> not provide the appropriate hooks for a userspace MLME to support a
driver using this 802.11n API.
>
> -Michael Wu
>
>
Meanwhile I don't see feasible user space implementation for this as
data packet classification is done in kernel. BACK streams are
dynamically  opened and teared down per TID according current traffic
shape.
This is new feature and the user space handling is not well understand
yet. I suggest that we proceed with patches and let the evolution making
its steps.

If someone has concrete suggestion on implementing it as user space MLME
I will be glad to hear.
Thanks


Tomas

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Michael Wu]

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

On Wednesday 11 April 2007 18:23, mabbas wrote:
> where i can get mac80211 userspace mlme?
> Mohamed
It's wpa_supplicant with mlme code turned on.

Git repo: git://w1.fi/srv/git/hostap.git

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Tomas Winkler]

So you monitor your traffic load in driver, then notify user space
application which opens or tears down for you the BA stream?
What would be the notification mechanism ?


On 4/12/07, Simon Barber <simon@devicescape.com> wrote:
> Setup and tear down of BA should be done by userspace - actual
> processing of the BA frames themselves should be done in mac80211. This
> allows maximum flexibility from userspace to control the policy of when
> to setup and teardown BA streams.
>
> Simon
>
> -----Original Message-----
> From: Tomas Winkler [mailto:tomasw@gmail.com]
> Sent: Wednesday, April 11, 2007 3:04 PM
> To: Michael Wu
> Cc: mabbas; John W. Linville; linux-wireless@vger.kernel.org; Jouni
> Malinen; Simon Barber
> Subject: Re: [patch 2/5] Add basic support for IEEE 802.11n discovery
> and association
>
> On 4/11/07, Michael Wu <flamingice@sourmilk.net> wrote:
> > On Wednesday 11 April 2007 16:04, mabbas wrote:
> > > I am not familiar with userspace mlme what needed to be done?
> > mac80211 has a userspace mlme mode which defers all things dealing
> > with management frames to userspace (wpa_supplicant). It basically
> > disables all the code in ieee80211_sta.c and routes management frames
> > to a special interface where wpa_supplicant (or hostap) handles
> > things. This is preferred and the ieee80211_sta.c code is there for
> backwards compatibility.
> >
> > No major features should be added to the in-kernel MLME
> > (ieee80211_sta.c) but even more importantly, your patch series does
> > not provide the appropriate hooks for a userspace MLME to support a
> driver using this 802.11n API.
> >
> > -Michael Wu
> >
> >
> Meanwhile I don't see feasible user space implementation for this as
> data packet classification is done in kernel. BACK streams are
> dynamically  opened and teared down per TID according current traffic
> shape.
> This is new feature and the user space handling is not well understand
> yet. I suggest that we proceed with patches and let the evolution making
> its steps.
>
> If someone has concrete suggestion on implementing it as user space MLME
> I will be glad to hear.
> Thanks
>
>
> Tomas
>

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Andy Green]

Tomas Winkler wrote:

> So you monitor your traffic load in driver, then notify user space
> application which opens or tears down for you the BA stream?
> What would be the notification mechanism ?

Since hopefully the concept will involve a kernelside-filtered 
monitoring action via libpcap, the stack could for example inject a 
periodic stats summary packet down the monitor interface back to userspace.

-Andy

RE: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Simon Barber]

There are several possible notification mechanisms - according to the
application's needs. It could be part of tc - using a qdisc, or libpcap,
or something hardwired into mac80211 or something else. Someone might
want this controlled by a userspace application (e.g. video streaming).
Someone else might want an automatic algorithm.

Simon 

-----Original Message-----
From: Tomas Winkler [mailto:tomasw@gmail.com] 
Sent: Wednesday, April 11, 2007 4:05 PM
To: Simon Barber
Cc: Michael Wu; mabbas; John W. Linville;
linux-wireless@vger.kernel.org; Jouni Malinen
Subject: Re: [patch 2/5] Add basic support for IEEE 802.11n discovery
and association

So you monitor your traffic load in driver, then notify user space
application which opens or tears down for you the BA stream?
What would be the notification mechanism ?


On 4/12/07, Simon Barber <simon@devicescape.com> wrote:
> Setup and tear down of BA should be done by userspace - actual 
> processing of the BA frames themselves should be done in mac80211. 
> This allows maximum flexibility from userspace to control the policy 
> of when to setup and teardown BA streams.
>
> Simon
>
> -----Original Message-----
> From: Tomas Winkler [mailto:tomasw@gmail.com]
> Sent: Wednesday, April 11, 2007 3:04 PM
> To: Michael Wu
> Cc: mabbas; John W. Linville; linux-wireless@vger.kernel.org; Jouni 
> Malinen; Simon Barber
> Subject: Re: [patch 2/5] Add basic support for IEEE 802.11n discovery 
> and association
>
> On 4/11/07, Michael Wu <flamingice@sourmilk.net> wrote:
> > On Wednesday 11 April 2007 16:04, mabbas wrote:
> > > I am not familiar with userspace mlme what needed to be done?
> > mac80211 has a userspace mlme mode which defers all things dealing 
> > with management frames to userspace (wpa_supplicant). It basically 
> > disables all the code in ieee80211_sta.c and routes management 
> > frames to a special interface where wpa_supplicant (or hostap) 
> > handles things. This is preferred and the ieee80211_sta.c code is 
> > there for
> backwards compatibility.
> >
> > No major features should be added to the in-kernel MLME
> > (ieee80211_sta.c) but even more importantly, your patch series does 
> > not provide the appropriate hooks for a userspace MLME to support a
> driver using this 802.11n API.
> >
> > -Michael Wu
> >
> >
> Meanwhile I don't see feasible user space implementation for this as 
> data packet classification is done in kernel. BACK streams are 
> dynamically  opened and teared down per TID according current traffic 
> shape.
> This is new feature and the user space handling is not well understand

> yet. I suggest that we proceed with patches and let the evolution 
> making its steps.
>
> If someone has concrete suggestion on implementing it as user space 
> MLME I will be glad to hear.
> Thanks
>
>
> Tomas
>

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Tomas Winkler]

Actually the recipient part I can see be implemented in the  user
space MLME. The initiator  part requires request from  kernel to user
space to start the BA negotiation.
Still there is some amount  internal state information that need to be
passed back and forth that make user space MLME to be a long shot, bat
eventually we get there.
Dynamic algorithm look like best option to me as keeping underutilized
stream only hurts the performance. User application may tune
thresholds and timeouts and of course must tag the traffic.


On 4/12/07, Simon Barber <simon@devicescape.com> wrote:
> There are several possible notification mechanisms - according to the
> application's needs. It could be part of tc - using a qdisc, or libpcap,
> or something hardwired into mac80211 or something else. Someone might
> want this controlled by a userspace application (e.g. video streaming).
> Someone else might want an automatic algorithm.
>
> Simon
>
> -----Original Message-----
> From: Tomas Winkler [mailto:tomasw@gmail.com]
> Sent: Wednesday, April 11, 2007 4:05 PM
> To: Simon Barber
> Cc: Michael Wu; mabbas; John W. Linville;
> linux-wireless@vger.kernel.org; Jouni Malinen
> Subject: Re: [patch 2/5] Add basic support for IEEE 802.11n discovery
> and association
>
> So you monitor your traffic load in driver, then notify user space
> application which opens or tears down for you the BA stream?
> What would be the notification mechanism ?
>
>
> On 4/12/07, Simon Barber <simon@devicescape.com> wrote:
> > Setup and tear down of BA should be done by userspace - actual
> > processing of the BA frames themselves should be done in mac80211.
> > This allows maximum flexibility from userspace to control the policy
> > of when to setup and teardown BA streams.
> >
> > Simon
> >
> > -----Original Message-----
> > From: Tomas Winkler [mailto:tomasw@gmail.com]
> > Sent: Wednesday, April 11, 2007 3:04 PM
> > To: Michael Wu
> > Cc: mabbas; John W. Linville; linux-wireless@vger.kernel.org; Jouni
> > Malinen; Simon Barber
> > Subject: Re: [patch 2/5] Add basic support for IEEE 802.11n discovery
> > and association
> >
> > On 4/11/07, Michael Wu <flamingice@sourmilk.net> wrote:
> > > On Wednesday 11 April 2007 16:04, mabbas wrote:
> > > > I am not familiar with userspace mlme what needed to be done?
> > > mac80211 has a userspace mlme mode which defers all things dealing
> > > with management frames to userspace (wpa_supplicant). It basically
> > > disables all the code in ieee80211_sta.c and routes management
> > > frames to a special interface where wpa_supplicant (or hostap)
> > > handles things. This is preferred and the ieee80211_sta.c code is
> > > there for
> > backwards compatibility.
> > >
> > > No major features should be added to the in-kernel MLME
> > > (ieee80211_sta.c) but even more importantly, your patch series does
> > > not provide the appropriate hooks for a userspace MLME to support a
> > driver using this 802.11n API.
> > >
> > > -Michael Wu
> > >
> > >
> > Meanwhile I don't see feasible user space implementation for this as
> > data packet classification is done in kernel. BACK streams are
> > dynamically  opened and teared down per TID according current traffic
> > shape.
> > This is new feature and the user space handling is not well understand
>
> > yet. I suggest that we proceed with patches and let the evolution
> > making its steps.
> >
> > If someone has concrete suggestion on implementing it as user space
> > MLME I will be glad to hear.
> > Thanks
> >
> >
> > Tomas
> >
>

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Johannes Berg]

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

On Thu, 2007-04-12 at 00:25 +0100, Andy Green wrote:

> Since hopefully the concept will involve a kernelside-filtered 
> monitoring action via libpcap, the stack could for example inject a 
> periodic stats summary packet down the monitor interface back to userspace.

Eek. No *new* fake packets any more please.

johannes

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

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Andy Green]

Johannes Berg wrote:
> On Thu, 2007-04-12 at 00:25 +0100, Andy Green wrote:
> 
>> Since hopefully the concept will involve a kernelside-filtered 
>> monitoring action via libpcap, the stack could for example inject a 
>> periodic stats summary packet down the monitor interface back to userspace.
> 
> Eek. No *new* fake packets any more please.

Just curious, why is that evil?  It seems if the usermode MLME is 
interested in what are basically Out Of Band 1Mbps management packets 
that share the same air as the payload packets and so appear on Monitor 
mode interfaces, then a local OOB class of packet doesn't break such new 
ground.  And it does neatly fit the communication semantic already used.

-Andy

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[Johannes Berg]

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

On Thu, 2007-04-12 at 10:01 +0100, Andy Green wrote:

> Just curious, why is that evil?  It seems if the usermode MLME is 
> interested in what are basically Out Of Band 1Mbps management packets 
> that share the same air as the payload packets and so appear on Monitor 
> mode interfaces, then a local OOB class of packet doesn't break such new 
> ground.  And it does neatly fit the communication semantic already used.

Oh but those aren't *really* fake then. In this case, I'm ambivalent,
but the stuff we have in there for some things like radar notification
is just ugly.

johannes

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

Re: [patch 2/5] Add basic support for IEEE 802.11n discovery and association

[mabbas]

Michael Wu wrote:
> On Wednesday 11 April 2007 18:23, mabbas wrote:
>   
>> where i can get mac80211 userspace mlme?
>> Mohamed
>>     
> It's wpa_supplicant with mlme code turned on.
>
> Git repo: git://w1.fi/srv/git/hostap.git
>
> -Michael
just adding the hooks to userspace mlme would be enough to get these 
patches accepted. then I can look into wpa_supplicant.
Mohamed
> 	
>