[PATCH] wifi: mac80211: fix assigning channel in activate links

[PATCH] wifi: mac80211: fix assigning channel in activate links

[Aditya Kumar Singh]

The current flow in _ieee80211_set_active_links() does not align with the
operational requirements of drivers that groups multiple hardware
under a single wiphy. These drivers (e.g ath12k) rely on channel
assignment to determine the appropriate hardware for each link. Without
this, the drivers cannot correctly establish the link interface.

Currently in _ieee80211_set_active_links(), after calling
drv_change_vif_links() on the driver, the state of all connected stations
is updated via drv_change_sta_links(). This is followed by handling keys
in the links, and finally, assigning the channel to the links.
Consequently, drv_change_sta_links() prompts drivers to create the station
entry at their level and within their firmware. However, since channels
have not yet been assigned to links at this stage, drivers have not
created the necessary link interface for establishing link stations,
leading to failures in activating the links.

Therefore, modify the logic so that after drv_change_vif_links(), channels
are assigned to all links. Following this, the flow should proceed to
station handling.

Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
 net/mac80211/link.c | 89 ++++++++++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/net/mac80211/link.c b/net/mac80211/link.c
index 0bbac64d5fa0..38c496cb28fa 100644
--- a/net/mac80211/link.c
+++ b/net/mac80211/link.c
@@ -363,6 +363,53 @@ static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
+	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);
+
+		/*
+		 * This call really should not fail. Unfortunately, it appears
+		 * that this may happen occasionally with some drivers. Should
+		 * it happen, we are stuck in a bad place as going backwards is
+		 * not really feasible.
+		 *
+		 * So lets just tell link_use_channel that it must not fail to
+		 * assign the channel context (from mac80211's perspective) and
+		 * assume the driver is going to trigger a recovery flow if it
+		 * had a failure.
+		 * That really is not great nor guaranteed to work. But at least
+		 * the internal mac80211 state remains consistent and there is
+		 * a chance that we can recover.
+		 */
+		ret = _ieee80211_link_use_channel(link,
+						  &link->conf->chanreq,
+						  IEEE80211_CHANCTX_SHARED,
+						  true);
+		WARN_ON_ONCE(ret);
+
+		/*
+		 * inform about the link info changed parameters after all
+		 * stations are also added
+		 */
+		ieee80211_mgd_set_link_qos_params(link);
+		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);
+
+	}
+
 	for_each_set_bit(link_id, &rem, IEEE80211_MLD_MAX_NUM_LINKS) {
 		struct ieee80211_link_data *link;
 
@@ -423,48 +470,6 @@ static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
 		__ieee80211_sta_recalc_aggregates(sta, active_links);
 	}
 
-	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);
-
-		/*
-		 * This call really should not fail. Unfortunately, it appears
-		 * that this may happen occasionally with some drivers. Should
-		 * it happen, we are stuck in a bad place as going backwards is
-		 * not really feasible.
-		 *
-		 * So lets just tell link_use_channel that it must not fail to
-		 * assign the channel context (from mac80211's perspective) and
-		 * assume the driver is going to trigger a recovery flow if it
-		 * had a failure.
-		 * That really is not great nor guaranteed to work. But at least
-		 * the internal mac80211 state remains consistent and there is
-		 * a chance that we can recover.
-		 */
-		ret = _ieee80211_link_use_channel(link,
-						  &link->conf->chanreq,
-						  IEEE80211_CHANCTX_SHARED,
-						  true);
-		WARN_ON_ONCE(ret);
-
-		ieee80211_mgd_set_link_qos_params(link);
-		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);
-	}
-
 	old_active = sdata->vif.active_links;
 	sdata->vif.active_links = active_links;
 

base-commit: 5a4d42c1688c88f3be6aef46b0ea6c32694cd2b8
-- 
2.34.1

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Johannes Berg]

> +		/*
> +		 * inform about the link info changed parameters after all
> +		 * stations are also added
> +		 */

I don't understand that comment - you're not doing anything with the
stations here? And per the commit log it's explicitly _not_ after doing
the AP station. I'm not sure we should set up everything before the AP
station?

> +						  BSS_CHANGED_HE_BSS_COLOR);
> +
> +	}

You make it look like you just moved code but also snuck in a new blank
line :)

johannes

