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. > > 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
