[PATCH v2] wifi: mac80211: re-order assigning channel in activate links

[PATCH v2] wifi: mac80211: re-order 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, re-order the logic so that after drv_change_vif_links() and
removing the old links, channels are assigned to newly added links.
Following this, the flow proceeds to station handling.

Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
v2: * assigned channel after removing older links.
    * kept the call to link_info_changed() in the original place itself.
    * got rid of using word 'fix'

---
 net/mac80211/link.c | 51 +++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/net/mac80211/link.c b/net/mac80211/link.c
index 0bbac64d5fa0..019e1c1311b4 100644
--- a/net/mac80211/link.c
+++ b/net/mac80211/link.c
@@ -385,6 +385,37 @@ static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
 						 jiffies);
 	}
 
+	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
+		 */
+	}
+
 	list_for_each_entry(sta, &local->sta_list, list) {
 		if (sdata != sta->sdata)
 			continue;
@@ -428,26 +459,6 @@ static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
 
 		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 |

base-commit: 5a4d42c1688c88f3be6aef46b0ea6c32694cd2b8
-- 
2.34.1

Re: [PATCH v2] wifi: mac80211: re-order assigning channel in activate links

[Johannes Berg]

On Tue, 2024-10-01 at 14:20 +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. 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, re-order the logic so that after drv_change_vif_links() and
> removing the old links, channels are assigned to newly added links.
> Following this, the flow proceeds to station handling.
> 

I tried this again but I fear it fundamentally cannot work with iwlwifi.

We have this comment:

        /* Initialize rate control for the AP station, since we might be
         * doing a link switch here - we cannot initialize it before since
         * this needs the phy context assigned (and in FW?), and we cannot
         * do it later because it needs to be initialized as soon as we're
         * able to TX on the link, i.e. when active.
         */

which sort of indicates that we're working around it, but it also
correctly says that we cannot activate a link before we have the (link)
station.

In the flow as you changed it we'd activate the link in firmware before
the stations are added, but that isn't allowed. There's not really a
good place to hook into after the station is added, unless we somehow
want to activate the link from the station change, but that seems ...
odd to say the least? Though I guess it's already somewhat odd to init
rate control here as written now...

Maybe we can hook into the later link info change. This seems to
initially work, but still doing more tests:


diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c
index 9753d2c1df3e..5d90fb53b762 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c
@@ -343,33 +343,20 @@ __iwl_mvm_mld_assign_vif_chanctx(struct iwl_mvm *mvm,
 	if (ret)
 		goto out;
 
-	/* Initialize rate control for the AP station, since we might be
-	 * doing a link switch here - we cannot initialize it before since
-	 * this needs the phy context assigned (and in FW?), and we cannot
-	 * do it later because it needs to be initialized as soon as we're
-	 * able to TX on the link, i.e. when active.
+	/*
+	 * if link switching (link not active yet) we'll activate it in
+	 * firmware later on link-info change, which mac80211 guarantees
+	 * for link switch after the stations are set up
 	 */
-	if (mvmvif->ap_sta) {
-		struct ieee80211_link_sta *link_sta;
-
-		rcu_read_lock();
-		link_sta = rcu_dereference(mvmvif->ap_sta->link[link_id]);
-
-		if (!WARN_ON_ONCE(!link_sta))
-			iwl_mvm_rs_rate_init(mvm, vif, mvmvif->ap_sta,
-					     link_conf, link_sta,
-					     phy_ctxt->channel->band);
-		rcu_read_unlock();
+	if (ieee80211_vif_link_active(vif, link_conf->link_id)) {
+		ret = iwl_mvm_link_changed(mvm, vif, link_conf,
+					   LINK_CONTEXT_MODIFY_ACTIVE |
+					   LINK_CONTEXT_MODIFY_RATES_INFO,
+					   true);
+		if (ret)
+			goto out;
 	}
 
-	/* then activate */
-	ret = iwl_mvm_link_changed(mvm, vif, link_conf,
-				   LINK_CONTEXT_MODIFY_ACTIVE |
-				   LINK_CONTEXT_MODIFY_RATES_INFO,
-				   true);
-	if (ret)
-		goto out;
-
 	if (vif->type == NL80211_IFTYPE_STATION)
 		iwl_mvm_send_ap_tx_power_constraint_cmd(mvm, vif,
 							link_conf,
@@ -786,6 +773,11 @@ iwl_mvm_mld_link_info_changed_station(struct iwl_mvm *mvm,
 	if (WARN_ON_ONCE(!mvmvif->link[link_conf->link_id]))
 		return;
 
+	/* not yet marked active in vif means during link switch */
+	if (!ieee80211_vif_link_active(vif, link_conf->link_id) &&
+	    vif->cfg.assoc && mvmvif->link[link_conf->link_id]->phy_ctxt)
+		link_changes |= LINK_CONTEXT_MODIFY_ACTIVE;
+
 	has_he = link_conf->he_support && !iwlwifi_mod_params.disable_11ax;
 	has_eht = link_conf->eht_support && !iwlwifi_mod_params.disable_11be;
 


johannes

Re: [PATCH v2] wifi: mac80211: re-order assigning channel in activate links

[Aditya Kumar Singh]

On 10/1/24 15:29, Johannes Berg wrote:
> On Tue, 2024-10-01 at 14:20 +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. 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, re-order the logic so that after drv_change_vif_links() and
>> removing the old links, channels are assigned to newly added links.
>> Following this, the flow proceeds to station handling.
>>
> 
> I tried this again but I fear it fundamentally cannot work with iwlwifi.
> 
> We have this comment:
> 
>          /* Initialize rate control for the AP station, since we might be
>           * doing a link switch here - we cannot initialize it before since
>           * this needs the phy context assigned (and in FW?), and we cannot
>           * do it later because it needs to be initialized as soon as we're
>           * able to TX on the link, i.e. when active.
>           */
> 
> which sort of indicates that we're working around it, but it also
> correctly says that we cannot activate a link before we have the (link)
> station.
> 
> In the flow as you changed it we'd activate the link in firmware before
> the stations are added, but that isn't allowed. There's not really a

Is this a generic expectation? And that too only for ML STA? Since at 
least for ML AP, we could have links in firmware active and later when 
station connects, we create link stations.

> good place to hook into after the station is added, unless we somehow
> want to activate the link from the station change, but that seems ...
> odd to say the least? Though I guess it's already somewhat odd to init
> rate control here as written now...
> 
> Maybe we can hook into the later link info change. This seems to
> initially work, but still doing more tests:
> 

sure, hoping that it passes all ;)

-- 
Aditya

Re: [PATCH v2] wifi: mac80211: re-order assigning channel in activate links

[Aditya Kumar Singh]

On 10/1/24 15:49, Aditya Kumar Singh wrote:
> On 10/1/24 15:29, Johannes Berg wrote:
>> On Tue, 2024-10-01 at 14:20 +0530, Aditya Kumar Singh wrote:
...
>>
>> which sort of indicates that we're working around it, but it also
>> correctly says that we cannot activate a link before we have the (link)
>> station.
>>
>> In the flow as you changed it we'd activate the link in firmware before
>> the stations are added, but that isn't allowed. There's not really a
> 
> Is this a generic expectation? And that too only for ML STA? Since at 
> least for ML AP, we could have links in firmware active and later when 
> station connects, we create link stations.
> 
>> good place to hook into after the station is added, unless we somehow
>> want to activate the link from the station change, but that seems ...
>> odd to say the least? Though I guess it's already somewhat odd to init
>> rate control here as written now...
>>
>> Maybe we can hook into the later link info change. This seems to
>> initially work, but still doing more tests:
>>
> 
> sure, hoping that it passes all ;)
> 

Hi Johannes,

Is your testing complete? Do you see any issues with this change?

-- 
Aditya

Re: [PATCH v2] wifi: mac80211: re-order assigning channel in activate links

[Johannes Berg]

On Tue, 2024-10-08 at 10:43 +0530, Aditya Kumar Singh wrote:
> 
> 
> Is your testing complete? Do you see any issues with this change?

Yeah, it can be made to work, but we're still discussing the best way
with some internals (slowly, due to the holidays in Israel).

I think you can just leave this patch as is, and I'll keep it pending in
patchwork until we figure it out, and then I'll just amend it with the
iwlwifi changes when I merge it.

johannes

Re: [PATCH v2] wifi: mac80211: re-order assigning channel in activate links

[Aditya Kumar Singh]

On 10/8/24 13:06, Johannes Berg wrote:
> On Tue, 2024-10-08 at 10:43 +0530, Aditya Kumar Singh wrote:
>>
>>
>> Is your testing complete? Do you see any issues with this change?
> 
> Yeah, it can be made to work, but we're still discussing the best way
> with some internals (slowly, due to the holidays in Israel).
> 
> I think you can just leave this patch as is, and I'll keep it pending in
> patchwork until we figure it out, and then I'll just amend it with the
> iwlwifi changes when I merge it.

Sure, thanks.


-- 
Aditya