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

[Johannes Berg <johannes@sipsolutions.net>]

[-- 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 --]