From: Johannes Berg <[email protected]>
Implement an API function and debugfs file to switch
active links.
Also provide an async version of the API so drivers
can call it in arbitrary contexts, e.g. while in the
authorized callback.
Signed-off-by: Johannes Berg <[email protected]>
---
include/net/mac80211.h | 41 ++++++++
net/mac80211/debugfs_netdev.c | 26 ++++++
net/mac80211/ieee80211_i.h | 4 +
net/mac80211/iface.c | 12 +++
net/mac80211/key.c | 34 +++++++
net/mac80211/key.h | 3 +
net/mac80211/link.c | 171 ++++++++++++++++++++++++++++++++++
7 files changed, 291 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 873e81a45a97..ac2bad57933f 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -7184,4 +7184,45 @@ static inline bool ieee80211_is_tx_data(struct sk_buff *skb)
ieee80211_is_data(hdr->frame_control);
}
+/**
+ * ieee80211_set_active_links - set active links in client mode
+ * @vif: interface to set active links on
+ * @active_links: the new active links bitmap
+ *
+ * This changes the active links on an interface. The interface
+ * must be in client mode (in AP mode, all links are always active),
+ * and @active_links must be a subset of the vif's valid_links.
+ *
+ * If a link is switched off and another is switched on at the same
+ * time (e.g. active_links going from 0x1 to 0x10) then you will get
+ * a sequence of calls like
+ * - change_vif_links(0x11)
+ * - unassign_vif_chanctx(link_id=0)
+ * - 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)
+ * - assign_vif_chanctx(link_id=4)
+ * - change_vif_links(0x10)
+ *
+ * Note: This function acquires some mac80211 locks and must not
+ * be called with any driver locks held that could cause a
+ * lock dependency inversion. Best call it without locks.
+ */
+int ieee80211_set_active_links(struct ieee80211_vif *vif, u16 active_links);
+
+/**
+ * ieee80211_set_active_links_async - asynchronously set active links
+ * @vif: interface to set active links on
+ * @active_links: the new active links bitmap
+ *
+ * See ieee80211_set_active_links() for more information, the only
+ * difference here is that the link change is triggered async and
+ * can be called in any context, but the link switch will only be
+ * completed after it returns.
+ */
+void ieee80211_set_active_links_async(struct ieee80211_vif *vif,
+ u16 active_links);
+
#endif /* MAC80211_H */
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index 1e5b041a5cea..5b014786fd2d 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -570,6 +570,30 @@ static ssize_t ieee80211_if_parse_tsf(
}
IEEE80211_IF_FILE_RW(tsf);
+static ssize_t ieee80211_if_fmt_valid_links(const struct ieee80211_sub_if_data *sdata,
+ char *buf, int buflen)
+{
+ return snprintf(buf, buflen, "0x%x\n", sdata->vif.valid_links);
+}
+IEEE80211_IF_FILE_R(valid_links);
+
+static ssize_t ieee80211_if_fmt_active_links(const struct ieee80211_sub_if_data *sdata,
+ char *buf, int buflen)
+{
+ return snprintf(buf, buflen, "0x%x\n", sdata->vif.active_links);
+}
+
+static ssize_t ieee80211_if_parse_active_links(struct ieee80211_sub_if_data *sdata,
+ const char *buf, int buflen)
+{
+ u16 active_links;
+
+ if (kstrtou16(buf, 0, &active_links))
+ return -EINVAL;
+
+ return ieee80211_set_active_links(&sdata->vif, active_links) ?: buflen;
+}
+IEEE80211_IF_FILE_RW(active_links);
#ifdef CONFIG_MAC80211_MESH
IEEE80211_IF_FILE(estab_plinks, u.mesh.estab_plinks, ATOMIC);
@@ -670,6 +694,8 @@ static void add_sta_files(struct ieee80211_sub_if_data *sdata)
DEBUGFS_ADD_MODE(uapsd_queues, 0600);
DEBUGFS_ADD_MODE(uapsd_max_sp_len, 0600);
DEBUGFS_ADD_MODE(tdls_wider_bw, 0600);
+ DEBUGFS_ADD_MODE(valid_links, 0200);
+ DEBUGFS_ADD_MODE(active_links, 0600);
}
static void add_ap_files(struct ieee80211_sub_if_data *sdata)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 977aea4467e0..4e1d4c339f2d 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1081,6 +1081,10 @@ struct ieee80211_sub_if_data {
struct ieee80211_link_data deflink;
struct ieee80211_link_data __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS];
+ /* for ieee80211_set_active_links_async() */
+ struct work_struct activate_links_work;
+ u16 desired_active_links;
+
#ifdef CONFIG_MAC80211_DEBUGFS
struct {
struct dentry *subdir_stations;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index bf1c44d53c30..30c62b63d179 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -683,6 +683,8 @@ static int ieee80211_stop(struct net_device *dev)
ieee80211_stop_mbssid(sdata);
}
+ cancel_work_sync(&sdata->activate_links_work);
+
wiphy_lock(sdata->local->hw.wiphy);
ieee80211_do_stop(sdata, true);
wiphy_unlock(sdata->local->hw.wiphy);
@@ -1653,6 +1655,15 @@ static void ieee80211_recalc_smps_work(struct work_struct *work)
ieee80211_recalc_smps(sdata, &sdata->deflink);
}
+static void ieee80211_activate_links_work(struct work_struct *work)
+{
+ struct ieee80211_sub_if_data *sdata =
+ container_of(work, struct ieee80211_sub_if_data,
+ activate_links_work);
+
+ ieee80211_set_active_links(&sdata->vif, sdata->desired_active_links);
+}
+
/*
* Helper function to initialise an interface to a specific type.
*/
@@ -1690,6 +1701,7 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
skb_queue_head_init(&sdata->status_queue);
INIT_WORK(&sdata->work, ieee80211_iface_work);
INIT_WORK(&sdata->recalc_smps, ieee80211_recalc_smps_work);
+ INIT_WORK(&sdata->activate_links_work, ieee80211_activate_links_work);
switch (type) {
case NL80211_IFTYPE_P2P_GO:
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index f6f0f65fb255..e8f6c1e5eabf 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -1445,3 +1445,37 @@ void ieee80211_key_replay(struct ieee80211_key_conf *keyconf)
}
}
EXPORT_SYMBOL_GPL(ieee80211_key_replay);
+
+int ieee80211_key_switch_links(struct ieee80211_sub_if_data *sdata,
+ unsigned long del_links_mask,
+ unsigned long add_links_mask)
+{
+ struct ieee80211_key *key;
+ int ret;
+
+ list_for_each_entry(key, &sdata->key_list, list) {
+ if (key->conf.link_id < 0 ||
+ !(del_links_mask & BIT(key->conf.link_id)))
+ continue;
+
+ /* shouldn't happen for per-link keys */
+ WARN_ON(key->sta);
+
+ ieee80211_key_disable_hw_accel(key);
+ }
+
+ list_for_each_entry(key, &sdata->key_list, list) {
+ if (key->conf.link_id < 0 ||
+ !(add_links_mask & BIT(key->conf.link_id)))
+ continue;
+
+ /* shouldn't happen for per-link keys */
+ WARN_ON(key->sta);
+
+ ret = ieee80211_key_enable_hw_accel(key);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index 518af24aab56..f3df97df4b72 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -165,6 +165,9 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata,
void ieee80211_free_sta_keys(struct ieee80211_local *local,
struct sta_info *sta);
void ieee80211_reenable_keys(struct ieee80211_sub_if_data *sdata);
+int ieee80211_key_switch_links(struct ieee80211_sub_if_data *sdata,
+ unsigned long del_links_mask,
+ unsigned long add_links_mask);
#define key_mtx_dereference(local, ref) \
rcu_dereference_protected(ref, lockdep_is_held(&((local)->key_mtx)))
diff --git a/net/mac80211/link.c b/net/mac80211/link.c
index 8df348a5edce..e309708abae8 100644
--- a/net/mac80211/link.c
+++ b/net/mac80211/link.c
@@ -9,6 +9,7 @@
#include <net/mac80211.h>
#include "ieee80211_i.h"
#include "driver-ops.h"
+#include "key.h"
void ieee80211_link_setup(struct ieee80211_link_data *link)
{
@@ -300,3 +301,173 @@ void ieee80211_vif_clear_links(struct ieee80211_sub_if_data *sdata)
ieee80211_free_links(sdata, links);
}
+
+static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
+ u16 active_links)
+{
+ struct ieee80211_bss_conf *link_confs[IEEE80211_MLD_MAX_NUM_LINKS];
+ struct ieee80211_local *local = sdata->local;
+ u16 old_active = sdata->vif.active_links;
+ unsigned long rem = old_active & ~active_links;
+ unsigned long add = active_links & ~old_active;
+ struct sta_info *sta;
+ unsigned int link_id;
+ int ret, i;
+
+ if (!ieee80211_sdata_running(sdata))
+ return -ENETDOWN;
+
+ if (sdata->vif.type != NL80211_IFTYPE_STATION)
+ return -EINVAL;
+
+ /* cannot activate links that don't exist */
+ if (active_links & ~sdata->vif.valid_links)
+ return -EINVAL;
+
+ /* nothing to do */
+ if (old_active == active_links)
+ return 0;
+
+ for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++)
+ link_confs[i] = sdata_dereference(sdata->vif.link_conf[i],
+ sdata);
+
+ if (add) {
+ sdata->vif.active_links |= active_links;
+ ret = drv_change_vif_links(local, sdata,
+ old_active,
+ sdata->vif.active_links,
+ link_confs);
+ if (ret) {
+ sdata->vif.active_links = old_active;
+ return ret;
+ }
+ }
+
+ 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);
+
+ /* FIXME: kill TDLS connections on the link */
+
+ ieee80211_link_release_channel(link);
+ }
+
+ 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);
+ 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);
+ }
+
+ 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);
+ WARN_ON_ONCE(ret);
+
+ 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);
+ ieee80211_mgd_set_link_qos_params(link);
+ }
+
+ old_active = sdata->vif.active_links;
+ sdata->vif.active_links = active_links;
+
+ if (rem) {
+ ret = drv_change_vif_links(local, sdata, old_active,
+ active_links, link_confs);
+ WARN_ON_ONCE(ret);
+ }
+
+ return 0;
+}
+
+int ieee80211_set_active_links(struct ieee80211_vif *vif, u16 active_links)
+{
+ struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+ struct ieee80211_local *local = sdata->local;
+ u16 old_active;
+ int ret;
+
+ sdata_lock(sdata);
+ mutex_lock(&local->sta_mtx);
+ mutex_lock(&local->mtx);
+ mutex_lock(&local->key_mtx);
+ old_active = sdata->vif.active_links;
+ if (old_active & active_links) {
+ /*
+ * if there's at least one link that stays active across
+ * the change then switch to it (to those) first, and
+ * then enable the additional links
+ */
+ ret = _ieee80211_set_active_links(sdata,
+ old_active & active_links);
+ if (!ret)
+ ret = _ieee80211_set_active_links(sdata, active_links);
+ } else {
+ /* otherwise switch directly */
+ ret = _ieee80211_set_active_links(sdata, active_links);
+ }
+ mutex_unlock(&local->key_mtx);
+ mutex_unlock(&local->mtx);
+ mutex_unlock(&local->sta_mtx);
+ sdata_unlock(sdata);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(ieee80211_set_active_links);
+
+void ieee80211_set_active_links_async(struct ieee80211_vif *vif,
+ u16 active_links)
+{
+ struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+
+ if (!ieee80211_sdata_running(sdata))
+ return;
+
+ if (sdata->vif.type != NL80211_IFTYPE_STATION)
+ return;
+
+ /* cannot activate links that don't exist */
+ if (active_links & ~sdata->vif.valid_links)
+ return;
+
+ /* nothing to do */
+ if (sdata->vif.active_links == active_links)
+ return;
+
+ sdata->desired_active_links = active_links;
+ schedule_work(&sdata->activate_links_work);
+}
+EXPORT_SYMBOL_GPL(ieee80211_set_active_links_async);
--
2.37.2
On 9/2/2022 10:12 PM, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> Implement an API function and debugfs file to switch
> active links.
>
> Also provide an async version of the API so drivers
> can call it in arbitrary contexts, e.g. while in the
> authorized callback.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> include/net/mac80211.h | 41 ++++++++
> net/mac80211/debugfs_netdev.c | 26 ++++++
> net/mac80211/ieee80211_i.h | 4 +
> net/mac80211/iface.c | 12 +++
> net/mac80211/key.c | 34 +++++++
> net/mac80211/key.h | 3 +
> net/mac80211/link.c | 171 ++++++++++++++++++++++++++++++++++
> 7 files changed, 291 insertions(+)
>
> ...
> +static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
> + u16 active_links)
> +{
> + struct ieee80211_bss_conf *link_confs[IEEE80211_MLD_MAX_NUM_LINKS];
> + struct ieee80211_local *local = sdata->local;
> + u16 old_active = sdata->vif.active_links;
> + unsigned long rem = old_active & ~active_links;
> + unsigned long add = active_links & ~old_active;
> + struct sta_info *sta;
> + unsigned int link_id;
> + int ret, i;
> +
> + if (!ieee80211_sdata_running(sdata))
> + return -ENETDOWN;
> +
> + if (sdata->vif.type != NL80211_IFTYPE_STATION)
> + return -EINVAL;
> +
> + /* cannot activate links that don't exist */
> + if (active_links & ~sdata->vif.valid_links)
> + return -EINVAL;
> +
> + /* nothing to do */
> + if (old_active == active_links)
> + return 0;
> +
> + for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++)
> + link_confs[i] = sdata_dereference(sdata->vif.link_conf[i],
> + sdata);
> +
> + if (add) {
> + sdata->vif.active_links |= active_links;
> + ret = drv_change_vif_links(local, sdata,
> + old_active,
> + sdata->vif.active_links,
> + link_confs);
> + if (ret) {
> + sdata->vif.active_links = old_active;
> + return ret;
> + }
> + }
> +
> + 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);
> +
> + /* FIXME: kill TDLS connections on the link */
> +
> + ieee80211_link_release_channel(link);
> + }
> +
> + 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?
> + 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?
> + 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?
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?
> + WARN_ON_ONCE(ret);
> +
> + 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);
> + ieee80211_mgd_set_link_qos_params(link);
> + }
> +
> + old_active = sdata->vif.active_links;
> + sdata->vif.active_links = active_links;
> +
> + if (rem) {
> + ret = drv_change_vif_links(local, sdata, old_active,
> + active_links, link_confs);
> + WARN_ON_ONCE(ret);
> + }
> +
> + return 0;
> +}
> +
...
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.)
> > + 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.
> > + 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.
> 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.
johannes
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
On Mon, 2023-03-27 at 16:40 +0800, Wen Gong wrote:
> >
> > > > + 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.
OK. Still maybe we should change it for consistency? I can try that
later with our driver.
johannes
On 3/27/2023 5:04 PM, Johannes Berg wrote:
> On Mon, 2023-03-27 at 16:40 +0800, Wen Gong wrote:
>>>>> + 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.
> OK. Still maybe we should change it for consistency? I can try that
> later with our driver.
>
> johannes
I think it is not urgent for that:)
And lower-drvier should also handler different case.
>
On 3/27/2023 4:31 PM, Johannes Berg wrote:
...
>> 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.
>
> johannes
May I know how did you test it?
Did you test with tool like this with parameter "ActiveTxMultiLinks"?
".\sigma-dut -l
sta_set_rfeature,Interface,wlan0,prog,EHT,ActiveTxMultiLinks,02:03:7f:95:21:97,ActiveRxMultiLinks,02:03:7f:95:21:97"
On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
> On 3/27/2023 4:31 PM, Johannes Berg wrote:
> ...
> > > 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.
> >
> > johannes
> May I know how did you test it?
Just writing to the debugfs file. We have various tests using that now.
johannes
On 3/28/2023 3:39 PM, Johannes Berg wrote:
> On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
>> On 3/27/2023 4:31 PM, Johannes Berg wrote:
>> ...
>>>> 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.
>>>
>>> johannes
>> May I know how did you test it?
> Just writing to the debugfs file. We have various tests using that now.
>
Do you mean the various tests using debugfs or using
ieee80211_set_active_links() directly?
> johannes
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.)
>
>
Thanks Johannes,
Also it does not have BSS_CHANGED_ASSOC(exists in
ieee80211_set_associated()) for
ieee80211_link_info_change_notify()/ieee80211_vif_cfg_change_notify().
So I think low-level driver also need to auto add BSS_CHANGED_ASSOC
logic for the added link as well as the pairwise key you said, right?
On Mon, 2023-04-03 at 22:21 +0800, Wen Gong wrote:
>
> Also it does not have BSS_CHANGED_ASSOC(exists in
> ieee80211_set_associated()) for
>
> ieee80211_link_info_change_notify()/ieee80211_vif_cfg_change_notify().
Well, that's clearly intentional though, since the assoc state is at the
MLD level, so changing a link doesn't affect the assoc state, right?
This is also done through _vif_cfg_change_notify().
> So I think low-level driver also need to auto add BSS_CHANGED_ASSOC
> logic for the added link as well as the pairwise key you said, right?
>
That doesn't make a lot of sense as written, IMHO, but yes if you have
something you need to do on links during assoc, then you'd have to take
care of that as well, just like the keys.
johannes
On Mon, 2023-04-03 at 22:15 +0800, Wen Gong wrote:
> On 3/28/2023 3:39 PM, Johannes Berg wrote:
> > On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
> > > On 3/27/2023 4:31 PM, Johannes Berg wrote:
> > > ...
> > > > > 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.
> > > >
> > > > johannes
> > > May I know how did you test it?
> > Just writing to the debugfs file. We have various tests using that now.
> >
> Do you mean the various tests using debugfs or using
> ieee80211_set_active_links() directly?
>
Yeah the tests write to debugfs.
johannes