Received-SPF: pass (google.com: domain of linux-wireless+bounces-3659-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: <e5893311f9a5cf31f2bcb1eff9018e0924f83eba.camel@sipsolutions.net>
Subject: Re: [PATCH 2/2] wifi: mac80211_hwsim: add support for
 switch_vif_chanctx callback
From: Johannes Berg <johannes@sipsolutions.net>
To: Aditya Kumar Singh <quic_adisi@quicinc.com>
Cc: linux-wireless@vger.kernel.org
Date: Fri, 16 Feb 2024 08:12:32 +0100
In-Reply-To: <78c51497-dd6b-4d7a-b789-e3823e89a96a@quicinc.com>
References: <20240215162811.506065-1-quic_adisi@quicinc.com>
	 <20240215162811.506065-3-quic_adisi@quicinc.com>
	 <de92380b3ec6d4e6bf854479fb3eca5ae88cf1fc.camel@sipsolutions.net>
	 <78c51497-dd6b-4d7a-b789-e3823e89a96a@quicinc.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 Fri, 2024-02-16 at 08:45 +0530, Aditya Kumar Singh wrote:
>=20
> > > +	for (i =3D 0; i < n_vifs; i++) {
> > > +		hwsim_check_chanctx_magic(vifs[i].old_ctx);
> > > +		wiphy_dbg(hw->wiphy,
> > > +			  "switch vif channel context: %d MHz/width: %d/cfreqs:%d/%d MHz =
-> %d MHz/width: %d/cfreqs:%d/%d MHz\n",
> > > +			  vifs[i].old_ctx->def.chan->center_freq,
> > > +			  vifs[i].old_ctx->def.width,
> > > +			  vifs[i].old_ctx->def.center_freq1,
> > > +			  vifs[i].old_ctx->def.center_freq2,
> > > +			  vifs[i].new_ctx->def.chan->center_freq,
> > > +			  vifs[i].new_ctx->def.width,
> > > +			  vifs[i].new_ctx->def.center_freq1,
> > > +			  vifs[i].new_ctx->def.center_freq2);
> > > +		hwsim_set_chanctx_magic(vifs[i].new_ctx);
> >=20
> >=20
> > hwsim_set_chanctx_magic() is only partially correct, I think, this
> > depends on the mode? For CHANCTX_SWMODE_REASSIGN_VIF the chanctx should
> > already exist as well, so should also be hwsim_check_chanctx_magic() in
> > that case?
> >=20
>=20
> Oh yeah missed the mode dependency here. Thanks for pointing it out. So=
=20
> it should be something like -
>=20
> ...
>=20
> /* old already exist so check magic */
> hwsim_check_chanctx_magic(vifs[i].old_ctx);
>=20
> if (mode =3D=3D CHANCTX_SWMODE_REASSIGN_VIF) {
> 	/* Reassign then new would also exist, just interface is
> 	 * switching
> 	 */
> 	hwsim_check_chanctx_magic(vifs[i].new_ctx);
> } else {
> 	/* SWAP_* then new context does not exist hence set magic.
> 	 * Also old will no longer exist so clear the magic
> 	 */
> 	hwsim_set_chanctx_magic(vifs[i].new_ctx);
> 	hwsim_clear_chanctx_magic(vifs[i].old_ctx);
> }
>=20
> ...
>=20
> One thing, in patch should I keep those comments or those are noisy?=20
> Seems noisy to me (if at least mode documentation is referred then=20
> things are clear already)?
>=20

I'm not sure I care all that much, but I'd say even with the reference
to the modes, it's fairly easy to figure out at least by looking at the
mode docs?

I'd probably go for a switch () statement on the modes and even WARN on
invalid mode, while at it? hwsim is a test vehicle, after all.

No strong opinions on either (comments and switch) though.

johannes