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