Message ID | 1441705593-1184-16-git-send-email-ilan.peer@intel.com |
---|---|
State | Accepted |
Headers | show |
On Tue, Sep 08, 2015 at 12:46:22PM +0300, Ilan Peer wrote: > Fix the order of CSA, eCSA, secondary channel and wide bandwidth > channel switch wrapper elements in beacons and probe responses. > static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd, > + csa_pos = hostapd_eid_csa(hapd, pos); > + csa_pos = hostapd_eid_ecsa(hapd, pos); > + /* Secondary channel IE */ > + /* TODO: The spec doesn't specify a position for this IE */ > + pos = hostapd_eid_secondary_channel(hapd, pos); I could not find any language in REVmc/D4.2 that would indicate that the Secondary Channel Offset element would be included in Probe Response or Beacon frames. Could you please clarify why this element is added here (and same for ieee802_11_build_ap_params() below)? Please also note that there is an explicit note stating that Secondary Channel Offset element is never present with the Extended Channel Switch Announcement element in a frame.
On Sat, Oct 3, 2015 at 8:44 PM, Jouni Malinen <j@w1.fi> wrote: > On Tue, Sep 08, 2015 at 12:46:22PM +0300, Ilan Peer wrote: >> Fix the order of CSA, eCSA, secondary channel and wide bandwidth >> channel switch wrapper elements in beacons and probe responses. > >> static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd, > >> + csa_pos = hostapd_eid_csa(hapd, pos); > >> + csa_pos = hostapd_eid_ecsa(hapd, pos); > >> + /* Secondary channel IE */ >> + /* TODO: The spec doesn't specify a position for this IE */ >> + pos = hostapd_eid_secondary_channel(hapd, pos); > > I could not find any language in REVmc/D4.2 that would indicate that the > Secondary Channel Offset element would be included in Probe Response or > Beacon frames. Could you please clarify why this element is added here > (and same for ieee802_11_build_ap_params() below)? I went over the spec again, and it is indeed looks that Secondary channel offset shouldn't be added if CSA is done with CSA elements and not action frames. Anyway, this element is needed for 40mhz channel switch (since we don't have operating class). If we don't add this element it will break 40 mhz channel switch - so I don't understand how it is supposed to work if we comply with the spec. > Please also note that > there is an explicit note stating that Secondary Channel Offset element > is never present with the Extended Channel Switch Announcement element > in a frame. You're right, I found this note now :). However, if CSA is used together with ECSA for 40 mhz it will break the CSA only clients. I think the spec is buggy here.. But I guess we need to comply with the spec anyway, do we? Though, I'm not sure whether we can do anything other than disabling 40mhz CSA to fix this. Andrei > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap
On Tue, Oct 06, 2015 at 04:16:15PM +0300, Andrei Otcheretianski wrote: > You're right, I found this note now :). However, if CSA is used > together with ECSA > for 40 mhz it will break the CSA only clients. > > I think the spec is buggy here.. But I guess we need to comply with the spec > anyway, do we? > Though, I'm not sure whether we can do anything other than disabling 40mhz CSA > to fix this. If the standard is not correct or complete, the standard can be fixed/extended.. REVmc is in the sponsor ballot and comments can be filed against the latest draft to request changes. I can do that in the next round if you can identify the proposed changes that would be needed to make this functionality work properly. I'm fine with wpa_supplicant/hostapd not following the current standard if there is reason to believe the standard is not correct. As such, I don't think we have to remove the current behavior. Once TGmc has reviewed the comments, we may need to change the implementation to match the outcome from that review, though (if nothing else, to move the IE into the correct place which is unlikely to be the current location).
On Tue, Oct 6, 2015 at 4:35 PM, Jouni Malinen <j@w1.fi> wrote: > On Tue, Oct 06, 2015 at 04:16:15PM +0300, Andrei Otcheretianski wrote: >> You're right, I found this note now :). However, if CSA is used >> together with ECSA >> for 40 mhz it will break the CSA only clients. >> >> I think the spec is buggy here.. But I guess we need to comply with the spec >> anyway, do we? >> Though, I'm not sure whether we can do anything other than disabling 40mhz CSA >> to fix this. > > If the standard is not correct or complete, the standard can be > fixed/extended.. REVmc is in the sponsor ballot and comments can be > filed against the latest draft to request changes. I can do that in the > next round if you can identify the proposed changes that would be needed > to make this functionality work properly. > > I'm fine with wpa_supplicant/hostapd not following the current standard > if there is reason to believe the standard is not correct. As such, I > don't think we have to remove the current behavior. Once TGmc has > reviewed the comments, we may need to change the implementation to match > the outcome from that review, though (if nothing else, to move the IE > into the correct place which is unlikely to be the current location). After an internal discussion we had, looks that the spec is fine. The 11n spec requires support for eCSA, so secondary channel offset element isn't really needed. We'll remove it. Andrei > > -- > Jouni Malinen PGP id EFC895FA
diff --git a/src/ap/beacon.c b/src/ap/beacon.c index 5afaf59..32e054e 100644 --- a/src/ap/beacon.c +++ b/src/ap/beacon.c @@ -328,51 +328,12 @@ static u8 *hostapd_eid_ecsa(struct hostapd_data *hapd, u8 *eid) } -static u8 *hostapd_add_csa_elems(struct hostapd_data *hapd, u8 *pos, - u8 *start, unsigned int *csa_counter_off, - unsigned int *ecsa_counter_off) -{ - u8 *curr_pos = pos; - u8 *csa_pos = pos; - - if (!csa_counter_off || !ecsa_counter_off) - return pos; - - *csa_counter_off = 0; - *ecsa_counter_off = 0; - - curr_pos = hostapd_eid_csa(hapd, curr_pos); - - /* save an offset to the csa counter - should be last byte */ - if (curr_pos != pos) - *csa_counter_off = curr_pos - start - 1; - - csa_pos = curr_pos; - curr_pos = hostapd_eid_ecsa(hapd, curr_pos); - - /* save an offset to the eCSA counter - should be last byte */ - if (curr_pos != csa_pos) - *ecsa_counter_off = curr_pos - start - 1; - - /* at least one of ies is added */ - if (pos != curr_pos) { -#ifdef CONFIG_IEEE80211N - curr_pos = hostapd_eid_secondary_channel(hapd, curr_pos); -#endif -#ifdef CONFIG_IEEE80211AC - curr_pos = hostapd_eid_wb_chsw_wrapper(hapd, curr_pos); -#endif - } - return curr_pos; -} - - static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd, const struct ieee80211_mgmt *req, int is_p2p, size_t *resp_len) { struct ieee80211_mgmt *resp; - u8 *pos, *epos; + u8 *pos, *epos, *csa_pos; size_t buflen; #define MAX_PROBERESP_LEN 768 @@ -432,6 +393,13 @@ static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd, /* Power Constraint element */ pos = hostapd_eid_pwr_constraint(hapd, pos); + /* CSA IE */ + csa_pos = hostapd_eid_csa(hapd, pos); + if (csa_pos != pos) + hapd->cs_c_off_proberesp = csa_pos - (u8 *)resp - 1; + + pos = csa_pos; + /* ERP Information element */ pos = hostapd_eid_erp_info(hapd, pos); @@ -445,7 +413,18 @@ static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd, pos = hostapd_eid_rm_enabled_capab(hapd, pos, epos - pos); + /* eCSA IE */ + csa_pos = hostapd_eid_ecsa(hapd, pos); + if (csa_pos != pos) + hapd->cs_c_off_ecsa_proberesp = csa_pos - (u8 *)resp - 1; + + pos = csa_pos; + #ifdef CONFIG_IEEE80211N + /* Secondary channel IE */ + /* TODO: The spec doesn't specify a position for this IE */ + pos = hostapd_eid_secondary_channel(hapd, pos); + pos = hostapd_eid_ht_capabilities(hapd, pos); pos = hostapd_eid_ht_operation(hapd, pos); #endif /* CONFIG_IEEE80211N */ @@ -459,10 +438,6 @@ static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd, pos = hostapd_eid_adv_proto(hapd, pos); pos = hostapd_eid_roaming_consortium(hapd, pos); - pos = hostapd_add_csa_elems(hapd, pos, (u8 *)resp, - &hapd->cs_c_off_proberesp, - &hapd->cs_c_off_ecsa_proberesp); - #ifdef CONFIG_FST if (hapd->iface->fst_ies) { os_memcpy(pos, wpabuf_head(hapd->iface->fst_ies), @@ -475,6 +450,9 @@ static u8 * hostapd_gen_probe_resp(struct hostapd_data *hapd, if (hapd->iconf->ieee80211ac && !hapd->conf->disable_11ac) { pos = hostapd_eid_vht_capabilities(hapd, pos); pos = hostapd_eid_vht_operation(hapd, pos); + + /* Channel Switch Wrapper IE */ + pos = hostapd_eid_wb_chsw_wrapper(hapd, pos); } if (hapd->conf->vendor_vht) pos = hostapd_eid_vendor_vht(hapd, pos); @@ -915,7 +893,7 @@ int ieee802_11_build_ap_params(struct hostapd_data *hapd, size_t resp_len = 0; #ifdef NEED_AP_MLME u16 capab_info; - u8 *pos, *tailpos; + u8 *pos, *tailpos, *csa_pos; #define BEACON_HEAD_BUF_SIZE 256 #define BEACON_TAIL_BUF_SIZE 512 @@ -996,6 +974,13 @@ int ieee802_11_build_ap_params(struct hostapd_data *hapd, /* Power Constraint element */ tailpos = hostapd_eid_pwr_constraint(hapd, tailpos); + /* CSA IE */ + csa_pos = hostapd_eid_csa(hapd, tailpos); + if (csa_pos != tailpos) + hapd->cs_c_off_beacon = csa_pos - tail - 1; + + tailpos = csa_pos; + /* ERP Information element */ tailpos = hostapd_eid_erp_info(hapd, tailpos); @@ -1013,7 +998,18 @@ int ieee802_11_build_ap_params(struct hostapd_data *hapd, tailpos = hostapd_eid_bss_load(hapd, tailpos, tail + BEACON_TAIL_BUF_SIZE - tailpos); + /* eCSA IE */ + csa_pos = hostapd_eid_ecsa(hapd, tailpos); + if (csa_pos != tailpos) + hapd->cs_c_off_ecsa_beacon = csa_pos - tail - 1; + + tailpos = csa_pos; + #ifdef CONFIG_IEEE80211N + /* Secondary Channel IE */ + /* TODO: The spec doesn't specify secondary channel IE position */ + tailpos = hostapd_eid_secondary_channel(hapd, tailpos); + tailpos = hostapd_eid_ht_capabilities(hapd, tailpos); tailpos = hostapd_eid_ht_operation(hapd, tailpos); #endif /* CONFIG_IEEE80211N */ @@ -1029,9 +1025,6 @@ int ieee802_11_build_ap_params(struct hostapd_data *hapd, tailpos = hostapd_eid_interworking(hapd, tailpos); tailpos = hostapd_eid_adv_proto(hapd, tailpos); tailpos = hostapd_eid_roaming_consortium(hapd, tailpos); - tailpos = hostapd_add_csa_elems(hapd, tailpos, tail, - &hapd->cs_c_off_beacon, - &hapd->cs_c_off_ecsa_beacon); #ifdef CONFIG_FST if (hapd->iface->fst_ies) { @@ -1045,6 +1038,7 @@ int ieee802_11_build_ap_params(struct hostapd_data *hapd, if (hapd->iconf->ieee80211ac && !hapd->conf->disable_11ac) { tailpos = hostapd_eid_vht_capabilities(hapd, tailpos); tailpos = hostapd_eid_vht_operation(hapd, tailpos); + tailpos = hostapd_eid_wb_chsw_wrapper(hapd, tailpos); } if (hapd->conf->vendor_vht) tailpos = hostapd_eid_vendor_vht(hapd, tailpos); diff --git a/src/ap/ieee802_11_ht.c b/src/ap/ieee802_11_ht.c index 3422269..975f018 100644 --- a/src/ap/ieee802_11_ht.c +++ b/src/ap/ieee802_11_ht.c @@ -112,7 +112,8 @@ u8 *hostapd_eid_secondary_channel(struct hostapd_data *hapd, u8 *eid) { u8 sec_ch; - if (!hapd->cs_freq_params.sec_channel_offset) + if (!hapd->cs_freq_params.channel || + !hapd->cs_freq_params.sec_channel_offset) return eid; if (hapd->cs_freq_params.sec_channel_offset == -1) diff --git a/src/ap/ieee802_11_vht.c b/src/ap/ieee802_11_vht.c index 214adf7..6c72e54 100644 --- a/src/ap/ieee802_11_vht.c +++ b/src/ap/ieee802_11_vht.c @@ -136,7 +136,8 @@ u8 *hostapd_eid_wb_chsw_wrapper(struct hostapd_data *hapd, u8 *eid) u8 bw, chan1, chan2 = 0; int freq1; - if (!hapd->cs_freq_params.vht_enabled) + if (!hapd->cs_freq_params.channel || + !hapd->cs_freq_params.vht_enabled) return eid; /* bandwidth: 0: 40, 1: 80, 2: 160, 3: 80+80 */