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

[Wen Gong <quic_wgong@quicinc.com>]

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