> +
>  	for_each_set_bit(link_id, &rem, IEEE80211_MLD_MAX_NUM_LINKS) {
>  		struct ieee80211_link_data *link;

I also think you put this code too early now - you're now first using
more channel contexts by way of _ieee80211_link_use_channel() before you
even release the ones from deactivated ("rem" bitmap in this code)
links. That doesn't seem like it could work correctly in general.

johannes

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Johannes Berg]

On Tue, 2024-10-01 at 09:45 +0530, Aditya Kumar Singh wrote:
> The current flow in _ieee80211_set_active_links() does not align with the
> operational requirements of drivers that groups multiple hardware
> under a single wiphy.

Btw, given that, I'd argue it's not really "fix ..." right now, since
there's no such driver and no real bug? Dunno. But "fix" makes it sound
like there's a bugfix, and it isn't now, it's just preparing for future
ath12k compatibility.

johannes

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Aditya Kumar Singh]

On 10/1/24 12:01, Johannes Berg wrote:
> 
>> +		/*
>> +		 * inform about the link info changed parameters after all
>> +		 * stations are also added
>> +		 */
> 
> I don't understand that comment - you're not doing anything with the
> stations here? And per the commit log it's explicitly _not_ after doing
> the AP station. I'm not sure we should set up everything before the AP
> station?
> 

Okay let me try to explain the situation here -

In the *if (add)* block, drivers are informed about the new links added 
via drv_change_vif_links(). For drivers like ath12k, we know which link 
is changing, but since the channel contexts for the new links aren’t 
available yet, the driver can’t determine which link the firmware should 
create the interface for. This is because we have more than one firmware 
operating under this interface (grouped multiple hardware under single 
wiphy). Therefore, this notification isn’t very helpful in creating the 
link interface now.

Next, in the loop list_for_each_entry(sta, &local->sta_list, list), 
drv_change_sta_links() is called to notify drivers that the links for a 
given station (STA) have changed. Drivers use this callback to create 
the link stations after the STA has moved to the authorized state.
At this point, the driver knows which ML STA and which link STA to 
create. However, to create a link STA, the corresponding link interface 
must exist first.

Currently, it doesn’t, so the driver can’t add the link STA.

Later, in the loop for_each_set_bit(link_id, &add, 
IEEE80211_MLD_MAX_NUM_LINKS), channels are added. At this stage, the 
driver will actually create the link on the interface at its own level. 
Since here using the channel information, appropriate firmware can be 
picked. For example 2 GHz or 5 GHz or 6 GHz firmware.



>> +						  BSS_CHANGED_HE_BSS_COLOR);
>> +
>> +	}
> 
> You make it look like you just moved code but also snuck in a new blank
> line :)
> 

:) sorry about that. Will fix in next version.

> 
>> +
>>   	for_each_set_bit(link_id, &rem, IEEE80211_MLD_MAX_NUM_LINKS) {
>>   		struct ieee80211_link_data *link;
> 
> I also think you put this code too early now - you're now first using
> more channel contexts by way of _ieee80211_link_use_channel() before you
> even release the ones from deactivated ("rem" bitmap in this code)
> links. That doesn't seem like it could work correctly in general.
> 

hmm... yeah true that. May be I will move this once the old links are 
removed?

-- 
Aditya

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Aditya Kumar Singh]

On 10/1/24 12:02, Johannes Berg wrote:
> On Tue, 2024-10-01 at 09:45 +0530, Aditya Kumar Singh wrote:
>> The current flow in _ieee80211_set_active_links() does not align with the
>> operational requirements of drivers that groups multiple hardware
>> under a single wiphy.
> 
> Btw, given that, I'd argue it's not really "fix ..." right now, since
> there's no such driver and no real bug? Dunno. But "fix" makes it sound
> like there's a bugfix, and it isn't now, it's just preparing for future
> ath12k compatibility.
> 

I see your point. Sure will rephrase the message and will not use 'fix' 
word in next version.

-- 
Aditya

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Johannes Berg]

On Tue, 2024-10-01 at 12:47 +0530, Aditya Kumar Singh wrote:
> On 10/1/24 12:01, Johannes Berg wrote:
> > 
> > > +		/*
> > > +		 * inform about the link info changed parameters after all
> > > +		 * stations are also added
> > > +		 */
> > 
> > I don't understand that comment - you're not doing anything with the
> > stations here? And per the commit log it's explicitly _not_ after doing
> > the AP station. I'm not sure we should set up everything before the AP
> > station?
> > 
> 
> Okay let me try to explain the situation here -
> 
> In the *if (add)* block, drivers are informed about the new links added 
> via drv_change_vif_links().

