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
Attachment:
signature.asc
Description: This is a digitally signed message part
