From: Ron Rindjunsky <[email protected]>
This patch gives a framework that will enable the mac80211 to inform
to low level driver about association status changes + changes in 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 obsolete current ops for each BSS change such as erp or wmm.
In legacy networks the B G coexistence is handled 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_erp_info will be used to inform about erp changes.
2 - struct ieee80211_wmm_info will be used to inform about wmm changes
3 - struct ieee80211_bss_data will be used to transfer this data to
low-level driver
4 - bss_info_changed will triger the change in low-level driver
Signed-off-by: Ron Rindjunsky <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
include/net/mac80211.h | 29 ++++++++++++++++
net/mac80211/ieee80211_sta.c | 77 +++++++++++++++++++++++++-----------------
2 files changed, 75 insertions(+), 31 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2b1bffb..fbd7e31 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -237,6 +237,33 @@ struct ieee80211_low_level_stats {
unsigned int dot11RTSSuccessCount;
};
+/* next structs enable the mac80211 to send association 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 structs */
+struct ieee80211_erp_info {
+ u8 changes;
+ int cts_protection;
+ int preamble;
+};
+
+struct ieee80211_wmm_info {
+ int queue;
+ struct ieee80211_tx_queue_params *params;
+};
+
+#define ASSOC_CHNAGED_STATE (1<<0)
+#define ASSOC_CHNAGED_AID (1<<1)
+#define ASSOC_CHNAGED_ERP (1<<2)
+#define ASSOC_CHNAGED_WMM (1<<3)
+
+struct ieee80211_bss_data {
+ u8 changed_map; /* use ASSOC_CHNAGED_.. to indicate changed element */
+ u8 assoc; /* 0 not assoc, 1 assoc */
+ u16 aid;
+ struct ieee80211_erp_info erp_info;
+ struct ieee80211_wmm_info wmm_info;
+};
+
/* 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). */
@@ -1021,6 +1048,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_sta.c b/net/mac80211/ieee80211_sta.c
index 7c93f29..f38eb5a 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -408,56 +408,66 @@ 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_bss_data *bss_data)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
union iwreq_data wrqu;
- if (!!(ifsta->flags & IEEE80211_STA_ASSOCIATED) == assoc)
+ if (!bss_data->changed_map)
return;
- if (assoc) {
- struct ieee80211_sub_if_data *sdata;
- struct ieee80211_sta_bss *bss;
+ if (bss_data->changed_map & ASSOC_CHNAGED_STATE) {
+ if (bss_data->assoc) {
+ struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_sta_bss *bss;
- ifsta->flags |= IEEE80211_STA_ASSOCIATED;
+ ifsta->flags |= IEEE80211_STA_ASSOCIATED;
- sdata = IEEE80211_DEV_TO_SUB_IF(dev);
- if (sdata->type != IEEE80211_IF_TYPE_STA)
- return;
+ sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ if (sdata->type != IEEE80211_IF_TYPE_STA)
+ return;
- bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
- if (bss) {
- if (bss->has_erp_value)
- ieee80211_handle_erp_ie(dev, bss->erp_value);
- ieee80211_rx_bss_put(dev, bss);
- }
+ bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
+ 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, 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_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);
+ 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;
+
if (deauth)
ifsta->auth_tries = 0;
ifsta->assoc_tries = 0;
- ieee80211_set_associated(dev, ifsta, 0);
+ bss_data.assoc = 0;
+ bss_data.changed_map = ASSOC_CHNAGED_STATE;
+ ieee80211_set_associated(dev, ifsta, &bss_data);
}
static void ieee80211_sta_tx(struct net_device *dev, struct sk_buff *skb,
@@ -1162,6 +1172,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;
u8 *pos;
int i, j;
@@ -1249,7 +1260,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.changed_map = ASSOC_CHNAGED_STATE;
+ bss_data.aid = aid;
+ bss_data.changed_map |= ASSOC_CHNAGED_AID;
+ ieee80211_set_associated(dev, ifsta, &bss_data);
/* 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.
On Thu, 2007-10-18 at 20:46 +0200, Tomas Winkler wrote:
> > I think this is a bit dangerous. Above, you defined the structure member
> > 'changed_map' which I personally would read as "take care, in this
> > structure the fields X, Y and Z changed over what I gave you last time".
> > However, the way it's used here is as a 'valid' bitmap, in "In this
> > struct, only the fields X, Y and Z are valid". This is a huge difference
> > if the driver happens to need multiple things at the same time, with the
> > approach you're doing here you'd need to keep track of everything in the
> > driver.
> >
> Yes we can have both valid and change bitmap to lower that burden from
> form driver.
Oh I just realised that I didn't read this closely enough. If you
actually keep this structure around then we don't need a 'valid' bitmap
at all because all info will be valid all the time. If possible, I think
that would be much preferable, but I haven't really given much thought
to how complex it would be to implement.
johannes
On Wednesday 17 October 2007 19:51:46 you wrote:
> +#define ASSOC_CHNAGED_STATE (1<<0)
> +#define ASSOC_CHNAGED_AID (1<<1)
> +#define ASSOC_CHNAGED_ERP (1<<2)
> +#define ASSOC_CHNAGED_WMM (1<<3)
> +
s/CHNAGED/CHANGED/
> + 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);
The SIOCGIOAP event should happen on both association and disassociation, not
just association.
-Michael Wu
Note: this is a RFC patch not completed. Somehow I messed up the header
Thanks for feedback.
On 10/18/07, Tomas Winkler <[email protected]> wrote:
> From: Ron Rindjunsky <[email protected]>
>
> This patch gives a framework that will enable the mac80211 to inform
> to low level driver about association status changes + changes in 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 obsolete current ops for each BSS change such as erp or wmm.
> In legacy networks the B G coexistence is handled 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_erp_info will be used to inform about erp changes.
> 2 - struct ieee80211_wmm_info will be used to inform about wmm changes
> 3 - struct ieee80211_bss_data will be used to transfer this data to
> low-level driver
> 4 - bss_info_changed will triger the change in low-level driver
>
> Signed-off-by: Ron Rindjunsky <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> ---
> include/net/mac80211.h | 29 ++++++++++++++++
> net/mac80211/ieee80211_sta.c | 77 +++++++++++++++++++++++++-----------------
> 2 files changed, 75 insertions(+), 31 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 2b1bffb..fbd7e31 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -237,6 +237,33 @@ struct ieee80211_low_level_stats {
> unsigned int dot11RTSSuccessCount;
> };
>
> +/* next structs enable the mac80211 to send association 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 structs */
> +struct ieee80211_erp_info {
> + u8 changes;
> + int cts_protection;
> + int preamble;
> +};
> +
> +struct ieee80211_wmm_info {
> + int queue;
> + struct ieee80211_tx_queue_params *params;
> +};
> +
> +#define ASSOC_CHNAGED_STATE (1<<0)
> +#define ASSOC_CHNAGED_AID (1<<1)
> +#define ASSOC_CHNAGED_ERP (1<<2)
> +#define ASSOC_CHNAGED_WMM (1<<3)
> +
> +struct ieee80211_bss_data {
> + u8 changed_map; /* use ASSOC_CHNAGED_.. to indicate changed element */
> + u8 assoc; /* 0 not assoc, 1 assoc */
> + u16 aid;
> + struct ieee80211_erp_info erp_info;
> + struct ieee80211_wmm_info wmm_info;
> +};
> +
> /* 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). */
> @@ -1021,6 +1048,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_sta.c b/net/mac80211/ieee80211_sta.c
> index 7c93f29..f38eb5a 100644
> --- a/net/mac80211/ieee80211_sta.c
> +++ b/net/mac80211/ieee80211_sta.c
> @@ -408,56 +408,66 @@ 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_bss_data *bss_data)
> {
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> union iwreq_data wrqu;
>
> - if (!!(ifsta->flags & IEEE80211_STA_ASSOCIATED) == assoc)
> + if (!bss_data->changed_map)
> return;
>
> - if (assoc) {
> - struct ieee80211_sub_if_data *sdata;
> - struct ieee80211_sta_bss *bss;
> + if (bss_data->changed_map & ASSOC_CHNAGED_STATE) {
> + if (bss_data->assoc) {
> + struct ieee80211_sub_if_data *sdata;
> + struct ieee80211_sta_bss *bss;
>
> - ifsta->flags |= IEEE80211_STA_ASSOCIATED;
> + ifsta->flags |= IEEE80211_STA_ASSOCIATED;
>
> - sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> - if (sdata->type != IEEE80211_IF_TYPE_STA)
> - return;
> + sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> + if (sdata->type != IEEE80211_IF_TYPE_STA)
> + return;
>
> - bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
> - if (bss) {
> - if (bss->has_erp_value)
> - ieee80211_handle_erp_ie(dev, bss->erp_value);
> - ieee80211_rx_bss_put(dev, bss);
> - }
> + bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
> + 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, 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_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);
> + 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;
> +
> if (deauth)
> ifsta->auth_tries = 0;
> ifsta->assoc_tries = 0;
> - ieee80211_set_associated(dev, ifsta, 0);
> + bss_data.assoc = 0;
> + bss_data.changed_map = ASSOC_CHNAGED_STATE;
> + ieee80211_set_associated(dev, ifsta, &bss_data);
> }
>
> static void ieee80211_sta_tx(struct net_device *dev, struct sk_buff *skb,
> @@ -1162,6 +1172,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;
> u8 *pos;
> int i, j;
>
> @@ -1249,7 +1260,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.changed_map = ASSOC_CHNAGED_STATE;
> + bss_data.aid = aid;
> + bss_data.changed_map |= ASSOC_CHNAGED_AID;
> + ieee80211_set_associated(dev, ifsta, &bss_data);
>
> /* 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.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Both remarks true, thanks
-----Original Message-----
From: Michael Wu [mailto:[email protected]]
Sent: Thursday, October 18, 2007 3:39 AM
To: Winkler, Tomas
Cc: [email protected]; [email protected];
[email protected]; Rindjunsky, Ron
Subject: Re: [PATCH 1/1] mac80211: adding bss_config to low driver ops
On Wednesday 17 October 2007 19:51:46 you wrote:
> +#define ASSOC_CHNAGED_STATE (1<<0)
> +#define ASSOC_CHNAGED_AID (1<<1)
> +#define ASSOC_CHNAGED_ERP (1<<2)
> +#define ASSOC_CHNAGED_WMM (1<<3)
> +
s/CHNAGED/CHANGED/
> + 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);
The SIOCGIOAP event should happen on both association and
disassociation, not
just association.
-Michael Wu
---------------------------------------------------------------------
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.
On 10/18/07, Johannes Berg <[email protected]> wrote:
> On Thu, 2007-10-18 at 01:51 +0200, Tomas Winkler wrote:
>
> > +/* next structs enable the mac80211 to send association 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 structs */
> > +struct ieee80211_erp_info {
> > + u8 changes;
> > + int cts_protection;
> > + int preamble;
> > +};
> > +
> > +struct ieee80211_wmm_info {
> > + int queue;
> > + struct ieee80211_tx_queue_params *params;
> > +};
>
> Please do kernel-doc style comments for both structures so I can
> incorporate this information easily into the mac80211 book. Also, some
> general guidelines in a DOC comment would be appreciated, like
>
> /**
> * DOC: BSS Status changes
> *
> * Describe what is expected of driver here
> */
>
We'll do
> > +struct ieee80211_bss_data {
> > + u8 changed_map; /* use ASSOC_CHNAGED_.. to indicate changed element */
> > + u8 assoc; /* 0 not assoc, 1 assoc */
> > + u16 aid;
> > + struct ieee80211_erp_info erp_info;
> > + struct ieee80211_wmm_info wmm_info;
> > +};
>
> Again, kernel-doc; also why do we actually need the sub-structuring into
> erp_info and wmm_info (and if you manage convince me that we do :), can
> we please call them just 'erp' and 'wmm' to save typing)?
>
This was just to not break all the drivers but you are right these
structures won't be needed
> This seems pretty ad-hoc to me. I think it'd be better to have them all
> in one structure and use just a single 'changed' parameter instead of
> having an extra one in the erp info. I would also use a 'flags'
> parameter instead of 'assoc' and then roll CTS protect and short
> preamble into those flags.
>
The bits in flags are possible just sometimes you need to pass a value
such as assoc id
or queue num so it will be a little mess.
> Also, the ASSOC_CHANGED_* flags should be renamed to BSS_STATE_CHANGED_*
> or so, they don't really affect just the association.
>
Fair enough
> Hmm. And here's a thought: don't we need all this information within
> mac80211 as well? Would it maybe make sense to embed it into the sta
> sdata structure and have the 'changed_map' not be in the structure but
> as a second parameter to the callback function? Then we could always use
> a pointer to the embedded structure that keeps track of this information
> and build the changed value dynamically. This saves having to initialise
> the structure all the time when calling the function and makes sure that
> even the unchanged parameters are always valid should the driver need
> them. [also see at the very end of this mail]
>
Sounds good
> > @@ -1021,6 +1048,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);
>
> Missing doc updates, but see above. I'd prefer the docs in this struct
> to just be a short note what the handler is and what the context is when
> it is invoked (like "must be atomic" or "runs under XY lock") and then
> for more details we refer to a DOC: section as I said above. That makes
> it much easier to collate it into the mac80211 book.
>
> > diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
> > index 7c93f29..f38eb5a 100644
> > --- a/net/mac80211/ieee80211_sta.c
> > +++ b/net/mac80211/ieee80211_sta.c
> > @@ -408,56 +408,66 @@ 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_bss_data *bss_data)
> > {
> > struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> > union iwreq_data wrqu;
> >
> > - if (!!(ifsta->flags & IEEE80211_STA_ASSOCIATED) == assoc)
> > + if (!bss_data->changed_map)
> > return;
>
> If we did the embedding thing then all this would probably be simpler,
> we could keep the 'bool assoc' parameter and have this function update
> the [read on after the quote]
>
> > - if (assoc) {
> > - struct ieee80211_sub_if_data *sdata;
> > - struct ieee80211_sta_bss *bss;
> > + if (bss_data->changed_map & ASSOC_CHNAGED_STATE) {
> > + if (bss_data->assoc) {
> > + struct ieee80211_sub_if_data *sdata;
> > + struct ieee80211_sta_bss *bss;
> >
> > - ifsta->flags |= IEEE80211_STA_ASSOCIATED;
> > + ifsta->flags |= IEEE80211_STA_ASSOCIATED;
>
> flag within the embedded struct instead of having another flag that
> keeps track of the same information.
>
> > - bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
> > - if (bss) {
> > - if (bss->has_erp_value)
> > - ieee80211_handle_erp_ie(dev, bss->erp_value);
> > - ieee80211_rx_bss_put(dev, bss);
> > - }
> > + bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
> > + if (bss) {
> > + if (bss->has_erp_value)
> > + ieee80211_handle_erp_ie(dev, bss->erp_value);
>
> [bad indenting]
>
> > static void ieee80211_sta_tx(struct net_device *dev, struct sk_buff *skb,
> > @@ -1162,6 +1172,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;
> > u8 *pos;
> > int i, j;
> >
> > @@ -1249,7 +1260,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.changed_map = ASSOC_CHNAGED_STATE;
> > + bss_data.aid = aid;
> > + bss_data.changed_map |= ASSOC_CHNAGED_AID;
> > + ieee80211_set_associated(dev, ifsta, &bss_data);
>
> I think this is a bit dangerous. Above, you defined the structure member
> 'changed_map' which I personally would read as "take care, in this
> structure the fields X, Y and Z changed over what I gave you last time".
> However, the way it's used here is as a 'valid' bitmap, in "In this
> struct, only the fields X, Y and Z are valid". This is a huge difference
> if the driver happens to need multiple things at the same time, with the
> approach you're doing here you'd need to keep track of everything in the
> driver.
>
Yes we can have both valid and change bitmap to lower that burden from
form driver.
Thanks for your valuable comments
> johannes
>
>
On Thu, 2007-10-18 at 18:48 +0200, Johannes Berg wrote:
> Hmm. And here's a thought: don't we need all this information within
> mac80211 as well? Would it maybe make sense to embed it into the sta
> sdata structure and have the 'changed_map' not be in the structure but
> as a second parameter to the callback function? Then we could always use
> a pointer to the embedded structure that keeps track of this information
> and build the changed value dynamically. This saves having to initialise
> the structure all the time when calling the function and makes sure that
> even the unchanged parameters are always valid should the driver need
> them. [also see at the very end of this mail]
Re-reading my own mail :)
Don't we need an if_id parameter to the callback so if the driver
supports multiple virtual STA interfaces it can make an effort to
support the settings of both BSSes? I'd be tempted to do it in mac80211
but I guess that different MAC designs have different levels of
configurability when multiple addresses are programmed so it seems the
only sane place to do it would be the driver since mac80211 cannot know
which settings are only supported once and where different settings can
be supported by the MAC.
johannes
On Thu, 2007-10-18 at 20:46 +0200, Tomas Winkler wrote:
> > /**
> > * DOC: BSS Status changes
> > *
> > * Describe what is expected of driver here
> > */
> >
>
> We'll do
Thanks. One of these days I'll get all the kernel-doc stuff sorted out
and actually post the mac80211 book for inclusion.
> > This seems pretty ad-hoc to me. I think it'd be better to have them all
> > in one structure and use just a single 'changed' parameter instead of
> > having an extra one in the erp info. I would also use a 'flags'
> > parameter instead of 'assoc' and then roll CTS protect and short
> > preamble into those flags.
> >
> The bits in flags are possible just sometimes you need to pass a value
> such as assoc id
> or queue num so it will be a little mess.
Oh right. I guess we need good docs as to which "changed" flag implies
which fields have actually changed since things like "assocation status
changed" implies that the AID is new *and* the "associated" flag is
changed too. On the other hand, maybe we should aim for some redundancy
and have a changed flag for each struct member and each flag?
Hmm. Thinking about this a bit more, the AID can actually change when
the association status didn't change, in the case where we associated to
another AP. Though we actually tell the driver that we disassociated
first, right? That makes me think we should maybe also make the BSSID
part of this call structure instead of having it in if_conf. Hmm.
> Yes we can have both valid and change bitmap to lower that burden from
> form driver.
Note that embedding the info might be a lot of work. Then again it might
just be some code shuffling and simplification. I haven't looked at it,
from the stuff I see here I just think it'd be worth it. Your tradeoffs
are probably different than mine though and in either case such a
callback is an improvement over current behaviour.
johannes
On Thu, 2007-10-18 at 01:51 +0200, Tomas Winkler wrote:
> +/* next structs enable the mac80211 to send association 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 structs */
> +struct ieee80211_erp_info {
> + u8 changes;
> + int cts_protection;
> + int preamble;
> +};
> +
> +struct ieee80211_wmm_info {
> + int queue;
> + struct ieee80211_tx_queue_params *params;
> +};
Please do kernel-doc style comments for both structures so I can
incorporate this information easily into the mac80211 book. Also, some
general guidelines in a DOC comment would be appreciated, like
/**
* DOC: BSS Status changes
*
* Describe what is expected of driver here
*/
> +struct ieee80211_bss_data {
> + u8 changed_map; /* use ASSOC_CHNAGED_.. to indicate changed element */
> + u8 assoc; /* 0 not assoc, 1 assoc */
> + u16 aid;
> + struct ieee80211_erp_info erp_info;
> + struct ieee80211_wmm_info wmm_info;
> +};
Again, kernel-doc; also why do we actually need the sub-structuring into
erp_info and wmm_info (and if you manage convince me that we do :), can
we please call them just 'erp' and 'wmm' to save typing)?
This seems pretty ad-hoc to me. I think it'd be better to have them all
in one structure and use just a single 'changed' parameter instead of
having an extra one in the erp info. I would also use a 'flags'
parameter instead of 'assoc' and then roll CTS protect and short
preamble into those flags.
Also, the ASSOC_CHANGED_* flags should be renamed to BSS_STATE_CHANGED_*
or so, they don't really affect just the association.
Hmm. And here's a thought: don't we need all this information within
mac80211 as well? Would it maybe make sense to embed it into the sta
sdata structure and have the 'changed_map' not be in the structure but
as a second parameter to the callback function? Then we could always use
a pointer to the embedded structure that keeps track of this information
and build the changed value dynamically. This saves having to initialise
the structure all the time when calling the function and makes sure that
even the unchanged parameters are always valid should the driver need
them. [also see at the very end of this mail]
> @@ -1021,6 +1048,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);
Missing doc updates, but see above. I'd prefer the docs in this struct
to just be a short note what the handler is and what the context is when
it is invoked (like "must be atomic" or "runs under XY lock") and then
for more details we refer to a DOC: section as I said above. That makes
it much easier to collate it into the mac80211 book.
> diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
> index 7c93f29..f38eb5a 100644
> --- a/net/mac80211/ieee80211_sta.c
> +++ b/net/mac80211/ieee80211_sta.c
> @@ -408,56 +408,66 @@ 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_bss_data *bss_data)
> {
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> union iwreq_data wrqu;
>
> - if (!!(ifsta->flags & IEEE80211_STA_ASSOCIATED) == assoc)
> + if (!bss_data->changed_map)
> return;
If we did the embedding thing then all this would probably be simpler,
we could keep the 'bool assoc' parameter and have this function update
the [read on after the quote]
> - if (assoc) {
> - struct ieee80211_sub_if_data *sdata;
> - struct ieee80211_sta_bss *bss;
> + if (bss_data->changed_map & ASSOC_CHNAGED_STATE) {
> + if (bss_data->assoc) {
> + struct ieee80211_sub_if_data *sdata;
> + struct ieee80211_sta_bss *bss;
>
> - ifsta->flags |= IEEE80211_STA_ASSOCIATED;
> + ifsta->flags |= IEEE80211_STA_ASSOCIATED;
flag within the embedded struct instead of having another flag that
keeps track of the same information.
> - bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
> - if (bss) {
> - if (bss->has_erp_value)
> - ieee80211_handle_erp_ie(dev, bss->erp_value);
> - ieee80211_rx_bss_put(dev, bss);
> - }
> + bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
> + if (bss) {
> + if (bss->has_erp_value)
> + ieee80211_handle_erp_ie(dev, bss->erp_value);
[bad indenting]
> static void ieee80211_sta_tx(struct net_device *dev, struct sk_buff *skb,
> @@ -1162,6 +1172,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;
> u8 *pos;
> int i, j;
>
> @@ -1249,7 +1260,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.changed_map = ASSOC_CHNAGED_STATE;
> + bss_data.aid = aid;
> + bss_data.changed_map |= ASSOC_CHNAGED_AID;
> + ieee80211_set_associated(dev, ifsta, &bss_data);
I think this is a bit dangerous. Above, you defined the structure member
'changed_map' which I personally would read as "take care, in this
structure the fields X, Y and Z changed over what I gave you last time".
However, the way it's used here is as a 'valid' bitmap, in "In this
struct, only the fields X, Y and Z are valid". This is a huge difference
if the driver happens to need multiple things at the same time, with the
approach you're doing here you'd need to keep track of everything in the
driver.
johannes