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

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

[Luis R. Rodriguez]

A regression was added through patch a4ed90d6:

"cfg80211: respect API on orig_flags on channel for beacon hint"

We did indeed respect _orig flags but the intention was not clearly
stated in the commit log. This patch fixes firmware issues picked
up by iwlwifi when we lift passive scan of beaconing restrictions
on channels its EEPROM has been configured to always enable.

By doing so though we also disallowed beacon hints on devices
registering their wiphy with custom world regulatory domains
enabled, this happens to be currently ath5k, ath9k and ar9170.
The passive scan and beacon restrictions on those devices would
never be lifted even if we did find a beacon and the hardware did
support such enhancements when world roaming.

Since Johannes indicates iwlwifi firmware cannot be changed to
allow beacon hinting we set up a flag now to specifically allow
drivers to disable beacon hints for devices which cannot use them.

We enable the flag on iwlwifi to disable beacon hints and by default
enable it for all other drivers. It should be noted beacon hints lift
passive scan flags and beacon restrictions when we receive a beacon from
an AP on any 5 GHz non-DFS channels, and channels 12-14 on the 2.4 GHz
band. We don't bother with channels 1-11 as those channels are allowed
world wide.

This should fix world roaming for ath5k, ath9k and ar9170, thereby
improving scan time when we receive the first beacon from any AP,
and also enabling beaconing operation (AP/IBSS/Mesh) on cards which
would otherwise not be allowed to do so. Drivers not using custom
regulatory stuff (wiphy_apply_custom_regulatory()) were not affected
by this as the orig_flags for the channels would have been cleared
upon wiphy registration.

I tested this with a world roaming ath5k card.

Cc: Jouni Malinen <jouni.malinen@atheros.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---

This issue affects 2.6.31, that's when the regression was introduced.

 drivers/net/wireless/iwlwifi/iwl-core.c     |    3 +++
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    3 +++
 include/net/cfg80211.h                      |    5 +++++
 net/wireless/reg.c                          |    9 +++++----
 net/wireless/reg.h                          |    3 ++-
 5 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 8570d56..071c05b 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -1600,6 +1600,9 @@ int iwl_setup_mac(struct iwl_priv *priv)
 
 	hw->wiphy->custom_regulatory = true;
 
+	/* Firmware does not support this */
+	hw->wiphy->disable_beacon_hints = true;
+
 	hw->wiphy->max_scan_ssids = PROBE_OPTION_MAX;
 	/* we create the 802.11 header and a zero-length SSID element */
 	hw->wiphy->max_scan_ie_len = IWL_MAX_PROBE_REQUEST - 24 - 2;
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 5ded898..ea051b7 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -3905,6 +3905,9 @@ static int iwl3945_setup_mac(struct iwl_priv *priv)
 
 	hw->wiphy->custom_regulatory = true;
 
