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 (?).
--
thomas
