Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <4b597bba9dc13c3260f174826de38b4f1bc3bf6e.camel@sipsolutions.net>
Subject: Re: [PATCH 2/3] wifi: mac80211: add support to allow driver to set
 active link while connection for station
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Wen Gong <quic_wgong@quicinc.com>, ath12k@lists.infradead.org
Cc:     linux-wireless@vger.kernel.org
Date:   Wed, 13 Sep 2023 10:58:38 +0200
In-Reply-To: <20230906103458.24092-3-quic_wgong@quicinc.com>
References: <20230906103458.24092-1-quic_wgong@quicinc.com>
         <20230906103458.24092-3-quic_wgong@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote:
> There are 2 fields valid_links and active_links in struct ieee80211_vif
> of mac80211. For station mode, valid_links is always include the bitmap
> of active_links. valid_links stores the bitmap of links which is created
> in mac80211, and mac80211 only indicate the info of links which is existe=
d
> in active_links to driver. Finally, the active_links is the bitmap of
> links which station can interactive with MLO AP it is connected to.
>=20
> Currently the active links is always only contain the primary link,
> primary link means the link used by the latest exchange of successful
> (Re)Association Request/Response frames. Then it will always have only
> one link in driver after connected.
>=20
> Hence add this ops in struct ieee80211_ops to allow driver to determine
> the active links bit map dynamically while connecting to MLO AP.

This commit log does nothing to address the question "why do we need
this", particularly since we already have
ieee80211_set_active_links(/_async), so the driver can pretty much
immediately enable all the links it wants.

I see no value in this patch whatsoever.

> @@ -166,6 +167,13 @@ static void ieee80211_set_vif_links_bitmaps(struct i=
eee80211_sub_if_data *sdata,
>  		WARN_ON(dormant_links);
>  		break;
>  	case NL80211_IFTYPE_STATION:
> +		active_links =3D drv_calculate_active_links(sdata->local, sdata,
> +							  valid_links & ~dormant_links);
> +		if (active_links) {
> +			sdata->vif.active_links =3D active_links;
> +			break;
> +		}
>=20

I also _really_ don't think this should operate at this low-level
infrastructure.

johannes