Re: [PATCH 24/27] wifi: mac80211: implement link switching

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 9/2/2022 10:12 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Implement an API function and debugfs file to switch
> active links.
>
> Also provide an async version of the API so drivers
> can call it in arbitrary contexts, e.g. while in the
> authorized callback.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   include/net/mac80211.h        |  41 ++++++++
>   net/mac80211/debugfs_netdev.c |  26 ++++++
>   net/mac80211/ieee80211_i.h    |   4 +
>   net/mac80211/iface.c          |  12 +++
>   net/mac80211/key.c            |  34 +++++++
>   net/mac80211/key.h            |   3 +
>   net/mac80211/link.c           | 171 ++++++++++++++++++++++++++++++++++
>   7 files changed, 291 insertions(+)
>
> ...
> +static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
> +				       u16 active_links)
> +{
> +	struct ieee80211_bss_conf *link_confs[IEEE80211_MLD_MAX_NUM_LINKS];
> +	struct ieee80211_local *local = sdata->local;
> +	u16 old_active = sdata->vif.active_links;
> +	unsigned long rem = old_active & ~active_links;
> +	unsigned long add = active_links & ~old_active;
> +	struct sta_info *sta;
> +	unsigned int link_id;
> +	int ret, i;
> +
> +	if (!ieee80211_sdata_running(sdata))
> +		return -ENETDOWN;
> +
> +	if (sdata->vif.type != NL80211_IFTYPE_STATION)
> +		return -EINVAL;
> +
> +	/* cannot activate links that don't exist */
> +	if (active_links & ~sdata->vif.valid_links)
> +		return -EINVAL;
> +
> +	/* nothing to do */
> +	if (old_active == active_links)
> +		return 0;
> +
> +	for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++)
> +		link_confs[i] = sdata_dereference(sdata->vif.link_conf[i],
> +						  sdata);
> +
> +	if (add) {
> +		sdata->vif.active_links |= active_links;
> +		ret = drv_change_vif_links(local, sdata,
> +					   old_active,
> +					   sdata->vif.active_links,
> +					   link_confs);
> +		if (ret) {
> +			sdata->vif.active_links = old_active;
> +			return ret;
> +		}
> +	}
> +
> +	for_each_set_bit(link_id, &rem, IEEE80211_MLD_MAX_NUM_LINKS) {
> +		struct ieee80211_link_data *link;
> +
> +		link = sdata_dereference(sdata->link[link_id], sdata);
> +
> +		/* FIXME: kill TDLS connections on the link */
> +
> +		ieee80211_link_release_channel(link);
> +	}
> +
> +	list_for_each_entry(sta, &local->sta_list, list) {
> +		if (sdata != sta->sdata)
> +			continue;
> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
> +					   old_active,
> +					   old_active | active_links);
> +		WARN_ON_ONCE(ret);
> +	}
> +
> +	ret = ieee80211_key_switch_links(sdata, rem, add);

I see ieee80211_key_switch_link() only handler the per-link(link_id >= 
0) keys,

So I think lower driver also install the pairwise keys(link_id = -1) for 
the added links at this moment?

> +	WARN_ON_ONCE(ret);
> +
> +	list_for_each_entry(sta, &local->sta_list, list) {
> +		if (sdata != sta->sdata)
> +			continue;
> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
> +					   old_active | active_links,
> +					   active_links);
> +		WARN_ON_ONCE(ret);
> +	}
> +

I see 2 times to call drv_change_sta_link() above, and with sequence  
old_active->old_active | active_links->active_links

May I know is it has some design here?

> +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
> +		struct ieee80211_link_data *link;
> +
> +		link = sdata_dereference(sdata->link[link_id], sdata);
> +
> +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
> +						 IEEE80211_CHANCTX_SHARED);

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?

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_links() has passed test case with some type lower driver/chip?

> +		WARN_ON_ONCE(ret);
> +
> +		ieee80211_link_info_change_notify(sdata, link,
> +						  BSS_CHANGED_ERP_CTS_PROT |
> +						  BSS_CHANGED_ERP_PREAMBLE |
> +						  BSS_CHANGED_ERP_SLOT |
> +						  BSS_CHANGED_HT |
> +						  BSS_CHANGED_BASIC_RATES |
> +						  BSS_CHANGED_BSSID |
> +						  BSS_CHANGED_CQM |
> +						  BSS_CHANGED_QOS |
> +						  BSS_CHANGED_TXPOWER |
> +						  BSS_CHANGED_BANDWIDTH |
> +						  BSS_CHANGED_TWT |
> +						  BSS_CHANGED_HE_OBSS_PD |
> +						  BSS_CHANGED_HE_BSS_COLOR);
> +		ieee80211_mgd_set_link_qos_params(link);
> +	}
> +
> +	old_active = sdata->vif.active_links;
> +	sdata->vif.active_links = active_links;
> +
> +	if (rem) {
> +		ret = drv_change_vif_links(local, sdata, old_active,
> +					   active_links, link_confs);
> +		WARN_ON_ONCE(ret);
> +	}
> +
> +	return 0;
> +}
> +
...

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

