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

Thomas Pedersen
2020-09-21T16:26:49Z

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

--
thomas