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

[Thomas Pedersen <thomas@adapt-ip.com>]

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