Received-SPF: pass (google.com: domain of linux-wireless+bounces-5992-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: <98081f4e417cf20beea41288ca7b75235b8fe0e0.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: mac80211: handle sdata->u.ap.active flag with MLO
From: Johannes Berg <johannes@sipsolutions.net>
To: Aditya Kumar Singh <quic_adisi@quicinc.com>
Cc: linux-wireless@vger.kernel.org
Date: Tue, 09 Apr 2024 09:27:24 +0200
In-Reply-To: <15bc7705-5da9-4c56-aa45-81972bad5e01@quicinc.com>
References: <20240326151141.3824454-1-quic_adisi@quicinc.com>
	 <bae1e71edcc13e73e8ef7f24d30140a4a6aefb98.camel@sipsolutions.net>
	 <15bc7705-5da9-4c56-aa45-81972bad5e01@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-04-09 at 09:27 +0530, Aditya Kumar Singh wrote:
> On 4/8/24 23:55, Johannes Berg wrote:
> > On Tue, 2024-03-26 at 20:41 +0530, Aditya Kumar Singh wrote:
> >=20
> > > @@ -1232,7 +1256,9 @@ ieee80211_assign_beacon(struct ieee80211_sub_if=
_data *sdata,
> > >   	}
> > >  =20
> > >   	rcu_assign_pointer(link->u.ap.beacon, new);
> > > -	sdata->u.ap.active =3D true;
> > > +
> > > +	if (ieee80211_num_beaconing_links(sdata) <=3D 1)
> > > +		sdata->u.ap.active =3D true;
> >=20
> > I don't understand this change. Neither the <=3D 1 really, nor the fact
> > that you actually _make_ this change.
> >=20
>=20
> The place above where we are checking number of beaconing links, at that=
=20
> point at least 1 should be active. Since before checking, we have done=
=20
> rcu_assign_pointer() so at least 1 should be there. That is why that=20
> condition.
>=20
> If it is more than 1, then this is not the first link which is going to=
=20
> come up and hence there is no need to set the flag again.

Hmm, OK, I guess that makes some sense. However, it's also completely
pointless, since setting it =3Dtrue when it's already =3Dtrue doesn't reall=
y
do anything. Adding the code seems to imply it should avoid setting it
in some cases, which isn't actually the case.

Besides, doing the counting is almost certainly far more expensive than
simply setting it to true when it's already true. Certainly the state
should be =3Dtrue after this function is called.

If you really think the extra write might be a problem (it isn't though)
then you'd still want to write it as "if (!active) active=3Dtrue" since
that's actually checking the right thing. But ... that really wouldn't
matter in all but the highest-performance code meant to deal with high
(CPU/core) parallelism.

So this is just a long-winded way of saying: don't do that, just keep it
unconditionally setting active.

> > > @@ -1486,7 +1488,10 @@ static int ieee80211_start_ap(struct wiphy *wi=
phy, struct net_device *dev,
> > >   		if (old)
> > >   			kfree_rcu(old, rcu_head);
> > >   		RCU_INIT_POINTER(link->u.ap.beacon, NULL);
> > > -		sdata->u.ap.active =3D false;
> > > +
> > > +		if (!ieee80211_num_beaconing_links(sdata))
> > > +			sdata->u.ap.active =3D false;
> >=20
> > =3D=3D 0 maybe?
> >=20
>=20
> Yeah can do. I prefer "!expr" over "expr =3D=3D 0". Do you have any prefe=
rence?

I think for something that actually _counts_, like here, I'd (slightly)
prefer =3D=3D0. It's obviously equivalent, but it seems more natural to
write "is number of beaconing links equal to zero" rather than "is not
number of beaconing links".

I may be influenced too much by Dan's thoughts on the matter ;-)

https://staticthinking.wordpress.com/2024/02/20/when-to-use-0/

> > Or maybe we should just save/restore the value instead?
> >=20
> > >   	list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
> > >   		netif_carrier_off(vlan->dev);
> > >  =20
> > > -	if (ieee80211_num_beaconing_links(sdata) <=3D 1)
> >=20
> > Unrelated, but it looks like the VLAN netif_carrier_off() handling abov=
e
> > is also wrong and should really go into this if block as well.
> >=20
>=20
> Yeah MLO VLAN changes would do that? The previous change was focusing on=
=20
> the AP mode alone and I did not want to break anything in VLAN so did=20
> not touch it there.

Sure sure, just drive-by observation.

johannes