| Message ID | 20200831205600.21058-7-thomas@adapt-ip.com |
|---|---|
| State | New |
| Headers | show |
| Series | add support for S1G association | expand |
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: > > +++ b/net/wireless/chan.c > @@ -33,6 +33,16 @@ void cfg80211_chandef_create(struct cfg80211_chan_def *chandef, > chandef->edmg.bw_config = 0; > chandef->edmg.channels = 0; > > + /* S1G allows a single width per channel, and since chan_type seems to > + * be for backwards compatibility only, ignore it and return the per > + * frequency width. > + */ > + if (chan->band == NL80211_BAND_S1GHZ) { > + chandef->width = ieee80211_s1g_channel_width(chan); > + chandef->center_freq1 = chan->center_freq; > + return; > + } Hmm. I'm not sure I want to let you get away with this? It might be ... convenient, but it's also confusing to see something like cfg80211_chandef_create(&out, some_channel, NL80211_CHAN_HT40PLUS); actually create an S1G channel width? Yes, this is mostly for backward compatibility, but it's also used in few (21 in the stack) places. Many of those aren't relevant, e.g. in net/mac80211/ibss.c it would obviously be clearer to handle the new NL80211_CHAN_WIDTH_* values with e.g. a cfg80211_get_s1g_chandef() function or so that does this derivation, instead of running into the default: /* fall back to 20 MHz for unsupported modes */ cfg80211_chandef_create(&chandef, cbss->channel, NL80211_CHAN_NO_HT); code. IOW, it seems to me that this function should actually instead throw a warning (and then perhaps configure something sane?), but not be the default code path. johannes
On 2020-09-18 03:38, Johannes Berg wrote: > On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote: >> >> +++ b/net/wireless/chan.c >> @@ -33,6 +33,16 @@ void cfg80211_chandef_create(struct >> cfg80211_chan_def *chandef, >> chandef->edmg.bw_config = 0; >> chandef->edmg.channels = 0; >> >> + /* S1G allows a single width per channel, and since chan_type seems >> to >> + * be for backwards compatibility only, ignore it and return the per >> + * frequency width. >> + */ >> + if (chan->band == NL80211_BAND_S1GHZ) { >> + chandef->width = ieee80211_s1g_channel_width(chan); >> + chandef->center_freq1 = chan->center_freq; >> + return; >> + } > > Hmm. I'm not sure I want to let you get away with this? > > It might be ... convenient, but it's also confusing to see something > like > > cfg80211_chandef_create(&out, some_channel, NL80211_CHAN_HT40PLUS); > > actually create an S1G channel width? Yes I agree that looks like it should be an error. > Yes, this is mostly for backward compatibility, but it's also used in > few (21 in the stack) places. Many of those aren't relevant, e.g. in > net/mac80211/ibss.c it would obviously be clearer to handle the new > NL80211_CHAN_WIDTH_* values with e.g. a cfg80211_get_s1g_chandef() > function or so that does this derivation, instead of running into the > > default: > /* fall back to 20 MHz for unsupported modes */ > cfg80211_chandef_create(&chandef, cbss->channel, > NL80211_CHAN_NO_HT); Yes, but then what would we pass to cfg80211_chandef_create()? We should probably avoid adding an additional NL80211_CHAN_S1G if enum nl80211_channel_type is legacy. It seems like NL80211_CHAN_NO_HT is often used as "give me the default channel width". See cfg80211_get_chandef_type() when it throws up its hands and returns NL80211_CHAN_NO_HT when encountering an unknown chandef->width. Also IBSS passes NL80211_CHAN_NO_HT when the BSS is actually 5 or 10MHz. Maybe (instead of adding a new nl80211_channel_type) cfg80211_chandef_create() throws a warning if anything but NL80211_CHAN_NO_HT is passed with an S1G frequency? > IOW, it seems to me that this function should actually instead throw a > warning (and then perhaps configure something sane?), but not be the > default code path. Yes, but I'd also like to avoid making the caller worry about the value of a parameter which only exists for HT reasons (?).
On Sun, 2020-09-20 at 21:59 -0700, Thomas Pedersen wrote: > > > default: > > /* fall back to 20 MHz for unsupported modes */ > > cfg80211_chandef_create(&chandef, cbss->channel, > > NL80211_CHAN_NO_HT); > > Yes, but then what would we pass to cfg80211_chandef_create()? I'd say we just shouldn't call it if there's a chance that it's an S1G channel? > We should > probably avoid adding an additional NL80211_CHAN_S1G if enum > nl80211_channel_type is legacy. Agree. > It seems like NL80211_CHAN_NO_HT is often used as "give me the default > channel width". See cfg80211_get_chandef_type() when it throws up its > hands > and returns NL80211_CHAN_NO_HT when encountering an unknown > chandef->width. > Also IBSS passes NL80211_CHAN_NO_HT when the BSS is actually 5 or 10MHz. Yeah, agree it's a bit of a mess, but I'm not really in favour of keeping that mess :) Also, that's a WARN_ON() there, so the NL80211_CHAN_NO_HT isn't meant to be anything *valid* in that case, just a value that doesn't cause crashes or other bad behaviour further down the road if we hit that path. > Maybe (instead of adding a new nl80211_channel_type) > cfg80211_chandef_create() throws a warning if anything but > NL80211_CHAN_NO_HT > is passed with an S1G frequency? I'd literally just add cfg80211_chandef_create_s1g() and just not have the argument at all? Or just fill the chandef manually, but of course that's a bit tedious sometimes. > > IOW, it seems to me that this function should actually instead throw a > > warning (and then perhaps configure something sane?), but not be the > > default code path. > > Yes, but I'd also like to avoid making the caller worry about the value > of a parameter which only exists for HT reasons (?). It mostly isn't even for HT reasons ... for HT, we could perfectly well fill the chandef directly, and do in many cases. johannes
On 2020-09-21 00:01, Johannes Berg wrote: > On Sun, 2020-09-20 at 21:59 -0700, Thomas Pedersen wrote: >> >> > default: >> > /* fall back to 20 MHz for unsupported modes */ >> > cfg80211_chandef_create(&chandef, cbss->channel, >> > NL80211_CHAN_NO_HT); >> >> Yes, but then what would we pass to cfg80211_chandef_create()? > > I'd say we just shouldn't call it if there's a chance that it's an S1G > channel? > >> We should >> probably avoid adding an additional NL80211_CHAN_S1G if enum >> nl80211_channel_type is legacy. > > Agree. > >> It seems like NL80211_CHAN_NO_HT is often used as "give me the default >> channel width". See cfg80211_get_chandef_type() when it throws up its >> hands >> and returns NL80211_CHAN_NO_HT when encountering an unknown >> chandef->width. >> Also IBSS passes NL80211_CHAN_NO_HT when the BSS is actually 5 or >> 10MHz. > > Yeah, agree it's a bit of a mess, but I'm not really in favour of > keeping that mess :) > > Also, that's a WARN_ON() there, so the NL80211_CHAN_NO_HT isn't meant > to > be anything *valid* in that case, just a value that doesn't cause > crashes or other bad behaviour further down the road if we hit that > path. > >> Maybe (instead of adding a new nl80211_channel_type) >> cfg80211_chandef_create() throws a warning if anything but >> NL80211_CHAN_NO_HT >> is passed with an S1G frequency? > > I'd literally just add > > cfg80211_chandef_create_s1g() > > and just not have the argument at all? Or just fill the chandef > manually, but of course that's a bit tedious sometimes. Then the caller has to make the determination which function to call based on the chan->band, but we've told the function implicitly by passing chan. It doesn't make sense to me to push that complexity onto the caller. That said, it actually looks like cfg80211_chandef_create() is only called when a chantype is passed to nl80211 (legacy), in DFS, or HT. After removing this hunk the S1G tests still pass, so how about we just drop it :) >> > IOW, it seems to me that this function should actually instead throw a >> > warning (and then perhaps configure something sane?), but not be the >> > default code path. >> >> Yes, but I'd also like to avoid making the caller worry about the >> value >> of a parameter which only exists for HT reasons (?). > > It mostly isn't even for HT reasons ... for HT, we could perfectly well > fill the chandef directly, and do in many cases. > > johannes
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c index bdc0f29dc6cd..8f48aff74c7b 100644 --- a/net/mac80211/chan.c +++ b/net/mac80211/chan.c @@ -536,7 +536,14 @@ static void ieee80211_del_chanctx(struct ieee80211_local *local, if (!local->use_chanctx) { struct cfg80211_chan_def *chandef = &local->_oper_chandef; - chandef->width = NL80211_CHAN_WIDTH_20_NOHT; + /* S1G doesn't have 20MHz, so get the correct width for the + * current channel. + */ + if (chandef->chan->band == NL80211_BAND_S1GHZ) + chandef->width = + ieee80211_s1g_channel_width(chandef->chan); + else + chandef->width = NL80211_CHAN_WIDTH_20_NOHT; chandef->center_freq1 = chandef->chan->center_freq; chandef->freq1_offset = chandef->chan->freq_offset; chandef->center_freq2 = 0; diff --git a/net/wireless/chan.c b/net/wireless/chan.c index 96e24ee4c7e8..a3bc50e419fd 100644 --- a/net/wireless/chan.c +++ b/net/wireless/chan.c @@ -33,6 +33,16 @@ void cfg80211_chandef_create(struct cfg80211_chan_def *chandef, chandef->edmg.bw_config = 0; chandef->edmg.channels = 0; + /* S1G allows a single width per channel, and since chan_type seems to + * be for backwards compatibility only, ignore it and return the per + * frequency width. + */ + if (chan->band == NL80211_BAND_S1GHZ) { + chandef->width = ieee80211_s1g_channel_width(chan); + chandef->center_freq1 = chan->center_freq; + return; + } + switch (chan_type) { case NL80211_CHAN_NO_HT: chandef->width = NL80211_CHAN_WIDTH_20_NOHT;
Until now, the wifi default channels were assumed to be NL80211_CHAN_NO_HT, or NL80211_CHAN_WIDTH_20_NOHT. S1G devices however do not support these channel types/width. When a default channel width is requested (during default chandef init, or chanctx removal when not using channel context), for S1G calculate the correct width. Fixes eg. configuring strange (20Mhz) width during a scan on the S1G band. Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com> --- net/mac80211/chan.c | 9 ++++++++- net/wireless/chan.c | 10 ++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-)