Re: [PATCH] cfg80211: fix regression on beacon world roaming feature

["Luis R. Rodriguez" <lrodriguez@atheros.com>]

On Fri, Jul 31, 2009 at 10:28 AM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:
> On Fri, Jul 31, 2009 at 9:43 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
>> On Fri, 2009-07-31 at 09:32 -0700, Luis R. Rodriguez wrote:
>>
>>> Its not that simple. Consider the fact we build our own custom
>>> regulatory domain and stuff what is intended for each one of them. The
>>> _orig stuff is set upon channel registration so it is correct that
>>> we'd have to avoid setting the channel flags prior to wiphy
>>> registration. How you do that is left up to implementation. Since
>>> cfg80211 will set the channel *_orig params based on
>>> wiphy_registration() you're only option is to either prevent your
>>> wiphy being registered with the flags set or to reset the channel
>>> flags on the reg_notifier(). The later seems like the way to go but we
>>> currently do not call the reg_notifier() upon beacon hints -- we
>>> currently only call the reg_notifier() upon regulatory domain changes
>>> and upon wiphy registration. My point is both of these options require
>>> considerable changes for 2.6.31.
>>
>> Aha. So the problem really is that we don't have a reg notifier on
>> updates due to beacons.
>
> Yeah, seems that's the core issue from the original implementation. We
> should have added that.
>
>>> > and set them in ath_reg_apply_beaconing_flags and
>>> > ath_reg_apply_active_scan_flags, changing the polarity?
>>> >
>>> > I mean, right now you tell cfg80211 you don't support it,
>>> > and then try
>>> > to support it anyhow.
>>>
>>> Not sure I follow, support what? The reg_notifier() is there for
>>> regulatory domain changes, we didn't call it upon beacon hints. We
>>> can, and I agree its the right approach, but not for 2.6.31.
>>
>> I'm thinking more in terms of iwlwifi here, for all intents and purposes
>> it doesn't support beaconing on channel N, so it only supports NO_IBSS
>> on that channel. Which is what it puts into flags before registration.
>
> Ah I see, well but remember also that devices may have custom world
> regulatory domains for which they can set the no-ibss/passive scan
> flags and then allow for it due to a beacon hint.
>
>>> > Instead, you could tell cfg80211 you _do_ support
>>> > it, and then not support them depending on the notifier? It seems like
>>> > that should work and not break cfg80211's assumption that you can never
>>> > ever support _more_ than registration flags (orig_flags).
>>>
>>> I'm not following at all, orig_flags do not tell cfg80211 the channel
>>> flags the device supports. Most devices set flag to 0 upon wiphy
>>> registration and therefore cfg80211 sets orig_flags to 0 as well
>>> during wiphy registration.
>>
>> Well ok, "supports" is a bad word since we're talking about
>> restrictions. But for iwlwifi the orig_flags _do_ tell cfg80211 the
>> channel restrictions that the device _requires_.
>
> Oh ok, got it, yeah agreed, but its important to note we never
> documented nor was I aware we wanted to treat flags set prior to wiphy
> registration as capabilities. Truth is the device is actually capable
> of active scanning and beaconing on those channels but the design of
> the firmware doesn't allow for it.
>
>> Your devices, however, do not _strictly_ _require_ those channel flags,
>> they just require them for compliance. Unlike iwlwifi which crashes when
>> you don't do it right :)
>>
>> So I think we're in agreement -- the proper solution would be to call
>> the reg notifier on beacon hints too.
>
> Yeah, unless we want to just document an exception to the rules, but I
> do agree that seems like a slippery slope.
>
>> What we do for .31 I can't say I
>> really care,
>
> OK, I do, please consider this patch for 2.6.31 and also on
> wireless-testing for now.
>
>>  how much work do you think this would be? It doesn't seem
>> all _that_ bad, at least for core changes?
>
> After some thinking the reg_notifier() does not seem to be the
> appropriate place for this. The reg_notifier() was designed to account
> for regulatory domain changes, not enhancements such as beacon hints.
> Consider the regulatory_request structure passed as the second
> argument to reg_notifier() and the currently supported types of
> regulatory domain changes, %REGDOM_SET_BY_*.
>
> What seems cleaner is to add a beacon_hint() notifier and use that at
> the end of handle_reg_beacon() so that we already give the driver the
> channel on which a beacon was received.

And this in itself would be a no-op if we use the patch I posted, and
we treat iwlwifi on this matter as an exception. So I'm now more happy
to support this patch as-is.

  Luis