Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <50719d34bc48d816d00b56d3d9efdb59e3e51a16.camel@sipsolutions.net>
Subject: Re: [PATCH 24/27] wifi: mac80211: implement link switching
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Wen Gong <quic_wgong@quicinc.com>, linux-wireless@vger.kernel.org
Cc:     ath11k@lists.infradead.org, ath12k@lists.infradead.org
Date:   Mon, 27 Mar 2023 10:31:24 +0200
In-Reply-To: <ca5177fe-3b9f-2309-9afd-1d5e827540f7@quicinc.com>
References: <20220902141259.377789-1-johannes@sipsolutions.net>
         <20220902161143.d99dfbe65c90.I92385ba882ec984a9a2ad18293173436657e82aa@changeid>
         <ca5177fe-3b9f-2309-9afd-1d5e827540f7@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

Hi,

> > +	list_for_each_entry(sta, &local->sta_list, list) {
> > +		if (sdata !=3D sta->sdata)
> > +			continue;
> > +		ret =3D drv_change_sta_links(local, sdata, &sta->sta,
> > +					   old_active,
> > +					   old_active | active_links);
> > +		WARN_ON_ONCE(ret);
> > +	}
> > +
> > +	ret =3D ieee80211_key_switch_links(sdata, rem, add);
>=20
> I see ieee80211_key_switch_link() only handler the per-link(link_id >=3D=
=20
> 0) keys,
>=20
> So I think lower driver also install the pairwise keys(link_id =3D -1) fo=
r=20
> the added links at this moment?

Well from mac80211 POV they're already installed, so we can't really
install them again. We'd have to remove them but that's racy, obviously.
So I think the low-level driver just has to handle that, e.g. when the
station links are updated (and the key belongs to the station.)

> > +	WARN_ON_ONCE(ret);
> > +
> > +	list_for_each_entry(sta, &local->sta_list, list) {
> > +		if (sdata !=3D sta->sdata)
> > +			continue;
> > +		ret =3D drv_change_sta_links(local, sdata, &sta->sta,
> > +					   old_active | active_links,
> > +					   active_links);
> > +		WARN_ON_ONCE(ret);
> > +	}
> > +
>=20
> I see 2 times to call drv_change_sta_link() above, and with sequence=C2=
=A0=20
> old_active->old_active | active_links->active_links
>=20
> May I know is it has some design here?

The problem is that we can't really have no links active even as an
intermediate step, so you can't just deactivate old and then activate
new.

> > +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
> > +		struct ieee80211_link_data *link;
> > +
> > +		link =3D sdata_dereference(sdata->link[link_id], sdata);
> > +
> > +		ret =3D ieee80211_link_use_channel(link, &link->conf->chandef,
> > +						 IEEE80211_CHANCTX_SHARED);
>=20
> For the 1st link of MLO connection/NON-MLO connetion, ieee80211_link_use_=
channel() is called before drv_change_sta_link(),
> And now it is after drv_change_sta_link(), May I know is it also has some=
 design here?

Hmm, probably not really, at least I don't remember anything about that.

Not sure it makes a huge difference? But I suppose we could change it, I
don't really see why not either.

> Also I see commit(8fb7e2ef4bab mac80211_hwsim: always activate all links)=
 and ieee80211_if_parse_active_links()
> will use ieee80211_set_active_links(), so I think ieee80211_set_active_li=
nks() has passed test case with some type lower driver/chip?

Yes, we have this working on iwlwifi/mvm.

johannes