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: <1b1976bd936be8530cac9a55726382450f691e3f.camel@sipsolutions.net>
Subject: Re: [PATCH v2 2/4] cfg80211: validate RU puncturing bitmap
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Aloka Dixit <quic_alokad@quicinc.com>,
        linux-wireless@vger.kernel.org
Date:   Thu, 24 Mar 2022 11:37:23 +0100
In-Reply-To: <20220323191640.31230-2-quic_alokad@quicinc.com>
References: <20220323191640.31230-1-quic_alokad@quicinc.com>
         <20220323191640.31230-2-quic_alokad@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk


> + * @ru_punct_bitmap: RU puncturing bitmap. Each bit represents a 20 MHz channel
> + *	with lowest bit corresponding to the smallest frequency. Bit set to 1
> + *	indicates that the channel is punctured, otherwise the channel is active
> + * @ru_punct_bitmap_supp_he: Indicates whether RU puncturing bitmap validation
> + *	should include OFDMA bitmaps.
>   */
>  struct cfg80211_chan_def {
>  	struct ieee80211_channel *chan;
> @@ -750,6 +755,8 @@ struct cfg80211_chan_def {
>  	u32 center_freq2;
>  	struct ieee80211_edmg edmg;
>  	u16 freq1_offset;
> +	u16 ru_punct_bitmap;
> +	bool ru_punct_bitmap_supp_he;

I don't feel we finished the discussion on why it should be in the
chandef.

On the one hand, I can see how you argue that it's a part of how the
channel is defined, after all, this is now in a sense different from the
non- or differently-punctured channel since some parts of it are "gone".

This makes saying that two chandefs are only identical if also their
puncturing matches completely sensible, as seen in the changed
implementation of cfg80211_chandef_identical().


On the other hand, however, I don't think this makes a lot of sense with
mac80211's channel contexts, and especially not in a client-side
implementation. If you were to e.g. have two simultaneous connections to
two different APs on the "same" channel but different puncturing
configuration, I'm not convinced it makes sense to treat that as
requiring two entirely separate channel contexts and have to go to
powersave on one AP to receive on the other etc.? At least from my
(Intel HW) POV that doesn't make much sense.

As a result, we treat the puncturing more of a bss_conf type thing, and
we indeed have a patch to do that for the client side, which I should
get sent out so we can have a better discussion about both sides.


But honestly part of that probably is that I don't really entirely
understand the need for puncturing in the first place.


(and also, you included all kinds of random other things in these
patches, specifically in nl80211.h, so there's no way I can apply them
as is anyway)

johannes