Re: [PATCH v2 06/22] {cfg,mac}80211: get correct default channel width for S1G

Thomas Pedersen
2020-09-21T04:59:37Z

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