Hi,

> > +	list_for_each_entry(sta, &local->sta_list, list) {
> > +		if (sdata != sta->sdata)
> > +			continue;
> > +		ret = drv_change_sta_links(local, sdata, &sta->sta,
> > +					   old_active,
> > +					   old_active | active_links);
> > +		WARN_ON_ONCE(ret);
> > +	}
> > +
> > +	ret = ieee80211_key_switch_links(sdata, rem, add);
> 
> I see ieee80211_key_switch_link() only handler the per-link(link_id >= 
> 0) keys,
> 
> So I think lower driver also install the pairwise keys(link_id = -1) for 
> 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 != sta->sdata)
> > +			continue;
> > +		ret = drv_change_sta_links(local, sdata, &sta->sta,
> > +					   old_active | active_links,
> > +					   active_links);
> > +		WARN_ON_ONCE(ret);
> > +	}
> > +
> 
> I see 2 times to call drv_change_sta_link() above, and with sequence  
> old_active->old_active | active_links->active_links
> 
> 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 = sdata_dereference(sdata->link[link_id], sdata);
> > +
> > +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
> > +						 IEEE80211_CHANCTX_SHARED);
> 
> 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_links() has passed test case with some type lower driver/chip?

Yes, we have this working on iwlwifi/mvm.

johannes

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/27/2023 4:31 PM, Johannes Berg wrote:
> Hi,
>
>>> +	list_for_each_entry(sta, &local->sta_list, list) {
>>> +		if (sdata != sta->sdata)
>>> +			continue;
>>> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
>>> +					   old_active,
>>> +					   old_active | active_links);
>>> +		WARN_ON_ONCE(ret);
>>> +	}
>>> +
>>> +	ret = ieee80211_key_switch_links(sdata, rem, add);
>> I see ieee80211_key_switch_link() only handler the per-link(link_id >=
>> 0) keys,
>>
>> So I think lower driver also install the pairwise keys(link_id = -1) for
>> 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.)
Got it, thanks.
>
>>> +	WARN_ON_ONCE(ret);
>>> +
>>> +	list_for_each_entry(sta, &local->sta_list, list) {
>>> +		if (sdata != sta->sdata)
>>> +			continue;
>>> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
>>> +					   old_active | active_links,
>>> +					   active_links);
>>> +		WARN_ON_ONCE(ret);
>>> +	}
>>> +
>> I see 2 times to call drv_change_sta_link() above, and with sequence
>> old_active->old_active | active_links->active_links
>>
>> 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.
Got it, thanks.
>
>>> +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
>>> +		struct ieee80211_link_data *link;
>>> +
>>> +		link = sdata_dereference(sdata->link[link_id], sdata);
>>> +
>>> +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
>>> +						 IEEE80211_CHANCTX_SHARED);
>> 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.
Not huge difference, I have made little change in lower-driver to match 
that. So it is OK now.
>> 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_links() has passed test case with some type lower driver/chip?
> Yes, we have this working on iwlwifi/mvm.

Got it, thanks.

I also have tested ieee80211_set_active_links() to enable the 2nd link 
for station success in my lower-driver after a little change in 
lower-driver.

>
> johannes

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

On Mon, 2023-03-27 at 16:40 +0800, Wen Gong wrote:
> > 
> > > > +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
> > > > +		struct ieee80211_link_data *link;
> > > > +
> > > > +		link = sdata_dereference(sdata->link[link_id], sdata);
> > > > +
> > > > +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
> > > > +						 IEEE80211_CHANCTX_SHARED);
> > > 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.
> Not huge difference, I have made little change in lower-driver to match 
> that. So it is OK now.

OK. Still maybe we should change it for consistency? I can try that
later with our driver.

