| Message ID | 1393597182-27218-1-git-send-email-michal.kazior@tieto.com |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, 2014-02-28 at 15:19 +0100, Michal Kazior wrote: > The center segment0 calculation for VHT20 ACS was > incorrect. This caused ACS to fail with: > "Could not set channel for kernel driver". Interesting. But see my previous patch - the kernel part obviously needs to be fixed but maybe in the beacon it should be set to 0? johannes
On 28 February 2014 15:43, Johannes Berg <johannes@sipsolutions.net> wrote: > On Fri, 2014-02-28 at 15:19 +0100, Michal Kazior wrote: >> The center segment0 calculation for VHT20 ACS was >> incorrect. This caused ACS to fail with: >> "Could not set channel for kernel driver". > > Interesting. But see my previous patch - the kernel part obviously > needs to be fixed but maybe in the beacon it should be set to 0? Won't setting it to 0 break older kernels with hostapd ACS for VHT20/VHT40? I'm not sure if that's what we want? Michał
On Fri, 2014-02-28 at 15:57 +0100, Michal Kazior wrote: > On 28 February 2014 15:43, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Fri, 2014-02-28 at 15:19 +0100, Michal Kazior wrote: > >> The center segment0 calculation for VHT20 ACS was > >> incorrect. This caused ACS to fail with: > >> "Could not set channel for kernel driver". > > > > Interesting. But see my previous patch - the kernel part obviously > > needs to be fixed but maybe in the beacon it should be set to 0? > > Won't setting it to 0 break older kernels with hostapd ACS for > VHT20/VHT40? I'm not sure if that's what we want? Well the *kernel* value in the chandef has to be set correctly, obviously. I'm merely pointing out that since my recent patch it's possible to (correctly, the spec says it's reserved) set the field to 0 in the VHT operation IE. johannes
On Fri, Feb 28, 2014 at 05:11:48PM +0100, Johannes Berg wrote: > On Fri, 2014-02-28 at 15:57 +0100, Michal Kazior wrote: > > On 28 February 2014 15:43, Johannes Berg <johannes@sipsolutions.net> wrote: > > > On Fri, 2014-02-28 at 15:19 +0100, Michal Kazior wrote: > > >> The center segment0 calculation for VHT20 ACS was > > >> incorrect. This caused ACS to fail with: > > >> "Could not set channel for kernel driver". > > > > > > Interesting. But see my previous patch - the kernel part obviously > > > needs to be fixed but maybe in the beacon it should be set to 0? > > > > Won't setting it to 0 break older kernels with hostapd ACS for > > VHT20/VHT40? I'm not sure if that's what we want? > > Well the *kernel* value in the chandef has to be set correctly, > obviously. I'm merely pointing out that since my recent patch it's > possible to (correctly, the spec says it's reserved) set the field to 0 > in the VHT operation IE. I'm having hard time trying to understand this discussion or well, more accurately, what was the outcome of the discussion that ended here.. The proposed change here seems to be changing vht_oper_centr_freq_seg0_idx value from pri_chan + 2 to pri_chan for 20 MHz channel width case. Is there any objection to that change and if so, why? As far as the Beacon frame is concerned, is the comment just pointing out that hostapd_eid_vht_operation() should ignore vht_oper_centr_freq_seg0_idx and set the field to 0 if this is a 20 MHz channel? If so, is anyone interesting in providing such patch? I agree that these fields seems to be marked Reserved in IEEE Std 802.11ac-2013 for this case and per IEEE Std 802.11-2014, reserved fields/subfields are set to 0 upon transmission. Does setting this field to 0 cause interop issues with earlier implementations before that "recent patch" mentioned above? (And if so, do we care?)
On Fri, 2014-03-14 at 16:42 +0200, Jouni Malinen wrote: > > > > Interesting. But see my previous patch - the kernel part obviously > > > > needs to be fixed but maybe in the beacon it should be set to 0? > > > > > > Won't setting it to 0 break older kernels with hostapd ACS for > > > VHT20/VHT40? I'm not sure if that's what we want? > > > > Well the *kernel* value in the chandef has to be set correctly, > > obviously. I'm merely pointing out that since my recent patch it's > > possible to (correctly, the spec says it's reserved) set the field to 0 > > in the VHT operation IE. > > I'm having hard time trying to understand this discussion or well, more > accurately, what was the outcome of the discussion that ended here.. The > proposed change here seems to be changing vht_oper_centr_freq_seg0_idx > value from pri_chan + 2 to pri_chan for 20 MHz channel width case. Is > there any objection to that change and if so, why? No, there's no objection to that change - that part is certainly correct. This just triggered my memory of the other patch that you recently applied, allowing the field to be set to zero in the IEs. > As far as the Beacon frame is concerned, is the comment just pointing > out that hostapd_eid_vht_operation() should ignore > vht_oper_centr_freq_seg0_idx and set the field to 0 if this is a 20 MHz > channel? If so, is anyone interesting in providing such patch? Yeah, pretty much. I sent you (and you applied) a patch that was allowing to set it to 0 in the config file instead, which is technically required for spec compliance (field is reserved in 20/40 MHz case), but in practice interoperability may be better with the field getting set, particularly with old broken mac80211 ;-) > I agree that these fields seems to be marked Reserved in IEEE Std > 802.11ac-2013 for this case and per IEEE Std 802.11-2014, reserved > fields/subfields are set to 0 upon transmission. 802.11-2014? I guess I need to find that. > Does setting this field to 0 cause interop issues with earlier > implementations before that "recent patch" mentioned above? (And if so, > do we care?) It does, setting it to 0 means that mac80211 until commit cb664981607a ("mac80211: fix association to 20/40 MHz VHT networks") will connect as an HT client, and ignore VHT. We probably shouldn't care though, and it's not a major issue (HT connection still works fine.) johannes
On Sat, Mar 15, 2014 at 03:34:40PM +0100, Johannes Berg wrote: > No, there's no objection to that change - that part is certainly > correct. This just triggered my memory of the other patch that you > recently applied, allowing the field to be set to zero in the IEs. OK, thanks. > Yeah, pretty much. I sent you (and you applied) a patch that was > allowing to set it to 0 in the config file instead, which is technically > required for spec compliance (field is reserved in 20/40 MHz case), but > in practice interoperability may be better with the field getting set, > particularly with old broken mac80211 ;-) So it may actually be fine to leave this as-is and allow configuration to be used to select whether to comply with the reserved=0 requirement.. > > I agree that these fields seems to be marked Reserved in IEEE Std > > 802.11ac-2013 for this case and per IEEE Std 802.11-2014, reserved > > fields/subfields are set to 0 upon transmission. > > 802.11-2014? I guess I need to find that. Good luck finding that.. :) That was supposed to be -2012. > > Does setting this field to 0 cause interop issues with earlier > > implementations before that "recent patch" mentioned above? (And if so, > > do we care?) > > It does, setting it to 0 means that mac80211 until commit cb664981607a > ("mac80211: fix association to 20/40 MHz VHT networks") will connect as > an HT client, and ignore VHT. We probably shouldn't care though, and > it's not a major issue (HT connection still works fine.) OK, I was assuming don't care, but this sounds like it is even less of an issue that I thought.
On Fri, Feb 28, 2014 at 03:19:42PM +0100, Michal Kazior wrote: > The center segment0 calculation for VHT20 ACS was > incorrect. This caused ACS to fail with: > "Could not set channel for kernel driver". Thanks, applied.
diff --git a/src/ap/acs.c b/src/ap/acs.c index f58b091..acb13b4 100644 --- a/src/ap/acs.c +++ b/src/ap/acs.c @@ -616,23 +616,26 @@ acs_find_ideal_chan(struct hostapd_iface *iface) static void acs_adjust_vht_center_freq(struct hostapd_iface *iface) { + int offset; + wpa_printf(MSG_DEBUG, "ACS: Adjusting VHT center frequency"); switch (iface->conf->vht_oper_chwidth) { case VHT_CHANWIDTH_USE_HT: - iface->conf->vht_oper_centr_freq_seg0_idx = - iface->conf->channel + 2; + offset = 2 * iface->conf->secondary_channel; break; case VHT_CHANWIDTH_80MHZ: - iface->conf->vht_oper_centr_freq_seg0_idx = - iface->conf->channel + 6; + offset = 6; break; default: /* TODO: How can this be calculated? Adjust * acs_find_ideal_chan() */ wpa_printf(MSG_INFO, "ACS: Only VHT20/40/80 is supported now"); - break; + return; } + + iface->conf->vht_oper_centr_freq_seg0_idx = + iface->conf->channel + offset; }
The center segment0 calculation for VHT20 ACS was incorrect. This caused ACS to fail with: "Could not set channel for kernel driver". Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- src/ap/acs.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)