+	/* Firmware does not support this */
+	hw->wiphy->disable_beacon_hints = true;
+
 	hw->wiphy->max_scan_ssids = PROBE_OPTION_MAX_3945;
 	/* we create the 802.11 header and a zero-length SSID element */
 	hw->wiphy->max_scan_ie_len = IWL_MAX_PROBE_REQUEST - 24 - 2;
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index fa72997..64df51d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1076,6 +1076,10 @@ struct cfg80211_ops {
  * 	channels at a later time. This can be used for devices which do not
  * 	have calibration information gauranteed for frequencies or settings
  * 	outside of its regulatory domain.
+ * @disable_beacon_hints: enable this if your driver needs to ensure that
+ *	passive scan flags and beaconing flags may not be lifted by cfg80211
+ *	due to regulatory beacon hints. For more information on beacon
+ *	hints read the documenation for regulatory_hint_found_beacon()
  * @reg_notifier: the driver's regulatory notification callback
  * @regd: the driver's regulatory domain, if one was requested via
  * 	the regulatory_hint() API. This can be used by the driver
@@ -1104,6 +1108,7 @@ struct wiphy {
 
 	bool custom_regulatory;
 	bool strict_regulatory;
+	bool disable_beacon_hints;
 
 	bool netnsok;
 
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index b3ac0aa..0f61ae6 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1095,17 +1095,18 @@ static void handle_reg_beacon(struct wiphy *wiphy,
 
 	chan->beacon_found = true;
 
+	if (wiphy->disable_beacon_hints)
+		return;
+
 	chan_before.center_freq = chan->center_freq;
 	chan_before.flags = chan->flags;
 
-	if ((chan->flags & IEEE80211_CHAN_PASSIVE_SCAN) &&
-	    !(chan->orig_flags & IEEE80211_CHAN_PASSIVE_SCAN)) {
+	if (chan->flags & IEEE80211_CHAN_PASSIVE_SCAN) {
 		chan->flags &= ~IEEE80211_CHAN_PASSIVE_SCAN;
 		channel_changed = true;
 	}
 
-	if ((chan->flags & IEEE80211_CHAN_NO_IBSS) &&
-	    !(chan->orig_flags & IEEE80211_CHAN_NO_IBSS)) {
+	if (chan->flags & IEEE80211_CHAN_NO_IBSS) {
 		chan->flags &= ~IEEE80211_CHAN_NO_IBSS;
 		channel_changed = true;
 	}
diff --git a/net/wireless/reg.h b/net/wireless/reg.h
index 662a9da..1471225 100644
--- a/net/wireless/reg.h
+++ b/net/wireless/reg.h
@@ -30,7 +30,8 @@ int set_regdom(const struct ieee80211_regdomain *rd);
  * non-radar 5 GHz channels.
  *
  * Drivers do not need to call this, cfg80211 will do it for after a scan
- * on a newly found BSS.
+ * on a newly found BSS. If you cannot make use of this feature you can
+ * set the wiphy->disable_beacon_hints to true.
  */
 int regulatory_hint_found_beacon(struct wiphy *wiphy,
 					struct ieee80211_channel *beacon_chan,
-- 
1.6.3.3

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

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Thu, 2009-07-30 at 17:43 -0700, Luis R. Rodriguez wrote:

> By doing so though we also disallowed beacon hints on devices
> registering their wiphy with custom world regulatory domains
> enabled, this happens to be currently ath5k, ath9k and ar9170.
> The passive scan and beacon restrictions on those devices would
> never be lifted even if we did find a beacon and the hardware did
> support such enhancements when world roaming.

I dislike this solution, it just adds proliferation of possible
behaviours. Can't those devices just set the _flags_ in their reg
notifier, and clear the corresponding bit in the orig_flags?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Luis R. Rodriguez]

On Fri, Jul 31, 2009 at 12:45 AM, Johannes
Berg<johannes@sipsolutions.net> wrote:
> On Thu, 2009-07-30 at 17:43 -0700, Luis R. Rodriguez wrote:
>
>> By doing so though we also disallowed beacon hints on devices
>> registering their wiphy with custom world regulatory domains
>> enabled, this happens to be currently ath5k, ath9k and ar9170.
>> The passive scan and beacon restrictions on those devices would
>> never be lifted even if we did find a beacon and the hardware did
>> support such enhancements when world roaming.
>
> I dislike this solution, it just adds proliferation of possible
> behaviours.

Agreed.

> Can't those devices

Which devices?

> just set the _flags_ in their reg
> notifier,

So if you mean devices that used custom reg stuff, the answer is no
because the patch which introduced the regression would prevent the
setting of the passive scan flag and beaconing restrictions so the
reg_notifier would never notice the difference.

If you mean iwlwifi devices then yes, this is possible but it also
means a big change and I'd like to see this regression fixed on
2.6.31.

> and clear the corresponding bit in the orig_flags?

iwlwifi would just have to touch flags, not orig_flags as orig_flags
should not be touched.

How about using the patch as is for now for wireless-testing and
2.6.31, and then have iwlwifi write their own reg_notifier() ?

  Luis

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

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Fri, 2009-07-31 at 08:49 -0700, Luis R. Rodriguez wrote:

> > just set the _flags_ in their reg
> > notifier,
> 
> So if you mean devices that used custom reg stuff, the answer is no
> because the patch which introduced the regression would prevent the
> setting of the passive scan flag and beaconing restrictions so the
> reg_notifier would never notice the difference.

I did mean those devices. Adding a reg notifier hook for iwlwifi seems a
little odd, given that cfg80211 is supposed to always honour the flags
(which it copies to orig_flags for that purpose).

But I don't see why ath couldn't do this:

 * do _not_ set the passive/no-ibss flags in flags (orig_flags)
 * in the reg notifier, always set the passive/no-ibss flag into 'flags'
   unless a beacon was noticed on that channel.

What am I missing?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Luis R. Rodriguez]

On Fri, Jul 31, 2009 at 8:56 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Fri, 2009-07-31 at 08:49 -0700, Luis R. Rodriguez wrote:
>
>> > just set the _flags_ in their reg
>> > notifier,
>>
>> So if you mean devices that used custom reg stuff, the answer is no
>> because the patch which introduced the regression would prevent the
>> setting of the passive scan flag and beaconing restrictions so the
>> reg_notifier would never notice the difference.
>
> I did mean those devices. Adding a reg notifier hook for iwlwifi seems a
> little odd, given that cfg80211 is supposed to always honour the flags
> (which it copies to orig_flags for that purpose).
>
> But I don't see why ath couldn't do this:
>
>  * do _not_ set the passive/no-ibss flags in flags (orig_flags)

That's up to cfg80211 through the custom regdomain, so yes, and in
fact we can WARN if these are set.

>  * in the reg notifier, always set the passive/no-ibss flag into 'flags'
>   unless a beacon was noticed on that channel.

That's fine too, we still need a fix for 2.6.31. Do you have any
thoughts on that?

  Luis

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

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

On Fri, 2009-07-31 at 09:03 -0700, Luis R. Rodriguez wrote:

> > I did mean those devices. Adding a reg notifier hook for iwlwifi seems a
> > little odd, given that cfg80211 is supposed to always honour the flags
> > (which it copies to orig_flags for that purpose).
> >
> > But I don't see why ath couldn't do this:
> >
> >  * do _not_ set the passive/no-ibss flags in flags (orig_flags)
> 
> That's up to cfg80211 through the custom regdomain, so yes, and in
> fact we can WARN if these are set.

Ah, that seems a little heavy-handed :)

> >  * in the reg notifier, always set the passive/no-ibss flag into 'flags'
> >   unless a beacon was noticed on that channel.
> 
> That's fine too, we still need a fix for 2.6.31. Do you have any
> thoughts on that?

Not really, tbh. Seems like all you'd have to do is remove the
IEEE80211_CHAN_PASSIVE_SCAN/IEEE80211_CHAN_NO_IBSS flags from your
channel registration, 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. 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).

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Luis R. Rodriguez]

On Fri, Jul 31, 2009 at 9:11 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Fri, 2009-07-31 at 09:03 -0700, Luis R. Rodriguez wrote:
>
>> > I did mean those devices. Adding a reg notifier hook for iwlwifi seems a
>> > little odd, given that cfg80211 is supposed to always honour the flags
>> > (which it copies to orig_flags for that purpose).
>> >
>> > But I don't see why ath couldn't do this:
>> >
>> >  * do _not_ set the passive/no-ibss flags in flags (orig_flags)
>>
>> That's up to cfg80211 through the custom regdomain, so yes, and in
>> fact we can WARN if these are set.
>
> Ah, that seems a little heavy-handed :)
>
>> >  * in the reg notifier, always set the passive/no-ibss flag into 'flags'
>> >   unless a beacon was noticed on that channel.
>>
>> That's fine too, we still need a fix for 2.6.31. Do you have any
>> thoughts on that?
>
> Not really, tbh. Seems like all you'd have to do is remove the
> IEEE80211_CHAN_PASSIVE_SCAN/IEEE80211_CHAN_NO_IBSS flags from your
> channel registration,

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.