johannes


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/27/2023 5:04 PM, Johannes Berg wrote:
> On Mon, 2023-03-27 at 16:40 +0800, Wen Gong wrote:
>>>>> +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
>>>>> +		struct ieee80211_link_data *link;
>>>>> +
>>>>> +		link = sdata_dereference(sdata->link[link_id], sdata);
>>>>> +
>>>>> +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
>>>>> +						 IEEE80211_CHANCTX_SHARED);
>>>> 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.
>> Not huge difference, I have made little change in lower-driver to match
>> that. So it is OK now.
> OK. Still maybe we should change it for consistency? I can try that
> later with our driver.
>
> johannes

I think it is not urgent for that:)

And lower-drvier should also handler different case.

>

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/27/2023 4:31 PM, Johannes Berg wrote:
...
>> 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_links() has passed test case with some type lower driver/chip?
> Yes, we have this working on iwlwifi/mvm.
>
> johannes
May I know how did you test it?

Did you test with tool like this with parameter "ActiveTxMultiLinks"?

  ".\sigma-dut -l 
sta_set_rfeature,Interface,wlan0,prog,EHT,ActiveTxMultiLinks,02:03:7f:95:21:97,ActiveRxMultiLinks,02:03:7f:95:21:97"


-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
> On 3/27/2023 4:31 PM, Johannes Berg wrote:
> ...
> > > 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_links() has passed test case with some type lower driver/chip?
> > Yes, we have this working on iwlwifi/mvm.
> > 
> > johannes
> May I know how did you test it?

Just writing to the debugfs file. We have various tests using that now.


johannes

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/28/2023 3:39 PM, Johannes Berg wrote:
> On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
>> On 3/27/2023 4:31 PM, Johannes Berg wrote:
>> ...
>>>> 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_links() has passed test case with some type lower driver/chip?
>>> Yes, we have this working on iwlwifi/mvm.
>>>
>>> johannes
>> May I know how did you test it?
> Just writing to the debugfs file. We have various tests using that now.
>
Do you mean the various tests using debugfs or using 
ieee80211_set_active_links() directly?
> johannes

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

On Mon, 2023-04-03 at 22:15 +0800, Wen Gong wrote:
> On 3/28/2023 3:39 PM, Johannes Berg wrote:
> > On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
> > > On 3/27/2023 4:31 PM, Johannes Berg wrote:
> > > ...
> > > > > 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_links() has passed test case with some type lower driver/chip?
> > > > Yes, we have this working on iwlwifi/mvm.
> > > > 
> > > > johannes
> > > May I know how did you test it?
> > Just writing to the debugfs file. We have various tests using that now.
> > 
> Do you mean the various tests using debugfs or using 
> ieee80211_set_active_links() directly?
> 

Yeah the tests write to debugfs.

johannes

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/27/2023 4:31 PM, Johannes Berg wrote:
> Hi,
>
>>> +	list_for_each_entry(sta, &local->sta_list, list) {
>>> +		if (sdata != sta->sdata)
>>> +			continue;
>>> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
>>> +					   old_active,
>>> +					   old_active | active_links);
>>> +		WARN_ON_ONCE(ret);
>>> +	}
>>> +
>>> +	ret = ieee80211_key_switch_links(sdata, rem, add);
>> I see ieee80211_key_switch_link() only handler the per-link(link_id >=
>> 0) keys,
>>
>> So I think lower driver also install the pairwise keys(link_id = -1) for
>> 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.)
>
>
Thanks Johannes,

Also it does not have BSS_CHANGED_ASSOC(exists in 
ieee80211_set_associated()) for

ieee80211_link_info_change_notify()/ieee80211_vif_cfg_change_notify().

So I think low-level driver also need to auto add BSS_CHANGED_ASSOC 
logic for the added link as well as the pairwise key you said, right?



-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

On Mon, 2023-04-03 at 22:21 +0800, Wen Gong wrote:
> 
> Also it does not have BSS_CHANGED_ASSOC(exists in 
> ieee80211_set_associated()) for
> 
> ieee80211_link_info_change_notify()/ieee80211_vif_cfg_change_notify().

Well, that's clearly intentional though, since the assoc state is at the
MLD level, so changing a link doesn't affect the assoc state, right?
This is also done through _vif_cfg_change_notify().

> So I think low-level driver also need to auto add BSS_CHANGED_ASSOC 
> logic for the added link as well as the pairwise key you said, right?
> 

That doesn't make a lot of sense as written, IMHO, but yes if you have
something you need to do on links during assoc, then you'd have to take
care of that as well, just like the keys.

johannes

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k