Received-SPF: pass (google.com: domain of linux-wireless+bounces-3517-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <94bd67a6f261d945917067334b633c78be665c6b.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 09:57:15 +0100
In-Reply-To: <f489a916f416cb37b527868c4be0c59547bdeb4d.camel@realtek.com>
References: <20240129184108.49639-17-johannes@sipsolutions.net>
	 <20240129194108.307183a5d2e5.I4d7fe2f126b2366c1312010e2900dfb2abffa0f6@changeid>
	 <f489a916f416cb37b527868c4be0c59547bdeb4d.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

Hi PK,

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,
> >  	}
> > =20
> >  	if (vif->bss_conf.eht_support) {
> > -		h2c->w4 |=3D le32_encode_bits(~vif->bss_conf.eht_puncturing,
> > +		u16 punct =3D vif->bss_conf.chanreq.oper.punctured;
> > +
> > +		h2c->w4 |=3D le32_encode_bits(~punct,
> >  					    CCTLINFO_G7_W4_ACT_SUBCH_CBW);
> > +		rcu_read_unlock();
>=20
> 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, rig=
ht?

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.

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 =3D=3D 0x8 whereas
bss_vif->bss_conf.chanreq.oper.punctured =3D=3D 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().

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.


And sorry about the locking bug! Not sure how that happened :(

johannes