Message ID | 20200831205600.21058-1-thomas@adapt-ip.com |
---|---|
Headers | show |
Series | add support for S1G association | expand |
On 2020-08-31 13:55, Thomas Pedersen wrote: > S1G channels have a minimum bandwidth of 1Mhz, and there > is a 1:1 mapping of allowed bandwidth to channel number. > > Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com> > > --- > > v2: > - drop the freq_reg_info() interface changes and move the > check for S1G band inside. Fixes a driver compile > error. > - fix iterating past bws[] in __freq_reg_info() by > setting initial element to 0. > Reported-by: kernel test robot <lkp@intel.com> > --- > net/wireless/reg.c | 70 ++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 58 insertions(+), 12 deletions(-) > > diff --git a/net/wireless/reg.c b/net/wireless/reg.c > index 0ab7808fcec8..be6f54b70ad3 100644 > --- a/net/wireless/reg.c > +++ b/net/wireless/reg.c > @@ -1617,9 +1617,11 @@ __freq_reg_info(struct wiphy *wiphy, u32 > center_freq, u32 min_bw) > { > const struct ieee80211_regdomain *regd = reg_get_regdomain(wiphy); > const struct ieee80211_reg_rule *reg_rule = NULL; > + const u32 bws[] = {0, 1, 2, 4, 5, 8, 10, 16, 20}; > + int i = sizeof(bws) / sizeof(u32) - 1; This could be 'int i = ARRAY_SIZE(bws) - 1'. > u32 bw; > > - for (bw = MHZ_TO_KHZ(20); bw >= min_bw; bw = bw / 2) { > + for (bw = MHZ_TO_KHZ(bws[i]); bw >= min_bw; bw = > MHZ_TO_KHZ(bws[i--])) { > reg_rule = freq_reg_info_regd(center_freq, regd, bw); > if (!IS_ERR(reg_rule)) > return reg_rule; > @@ -1631,7 +1633,9 @@ __freq_reg_info(struct wiphy *wiphy, u32 > center_freq, u32 min_bw) > const struct ieee80211_reg_rule *freq_reg_info(struct wiphy *wiphy, > u32 center_freq) > { > - return __freq_reg_info(wiphy, center_freq, MHZ_TO_KHZ(20)); > + u32 min_bw = center_freq < MHZ_TO_KHZ(1000) ? 1 : 20; > + > + return __freq_reg_info(wiphy, center_freq, MHZ_TO_KHZ(min_bw)); > } > EXPORT_SYMBOL(freq_reg_info); > > @@ -1659,6 +1663,7 @@ static uint32_t reg_rule_to_chan_bw_flags(const > struct ieee80211_regdomain *regd > { > const struct ieee80211_freq_range *freq_range = NULL; > u32 max_bandwidth_khz, center_freq_khz, bw_flags = 0; > + bool is_s1g = chan->band == NL80211_BAND_S1GHZ; > > freq_range = ®_rule->freq_range; > > @@ -1678,16 +1683,57 @@ static uint32_t > reg_rule_to_chan_bw_flags(const struct ieee80211_regdomain *regd > MHZ_TO_KHZ(20))) > bw_flags |= IEEE80211_CHAN_NO_20MHZ; > > - if (max_bandwidth_khz < MHZ_TO_KHZ(10)) > - bw_flags |= IEEE80211_CHAN_NO_10MHZ; > - if (max_bandwidth_khz < MHZ_TO_KHZ(20)) > - bw_flags |= IEEE80211_CHAN_NO_20MHZ; > - if (max_bandwidth_khz < MHZ_TO_KHZ(40)) > - bw_flags |= IEEE80211_CHAN_NO_HT40; > - if (max_bandwidth_khz < MHZ_TO_KHZ(80)) > - bw_flags |= IEEE80211_CHAN_NO_80MHZ; > - if (max_bandwidth_khz < MHZ_TO_KHZ(160)) > - bw_flags |= IEEE80211_CHAN_NO_160MHZ; > + if (is_s1g) { > + /* S1G is strict about non overlapping channels. We can > + * calculate which bandwidth is allowed per channel by finding > + * the largest bandwidth which cleanly divides the freq_range. > + */ > + int edge_offset; > + int ch_bw = max_bandwidth_khz; > + > + while (ch_bw) { > + edge_offset = (center_freq_khz - ch_bw / 2) - > + freq_range->start_freq_khz; > + if (edge_offset % ch_bw == 0) { > + switch (KHZ_TO_MHZ(ch_bw)) { > + case 1: > + bw_flags |= IEEE80211_CHAN_1MHZ; > + break; > + case 2: > + bw_flags |= IEEE80211_CHAN_2MHZ; > + break; > + case 4: > + bw_flags |= IEEE80211_CHAN_4MHZ; > + break; > + case 8: > + bw_flags |= IEEE80211_CHAN_8MHZ; > + break; > + case 16: > + bw_flags |= IEEE80211_CHAN_16MHZ; > + break; > + default: > + /* If we got here, no bandwidths fit on > + * this frequency, ie. band edge. > + */ > + bw_flags |= IEEE80211_CHAN_DISABLED; > + break; > + } > + break; > + } > + ch_bw /= 2; > + } > + } else { > + if (max_bandwidth_khz < MHZ_TO_KHZ(10)) > + bw_flags |= IEEE80211_CHAN_NO_10MHZ; > + if (max_bandwidth_khz < MHZ_TO_KHZ(20)) > + bw_flags |= IEEE80211_CHAN_NO_20MHZ; > + if (max_bandwidth_khz < MHZ_TO_KHZ(40)) > + bw_flags |= IEEE80211_CHAN_NO_HT40; > + if (max_bandwidth_khz < MHZ_TO_KHZ(80)) > + bw_flags |= IEEE80211_CHAN_NO_80MHZ; > + if (max_bandwidth_khz < MHZ_TO_KHZ(160)) > + bw_flags |= IEEE80211_CHAN_NO_160MHZ; > + } > return bw_flags; > }
On 2020-09-06 09:04, Johannes Berg wrote: > On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: >> >> Note the mac80211_hwsim S1G support introduces a regression in a few >> hostap hwsim tests. This is because when processing the reported >> bands, >> hostap assumes freq < 4000 is 11b, and the actual 11b/g band is >> overwritten by the S1G band info. Though it does count as a userspace >> regression, I'm not sure there is much to do about it besides apply a >> small patch to hostapd which treats freq < 2000 as an unknown band. >> >> After the hostap workaround >> (https://lists.infradead.org/pipermail/hostap/2020-August/038748.html), >> these patches continue to pass the hwsim tests as well as HEAD. > > > That sounds like we could "hack around" it by sending the S1G data > first, and then the 2.4 GHz, so the latter overwrites it on broken > versions? Yes that could work. > Not sure it's worth it though, I'd say it depends a bit on what real > hardware plans are? > > I mean, if it's only hwsim for now ... who cares? And if it's going to > be special hardware that only does S1G, then also meh, you need newer > versions to support it, big deal. AFAIK there are no multi-band S1G chips. The initial focus (from WFA) seems to be industrial IOT. > But if OTOH a commonly used chipset like e.g. ath9k or ath10k will get > S1G support, then that'd be more relevant?
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: > An S1G BSS can beacon at either 1 or 2 MHz and the channel > width is unique to a given frequency. Ignore scan channel > width for now and use the allowed channel width. > > Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com> > --- > net/mac80211/scan.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c > index 5ac2785cdc7b..5002791fe165 100644 > --- a/net/mac80211/scan.c > +++ b/net/mac80211/scan.c > @@ -905,6 +905,17 @@ static void ieee80211_scan_state_set_channel(struct ieee80211_local *local, > local->scan_chandef.center_freq1 = chan->center_freq; > local->scan_chandef.freq1_offset = chan->freq_offset; > local->scan_chandef.center_freq2 = 0; > + > + /* For scanning on the S1G band, ignore scan_width (which is constant > + * across all channels) for now since channel width is specific to each > + * channel. Detect the required channel width here and likely revisit > + * later. Maybe scan_width could be used to build the channel scan list? > + */ > + if (chan->band == NL80211_BAND_S1GHZ) { > + local->scan_chandef.width = ieee80211_s1g_channel_width(chan); > + goto set_channel; > + } nit: double space after 'goto' but really I came to say that this probably changes then, if you don't convince me about the stuff in the previous patch review? :) So I'm leaving this patch also for now - have applied 1-5 so far. johannes
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: > Extract the BSS primary channel from the S1G Operation > element. Out of curiosity, do you even need to? I mean ... you know what channel you received it on, surely? > @@ -1318,15 +1318,26 @@ cfg80211_get_bss_channel(struct wiphy *wiphy, const u8 *ie, size_t ielen, > tmp = cfg80211_find_ie(WLAN_EID_DS_PARAMS, ie, ielen); > if (tmp && tmp[1] == 1) { > channel_number = tmp[2]; > - } else { > - tmp = cfg80211_find_ie(WLAN_EID_HT_OPERATION, ie, ielen); > - if (tmp && tmp[1] >= sizeof(struct ieee80211_ht_operation)) { > - struct ieee80211_ht_operation *htop = (void *)(tmp + 2); > + goto found_channel; > + } > > - channel_number = htop->primary_chan; > - } > + tmp = cfg80211_find_ie(WLAN_EID_HT_OPERATION, ie, ielen); > + if (tmp && tmp[1] >= sizeof(struct ieee80211_ht_operation)) { > + struct ieee80211_ht_operation *htop = (void *)(tmp + 2); > + > + channel_number = htop->primary_chan; > + goto found_channel; > + } > + > + tmp = cfg80211_find_ie(WLAN_EID_S1G_OPERATION, ie, ielen); > + if (tmp && tmp[1] >= sizeof(struct ieee80211_s1g_oper_ie)) { > + struct ieee80211_s1g_oper_ie *s1gop = (void *)(tmp + 2); > + > + channel_number = s1gop->primary_ch; > + goto found_channel; > } I *am* a bit worried about this though - do you really want to try to parse DS elements on S1G, or S1G elements on other bands? Seems like there ought to be a band check here? johannes
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: > > + /* Detect whether this was an S1G Association Response and adjust IE > + * location accordingly. > + */ > + rcu_read_lock(); > + ies = rcu_dereference(bss->ies); > + if (WARN_ON(!ies)) { > + rcu_read_unlock(); > + return; > + } > + s1g = cfg80211_find_ie(WLAN_EID_S1G_CAPABILITIES, ies->data, ies->len); > + if (s1g) { > + cr.resp_ie = (u8 *)&mgmt->u.s1g_assoc_resp.variable; > + cr.resp_ie_len = > + len - offsetof(struct ieee80211_mgmt, > + u.s1g_assoc_resp.variable); > + } > + rcu_read_unlock(); That ... is rather strange? Why not check bss->channel->band? johannes
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: > For now just skip the duration calculation for frames > transmitted on the S1G band and avoid a warning. > > Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com> > --- > net/mac80211/tx.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c > index d2136007e2eb..bef19616c5f0 100644 > --- a/net/mac80211/tx.c > +++ b/net/mac80211/tx.c > @@ -82,6 +82,10 @@ static __le16 ieee80211_duration(struct ieee80211_tx_data *tx, > > erp = txrate->flags & IEEE80211_RATE_ERP_G; > > + /* TODO */ > + if (sband->band == NL80211_BAND_S1GHZ) > + return 0; I'm not even sure I'd leave a TODO there ... just say /* device is expected to do this */ or so? johannes
Oops, sorry. I just realized I've been replying to v2, while *looking* at v3 in patchwork ... I'll continue on v3, but this seems mostly the same. johannes
On 2020-09-18 03:40, Johannes Berg wrote: > On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: >> An S1G BSS can beacon at either 1 or 2 MHz and the channel >> width is unique to a given frequency. Ignore scan channel >> width for now and use the allowed channel width. >> >> Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com> >> --- >> net/mac80211/scan.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c >> index 5ac2785cdc7b..5002791fe165 100644 >> --- a/net/mac80211/scan.c >> +++ b/net/mac80211/scan.c >> @@ -905,6 +905,17 @@ static void >> ieee80211_scan_state_set_channel(struct ieee80211_local *local, >> local->scan_chandef.center_freq1 = chan->center_freq; >> local->scan_chandef.freq1_offset = chan->freq_offset; >> local->scan_chandef.center_freq2 = 0; >> + >> + /* For scanning on the S1G band, ignore scan_width (which is >> constant >> + * across all channels) for now since channel width is specific to >> each >> + * channel. Detect the required channel width here and likely >> revisit >> + * later. Maybe scan_width could be used to build the channel scan >> list? >> + */ >> + if (chan->band == NL80211_BAND_S1GHZ) { >> + local->scan_chandef.width = ieee80211_s1g_channel_width(chan); >> + goto set_channel; >> + } > > nit: double space after 'goto' > > but really I came to say that this probably changes then, if you don't > convince me about the stuff in the previous patch review? :) > > So I'm leaving this patch also for now - have applied 1-5 so far. Thanks. I'm not really sure what else would make sense here? scan_req->scan_width is constant across all channels in scan_req->channels so for S1G we can either filter the scan_req channels list based on scan_width (kind of strange and unexpected), or deduce the correct chanenl width for each channel in the list and ignore scan_width (mostly correct). It seems like scan_width is currently only used for scanning at 5 or 10MHz anyway? -- thomas
On 2020-09-18 03:45, Johannes Berg wrote: > On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: >> Extract the BSS primary channel from the S1G Operation >> element. > > Out of curiosity, do you even need to? > > I mean ... you know what channel you received it on, surely? Consider the case where the BSS is operating @ 2Mhz, but primary is one of the 1Mhz channels. The hardware (or driver) may not be able to tell you exactly which primary channel (upper or lower) the packet came in on. >> @@ -1318,15 +1318,26 @@ cfg80211_get_bss_channel(struct wiphy *wiphy, >> const u8 *ie, size_t ielen, >> tmp = cfg80211_find_ie(WLAN_EID_DS_PARAMS, ie, ielen); >> if (tmp && tmp[1] == 1) { >> channel_number = tmp[2]; >> - } else { >> - tmp = cfg80211_find_ie(WLAN_EID_HT_OPERATION, ie, ielen); >> - if (tmp && tmp[1] >= sizeof(struct ieee80211_ht_operation)) { >> - struct ieee80211_ht_operation *htop = (void *)(tmp + 2); >> + goto found_channel; >> + } >> >> - channel_number = htop->primary_chan; >> - } >> + tmp = cfg80211_find_ie(WLAN_EID_HT_OPERATION, ie, ielen); >> + if (tmp && tmp[1] >= sizeof(struct ieee80211_ht_operation)) { >> + struct ieee80211_ht_operation *htop = (void *)(tmp + 2); >> + >> + channel_number = htop->primary_chan; >> + goto found_channel; >> + } >> + >> + tmp = cfg80211_find_ie(WLAN_EID_S1G_OPERATION, ie, ielen); >> + if (tmp && tmp[1] >= sizeof(struct ieee80211_s1g_oper_ie)) { >> + struct ieee80211_s1g_oper_ie *s1gop = (void *)(tmp + 2); >> + >> + channel_number = s1gop->primary_ch; >> + goto found_channel; >> } > > I *am* a bit worried about this though - do you really want to try to > parse DS elements on S1G, or S1G elements on other bands? Seems like > there ought to be a band check here? OK. I'll rework this to handle garbage input a little better.
On 2020-09-18 03:50, Johannes Berg wrote: > On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: >> >> + /* Detect whether this was an S1G Association Response and adjust IE >> + * location accordingly. >> + */ >> + rcu_read_lock(); >> + ies = rcu_dereference(bss->ies); >> + if (WARN_ON(!ies)) { >> + rcu_read_unlock(); >> + return; >> + } >> + s1g = cfg80211_find_ie(WLAN_EID_S1G_CAPABILITIES, ies->data, >> ies->len); >> + if (s1g) { >> + cr.resp_ie = (u8 *)&mgmt->u.s1g_assoc_resp.variable; >> + cr.resp_ie_len = >> + len - offsetof(struct ieee80211_mgmt, >> + u.s1g_assoc_resp.variable); >> + } >> + rcu_read_unlock(); > > That ... is rather strange? > > Why not check bss->channel->band? Thanks! That saves a lot of work :)
On 2020-09-18 03:52, Johannes Berg wrote: > On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: >> For now just skip the duration calculation for frames >> transmitted on the S1G band and avoid a warning. >> >> Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com> >> --- >> net/mac80211/tx.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c >> index d2136007e2eb..bef19616c5f0 100644 >> --- a/net/mac80211/tx.c >> +++ b/net/mac80211/tx.c >> @@ -82,6 +82,10 @@ static __le16 ieee80211_duration(struct >> ieee80211_tx_data *tx, >> >> erp = txrate->flags & IEEE80211_RATE_ERP_G; >> >> + /* TODO */ >> + if (sband->band == NL80211_BAND_S1GHZ) >> + return 0; > > > I'm not even sure I'd leave a TODO there ... just say > > /* device is expected to do this */ Eventually it would be nice if mac80211 could calculate duration for S1G? Do you know why it doesn't for HT/VHT?
On Sun, 2020-09-20 at 23:03 -0700, Thomas Pedersen wrote: > > > I'm not even sure I'd leave a TODO there ... just say > > > > /* device is expected to do this */ > > Eventually it would be nice if mac80211 could calculate duration for > S1G? Do you know why it doesn't for HT/VHT? I'm not sure it matters. It almost doesn't even matter for legacy (CCK/OFDM) because devices practically always do it anyway? And for HT/VHT it's just quite a bit more complicated, so we never bothered, since it wasn't necessary. It couldn't even really be done since there are A-MPDUs and other things happening at a lower layer. johannes
On Sun, 2020-09-20 at 22:12 -0700, Thomas Pedersen wrote: > On 2020-09-18 03:45, Johannes Berg wrote: > > On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: > > > Extract the BSS primary channel from the S1G Operation > > > element. > > > > Out of curiosity, do you even need to? > > > > I mean ... you know what channel you received it on, surely? > > Consider the case where the BSS is operating @ 2Mhz, but primary is one > of > the 1Mhz channels. The hardware (or driver) may not be able to tell you > exactly which primary channel (upper or lower) the packet came in on. Ah, OK, makes sense. Somehow based on a comment somewhere else I thought you were saying that the channels are basically all unique in their (center frequency, bandwidth) tuple, and was assuming you'd actually have to scan them that way. johannes
On Sun, 2020-09-20 at 22:06 -0700, Thomas Pedersen wrote: > > > > + /* For scanning on the S1G band, ignore scan_width (which is > > > constant > > > + * across all channels) for now since channel width is specific to > > > each > > > + * channel. Detect the required channel width here and likely > > > revisit > > > + * later. Maybe scan_width could be used to build the channel scan > > > list? > > > + */ > > > + if (chan->band == NL80211_BAND_S1GHZ) { > > > + local->scan_chandef.width = ieee80211_s1g_channel_width(chan); > > > + goto set_channel; > > > + } > > > > nit: double space after 'goto' > > > > but really I came to say that this probably changes then, if you don't > > convince me about the stuff in the previous patch review? :) > > > > So I'm leaving this patch also for now - have applied 1-5 so far. > > Thanks. I'm not really sure what else would make sense here? > scan_req->scan_width is constant across all channels in > scan_req->channels so for S1G we can either filter the scan_req channels > list based on scan_width (kind of strange and unexpected), or deduce the > correct chanenl width for each channel in the list and ignore scan_width > (mostly correct). It seems like scan_width is currently only used for > scanning at 5 or 10MHz anyway? Yeah, that's true, it's sort of undefined if you're not in 5 or 10, and then we currently assume it's 20, but obviously for S1G we should assume then it's 1 MHz or something. FWIW, here's probably where I thought you have a unique (freq, bw) tuple and thus shouldn't really need to have the parsing in the other patch? But I never looked at the spec so far ... Anyway, wrt. the code here, I think perhaps you should just simply remove the reference to scan_width? I'm not sure what you'd really do with it, since it's a 5/10 MHz thing? TBH though, offhand I don't even know how the 5/10 MHz scanning is supposed to work? johannes