Re: [RFC] wireless: Add 802.11d support

[Johannes Berg <johannes@sipsolutions.net>]

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

On Thu, 2008-10-30 at 21:43 -0700, Luis R. Rodriguez wrote:

> +struct ieee80211_regdomain *ieee80211_country_ie_2_rd(
> +				u8 *country_ie, u8 country_ie_len)
> +{
> +	struct ieee80211_regdomain *rd = NULL;
> +	u32 flags = 0;
> +	u32 num_rules = 0, size_of_regd = 0;
> +	u8 *triplets_start = NULL;
> +	u8 len_at_triplet = 0;
> +	unsigned int i = 0;
> +	/* Used too often */
> +	int triplet_size = sizeof(struct ieee80211_country_ie_triplet);
> +
> +	rd->alpha2[0] = country_ie[0];
> +	rd->alpha2[1] = country_ie[1];
> +
> +	/*
> +	 * Third octet can be:
> +	 *    'I' - Indoor
> +	 *    'O' - Outdoor
> +	 *
> +	 *  anything else we assume is no restrictions
> +	 */
> +	if (country_ie[2] == 'I')
> +		flags = NL80211_RRF_NO_OUTDOOR;
> +	else if (country_ie[2] == 'O')
> +		flags = NL80211_RRF_NO_INDOOR;
> +
> +	country_ie += 3;
> +	country_ie_len -= 3;

This needs to verify that the country IE is valid to start with, i.e. >=
3 bytes long.

<snipped, will review algorithm and stuff later>

> +#ifdef CONFIG_WIRELESS_OLD_REGULATORY
> +		/* We can't expect to have anything to intersect with...
> +		 * so use standard low values */
> +		freq_range->max_bandwidth_khz = MHZ_TO_KHZ(40);
> +		power_rule->max_antenna_gain = DBI_TO_MBI(6);
> +		power_rule->max_eirp = DBM_TO_MBM(20);
> +#else
> +		/* Large arbitrary values, we intersect later */
> +		/* Increment this if we ever support >= 40 MHz channels
> +		 * in IEEE 802.11 */
> +		freq_range->max_bandwidth_khz = MHZ_TO_KHZ(40);
> +		power_rule->max_antenna_gain = DBI_TO_MBI(100);
> +		power_rule->max_eirp = DBM_TO_MBM(100);
> +#endif

Why is this necessary? In case of OLD_REGULATORY we _do_ have values to
intersect with, from the old hardcoded stuff. Please don't treat that so
special, just treat old regulatory as though we had pre-loaded some
regulatory information into the kernel.


> +#ifdef CONFIG_WIRELESS_OLD_REGULATORY
> +	/* If the userspace regulatory agent is installed when
> +	 * OLD_REGULATORY is enabled then great, if not then oh well,
> +	 * we'll use low ball values */
> +	return 0;
> +#endif

Same here.

> +	if (!r) {
> +		printk(KERN_ERR "cfg80211: calling CRDA failed - "
> +			"unable to get country regulatory information "
> +			"for country IE\n");
> +		return -EINVAL;
> +	}

This printk is misplaced, if it should be there at all it should be in
cfg80211, mac80211 need not know anything about crda. Also, !r is
incorrect, just remove the whole code block and do

return regulatory_hint_ie(...);

Also, the printk shouldn't be there at all because otherwise it'll
happen all the time when a country IE is received w/o crda installed,
flooding the logs.

> +	if (elems.country_elem) {
> +		r = ieee80211_sta_process_country_ie(ifsta, local,
> +			elems.country_elem, elems.country_elem_len);

Some code here or in the processing function should compare the country
element and only process it if it changed to avoid calling crda 10 times
per second....

> +#ifdef CONFIG_MAC80211_VERBOSE_SPECT_MGMT_DEBUG
> +		if (!r)

!r is wrong again

johannes

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