[RFC 3/3] wpa_supplicant: Report 40MHz Intolerance to associated AP.

[Jouni Malinen]

On Sun, Jul 17, 2011 at 11:36:07AM +0530, Rajkumar Manoharan wrote:
> This patch adds support for 40MHz intolerance handling in STA
> mode. STA should report of any 40MHz intolerance in case if it
> finds a non-HT AP or a HT AP which prohibits 40MHz transmission
> (i.e 40MHz intolerant bit is set in HT capabilities IE).
> 
> STA shall report this condition using 20/40 BSS coexistence
> action frame.

> diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
> @@ -951,6 +951,8 @@ static int _wpa_supplicant_event_scan_results(struct wpa_supplicant *wpa_s,
> +	wpa_supplicant_proc_40mhz_intolerant(wpa_s);

Hmm.. Is this done either too infrequently or too frequently? The former
case may need a new timer that enforces the at least one scan per
dot11BSSWidthTriggerScanInterval seconds. The latter case may need some
changes either here or maybe more likely in
wpa_supplicant_proc_40mhz_intolerant() to avoid sending too frequent
reports. I can understand sending the report immediately if something
changes, but why would the station send these after every scan if there
were no changes?

Have you looked at the frames that the AP may use to change OBSS scan
parameters or request a scan? They could also affect this behavior. In
addition, the scanning is only required for 40 MHz capable HT STAs, so
this should really be conditional on the local driver supporting HT and
40 MHz (and 2.4 GHz for that matter, but more or less all cards do
that).

> diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c

> +void sme_send_2040_bss_coex(struct wpa_supplicant *wpa_s,
> +			    u8 *chan_list, u8 n_channels)

Please use 'const u8 *' here since chan_list is not modified.

> +	pos = wpabuf_put(buf, 2);
> +	pos[0] = WLAN_ACTION_PUBLIC;
> +	pos[1] = WLAN_PA_20_40_BSS_COEX;

wpabuf_put_u8(buf, WLAN_ACTION_PUBLIC);
wpabuf_put_u8(buf, WLAN_PA_20_40_BSS_COEX);

would be the preferred way of constructing the frame.

> +	freq_to_channel(wpa_s->current_bss->freq, &ic_report->reg_class, 0);

Please use NULL instead of 0 as a pointer.


> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c

> +void wpa_supplicant_proc_40mhz_intolerant(struct wpa_supplicant *wpa_s)

> +	u8 intol_channels[28], tmp_channel;

> +	if (wpa_s->wpa_state != WPA_COMPLETED)
> +		return;
> +
> +	/* Check for not-ht association */
> +	ie = wpa_bss_get_ie(wpa_s->current_bss, WLAN_EID_HT_CAP);

It would be safer to verify that wpa_s->current_bss != NULL even though
in general, it should be in WPA_COMPLETED state.

> +	os_memset(intol_channels, 0, 28);

sizeof(intel_channels) instead of 28

> +	assoc_band = freq_to_channel(wpa_s->current_bss->freq, 0, 0);

NULL instead of 0 for pointers

> +	dl_list_for_each_safe(bss, n, &wpa_s->bss, struct wpa_bss, list) {

This loop does not seem to be removing any BSS entries, so
dl_list_for_each would be more appropriate.

> +			/* To avoid channel duplication */
> +			if (!n_channels ||
> +			    intol_channels[n_channels - 1] != tmp_channel)
> +				intol_channels[n_channels++] = tmp_channel;

Shouldn't there be some bounds checking here for the intol_channels
buffer? Where did the size (28 octets) come from?

> +			cap = (struct ieee80211_ht_capabilities *) ie + 2;
> +			if (le_to_host16(cap->ht_capabilities_info) &

Is this guaranteed to be 16-bit aligned? I would assume not and as such,
WPA_GET_LE16 should be used instead of a read that can result in
unaligned access.

> +	if (found_intolerant)
> +		sme_send_2040_bss_coex(wpa_s, intol_channels, n_channels);

This function seemed to be used only when wpa_supplicant SME is in use.
As such, it could be either moved into sme.c or at least the contents
ifdef'ed out (CONFIG_SME) to reduce binary size for non-SME builds.
 
-- 
Jouni Malinen                                            PGP id EFC895FA