Received-SPF: pass (google.com: domain of linux-wireless+bounces-5206-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <2c8338a29d3c42ebbf1fd5efd348c14471fa5004.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: mac80211: validate link status before deciding on
 off channel Tx
From: Johannes Berg <johannes@sipsolutions.net>
To: Aditya Kumar Singh <quic_adisi@quicinc.com>
Cc: linux-wireless@vger.kernel.org, Sriram R <quic_srirrama@quicinc.com>
Date: Mon, 25 Mar 2024 16:21:42 +0100
In-Reply-To: <20240312154620.242773-1-quic_adisi@quicinc.com>
References: <20240312154620.242773-1-quic_adisi@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-03-12 at 21:16 +0530, Aditya Kumar Singh wrote:

> In such cases, the bss active flag might not provide the exact
> status of the MLD links.

That's ... true I guess, but then I'm suspicious, why are you sending
this patch? Does that mean 'active' is managed incorrectly and actually
becomes false when one link is removed, rather than all? Can you fix
that too? And if you fix that ... yeah we probably still should have
this patch but ... _without_ this:

> +	/* This is consolidated status of the MLD or non ML bss */
> +	if (sdata->bss->active)
> +		return true;

I'd think?

> While at it, when source address is same as the link conf's address and
> if userspace requested Tx on a specific link, add changes to use the same
> link id if the link bss is matching the requested channel.

Why not separate that? It's really not related much?

> +	if (link_id < 0)
> +		return false;
> +
> +	if (!sdata->vif.valid_links)
> +		return false;
> +
> +	if (!(sdata->vif.valid_links & BIT(link_id)))
> +		return false;

The second condition seems useless then?

But probably better to check *active* links, and then might as well use
ieee80211_vif_link_active()?

> +	link =3D sdata_dereference(sdata->link[link_id], sdata);
> +	if (!link)
> +		return false;

That might be a WARN_ON()? After all, if links are valid (or active per
above) that really should be there?

>  int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>  		      struct cfg80211_mgmt_tx_params *params, u64 *cookie)
>  {
> @@ -817,7 +850,7 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wir=
eless_dev *wdev,
>  	case NL80211_IFTYPE_P2P_GO:
>  		if (sdata->vif.type !=3D NL80211_IFTYPE_ADHOC &&
>  		    !ieee80211_vif_is_mesh(&sdata->vif) &&
> -		    !sdata->bss->active)
> +		    !ieee80211_is_link_bss_active(sdata, params->link_id))
>  			need_offchan =3D true;
> =20
>  		rcu_read_lock();
> @@ -897,8 +930,17 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wi=
reless_dev *wdev,
>  				break;
>  			}
> =20
> -			if (ether_addr_equal(conf->addr, mgmt->sa))
> +			if (ether_addr_equal(conf->addr, mgmt->sa)) {
> +				/* If userspace requested Tx on a specific link
> +				 * use the same link id if the link bss is matching
> +				 * the requested chan.
> +				 */
> +				if (sdata->vif.valid_links &&
> +				    params->link_id >=3D 0 && params->link_id =3D=3D i &&
> +				    params->chan =3D=3D chanctx_conf->def.chan)
> +					link_id =3D i;
>  				break;
> +			}
> =20
>  			chanctx_conf =3D NULL;
>  		}
>=20
> base-commit: c2b25092864a16c7865e406badedece5cc25fc2b