Sure.

> For drivers like ath12k, we know which link 
> is changing, but since the channel contexts for the new links aren’t 
> available yet, the driver can’t determine which link the firmware should 
> create the interface for. This is because we have more than one firmware 
> operating under this interface (grouped multiple hardware under single 
> wiphy). Therefore, this notification isn’t very helpful in creating the 
> link interface now.

Yes, you said that.

> Next, in the loop list_for_each_entry(sta, &local->sta_list, list), 
> drv_change_sta_links() is called to notify drivers that the links for a 
> given station (STA) have changed. Drivers use this callback to create 
> the link stations after the STA has moved to the authorized state.

Yes, I also got that.

> At this point, the driver knows which ML STA and which link STA to 
> create. However, to create a link STA, the corresponding link interface 
> must exist first.

Right, you said that too :)

> Currently, it doesn’t, so the driver can’t add the link STA.

Right.

But that doesn't explain the *comment*, which literally says:

   inform about the link info changed parameters after all stations are
   also added

but you
 (a) don't add stations here
 (b) if you're thinking about link stations, the link stations are
     only added _after_ this comment and the link info change ...


> Later, in the loop for_each_set_bit(link_id, &add, 
> IEEE80211_MLD_MAX_NUM_LINKS), channels are added. At this stage, the 
> driver will actually create the link on the interface at its own level. 
> Since here using the channel information, appropriate firmware can be 
> picked. For example 2 GHz or 5 GHz or 6 GHz firmware.

Picking "firmware" sounds very odd here, I'd say you mean "which device
to pick"?

> hmm... yeah true that. May be I will move this once the old links are 
> removed?
> 

I'd think at least that?

But also this seems to break out driver for other reasons, because it
initializes rate control somewhere here and needs a station for that.
Didn't look deeply into that yet though.

johannes

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Aditya Kumar Singh]

On 10/1/24 12:52, Johannes Berg wrote:
> But that doesn't explain the*comment*, which literally says:
> 
>     inform about the link info changed parameters after all stations are
>     also added
> 
> but you
>   (a) don't add stations here
>   (b) if you're thinking about link stations, the link stations are
>       only added_after_  this comment and the link info change ...
> 

Oops! my bad. Previously I was thinking like this -

First iterate and do only _ieee80211_link_use_channel() this part. Then 
let the flow as usual and after stations are added, do the 
link_info_changed() part.


> 
>> Later, in the loop for_each_set_bit(link_id, &add,
>> IEEE80211_MLD_MAX_NUM_LINKS), channels are added. At this stage, the
>> driver will actually create the link on the interface at its own level.
>> Since here using the channel information, appropriate firmware can be
>> picked. For example 2 GHz or 5 GHz or 6 GHz firmware.
> Picking "firmware" sounds very odd here, I'd say you mean "which device
> to pick"?

:) Yeah.

> 
>> hmm... yeah true that. May be I will move this once the old links are
>> removed?
>>
> I'd think at least that?
> 
> But also this seems to break out driver for other reasons, because it
> initializes rate control somewhere here and needs a station for that.
> Didn't look deeply into that yet though.

Okay so doing as I said above could work -

if (add) {
	...
}

for_each_set_bit(link_id, &rem, ..) {
	...
}

for_each_set_bit(link_id, &add ...) {
	_ieee80211_link_use_channel()
}

list_for_each_entry(sta, &local->sta_list, list) {
	...
}

...

for_each_set_bit(link_id, &add ....) {
	now call
	ieee80211_mgd_set_link_qos_params()
	ieee80211_link_info_change_notify()
}

...


At least I tried both of these ways in hwsim. I dont see any failures. 
Hence I thought why not move whole for loop to top instead.


-- 
Aditya

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Johannes Berg]

On Tue, 2024-10-01 at 13:07 +0530, Aditya Kumar Singh wrote:
> 
> First iterate and do only _ieee80211_link_use_channel() this part. Then 
> let the flow as usual and after stations are added, do the 
> link_info_changed() part.

