Received-SPF: pass (google.com: domain of linux-wireless+bounces-3648-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <189c5d80de6b640686030eef00a1c82f191e5cac.camel@sipsolutions.net>
Subject: Re: [PATCH 1/2] wifi: mac80211: check beacon countdown is complete
 on per link basis
From: Johannes Berg <johannes@sipsolutions.net>
To: Aditya Kumar Singh <quic_adisi@quicinc.com>
Cc: linux-wireless@vger.kernel.org
Date: Thu, 15 Feb 2024 20:48:07 +0100
In-Reply-To: <20240215162811.506065-2-quic_adisi@quicinc.com>
References: <20240215162811.506065-1-quic_adisi@quicinc.com>
	 <20240215162811.506065-2-quic_adisi@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


> -bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
> +bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif,
> +					 unsigned int link_id)
>  {
>  	struct ieee80211_sub_if_data *sdata =3D vif_to_sdata(vif);
> +	struct ieee80211_link_data *link;
>  	struct beacon_data *beacon =3D NULL;
>  	u8 *beacon_data;
>  	size_t beacon_data_len;
> @@ -5106,9 +5108,17 @@ bool ieee80211_beacon_cntdwn_is_complete(struct ie=
ee80211_vif *vif)
>  	if (!ieee80211_sdata_running(sdata))
>  		return false;
> =20
> +	if (WARN_ON(link_id >=3D IEEE80211_MLD_MAX_NUM_LINKS))
> +		return 0;
> +
>  	rcu_read_lock();
> +
> +	link =3D rcu_dereference(sdata->link[link_id]);
> +	if (!link)
> +		goto out;
>=20

Maybe that should be a warning too? Not sure I see any case where the
driver can/should call it with a link that's not even there?

Oh ... and maybe it should check if the link is active? We had the
sdata_running() check before, but that doesn't mean much for MLO?

Though then again we have the check below anyway:

>  	if (vif->type =3D=3D NL80211_IFTYPE_AP) {
> -		beacon =3D rcu_dereference(sdata->deflink.u.ap.beacon);
> +		beacon =3D rcu_dereference(link->u.ap.beacon);
>  		if (WARN_ON(!beacon || !beacon->tail))
>  			goto out;
>=20

So that will just be NULL if it's not active... so I guess that's fine.

johannes