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
