[PATCH 0/2] mac80211: [RFC] pass BSS info to low level driver

[PATCH 0/2] mac80211: [RFC] pass BSS info to low level driver

[Ron Rindjunsky]

The next two patches introduce a framework that can replace existing set
of low-level driver calls for each advertised change of BSS IE (for erp, wmm)
with a single call. further more, this call can inform low-level driver about
association state changes.
PATCH 1/2 - general framework
PATCH 2/2 - shows how this framework replaces erp_ie_changed
---------------------------------------------------------------------
Intel Israel (74) Limited

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

[PATCH 1/2] mac80211: [RFC] adding bss_config to low level driver ops

[Ron Rindjunsky]

This patch gives a framwork that will enable the mac80211 to inform to
low level driver about association status + the bss status due to changes
in BSS capabilities advertised by AP or in AP mode changes to be advertised
and configured to.
This will also obselete current ops for each BSS change such as erp or wmm.
In legacy networks the B G coexistence is handeled by ERP while in HT there
are many more issues such as 20/40Mhz, GF etc. It will be counterproductive
to implement handlers for each parameter, it is simpler to consolidate all
in one handler under common structure

1 - struct ieee80211_bss_data will be used to transfer data to low-level
driver
2 - bss_info_change will triger the change in low-level driver

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 include/net/mac80211.h       |   43 +++++++++++++++++++
 net/mac80211/ieee80211_i.h   |    1 +
 net/mac80211/ieee80211_sta.c |   92 +++++++++++++++++++++++++-----------------
 3 files changed, 99 insertions(+), 37 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2b1bffb..2c908ae 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -237,6 +237,41 @@ struct ieee80211_low_level_stats {
 	unsigned int dot11RTSSuccessCount;
 };
 
+#define BSS_VERIFY_STATE_ASSOC		(1<<0)
+#define BSS_VERIFY_STATE_AID		(1<<1)
+#define BSS_VERIFY_STATE_ERP_CTS_PROT	(1<<2)
+#define BSS_VERIFY_STATE_ERP_PREAMBLE	(1<<3)
+#define BSS_VERIFY_STATE_WMM		(1<<4)
+
+/**
+ * struct ieee80211_bss_data - holds the BSS's changing parameters
+ *
+ * enables the mac80211 to send association status notification to
+ * low-level driver, as well as data that may be changed through the
+ * lifetime of the BSS, after it was parsed to meaningful parameters
+ *
+ * @verify_map: use BSS_VERIFY_STATE.. to indicates the desired element
+ * @assoc: association status, 0: not associated, 1: associated
+ * @aid: association ID number, valid only when assoc is 1
+ * @use_cts_prot: use CTS protection, 0: off, 1: activate
+ * @preamble_length: 802.11b preamble leangth, 0: short, 1: long
+ * @queue: relevant queue id for the below wmm params
+ * @params: queue needed configuration
+ *
+ */
+struct ieee80211_bss_data {
+	u8 verify_map;
+	/* association related data */
+	u8 assoc;
+	u16 aid;
+	/* erp related data */
+	u8 use_cts_prot;
+	u8 preamble_length;
+	/* wmm related data */
+	u8 queue;
+	struct ieee80211_tx_queue_params *params;
+};
+
 /* Transmit control fields. This data structure is passed to low-level driver
  * with each TX frame. The low-level driver is responsible for configuring
  * the hardware to use given values (depending on what is supported). */
