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

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

[Aditya Kumar Singh]

The current flow in _ieee80211_set_active_links() during link removal
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 information to determine the appropriate hardware for each link.

Now, during the link removal process, the channel is first unassigned from
the links via a call to __ieee80211_link_release_channel(). After this, the
state of all connected stations is updated via drv_change_sta_links().
This is followed by handling keys in the links, and finally, removing the
link by calling drv_change_vif_links().

For above mentioned drivers (such as ath12k), with the above flow, once the
channel is unassigned from the link, the link would be deleted at the
driver and firmware level. However, at this point, the station still exist,
leading to failures in deactivating the links.

Additionally, if we consider the link addition flow [1], channels are first
assigned, and then stations are created. So conversely, during removal,
ideally, the station should be removed first, and then the channel should
be unassigned.

Therefore, re-order the logic so that stations are handled first and then
channel is unassigned.

[1]: https://lore.kernel.org/linux-wireless/20241001085034.2745669-1-quic_adisi@quicinc.com/

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

diff --git a/net/mac80211/link.c b/net/mac80211/link.c
index 58a76bcd6ae68670fbbe7fa7d07540c04ff996f8..3c46d2b2ee254fab324d57f4d0fbe94ace76d89d 100644
--- a/net/mac80211/link.c
+++ b/net/mac80211/link.c
@@ -367,28 +367,6 @@ static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
-	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);
-
-		ieee80211_teardown_tdls_peers(link);
-
-		__ieee80211_link_release_channel(link, true);
-
-		/*
-		 * If CSA is (still) active while the link is deactivated,
-		 * just schedule the channel switch work for the time we
-		 * had previously calculated, and we'll take the process
-		 * from there.
-		 */
-		if (link->conf->csa_active)
-			wiphy_delayed_work_queue(local->hw.wiphy,
-						 &link->u.mgd.csa.switch_work,
-						 link->u.mgd.csa.time -
-						 jiffies);
-	}
-
 	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
 		struct ieee80211_link_data *link;
 
@@ -458,6 +436,28 @@ 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, &rem, IEEE80211_MLD_MAX_NUM_LINKS) {
+		struct ieee80211_link_data *link;
+
+		link = sdata_dereference(sdata->link[link_id], sdata);
+
+		ieee80211_teardown_tdls_peers(link);
+
+		__ieee80211_link_release_channel(link, true);
+
+		/*
+		 * If CSA is (still) active while the link is deactivated,
+		 * just schedule the channel switch work for the time we
+		 * had previously calculated, and we'll take the process
+		 * from there.
+		 */
+		if (link->conf->csa_active)
+			wiphy_delayed_work_queue(local->hw.wiphy,
+						 &link->u.mgd.csa.switch_work,
+						 link->u.mgd.csa.time -
+						 jiffies);
+	}
+
 	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
 		struct ieee80211_link_data *link;
 

---
base-commit: b81e0211e9c70be9eb70924e4e29698bfbbbc03a
change-id: 20241205-unassign_activate_links-6a574859b997

RE: [PATCH] wifi: mac80211: re-order unassigning channel in activate links

[Zong-Zhe Yang]

Aditya Kumar Singh <quic_adisi@quicinc.com> wrote:
> 
> [...]
> 
>  net/mac80211/link.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/net/mac80211/link.c b/net/mac80211/link.c index
> 58a76bcd6ae68670fbbe7fa7d07540c04ff996f8..3c46d2b2ee254fab324d57f4d0fbe94ace76d8
> 9d 100644
> --- a/net/mac80211/link.c
> +++ b/net/mac80211/link.c
> @@ -367,28 +367,6 @@ static int _ieee80211_set_active_links(struct ieee80211_sub_if_data
> *sdata,
>                 }
>         }
> 
> -       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);
> -
> -               ieee80211_teardown_tdls_peers(link);
> -
> -               __ieee80211_link_release_channel(link, true);
> -
> -               /*
> -                * If CSA is (still) active while the link is deactivated,
> -                * just schedule the channel switch work for the time we
> -                * had previously calculated, and we'll take the process
> -                * from there.
> -                */
> -               if (link->conf->csa_active)
> -                       wiphy_delayed_work_queue(local->hw.wiphy,
> -                                                &link->u.mgd.csa.switch_work,
> -                                                link->u.mgd.csa.time -
> -                                                jiffies);
> -       }
> -
>         for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
>                 struct ieee80211_link_data *link;
> 
> @@ -458,6 +436,28 @@ 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, &rem, IEEE80211_MLD_MAX_NUM_LINKS) {
> +               struct ieee80211_link_data *link;
> +
> +               link = sdata_dereference(sdata->link[link_id], sdata);
> +
> +               ieee80211_teardown_tdls_peers(link);
> +
> +               __ieee80211_link_release_channel(link, true);
> +
> +               /*
> +                * If CSA is (still) active while the link is deactivated,
> +                * just schedule the channel switch work for the time we
> +                * had previously calculated, and we'll take the process
> +                * from there.
> +                */
> +               if (link->conf->csa_active)
> +                       wiphy_delayed_work_queue(local->hw.wiphy,
> +                                                &link->u.mgd.csa.switch_work,
> +                                                link->u.mgd.csa.time -
> +                                                jiffies);
> +       }
> +
>         for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
>                 struct ieee80211_link_data *link;
> 

Could you also update the description of ieee80211_set_active_links() (include/net/mac80211.h) to align the changes?
I think it would be like:

  change_vif_links(0x11)
  assign_vif_chanctx(link_id=4)
  change_sta_links(0x11) for each affected STA (the AP)
  [...]
  change_sta_links(0x10) for each affected STA (the AP)
  unassign_vif_chanctx(link_id=0)
  change_vif_links(0x10)

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

[Aditya Kumar Singh]

On 12/5/24 14:08, Zong-Zhe Yang wrote:
> Could you also update the description of ieee80211_set_active_links() (include/net/mac80211.h) to align the changes?
> I think it would be like:
> 
>    change_vif_links(0x11)
>    assign_vif_chanctx(link_id=4)
>    change_sta_links(0x11) for each affected STA (the AP)
>    [...]
>    change_sta_links(0x10) for each affected STA (the AP)
>    unassign_vif_chanctx(link_id=0)
>    change_vif_links(0x10)

Good point! I will do it in next version, thanks for pointing it out.

-- 
Aditya