Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Date:   Mon, 21 Sep 2020 09:26:40 -0700
From:   Thomas Pedersen <thomas@adapt-ip.com>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v2 06/22] {cfg,mac}80211: get correct default channel
 width for S1G
In-Reply-To: <cb2cf1d923975b0115866b5e92efd91950a1b56d.camel@sipsolutions.net>
References: <20200831205600.21058-1-thomas@adapt-ip.com>
 <20200831205600.21058-7-thomas@adapt-ip.com>
 <c5b93cd207ce780a56ad2689d7660fee48683fe9.camel@sipsolutions.net>
 <18730e757068a05f6531ea204791eae8@adapt-ip.com>
 <cb2cf1d923975b0115866b5e92efd91950a1b56d.camel@sipsolutions.net>
User-Agent: Roundcube Webmail/1.4.7
Message-ID: <8d24ea208788ac4dbd63f86bd1ee6eab@adapt-ip.com>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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