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

[Rajkumar Manoharan]

On Sun, Jul 17, 2011 at 10:56:19AM +0300, Jouni Malinen wrote:
> 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?
>
The report will be sent only if HT_INFO_HT_PARAM_REC_TRANS_CHNL_WIDTH is set
in current_bss->beacon_ie (i.e AP supports HT40). Does it make sense?

> 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) &
>
Maximum number of channels in each band (24 channels in 5GHz). Because we do
check intolerance only on associated chanel's band.

> 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