Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <319ecb67faac8a2e50408f2bfa28f2431a6e6b9a.camel@sipsolutions.net>
Subject: Re: [PATCH v3 0/9] wifi: cfg80211/mac80211: extend 6 GHz support
 for all power modes
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Aditya Kumar Singh <quic_adisi@quicinc.com>
Cc:     linux-wireless@vger.kernel.org
Date:   Tue, 29 Aug 2023 19:50:16 +0200
In-Reply-To: <20230315132904.31779-1-quic_adisi@quicinc.com>
References: <20230315132904.31779-1-quic_adisi@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

Hi,

So ... yeah. I shied away from even reviewing this because it's such a
messy topic and we've had so many threads about this. Sorry about that.


On Wed, 2023-03-15 at 18:58 +0530, Aditya Kumar Singh wrote:
> 6 GHz introduces various power modes of operation. Currently, based
> on the power mode, channel's Power Spectral Density (PSD) value,
> Regulatory power value, as well as channel disabled flag can change.
> For single interface, current implementation works just fine. But for
> multi-interfaces, for example AP-STA concurrency, two different channel
> context needs to be maintained. This is because, STA power mode also
> depends on the AP's power mode it is going to associate with. Hence,
> PSD value, regulatory power value and channel disabled flag might vary.
> In this case, same channel context cannot be used for both AP and STA.
>=20
> Therefore, to support multiple channel space for each power mode, the
> 6 GHz channels needs a separate storage space in data structure
> ieee80211_supported_band. Because of this, the existing APIs to get the
> channel/frequency from frequency/channel will not work for 6 GHz band.
>=20
> Add support to store all possible 6 GHz channel pools according to the
> power mode as well as add API support for getting channel/frequency info
> from the new struct ieee80211_6ghz_channel.
>=20
>=20
> Why different channel pools and not array of varying member in the same c=
hannel?:


I really don't understand.

Why do you even need to set *from userspace* the power mode for a
client? That ... doesn't make that much sense?

I mean, you're explaining here the mechanics, which is all fine, but
what's the "theory of operation"? Yes, I get that 6 GHz spectrum use
introduced power modes, but why do we even need to handle it this way?
None of this or the patches actually introduce any rationale?

Also, I'll note that the patch subjects with "cfg80211" or "mac80211"
are _really_ unclear. Sometimes you have "cfg80211" patches that mostly
change mac80211, etc. It'd make reviewing easier if you actually did
limit cfg80211 patches to touching cfg80211 only (with the exception of
necessary API updates, of course), and mac80211 patches to implementing
the new cfg80211 APIs. The first patch is probably neither, you can mark
it as ieee80211.

And then trivial things like - don't introduce a bug only to fix it in
the next patch! I mean, really?

I _still_ don't like "(A)" operation with different channel pointers -
you talk about introducing bugs with (B) but at least those would be
bugs in the new parts; (A) is almost certainly introducing bugs in
existing code that compares pointers and you never even know about it.

Also, related to that, is kind of a fundamental question - should we
really think that two *channels* are different because you use different
(TX) power control/scheme on them? That seems a bit strange, and you've
not gotten into that at all, from a semantic POV, only from a code POV.
I actually currently don't think that makes sense, but convince me
otherwise?

For a chandef, _maybe_? But also see the discussion around puncturing,
I'm not sure that's actually the right solution either.

johannes