Received-SPF: pass (google.com: domain of linux-wireless+bounces-3543-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <4d9d10ce2866467ae545507a99662eba70c2ad6e.camel@sipsolutions.net>
Subject: Re: [PATCH 15/15] wifi: cfg80211/mac80211: move puncturing into
 chandef
From: Johannes Berg <johannes@sipsolutions.net>
To: Ping-Ke Shih <pkshih@realtek.com>, "linux-wireless@vger.kernel.org"
	 <linux-wireless@vger.kernel.org>
Date: Tue, 13 Feb 2024 15:09:57 +0100
In-Reply-To: <d2b42434af3c81e467b0b2f24feb1b142686d35f.camel@realtek.com>
References: <20240129184108.49639-17-johannes@sipsolutions.net>
	 <20240129194108.307183a5d2e5.I4d7fe2f126b2366c1312010e2900dfb2abffa0f6@changeid>
	 <f489a916f416cb37b527868c4be0c59547bdeb4d.camel@realtek.com>
	 <94bd67a6f261d945917067334b633c78be665c6b.camel@sipsolutions.net>
	 <a17862ae423422b636c76f116e712e7ebed51c09.camel@realtek.com>
	 <e50e2336dd2717e7271e7e6f341437637192f717.camel@sipsolutions.net>
	 <d2b42434af3c81e467b0b2f24feb1b142686d35f.camel@realtek.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 (3.50.3-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-02-13 at 14:05 +0000, Ping-Ke Shih wrote:
> On Tue, 2024-02-13 at 13:41 +0100, Johannes Berg wrote:
> >=20
> > > > The other thing here is that I'm not entirely sure how the driver w=
orks,
> > > > chances are that this was previously a bug, and now is still a bug,
> > > > unless the driver doesn't really support channel contexts, or any f=
orm
> > > > of concurrency.
> > >=20
> > > This function is to initialize a station instance in firmware while
> > > associating, and the field of firmware command is to tell MAC hardwar=
e
> > > the sub-channels it can use to transmit, which should rely on
> > > bitmap of puncturing. Initially, we just wanted the field value to
> > > be ~0 (0xFFFF) to prevent TX stuck, but not fully implemented punctur=
ing
> > > feature.
> > >=20
> > > I think this is the reason you are confused.
> >=20
> > Not sure that explanations helps ;-)
>=20
> Oops. I assumed you want to know "how did it work to you?", and my answer
> was that we just wanted to fix TX stuck problem. But this story isn't=20
> interesting at all. XD

Ah, sorry! I thought you were talking about the HW/FW :)

> > If you have this per station how do you handle CCA? Which was kind of
> > the reason I moved it all back to the chandef? Not that this didn't mak=
e
> > the code simpler (in mac80211) either as a nice side effect :-)
>=20
> Do you mean CCA should consider punctured sub-channels? (CCA doesn't
> need to consider energy of punctured ones)

Yeah, I agree it _shouldn't_ consider energy of punctured sub-channels,
but then you can't really do it per station or even per vif since you'd
be listening without really knowing which peer/interface you're going to
transmit to/from.

Hence all the work of making it part of the channel context.

> The firmware command mentioned in this patch is used to control
> TX sub-channels from MAC to BB layers, and I think BB layer has another
> control registers related CCA I missed. Thanks for pointing this, I=20
> will check our BB team.

Heh. I have no idea :)

> > > > Though it _looks_ like you only support one channel context there, =
so
> > > > maybe also only one vif, and it doesn't matter? I'd probably still =
move
> > > > it over to the chan.c code though, it really does belong there more=
 as
> > > > discussed in the commit message of this change.
> > > >=20
> > > > But I didn't want to make those more semantic changes because I don=
't
> > > > know what logic your device applies here.
> > >=20
> > > We are going to support MCC and MLO, so we will/must consider more th=
an
> > > one channel context. Currently, rtw89 just consider 'deflink' not act=
ually
> > > 'links' that is the next main work we are doing.
> >=20
> > For MLO you have just one vif still, so it doesn't matter.
>=20
> I feel theoretically one MLO vif can consist of two links that use
> two channel contexts. Please correct me if this is wrong.=20

Yes, but I wouldn't think they're on the same (control) channel, but
usually two different bands? So you do get two chanctx indeed (sorry)
but they're pretty separate, never would I expect two links on a single
vif to share a chanctx, even though I guess technically it's not
impossible.

> But, yes currently we just have one vif. We will have two later.=20

Right.

> MCC is short for multi-channel concurrency that is a TDMA based concurren=
cy
> of STA + P2P using standard ieee802.11 power saving protocol and P2P GO N=
oA.

Aha! Sure, everyone implements things that way and has to, but the
acronym was new to me :)

johannes