include/net/mac80211.h | 2 +- net/mac80211/link.c | 44 ++++++++++++++++++++++---------------------- 2 files changed, 23 insertions(+), 23 deletions(-)
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>
---
Changes in v2:
- Changed function doc as well to reflect the correct flow.
- Link to v1: https://lore.kernel.org/r/20241205-unassign_activate_links-v1-1-84097a1abdeb@quicinc.com
---
include/net/mac80211.h | 2 +-
net/mac80211/link.c | 44 ++++++++++++++++++++++----------------------
2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5ce4dfa3fba55fd6c33e6261c340feea9e5031ed..6159dff1bdbc2e3e0ec40581efee66730eb7300d 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -7671,13 +7671,13 @@ static inline bool ieee80211_is_tx_data(struct sk_buff *skb)
* a sequence of calls like
*
* - change_vif_links(0x11)
- * - unassign_vif_chanctx(link_id=0)
* - assign_vif_chanctx(link_id=4)
* - change_sta_links(0x11) for each affected STA (the AP)
* (TDLS connections on now inactive links should be torn down)
* - remove group keys on the old link (link_id 0)
* - add new group keys (GTK/IGTK/BIGTK) on the new link (link_id 4)
* - change_sta_links(0x10) for each affected STA (the AP)
+ * - unassign_vif_chanctx(link_id=0)
* - change_vif_links(0x10)
*
* Return: 0 on success. An error code otherwise.
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> > Therefore, re-order the logic so that stations are handled first and then > channel is unassigned. > This causes memory leaks in my tests with iwlwifi. johannes
On Thu, 2024-12-05 at 12:43 +0100, Johannes Berg wrote: > > > > Therefore, re-order the logic so that stations are handled first and then > > channel is unassigned. > > > > This causes memory leaks in my tests with iwlwifi. > And also firmware crashes because the station is removed while it's still being used. johannes
On 12/5/24 18:30, Johannes Berg wrote: > On Thu, 2024-12-05 at 12:43 +0100, Johannes Berg wrote: >>> >>> Therefore, re-order the logic so that stations are handled first and then >>> channel is unassigned. >>> >> >> This causes memory leaks in my tests with iwlwifi. >> > > And also firmware crashes because the station is removed while it's > still being used. > So is this exposing some underlying issue with iwlwifi? Or this change will break drivers which does not group multiple hardware into single wiphy? Also, how about non-ML scenario in iwlwifi? There, first station is removed and then the interface goes down right? -- Aditya
On Thu, 2024-12-05 at 20:43 +0530, Aditya Kumar Singh wrote: > On 12/5/24 18:30, Johannes Berg wrote: > > On Thu, 2024-12-05 at 12:43 +0100, Johannes Berg wrote: > > > > > > > > Therefore, re-order the logic so that stations are handled first and then > > > > channel is unassigned. > > > > > > > > > > This causes memory leaks in my tests with iwlwifi. > > > > > > > And also firmware crashes because the station is removed while it's > > still being used. > > > > So is this exposing some underlying issue with iwlwifi? I don't think so? > Or this change > will break drivers which does not group multiple hardware into single > wiphy? Not necessarily, but it breaks iwlwifi because of the changed order of operations, and what it does with the firmware. I think the issue here is that we treat link active == link has channel context in iwlwifi, and an active link in client mode requires a station in firmware. Otherwise you cannot even deactivate a link, since that requires sending an NDP to the AP, but if you don't have the AP STA you can't do that ... I guess the driver could be changed to treat station links as active when they have the AP STA entry, but that seems ... difficult and strange, it would make it different between AP and client modes? Looking at your commit message more, I wonder if it really even makes sense to *delete* the link when the channel context is unassigned, rather than (similarly to iwlwifi) deactivating it and deleting it later when it's actually removed (change_vif_links)? You do know which hardware it is/was on, after all. And these two operations can *never* be atomic. Removing the STAs first might be something that's appropriate for AP mode, but I guess I'm more with iwlwifi here in that it doesn't seem quite right for client mode? > Also, how about non-ML scenario in iwlwifi? There, first station is > removed and then the interface goes down right? It's not so much about the interface but the link, it seems. johannes
On 12/6/24 15:37, Johannes Berg wrote: > On Thu, 2024-12-05 at 20:43 +0530, Aditya Kumar Singh wrote: >> On 12/5/24 18:30, Johannes Berg wrote: >>> On Thu, 2024-12-05 at 12:43 +0100, Johannes Berg wrote: >>>>> >>>>> Therefore, re-order the logic so that stations are handled first and then >>>>> channel is unassigned. >>>>> >>>> >>>> This causes memory leaks in my tests with iwlwifi. >>>> >>> >>> And also firmware crashes because the station is removed while it's >>> still being used. >>> >> >> So is this exposing some underlying issue with iwlwifi? > > I don't think so? > >> Or this change >> will break drivers which does not group multiple hardware into single >> wiphy? > > Not necessarily, but it breaks iwlwifi because of the changed order of > operations, and what it does with the firmware. > > I think the issue here is that we treat link active == link has channel > context in iwlwifi, and an active link in client mode requires a station > in firmware. Otherwise you cannot even deactivate a link, since that > requires sending an NDP to the AP, but if you don't have the AP STA you > can't do that ... Fair enough ... > > I guess the driver could be changed to treat station links as active > when they have the AP STA entry, but that seems ... difficult and > strange, it would make it different between AP and client modes? > > Looking at your commit message more, I wonder if it really even makes > sense to *delete* the link when the channel context is unassigned, > rather than (similarly to iwlwifi) deactivating it and deleting it later > when it's actually removed (change_vif_links)? You do know which > hardware it is/was on, after all. And these two operations can *never* > be atomic. Removing the STAs first might be something that's appropriate > for AP mode, but I guess I'm more with iwlwifi here in that it doesn't > seem quite right for client mode? I see your point. I need to experiment and see whether this way works or not for ath12k. Let me try that out. > >> Also, how about non-ML scenario in iwlwifi? There, first station is >> removed and then the interface goes down right? > > It's not so much about the interface but the link, it seems. > Sure.. -- Aditya
© 2016 - 2025 Red Hat, Inc.