> 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.

> 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.

  Luis

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

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

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.

> > 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.

> > 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_.

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. What we do for .31 I can't say I
really care, how much work do you think this would be? It doesn't seem
all _that_ bad, at least for core changes?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Luis R. Rodriguez]

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.

If we really want to stick to using the reg_notifier() on ath.ko we'd
need to make some changes to reg_notifier API, maybe consider a new
struct for the beacon hint event, and then have ath.ko iterate over
all channels and when a channel has chan->beacon_found lift the
flags.. A beacon_hint() callback seems cleaner and to the point.

  Luis

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

[Luis R. Rodriguez]

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

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

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]

On Thu, 2009-07-30 at 17:43 -0700, Luis R. Rodriguez wrote:
> A regression was added through patch a4ed90d6:
> 
> "cfg80211: respect API on orig_flags on channel for beacon hint"
> 
> We did indeed respect _orig flags but the intention was not clearly
> stated in the commit log. This patch fixes firmware issues picked
> up by iwlwifi when we lift passive scan of beaconing restrictions
> on channels its EEPROM has been configured to always enable.
> 
> By doing so though we also disallowed beacon hints on devices
> registering their wiphy with custom world regulatory domains
> enabled, this happens to be currently ath5k, ath9k and ar9170.
> The passive scan and beacon restrictions on those devices would
> never be lifted even if we did find a beacon and the hardware did
> support such enhancements when world roaming.
> 
> Since Johannes indicates iwlwifi firmware cannot be changed to
> allow beacon hinting we set up a flag now to specifically allow
> drivers to disable beacon hints for devices which cannot use them.
> 
> We enable the flag on iwlwifi to disable beacon hints and by default
> enable it for all other drivers. It should be noted beacon hints lift
> passive scan flags and beacon restrictions when we receive a beacon from
> an AP on any 5 GHz non-DFS channels, and channels 12-14 on the 2.4 GHz
> band. We don't bother with channels 1-11 as those channels are allowed
> world wide.
> 
> This should fix world roaming for ath5k, ath9k and ar9170, thereby
> improving scan time when we receive the first beacon from any AP,
> and also enabling beaconing operation (AP/IBSS/Mesh) on cards which
> would otherwise not be allowed to do so. Drivers not using custom
> regulatory stuff (wiphy_apply_custom_regulatory()) were not affected
> by this as the orig_flags for the channels would have been cleared
> upon wiphy registration.