@@ -922,6 +957,12 @@ enum ieee80211_erp_change_flags {
  * @config_interface: Handler for configuration requests related to interfaces
  *	(e.g. BSSID changes.)
  *
+ * @bss_info_changed: Handler for configuration requests related to BSS
+ * 	parameters that may vary during BSS's lifespan, and may affect low
+ * 	level driver (e.g. assoc/disassoc status, erp parameters).
+ * 	This function should not be used if no BSS has been set, unless
+ * 	for association indication. Must be atomic.
+ *
  * @configure_filter: Configure the device's RX filter.
  *	See the section "Frame filtering" for more information.
  *	This callback must be implemented and atomic.
@@ -1021,6 +1062,8 @@ struct ieee80211_ops {
 	int (*config)(struct ieee80211_hw *hw, struct ieee80211_conf *conf);
 	int (*config_interface)(struct ieee80211_hw *hw,
 				int if_id, struct ieee80211_if_conf *conf);
+	void (*bss_info_changed)(struct ieee80211_hw *hw,
+				 struct ieee80211_bss_data *bss_data);
 	void (*configure_filter)(struct ieee80211_hw *hw,
 				 unsigned int changed_flags,
 				 unsigned int *total_flags,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index d34a9de..5755c1e 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -276,6 +276,7 @@ struct ieee80211_if_sta {
 	u32 supp_rates_bits;
 
 	int wmm_last_param_set;
+	struct ieee80211_bss_data bss_data;
 };
 
 
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index d50f857..3ccb9aa 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -384,59 +384,70 @@ static void ieee80211_sta_send_associnfo(struct net_device *dev,
 
 
 static void ieee80211_set_associated(struct net_device *dev,
-				     struct ieee80211_if_sta *ifsta,
-				     bool assoc)
+				     struct ieee80211_if_sta *ifsta)
 {
 	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
 	union iwreq_data wrqu;
+	struct ieee80211_bss_data *bss_data = &ifsta->bss_data;
 
-	if (!!(ifsta->flags & IEEE80211_STA_ASSOCIATED) == assoc)
+	if (!bss_data->verify_map)
 		return;
+
+	if (bss_data->verify_map & BSS_VERIFY_STATE_ASSOC) {
+		if (bss_data->assoc) {
+			struct ieee80211_sub_if_data *sdata;
+			struct ieee80211_sta_bss *bss;
+
+			ifsta->flags |= IEEE80211_STA_ASSOCIATED;
+
+			sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+			if (sdata->type != IEEE80211_IF_TYPE_STA)
+				return;
+
+			bss = ieee80211_rx_bss_get(dev, ifsta->bssid,
+						local->hw.conf.channel,
+						ifsta->ssid, ifsta->ssid_len);
+			if (bss) {
+				if (bss->has_erp_value)
+				    ieee80211_handle_erp_ie(dev,
+							    bss->erp_value);
+				ieee80211_rx_bss_put(dev, bss);
+			}
 
-	if (assoc) {
-		struct ieee80211_sub_if_data *sdata;
-		struct ieee80211_sta_bss *bss;
-
-		ifsta->flags |= IEEE80211_STA_ASSOCIATED;
-
-		sdata = IEEE80211_DEV_TO_SUB_IF(dev);
-		if (sdata->type != IEEE80211_IF_TYPE_STA)
-			return;
+			netif_carrier_on(dev);
+			ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
+			memcpy(ifsta->prev_bssid,
+				sdata->u.sta.bssid, ETH_ALEN);
+			memcpy(wrqu.ap_addr.sa_data,
+				sdata->u.sta.bssid, ETH_ALEN);
+			ieee80211_sta_send_associnfo(dev, ifsta);
+		} else {
+			ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
 
-		bss = ieee80211_rx_bss_get(dev, ifsta->bssid,
-					   local->hw.conf.channel,
-					   ifsta->ssid, ifsta->ssid_len);
-		if (bss) {
-			if (bss->has_erp_value)
-				ieee80211_handle_erp_ie(dev, bss->erp_value);
-			ieee80211_rx_bss_put(dev, bss);
+			netif_carrier_off(dev);
+			ieee80211_reset_erp_info(dev);
+			memset(wrqu.ap_addr.sa_data, 0, ETH_ALEN);
 		}
-
-		netif_carrier_on(dev);
-		ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
-		memcpy(ifsta->prev_bssid, sdata->u.sta.bssid, ETH_ALEN);
-		memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
-		ieee80211_sta_send_associnfo(dev, ifsta);
-	} else {
-		ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
-
-		netif_carrier_off(dev);
-		ieee80211_reset_erp_info(dev);
-		memset(wrqu.ap_addr.sa_data, 0, ETH_ALEN);
+		wrqu.ap_addr.sa_family = ARPHRD_ETHER;
+		wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
+		ifsta->last_probe = jiffies;
+		ieee80211_led_assoc(local, bss_data->assoc);
 	}
-	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
-	wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
-	ifsta->last_probe = jiffies;
-	ieee80211_led_assoc(local, assoc);
+	if (local->ops->bss_info_changed)
+		local->ops->bss_info_changed(local_to_hw(local), bss_data);
 }
 
 static void ieee80211_set_disassoc(struct net_device *dev,
 				   struct ieee80211_if_sta *ifsta, int deauth)
 {
+	struct ieee80211_bss_data *bss_data = &ifsta->bss_data;
+
 	if (deauth)
 		ifsta->auth_tries = 0;
 	ifsta->assoc_tries = 0;
-	ieee80211_set_associated(dev, ifsta, 0);
+	bss_data->assoc = 0;
+	bss_data->verify_map = BSS_VERIFY_STATE_ASSOC;
+	ieee80211_set_associated(dev, ifsta);
 }
 
 static void ieee80211_sta_tx(struct net_device *dev, struct sk_buff *skb,
@@ -1139,6 +1150,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct net_device *dev,
 	u32 rates;
 	u16 capab_info, status_code, aid;
 	struct ieee802_11_elems elems;
+	struct ieee80211_bss_data *bss_data = &ifsta->bss_data;
 	u8 *pos;
 	int i, j;
 
@@ -1200,6 +1212,8 @@ static void ieee80211_rx_mgmt_assoc_resp(struct net_device *dev,
 		return;
 	}
 
+	bss_data->verify_map = 0;
+
 	/* it probably doesn't, but if the frame includes an ERP value then
 	 * update our stored copy */
 	if (elems.erp_info && elems.erp_info_len >= 1) {
@@ -1224,7 +1238,11 @@ static void ieee80211_rx_mgmt_assoc_resp(struct net_device *dev,
 	if (ifsta->assocresp_ies)
 		memcpy(ifsta->assocresp_ies, pos, ifsta->assocresp_ies_len);
 
-	ieee80211_set_associated(dev, ifsta, 1);
+	bss_data->assoc = 1;
+	bss_data->verify_map |= BSS_VERIFY_STATE_ASSOC;
+	bss_data->aid = aid;
+	bss_data->verify_map |= BSS_VERIFY_STATE_AID;
+	ieee80211_set_associated(dev, ifsta);
 
 	/* Add STA entry for the AP */
 	sta = sta_info_get(local, ifsta->bssid);
-- 
1.5.2.4
---------------------------------------------------------------------
Intel Israel (74) Limited

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

[PATCH 2/2] mac80211: [RFC] eliminating erp_ie_changed from low level driver ops

[Ron Rindjunsky]

This patch demonstrates how erp_ie_changed can be replaced by
the new bss_info_change call. it also saves one low level ops call during
association flow, as data of erp is sent during the association flow,
not after association has been completed.
As more IE will be incorporate inside this framework, it will save low
level driver ops call. note for example that ieee80211_rx_mgmt_beacon will
be able to send all IE changes at once, instead of current serial ops calls

NOTE - This patch breaks current drivers realizing erp_ie_changed, current
patch is for RFC purposes.

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 include/net/mac80211.h       |    4 ----
 net/mac80211/ieee80211.c     |   25 ++++++++++++++++---------
 net/mac80211/ieee80211_i.h   |    3 ++-
 net/mac80211/ieee80211_sta.c |   28 +++++++++-------------------
 4 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2c908ae..51fec24 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1013,8 +1013,6 @@ enum ieee80211_erp_change_flags {
  * @sta_notify: Notifies low level driver about addition or removal
  *	of assocaited station or AP.
  *
- * @erp_ie_changed: Handle ERP IE change notifications. Must be atomic.
- *
  * @conf_tx: Configure TX queue parameters (EDCF (aifs, cw_min, cw_max),
  *	bursting) for a hardware TX queue. The @queue parameter uses the
  *	%IEEE80211_TX_QUEUE_* constants. Must be atomic.
@@ -1089,8 +1087,6 @@ struct ieee80211_ops {
 			       u32 short_retry, u32 long_retr);
 	void (*sta_notify)(struct ieee80211_hw *hw, int if_id,
 			enum sta_notify_cmd, const u8 *addr);
-	void (*erp_ie_changed)(struct ieee80211_hw *hw, u8 changes,
-			       int cts_protection, int preamble);
 	int (*conf_tx)(struct ieee80211_hw *hw, int queue,
 		       const struct ieee80211_tx_queue_params *params);
 	int (*get_tx_stats)(struct ieee80211_hw *hw,
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 64fa720..5e4f4c4 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -523,25 +523,32 @@ int ieee80211_hw_config(struct ieee80211_local *local)
 	return ret;
 }
 
-void ieee80211_erp_info_change_notify(struct net_device *dev, u8 changes)
+void ieee80211_bss_info_change_notify(struct net_device *dev,
+				    struct ieee80211_bss_data *bss_data)
 {
 	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
-	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
-	if (local->ops->erp_ie_changed)
-		local->ops->erp_ie_changed(local_to_hw(local), changes,
-			!!(sdata->flags & IEEE80211_SDATA_USE_PROTECTION),
-			!(sdata->flags & IEEE80211_SDATA_SHORT_PREAMBLE));
+
+	if (!bss_data->verify_map)
+		return;
+
+	if (local->ops->bss_info_changed)
+		local->ops->bss_info_changed(local_to_hw(local), bss_data);
+
+	bss_data->verify_map = 0;
 }
 
 void ieee80211_reset_erp_info(struct net_device *dev)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct ieee80211_if_sta *ifsta = &sdata->u.sta;
 
 	sdata->flags &= ~(IEEE80211_SDATA_USE_PROTECTION |
 			IEEE80211_SDATA_SHORT_PREAMBLE);
-	ieee80211_erp_info_change_notify(dev,
-					 IEEE80211_ERP_CHANGE_PROTECTION |
-					 IEEE80211_ERP_CHANGE_PREAMBLE);
+	ifsta->bss_data.verify_map = (BSS_VERIFY_STATE_ERP_CTS_PROT &
+					BSS_VERIFY_STATE_ERP_PREAMBLE);
+	ifsta->bss_data.use_cts_prot = 0;
+	ifsta->bss_data.preamble_length = WLAN_ERP_PREAMBLE_LONG;
+	ieee80211_bss_info_change_notify(dev, &ifsta->bss_data);
 }
 
 void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 5755c1e..db78ee7 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -760,7 +760,8 @@ struct sta_info * ieee80211_ibss_add_sta(struct net_device *dev,
 					 u8 *addr);
 int ieee80211_sta_deauthenticate(struct net_device *dev, u16 reason);
 int ieee80211_sta_disassociate(struct net_device *dev, u16 reason);
-void ieee80211_erp_info_change_notify(struct net_device *dev, u8 changes);
+void ieee80211_bss_info_change_notify(struct net_device *dev,
+				      struct ieee80211_bss_data *bss_data);
 void ieee80211_reset_erp_info(struct net_device *dev);
 
 /* ieee80211_iface.c */
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 3ccb9aa..bf9f920 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -292,9 +292,9 @@ static void ieee80211_handle_erp_ie(struct net_device *dev, u8 erp_value)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct ieee80211_if_sta *ifsta = &sdata->u.sta;
+	struct ieee80211_bss_data *bss_data = &ifsta->bss_data;
 	int use_protection = (erp_value & WLAN_ERP_USE_PROTECTION) != 0;
 	int preamble_mode = (erp_value & WLAN_ERP_BARKER_PREAMBLE) != 0;
-	u8 changes = 0;
 
 	if (use_protection != !!(sdata->flags & IEEE80211_SDATA_USE_PROTECTION)) {
 		if (net_ratelimit()) {
@@ -308,7 +308,8 @@ static void ieee80211_handle_erp_ie(struct net_device *dev, u8 erp_value)
 			sdata->flags |= IEEE80211_SDATA_USE_PROTECTION;
 		else
 			sdata->flags &= ~IEEE80211_SDATA_USE_PROTECTION;
-		changes |= IEEE80211_ERP_CHANGE_PROTECTION;
+		bss_data->verify_map |= BSS_VERIFY_STATE_ERP_CTS_PROT;
+		bss_data->use_cts_prot = use_protection;
 	}
 
 	if (preamble_mode != !(sdata->flags & IEEE80211_SDATA_SHORT_PREAMBLE)) {
@@ -324,11 +325,9 @@ static void ieee80211_handle_erp_ie(struct net_device *dev, u8 erp_value)
 			sdata->flags &= ~IEEE80211_SDATA_SHORT_PREAMBLE;
 		else
 			sdata->flags |= IEEE80211_SDATA_SHORT_PREAMBLE;
-		changes |= IEEE80211_ERP_CHANGE_PREAMBLE;
+		bss_data->verify_map |= BSS_VERIFY_STATE_ERP_PREAMBLE;
+		bss_data->preamble_length = preamble_mode;
 	}
-
-	if (changes)
-		ieee80211_erp_info_change_notify(dev, changes);
 }
 
 
@@ -396,7 +395,6 @@ static void ieee80211_set_associated(struct net_device *dev,
 	if (bss_data->verify_map & BSS_VERIFY_STATE_ASSOC) {
 		if (bss_data->assoc) {
 			struct ieee80211_sub_if_data *sdata;
-			struct ieee80211_sta_bss *bss;
 
 			ifsta->flags |= IEEE80211_STA_ASSOCIATED;
 
@@ -404,16 +402,6 @@ static void ieee80211_set_associated(struct net_device *dev,
 			if (sdata->type != IEEE80211_IF_TYPE_STA)
 				return;
 
-			bss = ieee80211_rx_bss_get(dev, ifsta->bssid,
-						local->hw.conf.channel,
-						ifsta->ssid, ifsta->ssid_len);
-			if (bss) {
-				if (bss->has_erp_value)
-				    ieee80211_handle_erp_ie(dev,
-							    bss->erp_value);
-				ieee80211_rx_bss_put(dev, bss);
-			}
-
 			netif_carrier_on(dev);
 			ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
 			memcpy(ifsta->prev_bssid,
@@ -433,8 +421,7 @@ static void ieee80211_set_associated(struct net_device *dev,
 		ifsta->last_probe = jiffies;
 		ieee80211_led_assoc(local, bss_data->assoc);
 	}
-	if (local->ops->bss_info_changed)
-		local->ops->bss_info_changed(local_to_hw(local), bss_data);
+	ieee80211_bss_info_change_notify(dev, bss_data);
 }
 
 static void ieee80211_set_disassoc(struct net_device *dev,
@@ -1224,6 +1211,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct net_device *dev,
 		if (bss) {
 			bss->erp_value = elems.erp_info[0];
 			bss->has_erp_value = 1;
+			ieee80211_handle_erp_ie(dev, bss->erp_value);
 			ieee80211_rx_bss_put(dev, bss);
 		}
 	}
@@ -1704,6 +1692,8 @@ static void ieee80211_rx_mgmt_beacon(struct net_device *dev,
 		ieee80211_sta_wmm_params(dev, ifsta, elems.wmm_param,
 					 elems.wmm_param_len);
 	}
+
+	ieee80211_bss_info_change_notify(dev, &ifsta->bss_data);
 }
 
 
-- 
1.5.2.4
---------------------------------------------------------------------
Intel Israel (74) Limited

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

Re: [PATCH 2/2] mac80211: [RFC] eliminating erp_ie_changed from low level driver ops

[Johannes Berg]

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

On Wed, 2007-10-24 at 12:22 +0200, Ron Rindjunsky wrote:
>  			enum sta_notify_cmd, const u8 *addr);
> -	void (*erp_ie_changed)(struct ieee80211_hw *hw, u8 changes,
> -			       int cts_protection, int preamble);

you forgot to remove those ERP_CHANGE flags that are passed to changes
here.

johannes

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

Re: [PATCH 2/2] mac80211: [RFC] eliminating erp_ie_changed from low level driver ops

[Ron Rindzonski]

On 10/24/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2007-10-24 at 12:22 +0200, Ron Rindjunsky wrote:
> >                       enum sta_notify_cmd, const u8 *addr);
> > -     void (*erp_ie_changed)(struct ieee80211_hw *hw, u8 changes,
> > -                            int cts_protection, int preamble);
>
> you forgot to remove those ERP_CHANGE flags that are passed to changes
> here.
>

will do , thanks

> johannes
>
>

Re: [PATCH 1/2] mac80211: [RFC] adding bss_config to low level driver ops

[Johannes Berg]

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

On Wed, 2007-10-24 at 12:22 +0200, Ron Rindjunsky wrote:

> +#define BSS_VERIFY_STATE_ASSOC		(1<<0)

Why "verify"? Shouldn't it be "CHANGED" instead?

> +struct ieee80211_bss_data {
> +	u8 verify_map;

Seeing that you already used 5 of 8 bits and we're likely to increase
this in the future, I'd go with a u32 here. Also, have you attempted to
make all fields 'valid' all the time and if so, why is there no
'changed' bitmap passed to the callback?

> +	/* association related data */
> +	u8 assoc;
> +	u16 aid;

And then reorder these two for less padding.

> + * @bss_info_changed: Handler for configuration requests related to BSS
> + * 	parameters that may vary during BSS's lifespan, and may affect low
> + * 	level driver (e.g. assoc/disassoc status, erp parameters).
> + * 	This function should not be used if no BSS has been set, unless
> + * 	for association indication.

I don't understand the last sentence here. Can you elaborate? Is it
meant as a note for mac80211 hackers? If so, wouldn't it be more
appropriate to add it to the function that calls this? Also, you didn't
add the if_id which will even break Intel's driver once multi-MAC
support is added...

> @@ -276,6 +276,7 @@ struct ieee80211_if_sta {
>  	u32 supp_rates_bits;
>  
>  	int wmm_last_param_set;
> +	struct ieee80211_bss_data bss_data;

Are you sure this will work properly? The same stuff must also be used
for AP mode, when hostapd decides to change things, no?

> +				if (bss->has_erp_value)
> +				    ieee80211_handle_erp_ie(dev,
> +							    bss->erp_value);

Seeing that you're shuffling around this code anyway, I'd put patch 2
into this patch.
 
johannes

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

Re: [PATCH 1/2] mac80211: [RFC] adding bss_config to low level driver ops

[Ron Rindzonski]

On 10/24/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2007-10-24 at 12:22 +0200, Ron Rindjunsky wrote:
>
> > +#define BSS_VERIFY_STATE_ASSOC               (1<<0)
>
> Why "verify"? Shouldn't it be "CHANGED" instead?

there are cases (erp reset for example) that values are sent to the low level
driver even if there is no change from previous state, so "verify" is
for telling the driver: "hey, check this value to see if it's the one
configured in HW, and if not change it to delivered value"
in fact i had difficulty finding a suitable word describing this. any
suggestions will
be more then welcome...

>
> > +struct ieee80211_bss_data {
> > +     u8 verify_map;
>
> Seeing that you already used 5 of 8 bits and we're likely to increase
> this in the future, I'd go with a u32 here. Also, have you attempted to
> make all fields 'valid' all the time and if so, why is there no
> 'changed' bitmap passed to the callback?
>

right, i'll change it to u32.
i believe that if i'll just follow the current flows the struct will
always be valid as it will follow every change occurs in the mac80211,
so i'll add the "valid" only if i see it is really needed.
for the "changed" use - please see above 'changed/verify" naming dilemma.


> > +     /* association related data */
> > +     u8 assoc;
> > +     u16 aid;
>
> And then reorder these two for less padding.
>
> > + * @bss_info_changed: Handler for configuration requests related to BSS
> > + *   parameters that may vary during BSS's lifespan, and may affect low
> > + *   level driver (e.g. assoc/disassoc status, erp parameters).
> > + *   This function should not be used if no BSS has been set, unless
> > + *   for association indication.
>
> I don't understand the last sentence here. Can you elaborate? Is it
> meant as a note for mac80211 hackers? If so, wouldn't it be more
> appropriate to add it to the function that calls this? Also, you didn't
> add the if_id which will even break Intel's driver once multi-MAC
> support is added...
>

if_id will be added, thanks.
my initial thought was that no BSS configuration is possible without
association first to establish this BSS, but coming to think about it
AP flows can be different - config the driver even with no association
first. i'll remove this line, and inspect my code accordingly.

> > @@ -276,6 +276,7 @@ struct ieee80211_if_sta {
> >       u32 supp_rates_bits;
> >
> >       int wmm_last_param_set;
> > +     struct ieee80211_bss_data bss_data;
>
> Are you sure this will work properly? The same stuff must also be used
> for AP mode, when hostapd decides to change things, no?
>

i see your point. do you think that sub_if_data will be more suitable?

> > +                             if (bss->has_erp_value)
> > +                                 ieee80211_handle_erp_ie(dev,
> > +                                                         bss->erp_value);
>
> Seeing that you're shuffling around this code anyway, I'd put patch 2
> into this patch.
>

will do.

> johannes
>
>

Re: [PATCH 1/2] mac80211: [RFC] adding bss_config to low level driver ops

[Johannes Berg]

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

On Thu, 2007-10-25 at 11:59 +0200, Ron Rindzonski wrote:

> there are cases (erp reset for example) that values are sent to the low level
> driver even if there is no change from previous state, so "verify" is
> for telling the driver: "hey, check this value to see if it's the one
> configured in HW, and if not change it to delivered value"
> in fact i had difficulty finding a suitable word describing this. any
> suggestions will
> be more then welcome...

Ok that makes sense. OTOH, couldn't we simply compare to the state in
the structure and if it differs set the changed flag? Then we could name
it CHANGED_*?

> > Seeing that you already used 5 of 8 bits and we're likely to increase
> > this in the future, I'd go with a u32 here. Also, have you attempted to
> > make all fields 'valid' all the time and if so, why is there no
> > 'changed' bitmap passed to the callback?
> >
> 
> right, i'll change it to u32.
> i believe that if i'll just follow the current flows the struct will
> always be valid as it will follow every change occurs in the mac80211,
> so i'll add the "valid" only if i see it is really needed.
> for the "changed" use - please see above 'changed/verify" naming dilemma.

Ok, in that case I'd still put the "changed" or "verify" into a
parameter since it's not part of the structure really, only part of
passing the structure to the driver.

> > I don't understand the last sentence here. Can you elaborate? Is it
> > meant as a note for mac80211 hackers? If so, wouldn't it be more
> > appropriate to add it to the function that calls this? Also, you didn't
> > add the if_id which will even break Intel's driver once multi-MAC
> > support is added...
> >
> 
> if_id will be added, thanks.
> my initial thought was that no BSS configuration is possible without
> association first to establish this BSS, but coming to think about it
> AP flows can be different - config the driver even with no association
> first. i'll remove this line, and inspect my code accordingly.

Alright, thanks.

> > > @@ -276,6 +276,7 @@ struct ieee80211_if_sta {
> > >       u32 supp_rates_bits;
> > >
> > >       int wmm_last_param_set;
> > > +     struct ieee80211_bss_data bss_data;
> >
> > Are you sure this will work properly? The same stuff must also be used
> > for AP mode, when hostapd decides to change things, no?
> >
> 
> i see your point. do you think that sub_if_data will be more suitable?

Yeah, I think so. It takes up space anyway since we need to add it to AP
and that is, afaik, the largest structure in that union.

johannes

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