| Message ID | 20200831205600.21058-8-thomas@adapt-ip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | add support for S1G association | expand |
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 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?
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
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; + } + switch (scan_req->scan_width) { case NL80211_BSS_CHAN_WIDTH_5: local->scan_chandef.width = NL80211_CHAN_WIDTH_5; @@ -925,8 +936,14 @@ static void ieee80211_scan_state_set_channel(struct ieee80211_local *local, else local->scan_chandef.width = NL80211_CHAN_WIDTH_20_NOHT; break; + case NL80211_BSS_CHAN_WIDTH_1: + case NL80211_BSS_CHAN_WIDTH_2: + /* shouldn't get here, S1G handled above */ + WARN_ON(1); + break; } +set_channel: if (ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL)) skip = 1;
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(+)