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: > > > > > > The other thing here is that I'm not entirely sure how the driver works, > > > > 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 form > > > > of concurrency. > > > > > > This function is to initialize a station instance in firmware while > > > associating, and the field of firmware command is to tell MAC hardware > > > 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 puncturing > > > feature. > > > > > > I think this is the reason you are confused. > > > > Not sure that explanations helps ;-) > > 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 > 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 make > > the code simpler (in mac80211) either as a nice side effect :-) > > 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 > 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. > > > > > > > > But I didn't want to make those more semantic changes because I don't > > > > know what logic your device applies here. > > > > > > We are going to support MCC and MLO, so we will/must consider more than > > > one channel context. Currently, rtw89 just consider 'deflink' not actually > > > 'links' that is the next main work we are doing. > > > > For MLO you have just one vif still, so it doesn't matter. > > I feel theoretically one MLO vif can consist of two links that use > two channel contexts. Please correct me if this is wrong. 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. Right. > MCC is short for multi-channel concurrency that is a TDMA based concurrency > of STA + P2P using standard ieee802.11 power saving protocol and P2P GO NoA. Aha! Sure, everyone implements things that way and has to, but the acronym was new to me :) johannes