Ok, so after all the discussion this seems like the best option after
all.

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

except I'm not sure about this bit:

>         chan->beacon_found = true;
>  
> +       if (wiphy->disable_beacon_hints)
> +               return;

seems like the if might need to be before beacon_found=true?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Luis R. Rodriguez]

On Fri, Jul 31, 2009 at 10:52 AM, Johannes
Berg<johannes@sipsolutions.net> wrote:
> On Thu, 2009-07-30 at 17:43 -0700, Luis R. Rodriguez wrote:
>> A regression was added through patch a4ed90d6:
>>
>> "cfg80211: respect API on orig_flags on channel for beacon hint"
>>
>> We did indeed respect _orig flags but the intention was not clearly
>> stated in the commit log. This patch fixes firmware issues picked
>> up by iwlwifi when we lift passive scan of beaconing restrictions
>> on channels its EEPROM has been configured to always enable.
>>
>> By doing so though we also disallowed beacon hints on devices
>> registering their wiphy with custom world regulatory domains
>> enabled, this happens to be currently ath5k, ath9k and ar9170.
>> The passive scan and beacon restrictions on those devices would
>> never be lifted even if we did find a beacon and the hardware did
>> support such enhancements when world roaming.
>>
>> Since Johannes indicates iwlwifi firmware cannot be changed to
>> allow beacon hinting we set up a flag now to specifically allow
>> drivers to disable beacon hints for devices which cannot use them.
>>
>> We enable the flag on iwlwifi to disable beacon hints and by default
>> enable it for all other drivers. It should be noted beacon hints lift
>> passive scan flags and beacon restrictions when we receive a beacon from
>> an AP on any 5 GHz non-DFS channels, and channels 12-14 on the 2.4 GHz
>> band. We don't bother with channels 1-11 as those channels are allowed
>> world wide.
>>
>> This should fix world roaming for ath5k, ath9k and ar9170, thereby
>> improving scan time when we receive the first beacon from any AP,
>> and also enabling beaconing operation (AP/IBSS/Mesh) on cards which
>> would otherwise not be allowed to do so. Drivers not using custom
>> regulatory stuff (wiphy_apply_custom_regulatory()) were not affected
>> by this as the orig_flags for the channels would have been cleared
>> upon wiphy registration.
>
> Ok, so after all the discussion this seems like the best option after
> all.
>
> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

Thanks for the review.

> except I'm not sure about this bit:
>
>>         chan->beacon_found = true;
>>
>> +       if (wiphy->disable_beacon_hints)
>> +               return;
>
> seems like the if might need to be before beacon_found=true?

Nope, we need to ensure chan->beacon_found is set so that we don't
bother processing a beacon on this channel anymore. It saves iwlwifi
from triggering beacon hints onto the workqueue.

  Luis