That would seem to make sense, it also matches assoc flow better.
Although not sure that matters too much, since this is necessarily very
different as it's while associated anyway.

> > But also this seems to break out driver for other reasons, because it

type - I meant "our driver"

> > initializes rate control somewhere here and needs a station for that.
> > Didn't look deeply into that yet though.
> 
> Okay so doing as I said above could work -
> 
> if (add) {
> 	...
> }
> 
> for_each_set_bit(link_id, &rem, ..) {
> 	...
> }
> 
> for_each_set_bit(link_id, &add ...) {
> 	_ieee80211_link_use_channel()
> }
> 
> list_for_each_entry(sta, &local->sta_list, list) {
> 	...
> }
> 
> ...
> 
> for_each_set_bit(link_id, &add ....) {
> 	now call
> 	ieee80211_mgd_set_link_qos_params()
> 	ieee80211_link_info_change_notify()
> }
> 
> ...
> 
> 
> At least I tried both of these ways in hwsim. I dont see any failures. 
> Hence I thought why not move whole for loop to top instead.

Right, I don't know - I guess I should try with our driver?

johannes

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Aditya Kumar Singh]

On 10/1/24 13:38, Johannes Berg wrote:
> On Tue, 2024-10-01 at 13:07 +0530, Aditya Kumar Singh wrote:
>>
>> First iterate and do only _ieee80211_link_use_channel() this part. Then
>> let the flow as usual and after stations are added, do the
>> link_info_changed() part.
> 
> That would seem to make sense, it also matches assoc flow better.
> Although not sure that matters too much, since this is necessarily very
> different as it's while associated anyway.
> 
>>> But also this seems to break out driver for other reasons, because it
> 
> type - I meant "our driver"
> 
>>> initializes rate control somewhere here and needs a station for that.
>>> Didn't look deeply into that yet though.
>>
>> Okay so doing as I said above could work -
>>
>> if (add) {
>> 	...
>> }
>>
>> for_each_set_bit(link_id, &rem, ..) {
>> 	...
>> }
>>
>> for_each_set_bit(link_id, &add ...) {
>> 	_ieee80211_link_use_channel()
>> }
>>
>> list_for_each_entry(sta, &local->sta_list, list) {
>> 	...
>> }
>>
>> ...
>>
>> for_each_set_bit(link_id, &add ....) {
>> 	now call
>> 	ieee80211_mgd_set_link_qos_params()
>> 	ieee80211_link_info_change_notify()
>> }
>>
>> ...
>>
>>
>> At least I tried both of these ways in hwsim. I dont see any failures.
>> Hence I thought why not move whole for loop to top instead.
> 
> Right, I don't know - I guess I should try with our driver?
> 

Yes please that would be of great help. Let me send a next version 
having the changes as discussed above and then you could pick that for 
testing and let us know whether it is working as expected by Intel driver?

It would be better if other MLO based drivers could also test this? I 
mean if possible they could also test and let us know if this breaks any 
of their expectations! Or if not test, at least ack this change?


-- 
Aditya

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Johannes Berg]

On Tue, 2024-10-01 at 13:56 +0530, Aditya Kumar Singh wrote:
> Yes please that would be of great help. Let me send a next version 
> having the changes as discussed above and then you could pick that for 
> testing and let us know whether it is working as expected by Intel driver?

Sure. I suspect it'll work better if you do the ordering as you had
described in the comment.

> It would be better if other MLO based drivers could also test this? I 
> mean if possible they could also test and let us know if this breaks any 
> of their expectations! Or if not test, at least ack this change?

I guess they'll just have to speak up here too :)

johannes

Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Aditya Kumar Singh]

On 10/1/24 13:58, Johannes Berg wrote:
> On Tue, 2024-10-01 at 13:56 +0530, Aditya Kumar Singh wrote:
>> Yes please that would be of great help. Let me send a next version
>> having the changes as discussed above and then you could pick that for
>> testing and let us know whether it is working as expected by Intel driver?
> 
> Sure. I suspect it'll work better if you do the ordering as you had
> described in the comment.

Sure, done and I have sent the next version. Do let us know if you face 
any issues.

> 
>> It would be better if other MLO based drivers could also test this? I
>> mean if possible they could also test and let us know if this breaks any
>> of their expectations! Or if not test, at least ack this change?
> 
> I guess they'll just have to speak up here too :)

:)


-- 
Aditya