Hi Johannes,
On Tue, 2024-02-13 at 09:57 +0100, Johannes Berg wrote:
>
>
> Actually, sorry about this part of the patch. Pretty sure I meant to ask
> you, but then wanted to get things together before everything broke ...
>
> > > +++ b/drivers/net/wireless/realtek/rtw89/fw.c
> > > @@ -2495,8 +2495,11 @@ int rtw89_fw_h2c_assoc_cmac_tbl_g7(struct rtw89_dev *rtwdev,
> > > }
> > >
> > > if (vif->bss_conf.eht_support) {
> > > - h2c->w4 |= le32_encode_bits(~vif->bss_conf.eht_puncturing,
> > > + u16 punct = vif->bss_conf.chanreq.oper.punctured;
> > > +
> > > + h2c->w4 |= le32_encode_bits(~punct,
> > > CCTLINFO_G7_W4_ACT_SUBCH_CBW);
> > > + rcu_read_unlock();
> >
> > We don't deference chanctx to reference puncture value. Instead use the
> > value from vif->bss_conf.chanreq, so I think we don't need RCU locks, right?
>
> Well, clearly the rcu_read_unlock() is wrong since it's not paired with
> rcu_read_lock(). I don't know how neither I nor the robots noticed
> that?!
>
> 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.
>
> If you actually have the ability to support two connections (e.g. P2P
> and BSS client) then theoretically it's possible that you have two EHT
> connections with compatible puncturing, using the same channel context,
> but with different bandwidths, e.g.
>
> BSS 160 MHz | | | | | C | | | P |
> P2P | C | | | P |
>
> (P) indicates punctured subchannel, (C) indicates control channel
>
> In this case, p2p_vif->bss_conf.chanreq.oper.punctured == 0x8 whereas
> bss_vif->bss_conf.chanreq.oper.punctured == 0x80.
>
> However, you'd really be using the actual channel configuration from the
> channel context, which matches the BSS vif, so should be puncturing
> bitmap 0x80.
>
>
> So realistically, what you probably need/want to do is move this whole
> chunk of code to when the *channel context* changes, i.e. to
> rtw89_chanctx_ops_change(). You even get the
> IEEE80211_CHANCTX_CHANGE_PUNCTURING flag when the puncturing changes
> there. Also need it in rtw89_chanctx_ops_add() of course, and possibly
> in rtw89_chanctx_ops_remove().
Thanks for this hint. I will check my colleagues about the detail of
puncturing behavior next week, because people are offline for the lunar
new year. I will also check people about the beacon CSA mentioned in
another discussion thread.
>
> 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.
>
>
> And sorry about the locking bug! Not sure how that happened :(
>
I will send a patch to fix locking ahead.
Ping-Ke
