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;
Message-ID: <c5b93cd207ce780a56ad2689d7660fee48683fe9.camel@sipsolutions.net>
Subject: Re: [PATCH v2 06/22] {cfg,mac}80211: get correct default channel
 width for S1G
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Thomas Pedersen <thomas@adapt-ip.com>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>
Date:   Fri, 18 Sep 2020 12:38:08 +0200
In-Reply-To: <20200831205600.21058-7-thomas@adapt-ip.com>
References: <20200831205600.21058-1-thomas@adapt-ip.com>
         <20200831205600.21058-7-thomas@adapt-ip.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-1.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
> 
> +++ b/net/wireless/chan.c
> @@ -33,6 +33,16 @@ void cfg80211_chandef_create(struct cfg80211_chan_def *chandef,
>  	chandef->edmg.bw_config = 0;
>  	chandef->edmg.channels = 0;
>  
> +	/* S1G allows a single width per channel, and since chan_type seems to
> +	 * be for backwards compatibility only, ignore it and return the per
> +	 * frequency width.
> +	 */
> +	if (chan->band == NL80211_BAND_S1GHZ) {
> +		chandef->width = ieee80211_s1g_channel_width(chan);
> +		chandef->center_freq1 = chan->center_freq;
> +		return;
> +	}

Hmm. I'm not sure I want to let you get away with this?

It might be ... convenient, but it's also confusing to see something
like

	cfg80211_chandef_create(&out, some_channel, NL80211_CHAN_HT40PLUS);

actually create an S1G channel width?

Yes, this is mostly for backward compatibility, but it's also used in
few (21 in the stack) places. Many of those aren't relevant, e.g. in
net/mac80211/ibss.c it would obviously be clearer to handle the new
NL80211_CHAN_WIDTH_* values with e.g. a cfg80211_get_s1g_chandef()
function or so that does this derivation, instead of running into the

        default:
                /* fall back to 20 MHz for unsupported modes */
                cfg80211_chandef_create(&chandef, cbss->channel,
                                        NL80211_CHAN_NO_HT);

code.

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.

johannes