This is a preparation for supporting Multi-Link reconfigure link removal
procedure[IEEE P802.11be/D5.0 - 35.3.6.3 Removing affiliated APs] for
driver which supports offloaded Multi-Link reconfigure link removal.
Multi-Link reconfigure link removal offloaded drivers will take care
of updating the reconfiguration MLE in self and partner beacons. It
also updates the AP removal timer automatically and notifies once the
counter is expired.
This patchset also adds mac80211 routine to support sending link
removal command from userspace to offloaded driver which accepts
reconfigure Multi-Link IE and the TBTT count for the link to be removed.
Driver (ath12k) changes that utilize this will be posted in the future
versions.
Manish Dharanenthiran (2):
wifi: cfg80211/mac80211: Introduce nl80211 commands to support MLD
link removal offload
wifi: mac80211: Add support for link reconfigure removal
include/net/cfg80211.h | 63 ++++++++++++++++++++++
include/net/mac80211.h | 25 +++++++++
include/uapi/linux/nl80211.h | 30 +++++++++++
net/mac80211/cfg.c | 12 +++++
net/mac80211/driver-ops.h | 19 +++++++
net/mac80211/ieee80211_i.h | 4 ++
net/mac80211/link.c | 40 ++++++++++++++
net/mac80211/trace.h | 30 +++++++++++
net/wireless/core.h | 2 +
net/wireless/nl80211.c | 101 ++++++++++++++++++++++++++++++++++-
net/wireless/rdev-ops.h | 17 ++++++
net/wireless/trace.h | 56 +++++++++++++++++++
net/wireless/util.c | 18 +++++++
13 files changed, 415 insertions(+), 2 deletions(-)
base-commit: d69aef8084cc72df7b0f2583096d9b037c647ec8
--
2.34.1
Add mac80211 routine to support sending link removal command to
offloaded driver which accepts reconfigure Multi-Link IE and the TBTT
count for the link to be removed. To support this, introduce new
mac80211 ops "link_reconfig_remove" to initiate link removal procedure
in driver with Multi-Link reconfiguration IE and TBTT count received
from the userspace.
Also, add mac80211 routine "ieee80211_update_link_reconfig_remove_status"
which will be used by driver for sending TSF and current TBTT count
receive from driver during the following scenarios,
1) When first beacon with Multi-Link reconfigure IE is sent out in air,
mac80211 will notify the userspace that link removal is started and
it can proceed with further action like BTM etc.,
2) When last beacon with Multi-Link reconfigure IE (i.e. with link
removal tbtt count as 0) is sent out in air, mac80211 will notify the
userspace that link removal is completed. After which, userspace shall
initiate the disassociation of the peer(s) connected and removal of
the link completely.
Signed-off-by: Manish Dharanenthiran <[email protected]>
---
include/net/mac80211.h | 25 ++++++++++++++++++++++++
net/mac80211/cfg.c | 12 ++++++++++++
net/mac80211/driver-ops.h | 19 ++++++++++++++++++
net/mac80211/ieee80211_i.h | 4 ++++
net/mac80211/link.c | 40 ++++++++++++++++++++++++++++++++++++++
net/mac80211/trace.h | 30 ++++++++++++++++++++++++++++
6 files changed, 130 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f57c29de3a91..c4a0069a6cbb 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4394,6 +4394,10 @@ struct ieee80211_prep_tx_info {
* if the requested TID-To-Link mapping can be accepted or not.
* If it's not accepted the driver may suggest a preferred mapping and
* modify @ttlm parameter with the suggested TID-to-Link mapping.
+ * @link_reconfig_remove: Notifies the driver about the link to be
+ * scheduled for removal with ML reconfigure IE built for that particular
+ * link along with the TBTT count until which the beacon with ML
+ * reconfigure IE should be sent.
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -4778,6 +4782,9 @@ struct ieee80211_ops {
enum ieee80211_neg_ttlm_res
(*can_neg_ttlm)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct ieee80211_neg_ttlm *ttlm);
+ int (*link_reconfig_remove)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ const struct cfg80211_link_reconfig_removal_params *params);
};
/**
@@ -7600,6 +7607,24 @@ void ieee80211_set_active_links_async(struct ieee80211_vif *vif,
*/
void ieee80211_send_teardown_neg_ttlm(struct ieee80211_vif *vif);
+/* Defines for ML Reconfigure removal offload */
+
+/**
+ * ieee80211_update_link_reconfig_remove_status - Inform userspace about
+ * the removal status of link which is scheduled for removal
+ * @vif: interface in which reconfig removal status is received.
+ * @link_id: Link which is undergoing removal
+ * @tbtt_count: Current tbtt_count to be updated.
+ * @tsf: Beacon's timestamp value
+ * @action: Inform started or completed action to userspace
+ *
+ * For description, check cfg80211_link_reconfig_remove_status
+ */
+int ieee80211_update_link_reconfig_remove_status(struct ieee80211_vif *vif,
+ unsigned int link_id,
+ u8 tbtt_count, u64 tsf,
+ enum ieee80211_link_reconfig_remove_state action);
+
/* for older drivers - let's not document these ... */
int ieee80211_emulate_add_chanctx(struct ieee80211_hw *hw,
struct ieee80211_chanctx_conf *ctx);
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index f03452dc716d..3bb040c78e94 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -4923,6 +4923,17 @@ static void ieee80211_del_intf_link(struct wiphy *wiphy,
ieee80211_vif_set_links(sdata, wdev->valid_links, 0);
}
+static int
+ieee80211_link_reconfig_remove(struct wiphy *wiphy,
+ struct net_device *dev,
+ const struct cfg80211_link_reconfig_removal_params *params)
+{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+
+ return __ieee80211_link_reconfig_remove(local, sdata, params);
+}
+
static int sta_add_link_station(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
struct link_station_parameters *params)
@@ -5166,4 +5177,5 @@ const struct cfg80211_ops mac80211_config_ops = {
.del_link_station = ieee80211_del_link_station,
.set_hw_timestamp = ieee80211_set_hw_timestamp,
.set_ttlm = ieee80211_set_ttlm,
+ .link_reconfig_remove = ieee80211_link_reconfig_remove,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 5d078c0a2323..4e4ffad9e7a7 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1716,4 +1716,23 @@ drv_can_neg_ttlm(struct ieee80211_local *local,
return res;
}
+
+static inline int
+drv_link_reconfig_remove(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ const struct cfg80211_link_reconfig_removal_params *params)
+{
+ int ret = -EOPNOTSUPP;
+
+ trace_drv_link_reconfig_remove(local, sdata, params);
+
+ if (local->ops->link_reconfig_remove)
+ ret = local->ops->link_reconfig_remove(&local->hw,
+ &sdata->vif,
+ params);
+ trace_drv_return_int(local, ret);
+
+ return ret;
+}
+
#endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index def611e4e55f..3cafb7d52276 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2041,6 +2041,10 @@ static inline void ieee80211_vif_clear_links(struct ieee80211_sub_if_data *sdata
ieee80211_vif_set_links(sdata, 0, 0);
}
+int __ieee80211_link_reconfig_remove(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ const struct cfg80211_link_reconfig_removal_params *params);
+
/* tx handling */
void ieee80211_clear_tx_pending(struct ieee80211_local *local);
void ieee80211_tx_pending(struct tasklet_struct *t);
diff --git a/net/mac80211/link.c b/net/mac80211/link.c
index 43f9672fc7f1..aa9d05067590 100644
--- a/net/mac80211/link.c
+++ b/net/mac80211/link.c
@@ -501,3 +501,43 @@ void ieee80211_set_active_links_async(struct ieee80211_vif *vif,
wiphy_work_queue(sdata->local->hw.wiphy, &sdata->activate_links_work);
}
EXPORT_SYMBOL_GPL(ieee80211_set_active_links_async);
+
+int __ieee80211_link_reconfig_remove(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ const struct cfg80211_link_reconfig_removal_params *params)
+{
+ struct ieee80211_link_data *link;
+ int ret;
+
+ if (!ieee80211_sdata_running(sdata))
+ return -ENETDOWN;
+
+ if (sdata->vif.type != NL80211_IFTYPE_AP)
+ return -EINVAL;
+
+ link = sdata_dereference(sdata->link[params->link_id], sdata);
+ if (!link)
+ return -ENOLINK;
+
+ ret = drv_link_reconfig_remove(local, sdata, params);
+
+ return ret;
+}
+
+int ieee80211_update_link_reconfig_remove_status(struct ieee80211_vif *vif,
+ unsigned int link_id,
+ u8 tbtt_count, u64 tsf,
+ enum ieee80211_link_reconfig_remove_state status)
+{
+ struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+
+ if (vif->type != NL80211_IFTYPE_AP) {
+ sdata_err(sdata, "Discarding link reconfig status for unsupported vif type\n");
+ return -EINVAL;
+ }
+
+ return cfg80211_update_link_reconfig_remove_status(sdata->dev, link_id,
+ tbtt_count, tsf,
+ status);
+}
+EXPORT_SYMBOL(ieee80211_update_link_reconfig_remove_status);
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 8e758b5074bd..40fefdc34a79 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -3145,6 +3145,36 @@ TRACE_EVENT(drv_neg_ttlm_res,
LOCAL_PR_ARG, VIF_PR_ARG, __entry->res
)
);
+
+TRACE_EVENT(drv_link_reconfig_remove,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ const struct cfg80211_link_reconfig_removal_params *params),
+
+ TP_ARGS(local, sdata, params),
+
+ TP_STRUCT__entry(LOCAL_ENTRY
+ VIF_ENTRY
+ __field(u32, link_id)
+ __field(u16, count)
+ __dynamic_array(u8, frame, params->ie_len)
+ ),
+
+ TP_fast_assign(LOCAL_ASSIGN;
+ VIF_ASSIGN;
+ __entry->link_id = params->link_id;
+ memcpy(__get_dynamic_array(frame), params->ie,
+ params->ie_len);
+ __entry->count = params->link_removal_cntdown;
+ ),
+
+ TP_printk(LOCAL_PR_FMT ", " VIF_PR_FMT ", link_id :%u frame:0x%.2x count:%d",
+ LOCAL_PR_ARG, VIF_PR_ARG,
+ __entry->link_id,
+ le16_to_cpup((__le16 *)__get_dynamic_array(frame)),
+ __entry->count)
+);
+
#endif /* !__MAC80211_DRIVER_TRACE || TRACE_HEADER_MULTI_READ */
#undef TRACE_INCLUDE_PATH
--
2.34.1
This is a preparation for supporting Multi-Link reconfigure link removal
procedure[IEEE P802.11be/D5.0 - 35.3.6.3 Removing affiliated APs] for
driver which supports offloaded Multi-Link reconfigure link removal.
Multi-Link reconfigure link removal offloaded drivers will take care of
updating the reconfiguration MLE in self and partner beacons. It also
updates the AP removal timer automatically and notifies once the counter
is expired.
For such drivers AP link removal count(TBTT) and reconfiguration MLE
needs to be passed from userspace. AP link removal count indicates the
number of beacons the reconfiguration MLE will be present, after which
the link will be removed. To support this, NL80211_ATTR_AP_REMOVAL_COUNT
and NL80211_ATTR_IE are used.
In beacon offloaded drivers, to indicate status of ongoing link removal,
add two new commands NL80211_CMD_LINK_REMOVAL_STARTED,
NL80211_CMD_LINK_REMOVAL_COMPLETED. NL80211_CMD_LINK_REMOVAL_STARTED
will update timestamp of first beacon sent with reconfiguration MLE
using NL80211_ATTR_TSF attribute.
Co-developed-by: Rameshkumar Sundaram <[email protected]>
Signed-off-by: Rameshkumar Sundaram <[email protected]>
Signed-off-by: Manish Dharanenthiran <[email protected]>
---
include/net/cfg80211.h | 63 ++++++++++++++++++++++
include/uapi/linux/nl80211.h | 30 +++++++++++
net/wireless/core.h | 2 +
net/wireless/nl80211.c | 101 ++++++++++++++++++++++++++++++++++-
net/wireless/rdev-ops.h | 17 ++++++
net/wireless/trace.h | 56 +++++++++++++++++++
net/wireless/util.c | 18 +++++++
7 files changed, 285 insertions(+), 2 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 2e2be4fd2bb6..b6914f107b87 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4169,6 +4169,23 @@ struct mgmt_frame_regs {
u32 global_mcast_stypes, interface_mcast_stypes;
};
+/**
+ * struct cfg80211_link_reconfig_removal_params - Contains params needed for
+ * link reconfig removal
+ * @link_removal_cntdown: TBTT countdown value until which the beacon with ML
+ * reconfigure IE will be sent.
+ * @ie: ML reconfigure IE to be updated in beacon in the link going to be
+ * removed and in all affiliated links.
+ * @ie_len: ML reconfigure IE length
+ * @link_id: Link id of the link to be removed.
+ */
+struct cfg80211_link_reconfig_removal_params {
+ u16 link_removal_cntdown;
+ const u8 *ie;
+ size_t ie_len;
+ unsigned int link_id;
+};
+
/**
* struct cfg80211_ops - backend description for wireless configuration
*
@@ -4570,6 +4587,11 @@ struct mgmt_frame_regs {
*
* @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames.
* @set_ttlm: set the TID to link mapping.
+ *
+ * @link_reconfig_remove: Notifies the driver about the link to be
+ * scheduled for removal with ML reconfigure IE built for that particular
+ * link along with the TBTT count until which the beacon with ML
+ * reconfigure IE should be sent.
*/
struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -4931,6 +4953,9 @@ struct cfg80211_ops {
struct cfg80211_set_hw_timestamp *hwts);
int (*set_ttlm)(struct wiphy *wiphy, struct net_device *dev,
struct cfg80211_ttlm_params *params);
+ int (*link_reconfig_remove)(struct wiphy *wiphy,
+ struct net_device *dev,
+ const struct cfg80211_link_reconfig_removal_params *params);
};
/*
@@ -9509,4 +9534,42 @@ ssize_t wiphy_locked_debugfs_write(struct wiphy *wiphy, struct file *file,
void *data);
#endif
+enum ieee80211_link_reconfig_remove_state {
+ IEEE80211_LINK_RECONFIG_START,
+ IEEE80211_LINK_RECONFIG_COMPLETE,
+};
+
+/**
+ * cfg80211_update_link_reconfig_remove_status - Inform userspace about
+ * the removal status of link which is scheduled for removal
+ * @dev: the device on which the operation is requested
+ * @link_id: Link which is undergoing removal
+ * @tbtt_count: Current tbtt_count to be updated.
+ * @tsf: Beacon's timestamp value
+ * @status: Inform started or completed action to userspace based on the value
+ * received,
+ * i) 0 (IEEE80211_LINK_RECONFIG_START) - Send
+ * NL80211_CMD_LINK_REMOVAL_STARTED
+ * ii) 1 (IEEE80211_LINK_RECONFIG_COMPLETE) - Send
+ * NL80211_CMD_LINK_REMOVAL_COMPLETED
+ *
+ *
+ * This function is used to inform userspace about the ongoing link removal
+ * status. 'IEEE80211_LINK_RECONFIG_START' is issued when the first beacon with
+ * ML reconfigure IE is sent out. This event can be used by userspace to start
+ * the BTM in case of AP mode. And, IEEE80211_LINK_RECONFIG_COMPLETE is issued
+ * when the last beacon is sent with ML reconfigure IE. This is used to
+ * initiate the deletion of that link, also to trigger deauth/disassoc for the
+ * associated peer(s).
+ *
+ * Note: This API is currently used by drivers which supports offloaded
+ * Multi-Link reconfigure link removal. Returns failure if FEATURE FLAG is not
+ * set or success if NL message is sent.
+ */
+int
+cfg80211_update_link_reconfig_remove_status(struct net_device *dev,
+ unsigned int link_id,
+ u8 tbtt_count, u64 tsf,
+ enum ieee80211_link_reconfig_remove_state action);
+
#endif /* __NET_CFG80211_H */
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f917bc6c9b6f..bfc6080816b8 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1304,6 +1304,10 @@
* @NL80211_CMD_REMOVE_LINK: Remove a link from an interface. This may come
* without %NL80211_ATTR_MLO_LINK_ID as an easy way to remove all links
* in preparation for e.g. roaming to a regular (non-MLO) AP.
+ * To initiate link removal procedure, set below attributes,
+ * %NL80211_ATTR_AP_REMOVAL_COUNT - AP removal timer count(TBTT)
+ * %NL80211_ATTR_IE - ML reconfigure Information element
+ * Can be extended to update multiple TBTT & IE(s).
*
* @NL80211_CMD_ADD_LINK_STA: Add a link to an MLD station
* @NL80211_CMD_MODIFY_LINK_STA: Modify a link of an MLD station
@@ -1329,6 +1333,14 @@
* %NL80211_ATTR_MLO_TTLM_ULINK attributes are used to specify the
* TID to Link mapping for downlink/uplink traffic.
*
+ * @NL80211_CMD_LINK_REMOVAL_STARTED: Once first beacon with reconfiguration MLE
+ * is sent, userspace is notified with the TBTT and TSF value to indicate
+ * timestamp of that beacon using %NL80211_ATTR_AP_REMOVAL_COUNT and
+ * %NL80211_ATTR_TSF respectively.
+ *
+ * @NL80211_CMD_LINK_REMOVAL_COMPLETED: Once last beacon with reconfiguration
+ * MLE is sent, userspace is notified with completion.
+ *
* @NL80211_CMD_MAX: highest used command number
* @__NL80211_CMD_AFTER_LAST: internal use
*/
@@ -1586,6 +1598,10 @@ enum nl80211_commands {
NL80211_CMD_SET_TID_TO_LINK_MAPPING,
+ NL80211_CMD_LINK_REMOVAL_STARTED,
+
+ NL80211_CMD_LINK_REMOVAL_COMPLETED,
+
/* add new commands above here */
/* used to define NL80211_CMD_MAX below */
@@ -2856,6 +2872,13 @@ enum nl80211_commands {
* %NL80211_CMD_ASSOCIATE indicating the SPP A-MSDUs
* are used on this connection
*
+ * @NL80211_ATTR_AP_REMOVAL_COUNT: (u16) TBTT count up-to which reconfiguration
+ * MLE is sent. Also, userspace will be notified with this count once the
+ * first beacon with reconfiguration MLE is sent.
+ *
+ * @NL80211_ATTR_TSF: (u64) TSF value when the first beacon with reconfiguration
+ * MLE is sent.
+ *
* @NUM_NL80211_ATTR: total number of nl80211_attrs available
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
@@ -3401,6 +3424,9 @@ enum nl80211_attrs {
NL80211_ATTR_ASSOC_SPP_AMSDU,
+ NL80211_ATTR_AP_REMOVAL_COUNT,
+ NL80211_ATTR_TSF,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
@@ -6545,6 +6571,9 @@ enum nl80211_feature_flags {
* (signaling and payload protected) A-MSDUs and this shall be advertised
* in the RSNXE.
*
+ * @NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD: Driver/device which supports
+ * ML reconfig link removal offload.
+ *
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
*/
@@ -6620,6 +6649,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_OWE_OFFLOAD_AP,
NL80211_EXT_FEATURE_DFS_CONCURRENT,
NL80211_EXT_FEATURE_SPP_AMSDU_SUPPORT,
+ NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD,
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 118f2f619828..ce1a1aa048e2 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -552,6 +552,8 @@ void cfg80211_remove_links(struct wireless_dev *wdev);
int cfg80211_remove_virtual_intf(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev);
void cfg80211_wdev_release_link_bsses(struct wireless_dev *wdev, u16 link_mask);
+int cfg80211_link_reconfig_remove(struct wireless_dev *wdev,
+ const struct cfg80211_link_reconfig_removal_params *params);
/**
* struct cfg80211_colocated_ap - colocated AP information
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b4edba6b0b7b..252d019c4d45 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -826,6 +826,8 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[NL80211_ATTR_MLO_TTLM_DLINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8),
[NL80211_ATTR_MLO_TTLM_ULINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8),
[NL80211_ATTR_ASSOC_SPP_AMSDU] = { .type = NLA_FLAG },
+ [NL80211_ATTR_AP_REMOVAL_COUNT] = { .type = NLA_U8 },
+ [NL80211_ATTR_TSF] = { .type = NLA_U64 },
};
/* policy for the key attributes */
@@ -16103,6 +16105,9 @@ static int nl80211_remove_link(struct sk_buff *skb, struct genl_info *info)
unsigned int link_id = nl80211_link_id(info->attrs);
struct net_device *dev = info->user_ptr[1];
struct wireless_dev *wdev = dev->ieee80211_ptr;
+ struct cfg80211_link_reconfig_removal_params params = {};
+ bool is_ml_reconfig = false;
+ int ret = 0;
/* cannot remove if there's no link */
if (!info->attrs[NL80211_ATTR_MLO_LINK_ID])
@@ -16115,9 +16120,35 @@ static int nl80211_remove_link(struct sk_buff *skb, struct genl_info *info)
return -EINVAL;
}
- cfg80211_remove_link(wdev, link_id);
+ if (info->attrs[NL80211_ATTR_AP_REMOVAL_COUNT]) {
+ /* Parsing and sending information to driver about ML
+ * reconfiguration is supported only when
+ * NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD is set
+ */
+ if (!wiphy_ext_feature_isset(wdev->wiphy,
+ NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD))
+ return -EOPNOTSUPP;
- return 0;
+ /* If AP removal count is present, it is mandatory to have IE
+ * attribute as well, return error if not present
+ */
+ if (!info->attrs[NL80211_ATTR_IE])
+ return -EINVAL;
+
+ is_ml_reconfig = true;
+ params.ie = nla_data(info->attrs[NL80211_ATTR_IE]);
+ params.ie_len = nla_len(info->attrs[NL80211_ATTR_IE]);
+ params.link_removal_cntdown =
+ nla_get_u16(info->attrs[NL80211_ATTR_AP_REMOVAL_COUNT]);
+ params.link_id = link_id;
+ }
+
+ if (is_ml_reconfig)
+ ret = cfg80211_link_reconfig_remove(wdev, ¶ms);
+ else
+ cfg80211_remove_link(wdev, link_id);
+
+ return ret;
}
static int
@@ -20214,6 +20245,72 @@ void cfg80211_schedule_channels_check(struct wireless_dev *wdev)
}
EXPORT_SYMBOL(cfg80211_schedule_channels_check);
+int
+cfg80211_update_link_reconfig_remove_status(struct net_device *netdev,
+ unsigned int link_id,
+ u8 tbtt_count, u64 tsf,
+ enum ieee80211_link_reconfig_remove_state action)
+{
+ struct wiphy *wiphy = netdev->ieee80211_ptr->wiphy;
+ struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
+ struct sk_buff *msg;
+ void *hdr = NULL;
+ int ret = 0;
+
+ /* Only for ML reconfigure link removal offloaded driver, need to
+ * update the status about the ongoing link removal to userspace.
+ */
+ if (!wiphy_ext_feature_isset(wiphy,
+ NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD))
+ return -EOPNOTSUPP;
+
+ trace_cfg80211_update_link_reconfig_remove_status(wiphy, netdev,
+ link_id, tbtt_count,
+ tsf, action);
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+ if (!msg)
+ return -ENOMEM;
+
+ if (action == IEEE80211_LINK_RECONFIG_START)
+ hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_LINK_REMOVAL_STARTED);
+ else if (action == IEEE80211_LINK_RECONFIG_COMPLETE)
+ hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_LINK_REMOVAL_COMPLETED);
+
+ if (!hdr) {
+ ret = -ENOBUFS;
+ goto nla_put_failure;
+ }
+
+ if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
+ nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex)) {
+ ret = -EINVAL;
+ goto nla_put_failure;
+ }
+
+ if (nla_put_u8(msg, NL80211_ATTR_MLO_LINK_ID, link_id) ||
+ nla_put_u8(msg, NL80211_ATTR_AP_REMOVAL_COUNT, tbtt_count) ||
+ nla_put_u64_64bit(msg, NL80211_ATTR_TSF, tsf,
+ NL80211_ATTR_PAD)) {
+ ret = -EINVAL;
+ goto nla_put_failure;
+ }
+
+ genlmsg_end(msg, hdr);
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_MLME, GFP_ATOMIC);
+
+ return ret;
+
+ nla_put_failure:
+ genlmsg_cancel(msg, hdr);
+ nlmsg_free(msg);
+
+ return ret;
+}
+EXPORT_SYMBOL(cfg80211_update_link_reconfig_remove_status);
+
/* initialisation/exit functions */
int __init nl80211_init(void)
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 43897a5269b6..cbf5da73fbb0 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1459,6 +1459,23 @@ rdev_del_intf_link(struct cfg80211_registered_device *rdev,
trace_rdev_return_void(&rdev->wiphy);
}
+static inline int
+rdev_link_reconfig_remove(struct cfg80211_registered_device *rdev,
+ struct net_device *dev,
+ const struct cfg80211_link_reconfig_removal_params *params)
+{
+ int ret = -EOPNOTSUPP;
+
+ trace_rdev_link_reconfig_remove(&rdev->wiphy, dev, params);
+
+ if (rdev->ops->link_reconfig_remove)
+ ret = rdev->ops->link_reconfig_remove(&rdev->wiphy, dev,
+ params);
+
+ trace_rdev_return_int(&rdev->wiphy, ret);
+ return ret;
+}
+
static inline int
rdev_add_link_station(struct cfg80211_registered_device *rdev,
struct net_device *dev,
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index e039e66ab377..ac174afd768b 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -4005,6 +4005,62 @@ TRACE_EVENT(rdev_set_ttlm,
WIPHY_PR_ARG, NETDEV_PR_ARG)
);
+TRACE_EVENT(rdev_link_reconfig_remove,
+ TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+ const struct cfg80211_link_reconfig_removal_params *params),
+
+ TP_ARGS(wiphy, netdev, params),
+
+ TP_STRUCT__entry(WIPHY_ENTRY
+ NETDEV_ENTRY
+ __field(u32, link_id)
+ __field(u16, count)
+ __dynamic_array(u8, frame, params->ie_len)
+ ),
+
+ TP_fast_assign(WIPHY_ASSIGN;
+ NETDEV_ASSIGN;
+ __entry->link_id = params->link_id;
+ __entry->count = params->link_removal_cntdown;
+ memcpy(__get_dynamic_array(frame), params->ie,
+ params->ie_len);
+ ),
+
+ TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", link_id: %u frame:0x%.2x count:%d",
+ WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->link_id,
+ le16_to_cpup((__le16 *)__get_dynamic_array(frame)),
+ __entry->count)
+);
+
+TRACE_EVENT(cfg80211_update_link_reconfig_remove_status,
+ TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+ unsigned int link_id, u8 tbtt_count, u64 tsf,
+ enum ieee80211_link_reconfig_remove_state action),
+
+ TP_ARGS(wiphy, netdev, link_id, tbtt_count, tsf, action),
+
+ TP_STRUCT__entry(WIPHY_ENTRY
+ NETDEV_ENTRY
+ __field(u32, link_id)
+ __field(u8, tbtt_count)
+ __field(u64, tsf)
+ __field(enum ieee80211_link_reconfig_remove_state, action)
+ ),
+
+ TP_fast_assign(WIPHY_ASSIGN;
+ NETDEV_ASSIGN;
+ __entry->link_id = link_id;
+ __entry->tbtt_count = tbtt_count;
+ __entry->tsf = tsf;
+ __entry->action = action;
+ ),
+
+ TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", link_id: %u tbtt:%u tsf: %lld, action: %d",
+ WIPHY_PR_ARG, NETDEV_PR_ARG,
+ __entry->link_id, __entry->tbtt_count,
+ __entry->tsf, __entry->action)
+);
+
#endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
#undef TRACE_INCLUDE_PATH
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 2bde8a354631..69f09390257d 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2799,6 +2799,24 @@ bool cfg80211_iftype_allowed(struct wiphy *wiphy, enum nl80211_iftype iftype,
}
EXPORT_SYMBOL(cfg80211_iftype_allowed);
+int cfg80211_link_reconfig_remove(struct wireless_dev *wdev,
+ const struct cfg80211_link_reconfig_removal_params *params)
+{
+ struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
+ int ret = -EINVAL;
+
+ lockdep_assert_wiphy(wdev->wiphy);
+
+ /* Currently, removal of link from MLD is supported for AP BSS only, it
+ * can be extended for non-AP/STA MLD as well but that shall use
+ * action frame to update about its link reconfiguration.
+ */
+ if (wdev->iftype == NL80211_IFTYPE_AP)
+ ret = rdev_link_reconfig_remove(rdev, wdev->netdev, params);
+
+ return ret;
+}
+
void cfg80211_remove_link(struct wireless_dev *wdev, unsigned int link_id)
{
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
--
2.34.1
On Thu, 2024-03-28 at 11:22 +0530, Manish Dharanenthiran wrote:
>
> +int __ieee80211_link_reconfig_remove(struct ieee80211_local *local,
> + struct ieee80211_sub_if_data *sdata,
> + const struct cfg80211_link_reconfig_removal_params *params)
> +{
> + struct ieee80211_link_data *link;
> + int ret;
> +
> + if (!ieee80211_sdata_running(sdata))
> + return -ENETDOWN;
> +
> + if (sdata->vif.type != NL80211_IFTYPE_AP)
> + return -EINVAL;
> +
> + link = sdata_dereference(sdata->link[params->link_id], sdata);
> + if (!link)
> + return -ENOLINK;
> +
> + ret = drv_link_reconfig_remove(local, sdata, params);
> +
> + return ret;
Again ... remove the 'ret' variable, it serves no purpose at all.
> +}
> +
> +int ieee80211_update_link_reconfig_remove_status(struct ieee80211_vif *vif,
> + unsigned int link_id,
> + u8 tbtt_count, u64 tsf,
> + enum ieee80211_link_reconfig_remove_state status)
> +{
> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> +
> + if (vif->type != NL80211_IFTYPE_AP) {
> + sdata_err(sdata, "Discarding link reconfig status for unsupported vif type\n");
Uh, no. Remove that message please.
> +TRACE_EVENT(drv_link_reconfig_remove,
> + TP_PROTO(struct ieee80211_local *local,
> + struct ieee80211_sub_if_data *sdata,
> + const struct cfg80211_link_reconfig_removal_params *params),
> +
> + TP_ARGS(local, sdata, params),
> +
> + TP_STRUCT__entry(LOCAL_ENTRY
> + VIF_ENTRY
> + __field(u32, link_id)
> + __field(u16, count)
> + __dynamic_array(u8, frame, params->ie_len)
> + ),
All the same things about indentation apply here.
johannes
On Thu, 2024-03-28 at 11:22 +0530, Manish Dharanenthiran wrote:
>
> + * @ie: ML reconfigure IE to be updated in beacon in the link going to be
> + * removed and in all affiliated links.
> + * @ie_len: ML reconfigure IE length
Given the spec having moved away from "Information Element" in favour of
just "Element", I'd prefer to do the same in the code. Not that we
really want to rename all the "ie" stuff in the code now, but for new
code.
> + * @link_reconfig_remove: Notifies the driver about the link to be
> + * scheduled for removal with ML reconfigure IE built for that particular
> + * link along with the TBTT count until which the beacon with ML
> + * reconfigure IE should be sent.
Same here.
> +/**
> + * cfg80211_update_link_reconfig_remove_status - Inform userspace about
> + * the removal status of link which is scheduled for removal
> + * @dev: the device on which the operation is requested
> + * @link_id: Link which is undergoing removal
> + * @tbtt_count: Current tbtt_count to be updated.
> + * @tsf: Beacon's timestamp value
> + * @status: Inform started or completed action to userspace based on the value
> + * received,
> + * i) 0 (IEEE80211_LINK_RECONFIG_START) - Send
> + * NL80211_CMD_LINK_REMOVAL_STARTED
> + * ii) 1 (IEEE80211_LINK_RECONFIG_COMPLETE) - Send
> + * NL80211_CMD_LINK_REMOVAL_COMPLETED
I wouldn't list the values "0" and "1"? They're really quite
unimportant.
Also we've sometimes used just the nl80211 cmd value as a parameter,
could do that here too instead of adding IEEE80211_LINK_RECONFIG_START
and IEEE80211_LINK_RECONFIG_COMPLETE?
> + * This function is used to inform userspace about the ongoing link removal
> + * status. 'IEEE80211_LINK_RECONFIG_START' is issued when the first beacon with
> + * ML reconfigure IE is sent out. This event can be used by userspace to start
> + * the BTM in case of AP mode. And, IEEE80211_LINK_RECONFIG_COMPLETE is issued
> + * when the last beacon is sent with ML reconfigure IE. This is used to
> + * initiate the deletion of that link, also to trigger deauth/disassoc for the
> + * associated peer(s).
I'm not fully sure I understand this - the way I read it, userspace
actually request link removal with NL80211_CMD_REMOVE_LINK (at least
right now, will comment separately), why does it need an event too?
> + * Note: This API is currently used by drivers which supports offloaded
> + * Multi-Link reconfigure link removal. Returns failure if FEATURE FLAG is not
> + * set or success if NL message is sent.
What would be "non-offloaded ML reconfiguration link removal"? What does
offloaded even mean here? It's only counting down and then removing the
link, no? Isn't that fundamentally an operation that has to happen in
kernel or device space, since that's sending beacons? I don't see you'd
really want to time the updates to each beacon from userspace?
> +++ b/include/uapi/linux/nl80211.h
> @@ -1304,6 +1304,10 @@
> * @NL80211_CMD_REMOVE_LINK: Remove a link from an interface. This may come
> * without %NL80211_ATTR_MLO_LINK_ID as an easy way to remove all links
> * in preparation for e.g. roaming to a regular (non-MLO) AP.
> + * To initiate link removal procedure, set below attributes,
> + * %NL80211_ATTR_AP_REMOVAL_COUNT - AP removal timer count(TBTT)
> + * %NL80211_ATTR_IE - ML reconfigure Information element
> + * Can be extended to update multiple TBTT & IE(s).
I'm wondering if this really is the right API? For example, older
kernels would ignore all the other attributes and immediately delete the
link, is that what you want? Seems you might want to at least _know_
about the availability of (graceful) link removal.
Also, if it should piggy-back on an existing API, maybe better would be
STOP_AP? That's the operation that normally stops beaconing, although if
you just delete the link the kernel does have to clean that up ...
Or maybe it should just be a whole new operation? You're already adding
new commands for the events anyway, could use _START (dropping the "ED")
for both a command and an event. Though see above, not sure why you need
the event anyway.
So just not sure about this, any arguments either way? I guess if it
were part of STOP_AP you'd expect the link to stay around and just be
inactive, and then require it to be removed by userspace with
NL80211_CMD_REMOVE_LINK after it stops, so perhaps in some way doing it
via NL80211_CMD_REMOVE_LINK makes more sense so it will actually be
removed entirely.
> * @NL80211_CMD_ADD_LINK_STA: Add a link to an MLD station
> * @NL80211_CMD_MODIFY_LINK_STA: Modify a link of an MLD station
> @@ -1329,6 +1333,14 @@
> * %NL80211_ATTR_MLO_TTLM_ULINK attributes are used to specify the
> * TID to Link mapping for downlink/uplink traffic.
> *
> + * @NL80211_CMD_LINK_REMOVAL_STARTED: Once first beacon with reconfiguration MLE
> + * is sent, userspace is notified with the TBTT and TSF value to indicate
> + * timestamp of that beacon using %NL80211_ATTR_AP_REMOVAL_COUNT and
> + * %NL80211_ATTR_TSF respectively.
> + *
> + * @NL80211_CMD_LINK_REMOVAL_COMPLETED: Once last beacon with reconfiguration
> + * MLE is sent, userspace is notified with completion.
We already have NL80211_CMD_LINKS_REMOVED for the station side of this,
maybe we should just reuse that event also here?
> + * @NL80211_ATTR_AP_REMOVAL_COUNT: (u16) TBTT count up-to which reconfiguration
> + * MLE is sent. Also, userspace will be notified with this count once the
> + * first beacon with reconfiguration MLE is sent.
I'm not sure how that notification thing all makes sense when it's
something requested by userspace in the first place? Are you expecting
the underlying driver to actually _change_ it?
> + * @NL80211_ATTR_TSF: (u64) TSF value when the first beacon with reconfiguration
> + * MLE is sent.
Why is that needed? But also the name is very generic, which I don't
mind, but perhaps then the documentation should reflect that this use is
when it's used with that event etc.
> + * @NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD: Driver/device which supports
> + * ML reconfig link removal offload.
Ah, I guess that addresses the whole "can the driver do it" part.
Though like I said above I'm not sure why you call this offloaded, I'm
not convinced you'd ever want to do it in a non-offloaded way?
> + [NL80211_ATTR_TSF] = { .type = NLA_U64 },
Not sure that makes sense, it's not used for input?
> struct wireless_dev *wdev = dev->ieee80211_ptr;
> + struct cfg80211_link_reconfig_removal_params params = {};
> + bool is_ml_reconfig = false;
> + int ret = 0;
You can remove ret, just rewrite the code below a bit better.
> +int
> +cfg80211_update_link_reconfig_remove_status(struct net_device *netdev,
> + unsigned int link_id,
I feel like this should check the link_id against valid_links?
> + u8 tbtt_count, u64 tsf,
> + enum ieee80211_link_reconfig_remove_state action)
> +{
> + struct wiphy *wiphy = netdev->ieee80211_ptr->wiphy;
> + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
> + struct sk_buff *msg;
> + void *hdr = NULL;
> + int ret = 0;
Please read
https://staticthinking.wordpress.com/2024/02/28/return-0-is-better-than-return-ret/
I really do agree with him a lot here :-)
So please don't init ret = 0, and "return 0" on the success path.
> + /* Only for ML reconfigure link removal offloaded driver, need to
> + * update the status about the ongoing link removal to userspace.
> + */
> + if (!wiphy_ext_feature_isset(wiphy,
> + NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD))
> + return -EOPNOTSUPP;
> +
> + trace_cfg80211_update_link_reconfig_remove_status(wiphy, netdev,
> + link_id, tbtt_count,
> + tsf, action);
> +
> + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!msg)
> + return -ENOMEM;
> +
> + if (action == IEEE80211_LINK_RECONFIG_START)
> + hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_LINK_REMOVAL_STARTED);
> + else if (action == IEEE80211_LINK_RECONFIG_COMPLETE)
> + hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_LINK_REMOVAL_COMPLETED);
You can save this if you actually just have the driver pass the command
ID. However, seems like the function should update valid_links, just
like cfg80211_links_removed() does? Or maybe it should just become valid
to call cfg80211_links_removed() for AP interfaces in this context ...
> + nla_put_u8(msg, NL80211_ATTR_AP_REMOVAL_COUNT, tbtt_count) ||
> + nla_put_u64_64bit(msg, NL80211_ATTR_TSF, tsf,
> + NL80211_ATTR_PAD)) {
Those don't make sense after removal completed, so they shouldn't be
there. But again, perhaps that's just going to become
cfg80211_links_removed() anyway.
> + nla_put_failure:
> + genlmsg_cancel(msg, hdr);
The cancel is pointless if you just free the message, and then you don't
need to initialize hdr=NULL either.
> + nlmsg_free(msg);
>
> +TRACE_EVENT(rdev_link_reconfig_remove,
> + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
> + const struct cfg80211_link_reconfig_removal_params *params),
> +
> + TP_ARGS(wiphy, netdev, params),
That's pretty inconsistent indentation, I'd prefer to do it like the
other events, even if checkpatch complains a bit ...
> + TP_STRUCT__entry(WIPHY_ENTRY
> + NETDEV_ENTRY
> + __field(u32, link_id)
> + __field(u16, count)
> + __dynamic_array(u8, frame, params->ie_len)
> + ),
> +
> + TP_fast_assign(WIPHY_ASSIGN;
> + NETDEV_ASSIGN;
> + __entry->link_id = params->link_id;
> + __entry->count = params->link_removal_cntdown;
> + memcpy(__get_dynamic_array(frame), params->ie,
> + params->ie_len);
> + ),
Same here, this might make checkpatch happier, but honestly, I much
prefer the style that all the other events have ...
> +int cfg80211_link_reconfig_remove(struct wireless_dev *wdev,
> + const struct cfg80211_link_reconfig_removal_params *params)
> +{
> + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
> + int ret = -EINVAL;
> +
> + lockdep_assert_wiphy(wdev->wiphy);
> +
> + /* Currently, removal of link from MLD is supported for AP BSS only, it
> + * can be extended for non-AP/STA MLD as well but that shall use
> + * action frame to update about its link reconfiguration.
> + */
> + if (wdev->iftype == NL80211_IFTYPE_AP)
> + ret = rdev_link_reconfig_remove(rdev, wdev->netdev, params);
> +
> + return ret;
Oh come on, you don't even need the 'ret' variable.
johannes
On Thu, 2024-03-28 at 11:22 +0530, Manish Dharanenthiran wrote:
> This is a preparation for supporting Multi-Link reconfigure link removal
> procedure[IEEE P802.11be/D5.0 - 35.3.6.3 Removing affiliated APs] for
> driver which supports offloaded Multi-Link reconfigure link removal.
>
> Multi-Link reconfigure link removal offloaded drivers will take care
> of updating the reconfiguration MLE in self and partner beacons.
I think we need to flesh that out. I don't know if you saw the CSA
discussion, but I think it's pretty obvious that the same discussion
about partner links is going to apply here. That doesn't necessarily
mean that you _have_ to do that in this set (though I'd actually like to
see it to support hwsim, for testing purposes of both sides), but I
think the way it's written now the API doesn't consider how we might
update partner links if it's not automatically handled by firmware, etc.
johannes
On 3/27/2024 10:52 PM, Manish Dharanenthiran wrote:
[...]
> +/**
> + * cfg80211_update_link_reconfig_remove_status - Inform userspace about
> + * the removal status of link which is scheduled for removal
> + * @dev: the device on which the operation is requested
> + * @link_id: Link which is undergoing removal
> + * @tbtt_count: Current tbtt_count to be updated.
> + * @tsf: Beacon's timestamp value
> + * @status: Inform started or completed action to userspace based on the value
kernel-doc warns:
include/net/cfg80211.h:9573: warning: Function parameter or struct member 'action' not described in 'cfg80211_update_link_reconfig_remove_status'
include/net/cfg80211.h:9573: warning: Excess function parameter 'status' description in 'cfg80211_update_link_reconfig_remove_status'
> + * received,
> + * i) 0 (IEEE80211_LINK_RECONFIG_START) - Send
> + * NL80211_CMD_LINK_REMOVAL_STARTED
> + * ii) 1 (IEEE80211_LINK_RECONFIG_COMPLETE) - Send
> + * NL80211_CMD_LINK_REMOVAL_COMPLETED
> + *
> + *
> + * This function is used to inform userspace about the ongoing link removal
> + * status. 'IEEE80211_LINK_RECONFIG_START' is issued when the first beacon with
> + * ML reconfigure IE is sent out. This event can be used by userspace to start
> + * the BTM in case of AP mode. And, IEEE80211_LINK_RECONFIG_COMPLETE is issued
> + * when the last beacon is sent with ML reconfigure IE. This is used to
> + * initiate the deletion of that link, also to trigger deauth/disassoc for the
> + * associated peer(s).
> + *
> + * Note: This API is currently used by drivers which supports offloaded
> + * Multi-Link reconfigure link removal. Returns failure if FEATURE FLAG is not
> + * set or success if NL message is sent.
> + */
> +int
> +cfg80211_update_link_reconfig_remove_status(struct net_device *dev,
> + unsigned int link_id,
> + u8 tbtt_count, u64 tsf,
> + enum ieee80211_link_reconfig_remove_state action);
> +
> #endif /* __NET_CFG80211_H */
[...]
/jeff
On 3/28/2024 11:53 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 11:22 +0530, Manish Dharanenthiran wrote:
>>
>> +int __ieee80211_link_reconfig_remove(struct ieee80211_local *local,
>> + struct ieee80211_sub_if_data *sdata,
>> + const struct cfg80211_link_reconfig_removal_params *params)
>> +{
>> + struct ieee80211_link_data *link;
>> + int ret;
>> +
>> + if (!ieee80211_sdata_running(sdata))
>> + return -ENETDOWN;
>> +
>> + if (sdata->vif.type != NL80211_IFTYPE_AP)
>> + return -EINVAL;
>> +
>> + link = sdata_dereference(sdata->link[params->link_id], sdata);
>> + if (!link)
>> + return -ENOLINK;
>> +
>> + ret = drv_link_reconfig_remove(local, sdata, params);
>> +
>> + return ret;
>
> Again ... remove the 'ret' variable, it serves no purpose at all.
>
>> +}
>> +
>> +int ieee80211_update_link_reconfig_remove_status(struct ieee80211_vif *vif,
>> + unsigned int link_id,
>> + u8 tbtt_count, u64 tsf,
>> + enum ieee80211_link_reconfig_remove_state status)
>> +{
>> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
>> +
>> + if (vif->type != NL80211_IFTYPE_AP) {
>> + sdata_err(sdata, "Discarding link reconfig status for unsupported vif type\n");
>
> Uh, no. Remove that message please.
>
Sure, will make this if block in different way.
>> +TRACE_EVENT(drv_link_reconfig_remove,
>> + TP_PROTO(struct ieee80211_local *local,
>> + struct ieee80211_sub_if_data *sdata,
>> + const struct cfg80211_link_reconfig_removal_params *params),
>> +
>> + TP_ARGS(local, sdata, params),
>> +
>> + TP_STRUCT__entry(LOCAL_ENTRY
>> + VIF_ENTRY
>> + __field(u32, link_id)
>> + __field(u16, count)
>> + __dynamic_array(u8, frame, params->ie_len)
>> + ),
>
> All the same things about indentation apply here.
>
> johannes
>
On 3/28/2024 11:55 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 11:22 +0530, Manish Dharanenthiran wrote:
>> This is a preparation for supporting Multi-Link reconfigure link removal
>> procedure[IEEE P802.11be/D5.0 - 35.3.6.3 Removing affiliated APs] for
>> driver which supports offloaded Multi-Link reconfigure link removal.
>>
>> Multi-Link reconfigure link removal offloaded drivers will take care
>> of updating the reconfiguration MLE in self and partner beacons.
>
> I think we need to flesh that out. I don't know if you saw the CSA
> discussion, but I think it's pretty obvious that the same discussion
> about partner links is going to apply here. That doesn't necessarily
> mean that you _have_ to do that in this set (though I'd actually like to
> see it to support hwsim, for testing purposes of both sides), but I
> think the way it's written now the API doesn't consider how we might
> update partner links if it's not automatically handled by firmware, etc.
>
> johannes
Yeah, the intention for this RFC to get some insights from upstream
community about the usage of new NL commands and attributes for driver
which has the synchronization between partner links in the lower level.
Will try to send hwsim changes in a separate one where it handles for
both (offload/non-offload) cases.
I believe you are referring to discussion here:
https://lore.kernel.org/linux-wireless/[email protected]/
I thought of sending these API(s) with only offloaded driver in design
for this RFC which i will try de-couple or make it more generic while
sending the upcoming versions.
Also, for non-offloaded there are certain API like reporting TSF might
be redundant, since, we will do the offset and time calculation either
in hostapd or kernel directly. We are still finding ways on how to do
that for non-offloaded driver since this might need a similar or
additional handling as CSA.
On 3/28/2024 11:48 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 11:22 +0530, Manish Dharanenthiran wrote:
>>
>> + * @ie: ML reconfigure IE to be updated in beacon in the link going to be
>> + * removed and in all affiliated links.
>> + * @ie_len: ML reconfigure IE length
>
> Given the spec having moved away from "Information Element" in favour of
> just "Element", I'd prefer to do the same in the code. Not that we
> really want to rename all the "ie" stuff in the code now, but for new
> code.
>
>> + * @link_reconfig_remove: Notifies the driver about the link to be
>> + * scheduled for removal with ML reconfigure IE built for that particular
>> + * link along with the TBTT count until which the beacon with ML
>> + * reconfigure IE should be sent.
>
> Same here.
>
Sure, will address in all the places
>> +/**
>> + * cfg80211_update_link_reconfig_remove_status - Inform userspace about
>> + * the removal status of link which is scheduled for removal
>> + * @dev: the device on which the operation is requested
>> + * @link_id: Link which is undergoing removal
>> + * @tbtt_count: Current tbtt_count to be updated.
>> + * @tsf: Beacon's timestamp value
>> + * @status: Inform started or completed action to userspace based on the value
>> + * received,
>> + * i) 0 (IEEE80211_LINK_RECONFIG_START) - Send
>> + * NL80211_CMD_LINK_REMOVAL_STARTED
>> + * ii) 1 (IEEE80211_LINK_RECONFIG_COMPLETE) - Send
>> + * NL80211_CMD_LINK_REMOVAL_COMPLETED
>
> I wouldn't list the values "0" and "1"? They're really quite
> unimportant.
>
> Also we've sometimes used just the nl80211 cmd value as a parameter,
> could do that here too instead of adding IEEE80211_LINK_RECONFIG_START
> and IEEE80211_LINK_RECONFIG_COMPLETE?
>
I thought to add these in the description so that the action taken in
these API is more clear, but it worked in other way. Will address this
in next version.
>> + * This function is used to inform userspace about the ongoing link removal
>> + * status. 'IEEE80211_LINK_RECONFIG_START' is issued when the first beacon with
>> + * ML reconfigure IE is sent out. This event can be used by userspace to start
>> + * the BTM in case of AP mode. And, IEEE80211_LINK_RECONFIG_COMPLETE is issued
>> + * when the last beacon is sent with ML reconfigure IE. This is used to
>> + * initiate the deletion of that link, also to trigger deauth/disassoc for the
>> + * associated peer(s).
>
> I'm not fully sure I understand this - the way I read it, userspace
> actually request link removal with NL80211_CMD_REMOVE_LINK (at least
> right now, will comment separately), why does it need an event too?
>
>> + * Note: This API is currently used by drivers which supports offloaded
>> + * Multi-Link reconfigure link removal. Returns failure if FEATURE FLAG is not
>> + * set or success if NL message is sent.
>
> What would be "non-offloaded ML reconfiguration link removal"? What does
> offloaded even mean here? It's only counting down and then removing the
> link, no? Isn't that fundamentally an operation that has to happen in
> kernel or device space, since that's sending beacons? I don't see you'd
> really want to time the updates to each beacon from userspace?
>
By offloaded, we tried to convey that the synchronization of the
reconfiguration IE(TBTT count) between partner links is taken care by
the firmware instead of hostap/kernel.
Yeah as you mentioned it is actually counting down and then removing but
the thing here is, since the synchronization is done on the firmware
side, this event is used to send the timestamp value of the first beacon
sent with reconfiguration element and as well as the current the TBTT
count. This timestamp of the first beacon is needed to start the BTM
request/disassoc timer along with the value received from the driver.
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -1304,6 +1304,10 @@
>> * @NL80211_CMD_REMOVE_LINK: Remove a link from an interface. This may come
>> * without %NL80211_ATTR_MLO_LINK_ID as an easy way to remove all links
>> * in preparation for e.g. roaming to a regular (non-MLO) AP.
>> + * To initiate link removal procedure, set below attributes,
>> + * %NL80211_ATTR_AP_REMOVAL_COUNT - AP removal timer count(TBTT)
>> + * %NL80211_ATTR_IE - ML reconfigure Information element
>> + * Can be extended to update multiple TBTT & IE(s).
>
> I'm wondering if this really is the right API? For example, older
> kernels would ignore all the other attributes and immediately delete the
> link, is that what you want? Seems you might want to at least _know_
> about the availability of (graceful) link removal.
>
> Also, if it should piggy-back on an existing API, maybe better would be
> STOP_AP? That's the operation that normally stops beaconing, although if
> you just delete the link the kernel does have to clean that up ...
>
> Or maybe it should just be a whole new operation? You're already adding
> new commands for the events anyway, could use _START (dropping the "ED")
> for both a command and an event. Though see above, not sure why you need
> the event anyway.
>
> So just not sure about this, any arguments either way? I guess if it
> were part of STOP_AP you'd expect the link to stay around and just be
> inactive, and then require it to be removed by userspace with
> NL80211_CMD_REMOVE_LINK after it stops, so perhaps in some way doing it
> via NL80211_CMD_REMOVE_LINK makes more sense so it will actually be
> removed entirely.
With the new reconfiguration procedure, we are not stopping the beacon
or putting the BSS to inactive or such, we still continue to operate and
serve the peers connected to the AP MLD, rather we are trying to
intimate that this link is going to removed 'after this' many count just
like CSA. Station associated to this might take action based on its
capabilities or just ignore this element. That's the reason we went with
piggy-backing the existing remove link API but instead of removing
immediately we just by-passing when TBTT & IE attributes are present.
>
>> * @NL80211_CMD_ADD_LINK_STA: Add a link to an MLD station
>> * @NL80211_CMD_MODIFY_LINK_STA: Modify a link of an MLD station
>> @@ -1329,6 +1333,14 @@
>> * %NL80211_ATTR_MLO_TTLM_ULINK attributes are used to specify the
>> * TID to Link mapping for downlink/uplink traffic.
>> *
>> + * @NL80211_CMD_LINK_REMOVAL_STARTED: Once first beacon with reconfiguration MLE
>> + * is sent, userspace is notified with the TBTT and TSF value to indicate
>> + * timestamp of that beacon using %NL80211_ATTR_AP_REMOVAL_COUNT and
>> + * %NL80211_ATTR_TSF respectively.
>> + *
>> + * @NL80211_CMD_LINK_REMOVAL_COMPLETED: Once last beacon with reconfiguration
>> + * MLE is sent, userspace is notified with completion.
>
> We already have NL80211_CMD_LINKS_REMOVED for the station side of this,
> maybe we should just reuse that event also here?
>
This has other functionality as well similar to "cfg80211_remove_link",
can I add case similar to how I extended the "NL80211_CMD_REMOVE_LINK"
handler?
>> + * @NL80211_ATTR_AP_REMOVAL_COUNT: (u16) TBTT count up-to which reconfiguration
>> + * MLE is sent. Also, userspace will be notified with this count once the
>> + * first beacon with reconfiguration MLE is sent.
>
> I'm not sure how that notification thing all makes sense when it's
> something requested by userspace in the first place? Are you expecting
> the underlying driver to actually _change_ it?
>
>> + * @NL80211_ATTR_TSF: (u64) TSF value when the first beacon with reconfiguration
>> + * MLE is sent.
>
> Why is that needed? But also the name is very generic, which I don't
> mind, but perhaps then the documentation should reflect that this use is
> when it's used with that event etc.
>
Sure, will update the documentation.
>> + * @NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD: Driver/device which supports
>> + * ML reconfig link removal offload.
>
> Ah, I guess that addresses the whole "can the driver do it" part.
>
> Though like I said above I'm not sure why you call this offloaded, I'm
> not convinced you'd ever want to do it in a non-offloaded way?
>
>> + [NL80211_ATTR_TSF] = { .type = NLA_U64 },
>
> Not sure that makes sense, it's not used for input?
>
We are using this attribute to notify the timestamp to userspace, so
thought adding a policy would be good.
>> struct wireless_dev *wdev = dev->ieee80211_ptr;
>> + struct cfg80211_link_reconfig_removal_params params = {};
>> + bool is_ml_reconfig = false;
>> + int ret = 0;
>
> You can remove ret, just rewrite the code below a bit better.
>
>> +int
>> +cfg80211_update_link_reconfig_remove_status(struct net_device *netdev,
>> + unsigned int link_id,
>
> I feel like this should check the link_id against valid_links?
>
>> + u8 tbtt_count, u64 tsf,
>> + enum ieee80211_link_reconfig_remove_state action)
>> +{
>> + struct wiphy *wiphy = netdev->ieee80211_ptr->wiphy;
>> + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
>> + struct sk_buff *msg;
>> + void *hdr = NULL;
>> + int ret = 0;
>
> Please read
> https://staticthinking.wordpress.com/2024/02/28/return-0-is-better-than-return-ret/
>
> I really do agree with him a lot here :-)
>
> So please don't init ret = 0, and "return 0" on the success path.
>
Thanks for sharing this! Will address all the return variable
discrepancy here and in other places as well.
>> + /* Only for ML reconfigure link removal offloaded driver, need to
>> + * update the status about the ongoing link removal to userspace.
>> + */
>> + if (!wiphy_ext_feature_isset(wiphy,
>> + NL80211_EXT_FEATURE_MLD_LINK_REMOVAL_OFFLOAD))
>> + return -EOPNOTSUPP;
>> +
>> + trace_cfg80211_update_link_reconfig_remove_status(wiphy, netdev,
>> + link_id, tbtt_count,
>> + tsf, action);
>> +
>> + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>> + if (!msg)
>> + return -ENOMEM;
>> +
>> + if (action == IEEE80211_LINK_RECONFIG_START)
>> + hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_LINK_REMOVAL_STARTED);
>> + else if (action == IEEE80211_LINK_RECONFIG_COMPLETE)
>> + hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_LINK_REMOVAL_COMPLETED);
>
> You can save this if you actually just have the driver pass the command
> ID. However, seems like the function should update valid_links, just
> like cfg80211_links_removed() does? Or maybe it should just become valid
> to call cfg80211_links_removed() for AP interfaces in this context ...
>
we are not yet deleting the link as such though we say completed.
Complete here meant only for what the userspace has initiated. Its up to
the userspace to decide on what would be the next step. Like, it may
send broadcast BM, flush out all the peers connected and then remove the
link completely. Hence, didn't do any changes on the valid_links part.
>
>> + nla_put_u8(msg, NL80211_ATTR_AP_REMOVAL_COUNT, tbtt_count) ||
>> + nla_put_u64_64bit(msg, NL80211_ATTR_TSF, tsf,
>> + NL80211_ATTR_PAD)) {
>
> Those don't make sense after removal completed, so they shouldn't be
> there. But again, perhaps that's just going to become
> cfg80211_links_removed() anyway.
>
>> + nla_put_failure:
>> + genlmsg_cancel(msg, hdr);
>
> The cancel is pointless if you just free the message, and then you don't
> need to initialize hdr=NULL either.
>
Since i had hdr allocation inside if else if block and then have check
for NULL pointer, it made sense to initialize to NULL. But with changes
to send the command id, I will update this code in the next version.
>> + nlmsg_free(msg);
>>
>> +TRACE_EVENT(rdev_link_reconfig_remove,
>> + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
>> + const struct cfg80211_link_reconfig_removal_params *params),
>> +
>> + TP_ARGS(wiphy, netdev, params),
>
> That's pretty inconsistent indentation, I'd prefer to do it like the
> other events, even if checkpatch complains a bit ...
>
checkpatch had complained this and I though the expectation here is to
have similar indentation as well. Will update the trace.h in this and in
the next 2/2 patch as well.
>> + TP_STRUCT__entry(WIPHY_ENTRY
>> + NETDEV_ENTRY
>> + __field(u32, link_id)
>> + __field(u16, count)
>> + __dynamic_array(u8, frame, params->ie_len)
>> + ),
>> +
>> + TP_fast_assign(WIPHY_ASSIGN;
>> + NETDEV_ASSIGN;
>> + __entry->link_id = params->link_id;
>> + __entry->count = params->link_removal_cntdown;
>> + memcpy(__get_dynamic_array(frame), params->ie,
>> + params->ie_len);
>> + ),
>
> Same here, this might make checkpatch happier, but honestly, I much
> prefer the style that all the other events have ...
>
>> +int cfg80211_link_reconfig_remove(struct wireless_dev *wdev,
>> + const struct cfg80211_link_reconfig_removal_params *params)
>> +{
>> + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
>> + int ret = -EINVAL;
>> +
>> + lockdep_assert_wiphy(wdev->wiphy);
>> +
>> + /* Currently, removal of link from MLD is supported for AP BSS only, it
>> + * can be extended for non-AP/STA MLD as well but that shall use
>> + * action frame to update about its link reconfiguration.
>> + */
>> + if (wdev->iftype == NL80211_IFTYPE_AP)
>> + ret = rdev_link_reconfig_remove(rdev, wdev->netdev, params);
>> +
>> + return ret;
>
> Oh come on, you don't even need the 'ret' variable.
>
> johannes
Sure :) will address all the ret variable at once in the next version.
On 3/29/2024 2:32 AM, Jeff Johnson wrote:
> On 3/27/2024 10:52 PM, Manish Dharanenthiran wrote:
> [...]
>> +/**
>> + * cfg80211_update_link_reconfig_remove_status - Inform userspace about
>> + * the removal status of link which is scheduled for removal
>> + * @dev: the device on which the operation is requested
>> + * @link_id: Link which is undergoing removal
>> + * @tbtt_count: Current tbtt_count to be updated.
>> + * @tsf: Beacon's timestamp value
>> + * @status: Inform started or completed action to userspace based on the value
>
> kernel-doc warns:
>
> include/net/cfg80211.h:9573: warning: Function parameter or struct member 'action' not described in 'cfg80211_update_link_reconfig_remove_status'
> include/net/cfg80211.h:9573: warning: Excess function parameter 'status' description in 'cfg80211_update_link_reconfig_remove_status'
>
> /jeff
Will address that in the upcoming version.
Is there any way to run this locally similar to checkpatch? so that we
will make sure check this as well before sending to community in
upcoming patches.
Regards
Manish Dharanenthiran
On 3/29/2024 10:16 AM, Manish Dharanenthiran wrote:
> On 3/29/2024 2:32 AM, Jeff Johnson wrote:
>> kernel-doc warns:
>>
>> include/net/cfg80211.h:9573: warning: Function parameter or struct member 'action' not described in 'cfg80211_update_link_reconfig_remove_status'
>> include/net/cfg80211.h:9573: warning: Excess function parameter 'status' description in 'cfg80211_update_link_reconfig_remove_status'
[...]
> Is there any way to run this locally similar to checkpatch? so that we
> will make sure check this as well before sending to community in
> upcoming patches.
I use:
files=$(git diff --name-only $base HEAD)
scripts/kernel-doc -Wall -Werror -none $files