Re: [PATCH v3] cfg80211/mac80211: Add 802.11d support

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

On Fri, Nov 07, 2008 at 01:27:22PM -0800, Johannes Berg wrote:
> This replaces 3/3, right? Just got confused with the order.

Yes, it does.

> > +/*
> > + * IEEE 802.11-2007 7.3.2.9 Country information element
> > + *
> > + * Minimum length is 8 octects, ie len must be evenly
> 
> typo: octets

I'll fix.

> > +/**
> > + * regulatory_hint_11d - hints a country IE as a regulatory domain
> > + * @wiphy: the wireless device giving the hint (used only for reporting
> > + *	conflicts)
> > + * @country_ie: pointer to the country IE
> > + * @country_ie_len: length of the country IE
> > + *
> > + * We will intersect the rd with the what CRDA tells us should apply
> > + * for the alpha2 this country IE belongs to, this prevents APs from
> > + * sending us incorrect or outdated information against a country.
> > + */
> > +extern void regulatory_hint_11d(struct wiphy *wiphy,
> > +			        u8 *country_ie,
> > +			        u8 country_ie_len);
> 
> that looks good :)
> 
> > +	if (elems.country_elem) {
> > +		/* Note we are only reviewing this on beacons
> > +		 * we are associated to */
> 
> that sounds a little awkward, are we really associated to beacons? :)

I'll fix.

> > diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig
> > index ae7f226..c40a0f2 100644
> > --- a/net/wireless/Kconfig
> > +++ b/net/wireless/Kconfig
> > @@ -1,6 +1,15 @@
> >  config CFG80211
> >          tristate "Improved wireless configuration API"
> >  
> > +config CFG80211_REG_DEBUG
> > +        bool "cfg80211 regulatory debugging"
> 
> space indentation

I'll fix.

> > index 72825af..e2dda15 100644
> > --- a/net/wireless/core.c
> > +++ b/net/wireless/core.c
> > @@ -19,7 +19,6 @@
> >  #include "nl80211.h"
> >  #include "core.h"
> >  #include "sysfs.h"
> > -#include "reg.h"
> 
> why remove that?

Because core.h adds it now and as you can see core.c includes core.h

> > +/* Converts a country IE to a regulatory domain. A regulatory domain
> > + * structure has a lot of information which the IE doesn't yet have,
> > + * so for the other values we use upper max values as we will intersect
> > + * with our userspace regulatory agent to get lower bounds. */
> > +static struct ieee80211_regdomain *country_ie_2_rd(
> > +				u8 *country_ie,
> > +				u8 country_ie_len,
> > +				u32 *checksum)
> > +{
> > +	struct ieee80211_regdomain *rd = NULL;
> > +	unsigned int i = 0;
> > +	char alpha2[2];
> > +	u32 flags = 0;
> > +	u32 num_rules = 0, size_of_regd = 0;
> > +	u8 *triplets_start = NULL;
> > +	u8 len_at_triplet = 0;
> > +	/* Used too often */
> > +	int triplet_size = sizeof(struct ieee80211_country_ie_triplet);
> 
> It's always like '3', no?

Heh ok.

> > +	int last_sub_max_channel = 0;
> > +
> > +	*checksum = 0xDEADBEEF;
> > +
> > +	/* Country IE requirements */
> > +	BUG_ON(country_ie_len < IEEE80211_COUNTRY_IE_MIN_LEN ||
> > +		country_ie_len & 0x01);
> 
> That seems rather dangerous?

Nope, as you discovered below later on.

> > +		/* When dot11RegulatoryClassesRequired is supported
> > +		 * we can throw ext triplets as part of this soup,
> > +		 * for now we don't care when those change as we
> > +		 * don't support them */
> > +		*checksum ^= cur_channel ^ cur_sub_max_channel ^
> > +			triplet->chans.max_power;
> 
> Shouldn't you shift the checksum a bit so that you don't always just use
> the lowest byte of it?

Good point, I'll make use of the 32 bis in the next respin.

> > +		/* Note: this is not a IEEE requirement but
> > +		 * simply a memory requirement */
> > +		if (num_rules > NL80211_MAX_SUPP_REG_RULES)
> > +			return NULL;
> 
> We could set NL80211_MAX_SUPP_REG_RULES to 85 or something, then
> everything would fit. The IE is limited to 255 bytes after all. I don't
> think it matters though.

Good point, but where do  you see the limit is 255 bytes?
If it is I'd rather do this in a separate patch though after this too.

> > +		/* The +10 is since the regulatory domain expects
> > +		 * the actual band edge, not the center of freq for
> > +		 * its start and end freqs, assuming 20 MHz bandwidth on
> > +		 * the channels passed */
> > +		freq_range->start_freq_khz =
> > +			MHZ_TO_KHZ(ieee80211_channel_to_frequency(
> > +				triplet->chans.first_channel) + 10);
> > +		freq_range->end_freq_khz =
> > +			MHZ_TO_KHZ(ieee80211_channel_to_frequency(
> > +				triplet->chans.first_channel +
> > +					triplet->chans.num_channels) + 10);
> 
> Doesn't that have to be -10 in the start freq?

Thanks for picking that up.

> > +static bool reg_same_country_ie_hint(struct wiphy *wiphy,
> > +			u32 country_ie_checksum)
> > +{
> > +	if (!last_request->wiphy)
> > +		return false;
> > +	if (likely(last_request->wiphy != wiphy)) {
> > +		if (likely(!country_ie_integrity_changes(country_ie_checksum)))
> > +			return true;
> > +		return false;
> 
> how about just
> 	return !country_ie_integrity_changes(...);

Sure.

> > +	/* IE len must be evenly divisible by 2 */
> > +	if (country_ie_len & 0x01)
> > +		goto out;
> > +
> > +	if (country_ie_len < IEEE80211_COUNTRY_IE_MIN_LEN)
> > +		goto out;
> 
> ok nevermind, the BUG_ON above won't ever happen because of this.

Yeap.

> > +	/* Pending country IE processing, this can happen after we
> > +	 * call CRDA and wait for a response if a beacon was received before
> > +	 * we were able to process the last regulatory_hint_11d() call */
> > +	if (country_ie_regdomain)
> > +		goto out;
> > +
> > +	alpha2[0] = country_ie[0];
> > +	alpha2[1] = country_ie[1];
> > +
> > +	if (country_ie[2] == 'I')
> > +		env = ENVIRON_INDOOR;
> > +	else if (country_ie[2] == 'O')
> > +		env = ENVIRON_OUTDOOR;
> > +
> > +	/* We will run this for *every* beacon processed for the BSSID, so
> > +	 * we optimize an early check to exit out early if we don't have to
> > +	 * do anything */
> 
> hopefully soon we won't be processing that many beacons but only changed
> ones :)

Yeap...

> > +	rd = country_ie_2_rd(country_ie, country_ie_len, &checksum);
> > +	if (!rd)
> > +		goto out;
> > +
> > +	/* This shouldn't happen right now */
> > +	if (likely(reg_same_country_ie_hint(wiphy, checksum)))
> > +		goto out;
> 
> "shouldn't happen"

I meant that since we now check for alpha+env on the secondary devices
we shouldn't run into this.

> doesn't really go with likely()?

But if we do, it would be under some future circumstance like
suspend/resume and going to a different country or moving to
another AP as you had suggested. I'll add a WARN_ON() and also
add a good comment about this.

> Overall looks pretty good to me. Much better than before, don't you
> think? :)

Sure thing, I was just a bit hesitant to move IE handling code to
cfg80211 but in this case it makes sense. Sure makes it easier to follow
I hope.

  Luis