When a critical update occurs to any of elements inside beacon frame, AP
shall increment BSS Parameters Change Count(BPCC) subfield and set the
Critical Update flag subfield of the Capability Information to notify
client that the critical update occurred on AP. Refer section "35.3.10
BSS parameter critical update procedure" on IEEE P802.11be D4.0 for
details.
On beacon offload case, change in CU parameters should be sent to user
space either before or along with probe or assoc request frame receive
to ensure that user space uses latest CU values and BPCC while generating
response to the received frames. So, add the critical update parameters
as a new attribute to existing NL80211_CMD_FRAME command instead of
sending this on a separate NL80211 event.
Add an ieee80211_critical_update() API to send the parameters to cfg80211
and call it when event received from firmware to update critical
parameters to user space.
Driver (ath12k) changes that utilize this will be posted in the future
versions.
Based on the suggestion received on below link, add extended feature
NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD flag if driver handles
synchronization among all the links and update critical information on
partner link beacon for AP MLD and user space can update critical
information only on impacted link beacon template. Add this critical
update attribute on NL80211_CMD_FRAME only when this flag is set
by driver.
Link: https://lore.kernel.org/all/[email protected]/
Suggested-by: Johannes Berg <[email protected]>
Rathees Kumar R Chinannan (2):
wifi: nl80211: Add attribute to send critical update parameters
wifi: mac80211: Indicate ongoing critical update parameters
include/net/cfg80211.h | 10 +++
include/net/mac80211.h | 13 ++++
include/uapi/linux/nl80211.h | 104 +++++++++++++++++++++++++++++
net/mac80211/cfg.c | 43 +++++++++++-
net/mac80211/rx.c | 12 ++++
net/mac80211/tx.c | 9 +++
net/wireless/nl80211.c | 123 ++++++++++++++++++++++++++++++++++-
7 files changed, 312 insertions(+), 2 deletions(-)
--
2.34.1
Add NL80211_ATTR_RXMGMT_CRITICAL_UPDATE attribute to send critical
update parameters to hostapd on NL80211_CMD_FRAME.
User space application requires these CU params to update fields on probe
and assoc response frame. So, during probe or assoc request frame receive,
send these params as a new attribute on existing NL80211_CMD_FRAME for
AP MLD.
Change in CU parameters should be sent to user space either before or
along with probe or assoc request frame receive to ensure that user space
uses latest CU values and BPCC while generating response to the received
frames. So, add the critical update parameters as a new attribute to
existing NL80211_CMD_FRAME command instead of sending this on a separate
NL80211 event.
Based on the suggestion received on below link, add extended feature
NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD flag if driver handles
synchronization among all the links and update critical information on
partner link beacon for AP MLD and user space can update critical
information only on impacted link beacon template. Add this critical
update attribute on NL80211_CMD_FRAME only when this flag is set
by driver.
Link: https://lore.kernel.org/all/[email protected]/
Suggested-by: Johannes Berg <[email protected]>
Signed-off-by: Rathees Kumar R Chinannan <[email protected]>
---
include/net/cfg80211.h | 10 +++
include/uapi/linux/nl80211.h | 104 +++++++++++++++++++++++++++++
net/wireless/nl80211.c | 123 ++++++++++++++++++++++++++++++++++-
3 files changed, 236 insertions(+), 1 deletion(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 2e2be4fd2bb6..496a9d4956de 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -6142,7 +6142,11 @@ void wiphy_delayed_work_flush(struct wiphy *wiphy,
* unprotected beacon report
* @links: array of %IEEE80211_MLD_MAX_NUM_LINKS elements containing @addr
* @ap and @client for each link
+ * @links.ap.bpcc: Bss param change count value for each link
+ * @links.ap.switch_count: CSA/BCCA count for each link
+ * @links.ap.critical_flag: Critical update flag for each link
* @valid_links: bitmap describing what elements of @links are valid
+ * @critical_update: critical params updated on any wdev link
*/
struct wireless_dev {
struct wiphy *wiphy;
@@ -6247,6 +6251,9 @@ struct wireless_dev {
u8 addr[ETH_ALEN] __aligned(2);
union {
struct {
+ u8 bpcc;
+ u8 switch_count;
+ bool critical_flag;
unsigned int beacon_interval;
struct cfg80211_chan_def chandef;
} ap;
@@ -6256,6 +6263,7 @@ struct wireless_dev {
};
} links[IEEE80211_MLD_MAX_NUM_LINKS];
u16 valid_links;
+ bool critical_update;
};
static inline const u8 *wdev_address(struct wireless_dev *wdev)
@@ -8340,6 +8348,7 @@ void cfg80211_conn_failed(struct net_device *dev, const u8 *mac_addr,
* @flags: flags, as defined in &enum nl80211_rxmgmt_flags
* @rx_tstamp: Hardware timestamp of frame RX in nanoseconds
* @ack_tstamp: Hardware timestamp of ack TX in nanoseconds
+ * @critical_update: critical params updated for the received frame
*/
struct cfg80211_rx_info {
int freq;
@@ -8351,6 +8360,7 @@ struct cfg80211_rx_info {
u32 flags;
u64 rx_tstamp;
u64 ack_tstamp;
+ bool critical_update;
};
/**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f23ecbdd84a2..02948c29fc27 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2856,6 +2856,15 @@ enum nl80211_commands {
* %NL80211_CMD_ASSOCIATE indicating the SPP A-MSDUs
* are used on this connection
*
+ * @NL80211_ATTR_RXMGMT_CRITICAL_UPDATE: This is a nested attribute for driver
+ * supporting critical update feature for AP MLD. When used with
+ * %NL80211_CMD_FRAME it contains attribute defined in &enum nl80211_cu_attrs
+ * to send critical update params for list of MLDs. Driver adds this attribute
+ * only for probe, assoc and reassoc request frame. User-space can use these
+ * params to update CU fields on corresponding response frame. This attribute
+ * is needed only on beacon offload case and it is not needed on beacon
+ * non-offload case since user space itself has these data.
+ *
* @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 +3410,7 @@ enum nl80211_attrs {
NL80211_ATTR_ASSOC_SPP_AMSDU,
+ NL80211_ATTR_RXMGMT_CRITICAL_UPDATE,
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
@@ -6542,6 +6552,11 @@ enum nl80211_feature_flags {
* (signaling and payload protected) A-MSDUs and this shall be advertised
* in the RSNXE.
*
+ * @NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD: User space sets critical
+ * information only on impacted link through @NL80211_CMD_SET_BEACON.
+ * Driver/Device handles synchronization among all the links and update
+ * critical information on partner link beacon in case of MLD.
+ *
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
*/
@@ -6617,6 +6632,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_CRITICAL_UPDATE_OFFLOAD,
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
@@ -7987,4 +8003,92 @@ enum nl80211_ap_settings_flags {
NL80211_AP_SETTINGS_SA_QUERY_OFFLOAD_SUPPORT = 1 << 1,
};
+/*
+ * Critical update attribute length for a MLD list with two nested
+ * attributes. Each nla_nest_start() reserves four bytes.
+ */
+#define NL80211_CU_ATTR_MLDS_LEN 8
+
+/*
+ * Critical update attribute length for a particular MLD with two nested attributes.
+ * Each nla_nest_start() reserves four bytes and
+ * nla_put_u32(NL80211_CU_MLD_ATTR_IFINDEX) reserves eight bytes.
+ */
+
+#define NL80211_CU_ATTR_MLD_LEN 16
+
+/*
+ * Critical update attribute length for a particular link.
+ * Each nla_nest_start() reserves four bytes,
+ * nla_put_u8(NL80211_CU_MLD_LINK_ATTR_ID) reserves eight bytes,
+ * nla_put_flag(NL80211_CU_MLD_LINK_ATTR_CRITICAL_FLAG) reserves four bytes,
+ * nla_put_u8(NL80211_CU_MLD_LINK_ATTR_BPCC) reserves eight bytes and
+ * nla_put_u8(NL80211_CU_MLD_LINK_ATTR_SWITCH_COUNT) reserves eight bytes,
+ */
+#define NL80211_CU_ATTR_LINK_LEN 32
+
+/**
+ * nl80211_cu_attrs - critical update attributes
+ *
+ *
+ * @__NL80211_CU_ATTR_INVALID: invalid
+ * @NL80211_CU_ATTR_MLDS: nested attribute specifying list of MLDs,
+ * see &enum nl80211_cu_mld_attrs
+ * @__NL80211_CU_ATTR_LAST: internal use
+ * @NL80211_CU_ATTR_MAX: maximum critical update attribute
+ */
+enum nl80211_cu_attrs {
+ __NL80211_CU_ATTR_INVALID,
+
+ NL80211_CU_ATTR_MLDS,
+
+ /* keep last */
+ __NL80211_CU_ATTR_LAST,
+ NL80211_CU_ATTR_MAX = __NL80211_CU_ATTR_LAST - 1
+};
+
+/**
+ * nl80211_cu_mld_attrs - per mld critical update attributes
+ *
+ * @__NL80211_CU_MLD_ATTR_INVALID: invalid
+ * @NL80211_CU_MLD_ATTR_IFINDEX: network interface index (u32)
+ * @NL80211_CU_MLD_ATTR_LINKS: nested attribute specifying list of links
+ * on each mld, see &enum nl80211_cu_mld_link_attrs
+ * @__NL80211_CU_MLD_ATTR_LAST: internal use
+ * @NL80211_CU_MLD_ATTR_MAX: maximum per mld critical update attribute
+ */
+enum nl80211_cu_mld_attrs {
+ __NL80211_CU_MLD_ATTR_INVALID,
+
+ NL80211_CU_MLD_ATTR_IFINDEX,
+ NL80211_CU_MLD_ATTR_LINKS,
+
+ /* keep last */
+ __NL80211_CU_MLD_ATTR_LAST,
+ NL80211_CU_MLD_ATTR_MAX = __NL80211_CU_MLD_ATTR_LAST - 1
+};
+
+/**
+ * nl80211_cu_mld_link_attrs - per link critical update attributes
+ *
+ * @__NL80211_CU_MLD_LINK_ATTR_INVALID: invalid
+ * @NL80211_CU_MLD_LINK_ATTR_ID: link ID (u8) for the AP MLD
+ * @NL80211_CU_MLD_LINK_ATTR_CRITICAL_FLAG: critical update flag for the link
+ * @NL80211_CU_MLD_LINK_ATTR_BPCC: BSS parameter change count (u8) for the link
+ * @NL80211_CU_MLD_LINK_ATTR_SWITCH_COUNT: CSA / BCCA switch count (u8) for the link
+ * @__NL80211_CU_MLD_LINK_ATTR_LAST: internal use
+ * @NL80211_CU_MLD_LINK ATTR_MAX: maximum per link critical update attribute
+ */
+enum nl80211_cu_mld_link_attrs {
+ __NL80211_CU_MLD_LINK_ATTR_INVALID,
+
+ NL80211_CU_MLD_LINK_ATTR_ID,
+ NL80211_CU_MLD_LINK_ATTR_CRITICAL_FLAG,
+ NL80211_CU_MLD_LINK_ATTR_BPCC,
+ NL80211_CU_MLD_LINK_ATTR_SWITCH_COUNT,
+
+ /* keep last */
+ __NL80211_CU_MLD_LINK_ATTR_LAST,
+ NL80211_CU_MLD_LINK_ATTR_MAX = __NL80211_CU_MLD_LINK_ATTR_LAST - 1
+};
#endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b4edba6b0b7b..b2c6376705b3 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -468,6 +468,25 @@ static const struct netlink_range_validation nl80211_punct_bitmap_range = {
.max = 0xffff,
};
+static const struct nla_policy
+link_policy[NL80211_CU_MLD_LINK_ATTR_MAX + 1] = {
+ [NL80211_CU_MLD_LINK_ATTR_ID] = { .type = NLA_U8 },
+ [NL80211_CU_MLD_LINK_ATTR_CRITICAL_FLAG] = { .type = NLA_FLAG },
+ [NL80211_CU_MLD_LINK_ATTR_BPCC] = { .type = NLA_U8 },
+ [NL80211_CU_MLD_LINK_ATTR_SWITCH_COUNT] = { .type = NLA_U8 },
+};
+
+static const struct nla_policy
+mld_policy[NL80211_CU_MLD_ATTR_MAX + 1] = {
+ [NL80211_CU_MLD_ATTR_IFINDEX] = { .type = NLA_U32 },
+ [NL80211_CU_MLD_ATTR_LINKS] = NLA_POLICY_NESTED(link_policy),
+};
+
+static const struct nla_policy
+cu_policy[NL80211_CU_ATTR_MAX + 1] = {
+ [NL80211_CU_ATTR_MLDS] = NLA_POLICY_NESTED(mld_policy),
+};
+
static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[0] = { .strict_start_type = NL80211_ATTR_HE_OBSS_PD },
[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
@@ -826,6 +845,7 @@ 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_RXMGMT_CRITICAL_UPDATE] = NLA_POLICY_NESTED(cu_policy),
};
/* policy for the key attributes */
@@ -18843,6 +18863,91 @@ bool cfg80211_rx_unexpected_4addr_frame(struct net_device *dev,
}
EXPORT_SYMBOL(cfg80211_rx_unexpected_4addr_frame);
+static int nl80211_send_mgmt_critical_update_len(struct wireless_dev *wdev)
+{
+ struct wiphy *wiphy = wdev->wiphy;
+ struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
+ struct wireless_dev *tmp_wdev;
+ int link_id;
+ int cu_len = 0;
+
+ cu_len += NL80211_CU_ATTR_MLDS_LEN;
+ list_for_each_entry(tmp_wdev, &rdev->wiphy.wdev_list, list) {
+ cu_len += NL80211_CU_ATTR_MLD_LEN;
+ for_each_valid_link(tmp_wdev, link_id) {
+ cu_len += NL80211_CU_ATTR_LINK_LEN;
+ }
+ }
+ return cu_len;
+}
+
+/* Add critical update attribute when sending management frame
+ * to user space.
+ */
+static int nl80211_send_mgmt_critical_update(struct sk_buff *msg,
+ struct wireless_dev *wdev)
+{
+ struct wiphy *wiphy = wdev->wiphy;
+ struct wireless_dev *tmp_wdev;
+ struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
+ struct nlattr *critical_update;
+ struct nlattr *mld_list, *mld;
+ struct nlattr *link_list, *link;
+ struct net_device *tmp_netdev;
+ int link_id;
+ int i = 0, j = 0;
+
+ critical_update = nla_nest_start(msg, NL80211_ATTR_RXMGMT_CRITICAL_UPDATE);
+ if (!critical_update)
+ return -ENOBUFS;
+
+ mld_list = nla_nest_start(msg, NL80211_CU_ATTR_MLDS);
+ if (!mld_list)
+ return -ENOBUFS;
+
+ list_for_each_entry(tmp_wdev, &rdev->wiphy.wdev_list, list) {
+ if (!tmp_wdev->valid_links)
+ continue;
+ if (!tmp_wdev->critical_update)
+ continue;
+ mld = nla_nest_start(msg, i + 1);
+ if (!mld)
+ return -ENOBUFS;
+ tmp_netdev = tmp_wdev->netdev;
+ if (tmp_netdev &&
+ nla_put_u32(msg, NL80211_CU_MLD_ATTR_IFINDEX, tmp_netdev->ifindex))
+ return -ENOBUFS;
+ link_list = nla_nest_start(msg, NL80211_CU_MLD_ATTR_LINKS);
+ if (!link_list)
+ return -ENOBUFS;
+
+ for_each_valid_link(tmp_wdev, link_id) {
+ link = nla_nest_start(msg, j + 1);
+ if (!link)
+ return -ENOBUFS;
+ if (nla_put_u8(msg, NL80211_CU_MLD_LINK_ATTR_ID, link_id))
+ return -ENOBUFS;
+ if (tmp_wdev->links[link_id].ap.critical_flag &&
+ nla_put_flag(msg, NL80211_CU_MLD_LINK_ATTR_CRITICAL_FLAG))
+ return -ENOBUFS;
+ if (nla_put_u8(msg, NL80211_CU_MLD_LINK_ATTR_BPCC,
+ tmp_wdev->links[link_id].ap.bpcc))
+ return -ENOBUFS;
+ if (nla_put_u8(msg, NL80211_CU_MLD_LINK_ATTR_SWITCH_COUNT,
+ tmp_wdev->links[link_id].ap.switch_count))
+ return -ENOBUFS;
+ nla_nest_end(msg, link);
+ j++;
+ }
+ nla_nest_end(msg, link_list);
+ nla_nest_end(msg, mld);
+ i++;
+ }
+ nla_nest_end(msg, mld_list);
+ nla_nest_end(msg, critical_update);
+ return 0;
+}
+
int nl80211_send_mgmt(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev, u32 nlportid,
struct cfg80211_rx_info *info, gfp_t gfp)
@@ -18850,8 +18955,15 @@ int nl80211_send_mgmt(struct cfg80211_registered_device *rdev,
struct net_device *netdev = wdev->netdev;
struct sk_buff *msg;
void *hdr;
+ int cu_len = 0;
- msg = nlmsg_new(100 + info->len, gfp);
+ if (wiphy_ext_feature_isset(
+ wdev->wiphy,
+ NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD) &&
+ info->critical_update)
+ cu_len = nl80211_send_mgmt_critical_update_len(wdev);
+
+ msg = nlmsg_new(100 + info->len + cu_len, gfp);
if (!msg)
return -ENOMEM;
@@ -18885,6 +18997,15 @@ int nl80211_send_mgmt(struct cfg80211_registered_device *rdev,
NL80211_ATTR_PAD)))
goto nla_put_failure;
+ if (wiphy_ext_feature_isset(
+ wdev->wiphy,
+ NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD) &&
+ info->critical_update) {
+ if (nl80211_send_mgmt_critical_update(msg, wdev))
+ goto nla_put_failure;
+ /* Reset the flag after adding the critical update attribute*/
+ wdev->critical_update = false;
+ }
genlmsg_end(msg, hdr);
return genlmsg_unicast(wiphy_net(&rdev->wiphy), msg, nlportid);
--
2.34.1
User space application doesn't have the latest ongoing critical
update parameters like critical update flag, BSS param change
count (BPCC) and CSA/CCA switch count for each link. Add an
ieee80211_critical_update() API to send these params to cfg80211
and call it when event received from firmware during probe or assoc
or reassoc request frame receive to update critical parameters to
user space and needed only on beacon offload case.
Signed-off-by: Rathees Kumar R Chinannan <[email protected]>
---
include/net/mac80211.h | 13 +++++++++++++
net/mac80211/cfg.c | 43 +++++++++++++++++++++++++++++++++++++++++-
net/mac80211/rx.c | 12 ++++++++++++
net/mac80211/tx.c | 9 +++++++++
4 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 353488ab94a2..a369f0d7087d 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -7599,4 +7599,17 @@ int ieee80211_emulate_switch_vif_chanctx(struct ieee80211_hw *hw,
int n_vifs,
enum ieee80211_chanctx_switch_mode mode);
+/**
+ * ieee80211_critical_update - update critical params for each link
+ * @vif: the specified virtual interface
+ * @link_id: the link ID for MLO, otherwise 0
+ * @critical_flag: critical update information
+ * @bpcc: Bss parameter change count value
+ *
+ * The function is called when event received from firmware to update
+ * critical parameters to user space during probe or assoc or reassoc request
+ * frame receive and needed only on beacon offload case.
+ */
+void ieee80211_critical_update(struct ieee80211_vif *vif, unsigned int link_id,
+ bool critical_flag, u8 bpcc);
#endif /* MAC80211_H */
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index f03452dc716d..97ffd3028467 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3606,6 +3606,7 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif, unsigned int link_id)
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
struct ieee80211_local *local = sdata->local;
struct ieee80211_link_data *link_data;
+ struct wireless_dev *wdev = ieee80211_vif_to_wdev(vif);
if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
return;
@@ -3617,7 +3618,13 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif, unsigned int link_id)
rcu_read_unlock();
return;
}
-
+ if (wiphy_ext_feature_isset(
+ local->hw.wiphy,
+ NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD) &&
+ wdev->valid_links && wdev->links[link_id].ap.switch_count != 0) {
+ wdev->links[link_id].ap.switch_count = 0;
+ wdev->critical_update = true;
+ }
/* TODO: MBSSID with MLO changes */
if (vif->mbssid_tx_vif == vif) {
/* Trigger ieee80211_csa_finish() on the non-transmitting
@@ -3643,6 +3650,40 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif, unsigned int link_id)
}
EXPORT_SYMBOL(ieee80211_csa_finish);
+/**
+ * ieee80211_critical_update - update critical params for each link
+ * @vif: the specified virtual interface
+ * @link_id: the link ID for MLO, otherwise 0
+ * @critical_flag: critical update information
+ * @bpcc: Bss parameter change count value
+ *
+ * The function is called when event received from firmware to update
+ * critical parameters to user space during probe or assoc or reassoc request
+ * frame receive and needed only on beacon offload case.
+ */
+void ieee80211_critical_update(struct ieee80211_vif *vif, unsigned int link_id,
+ bool critical_flag, u8 bpcc)
+{
+ struct wireless_dev *wdev = ieee80211_vif_to_wdev(vif);
+ struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+ struct ieee80211_local *local = sdata->local;
+
+ if (!wiphy_ext_feature_isset(
+ local->hw.wiphy,
+ NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD) ||
+ !wdev->valid_links)
+ return;
+ if (WARN_ON(link_id > IEEE80211_MLD_MAX_NUM_LINKS))
+ return;
+ if (wdev->links[link_id].ap.critical_flag != critical_flag ||
+ wdev->links[link_id].ap.bpcc != bpcc) {
+ wdev->critical_update = true;
+ wdev->links[link_id].ap.critical_flag = critical_flag;
+ wdev->links[link_id].ap.bpcc = bpcc;
+ }
+}
+EXPORT_SYMBOL(ieee80211_critical_update);
+
void ieee80211_channel_switch_disconnect(struct ieee80211_vif *vif, bool block_tx)
{
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index c1f850138405..e60560b4e3e4 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3825,14 +3825,26 @@ static ieee80211_rx_result debug_noinline
ieee80211_rx_h_userspace_mgmt(struct ieee80211_rx_data *rx)
{
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
+ struct ieee80211_mgmt *mgmt = (void *)rx->skb->data;
+ __le16 stype;
+ struct wireless_dev *wdev = &rx->sdata->wdev;
+
struct cfg80211_rx_info info = {
.freq = ieee80211_rx_status_to_khz(status),
.buf = rx->skb->data,
.len = rx->skb->len,
.link_id = rx->link_id,
.have_link_id = rx->link_id >= 0,
+ .critical_update = false,
};
+ stype = mgmt->frame_control & cpu_to_le16(IEEE80211_FCTL_STYPE);
+ if (stype == cpu_to_le16(IEEE80211_STYPE_PROBE_REQ) ||
+ stype == cpu_to_le16(IEEE80211_STYPE_ASSOC_REQ) ||
+ stype == cpu_to_le16(IEEE80211_STYPE_REASSOC_REQ)) {
+ if (wdev->critical_update)
+ info.critical_update = true;
+ }
/* skip known-bad action frames and return them in the next handler */
if (status->rx_flags & IEEE80211_RX_MALFORMED_ACTION_FRM)
return RX_CONTINUE;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 6bf223e6cd1a..3603cc582b0d 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -5041,6 +5041,8 @@ u8 ieee80211_beacon_update_cntdwn(struct ieee80211_vif *vif, unsigned int link_i
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
struct ieee80211_link_data *link;
struct beacon_data *beacon = NULL;
+ struct wireless_dev *wdev = ieee80211_vif_to_wdev(vif);
+ struct ieee80211_local *local = sdata->local;
u8 count = 0;
if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
@@ -5063,6 +5065,13 @@ u8 ieee80211_beacon_update_cntdwn(struct ieee80211_vif *vif, unsigned int link_i
goto unlock;
count = __ieee80211_beacon_update_cntdwn(beacon);
+ if (wiphy_ext_feature_isset(
+ local->hw.wiphy,
+ NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD) &&
+ wdev->valid_links && wdev->links[link_id].ap.switch_count != count) {
+ wdev->links[link_id].ap.switch_count = count;
+ wdev->critical_update = true;
+ }
unlock:
rcu_read_unlock();
--
2.34.1
On Wed, 2024-04-03 at 21:52 +0530, Rathees Kumar R Chinannan wrote:
> Add NL80211_ATTR_RXMGMT_CRITICAL_UPDATE attribute to send critical
> update parameters to hostapd on NL80211_CMD_FRAME.
>
> User space application requires these CU params to update fields on probe
> and assoc response frame. So, during probe or assoc request frame receive,
> send these params as a new attribute on existing NL80211_CMD_FRAME for
> AP MLD.
>
> Change in CU parameters should be sent to user space either before or
> along with probe or assoc request frame receive to ensure that user space
> uses latest CU values and BPCC while generating response to the received
> frames. So, add the critical update parameters as a new attribute to
> existing NL80211_CMD_FRAME command instead of sending this on a separate
> NL80211 event.
>
> Based on the suggestion received on below link, add extended feature
> NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD flag if driver handles
> synchronization among all the links and update critical information on
> partner link beacon for AP MLD and user space can update critical
> information only on impacted link beacon template. Add this critical
> update attribute on NL80211_CMD_FRAME only when this flag is set
> by driver.
>
> Link: https://lore.kernel.org/all/[email protected]/
I'm not sure we're interpreting this the same way ;-)
At least now that I'm reading it again, I was thinking in that thread
about how beacons on partner links are updated. That whole thread was
about handling that?
> Suggested-by: Johannes Berg <[email protected]>
> Signed-off-by: Rathees Kumar R Chinannan <[email protected]>
> ---
> include/net/cfg80211.h | 10 +++
> include/uapi/linux/nl80211.h | 104 +++++++++++++++++++++++++++++
> net/wireless/nl80211.c | 123 ++++++++++++++++++++++++++++++++++-
> 3 files changed, 236 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 2e2be4fd2bb6..496a9d4956de 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -6142,7 +6142,11 @@ void wiphy_delayed_work_flush(struct wiphy *wiphy,
> * unprotected beacon report
> * @links: array of %IEEE80211_MLD_MAX_NUM_LINKS elements containing @addr
> * @ap and @client for each link
> + * @links.ap.bpcc: Bss param change count value for each link
BSS
> + * @links.ap.switch_count: CSA/BCCA count for each link
Had to think about BCCA a bit too much ... maybe explain it? :)
Also can someone actually be trying to do channel switch and color
change at the same time?
> + * @links.ap.critical_flag: Critical update flag for each link
> * @valid_links: bitmap describing what elements of @links are valid
> + * @critical_update: critical params updated on any wdev link
Why have two critical indications? And they're even named differently?
I'd probably rename the inner to 'critical_update' and remove the outer,
it's easy to loop over (valid) links?
> static inline const u8 *wdev_address(struct wireless_dev *wdev)
> @@ -8340,6 +8348,7 @@ void cfg80211_conn_failed(struct net_device *dev, const u8 *mac_addr,
> * @flags: flags, as defined in &enum nl80211_rxmgmt_flags
> * @rx_tstamp: Hardware timestamp of frame RX in nanoseconds
> * @ack_tstamp: Hardware timestamp of ack TX in nanoseconds
> + * @critical_update: critical params updated for the received frame
> */
> struct cfg80211_rx_info {
> int freq;
> @@ -8351,6 +8360,7 @@ struct cfg80211_rx_info {
> u32 flags;
> u64 rx_tstamp;
> u64 ack_tstamp;
> + bool critical_update;
Not sure what this means?
> + * @NL80211_ATTR_RXMGMT_CRITICAL_UPDATE: This is a nested attribute for driver
> + * supporting critical update feature for AP MLD. When used with
> + * %NL80211_CMD_FRAME it contains attribute defined in &enum nl80211_cu_attrs
> + * to send critical update params for list of MLDs. Driver adds this attribute
> + * only for probe, assoc and reassoc request frame. User-space can use these
> + * params to update CU fields on corresponding response frame. This attribute
> + * is needed only on beacon offload case and it is not needed on beacon
> + * non-offload case since user space itself has these data.
The wording of "beacon offload" vs. "beacon non-offload" seems, at best,
imprecise? This should be about offloading partner link updates, or
something like that?
Also I find this API a bit confusing, why is this sent in an RX frame
notification, which is stateless today, but you only send it once on the
first RX notification after it was set. Is that some kind of
optimisation?
I'd think it would make more sense to send a separate notification of
the data, and then you don't even need to store the data in the wdev.
Maybe then if we find out that it updates too frequently, we can defer
that separate notification (e.g. create the SKB for it, store a pointer
to that, and send it only on the first frame RX).
> @@ -3401,6 +3410,7 @@ enum nl80211_attrs {
>
> NL80211_ATTR_ASSOC_SPP_AMSDU,
>
> + NL80211_ATTR_RXMGMT_CRITICAL_UPDATE,
> /* add attributes here, update the policy in nl80211.c */
nit: blank line
> + * @NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD: User space sets critical
> + * information only on impacted link through @NL80211_CMD_SET_BEACON.
> + * Driver/Device handles synchronization among all the links and update
> + * critical information on partner link beacon in case of MLD.
That's not really _quite_ what this implies today.
I mean, yeah, that's the idea, but you're actually using it for
something else. I don't even mind, but maybe it'd make sense to put this
into a separate patch, that starts building out the infrastructure for
handling all of this, simply with this for starters to say "not needed
in this case"?
(I think maybe I'm missing the broader picture a bit in these patches)
> +/*
> + * Critical update attribute length for a MLD list with two nested
> + * attributes. Each nla_nest_start() reserves four bytes.
> + */
> +#define NL80211_CU_ATTR_MLDS_LEN 8
...
> +#define NL80211_CU_ATTR_MLD_LEN 16
...
> +#define NL80211_CU_ATTR_LINK_LEN 32
Please don't do that, use nla_attr_size() or something?
But maybe anyway we can find a better representation.
> +/**
> + * nl80211_cu_attrs - critical update attributes
> + *
> + *
nit: double blank line
> + * @__NL80211_CU_ATTR_INVALID: invalid
> + * @NL80211_CU_ATTR_MLDS: nested attribute specifying list of MLDs,
> + * see &enum nl80211_cu_mld_attrs
> + * @__NL80211_CU_ATTR_LAST: internal use
> + * @NL80211_CU_ATTR_MAX: maximum critical update attribute
> + */
> +enum nl80211_cu_attrs {
> + __NL80211_CU_ATTR_INVALID,
> +
> + NL80211_CU_ATTR_MLDS,
> +
> + /* keep last */
> + __NL80211_CU_ATTR_LAST,
> + NL80211_CU_ATTR_MAX = __NL80211_CU_ATTR_LAST - 1
> +};
But I'm not sure why you need this level anyway?
Why would you need to represent multiple MLDs for a single interface?
You've tagged it to RX (see above for comments on that) but that's per
interface anyway, so certainly per MLD?
> +++ b/net/wireless/nl80211.c
> @@ -468,6 +468,25 @@ static const struct netlink_range_validation nl80211_punct_bitmap_range = {
> .max = 0xffff,
> };
>
> +static const struct nla_policy
> +link_policy[NL80211_CU_MLD_LINK_ATTR_MAX + 1] = {
> + [NL80211_CU_MLD_LINK_ATTR_ID] = { .type = NLA_U8 },
> + [NL80211_CU_MLD_LINK_ATTR_CRITICAL_FLAG] = { .type = NLA_FLAG },
> + [NL80211_CU_MLD_LINK_ATTR_BPCC] = { .type = NLA_U8 },
> + [NL80211_CU_MLD_LINK_ATTR_SWITCH_COUNT] = { .type = NLA_U8 },
> +};
> +
> +static const struct nla_policy
> +mld_policy[NL80211_CU_MLD_ATTR_MAX + 1] = {
> + [NL80211_CU_MLD_ATTR_IFINDEX] = { .type = NLA_U32 },
> + [NL80211_CU_MLD_ATTR_LINKS] = NLA_POLICY_NESTED(link_policy),
> +};
> +
> +static const struct nla_policy
> +cu_policy[NL80211_CU_ATTR_MAX + 1] = {
> + [NL80211_CU_ATTR_MLDS] = NLA_POLICY_NESTED(mld_policy),
> +};
> +
> static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
> [0] = { .strict_start_type = NL80211_ATTR_HE_OBSS_PD },
> [NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
> @@ -826,6 +845,7 @@ 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_RXMGMT_CRITICAL_UPDATE] = NLA_POLICY_NESTED(cu_policy),
All of these are outgoing though, so probably shouldn't even have a
policy (which implies NLA_REJECT, i.e. not used incoming)?
> - msg = nlmsg_new(100 + info->len, gfp);
> + if (wiphy_ext_feature_isset(
> + wdev->wiphy,
> + NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD) &&
> + info->critical_update)
I don't even see how the flag check is needed here, since it relies on
the driver setting info->critical_update?
johannes
On Wed, 2024-04-03 at 21:52 +0530, Rathees Kumar R Chinannan wrote:
> When a critical update occurs to any of elements inside beacon frame, AP
> shall increment BSS Parameters Change Count(BPCC) subfield and set the
> Critical Update flag subfield of the Capability Information to notify
> client that the critical update occurred on AP. Refer section "35.3.10
> BSS parameter critical update procedure" on IEEE P802.11be D4.0 for
> details.
>
> On beacon offload case, change in CU parameters should be sent to user
> space either before or along with probe or assoc request frame receive
> to ensure that user space uses latest CU values and BPCC while generating
> response to the received frames. So, add the critical update parameters
> as a new attribute to existing NL80211_CMD_FRAME command instead of
> sending this on a separate NL80211 event.
>
> Add an ieee80211_critical_update() API to send the parameters to cfg80211
> and call it when event received from firmware to update critical
> parameters to user space.
Somewhat more conceptually, I wonder if we should really handle this
hybrid approach? You're offloading the beacon updates, why not offload
the probe/assoc response cases as well? Are they really _that_ much more
complex? What does the hostapd code for this look like?
Also, as we already discussed, this is fundamentally racy today, and
that cannot be fixed unless you really put it all into the firmware,
directly in the TX path, which is probably never going to happen.
So under the assumption that it already *is* racy, I'm not entirely sure
I see where this is needed at all?
You're basically handling two (kinds of) values here:
1a) CSA counters: these are today handled in mac80211, and we've
already decided that we need to handle them also in mac80211 (and
get offsets to the partner links from hostapd a la
NL80211_ATTR_CSA_C_OFFSETS_TX).
Yeah this is still racy, but you can't fix that without offloading
it all anyway.
1b) BCCA counters: these are missing today, but that should be fixed,
and then it's just the same as 1a.
2) BSS parameter change count and critical update flag: are these
really actually completely by the firmware? I'm not sure how that's
necessary, since hostapd initiates all the relevant operations? So
not sure why you need these at all, couldn't hostapd track this and
you just copy it across to the other links?
So for 1a/1b I don't even think we should push this to hostapd, it's not
necessary and it will just cause more API fragmentation, because already
*know* that we have to do things like NL80211_ATTR_CSA_C_OFFSETS_TX for
other devices (and hwsim) for this, just more complex.
Thus, I don't think this is right. We can handle your case the same way,
and you don't even need the NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD
flag right now, by just pushing the offsets into the kernel and handling
the values coming from the firmware by filling in at the right offsets.
Could perhaps get the offsets down into the driver and do it there, or
just handle it in mac80211, not sure.
But this then doesn't fragment the API here, because hostapd will just
give all the offsets, and the below stack (mac80211/driver/fw/hwsim)
will fill the values.
Not sure about the BPCC and critical update flag, but let's think about
how that would be handled on other hardware? It feels to me right now
that hostapd should already be in control and know this, and not need
any indications (it does control all links), but maybe I'm wrong (or
there's just some extra case the firmware might do) and it doesn't, but
then let's also consider how that would be handled in other hardware (or
think about hwsim instead), and find API that doesn't fragment so much.
Yes we'll always fragment whether or not the partner link beacons need
updating, but it shouldn't need all those different paths for everything
else too.
johannes
On 5/3/2024 2:42 PM, Johannes Berg wrote:
> On Wed, 2024-04-03 at 21:52 +0530, Rathees Kumar R Chinannan wrote:
>> Add NL80211_ATTR_RXMGMT_CRITICAL_UPDATE attribute to send critical
>> update parameters to hostapd on NL80211_CMD_FRAME.
>>
>> User space application requires these CU params to update fields on probe
>> and assoc response frame. So, during probe or assoc request frame receive,
>> send these params as a new attribute on existing NL80211_CMD_FRAME for
>> AP MLD.
>>
>> Change in CU parameters should be sent to user space either before or
>> along with probe or assoc request frame receive to ensure that user space
>> uses latest CU values and BPCC while generating response to the received
>> frames. So, add the critical update parameters as a new attribute to
>> existing NL80211_CMD_FRAME command instead of sending this on a separate
>> NL80211 event.
>>
>> Based on the suggestion received on below link, add extended feature
>> NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD flag if driver handles
>> synchronization among all the links and update critical information on
>> partner link beacon for AP MLD and user space can update critical
>> information only on impacted link beacon template. Add this critical
>> update attribute on NL80211_CMD_FRAME only when this flag is set
>> by driver.
>>
>> Link: https://lore.kernel.org/all/[email protected]/
>
> I'm not sure we're interpreting this the same way ;-)
>
> At least now that I'm reading it again, I was thinking in that thread
> about how beacons on partner links are updated. That whole thread was
> about handling that?
>
yes, it is about beacon update on partner links.
>> Suggested-by: Johannes Berg <[email protected]>
>> Signed-off-by: Rathees Kumar R Chinannan <[email protected]>
>> ---
>> include/net/cfg80211.h | 10 +++
>> include/uapi/linux/nl80211.h | 104 +++++++++++++++++++++++++++++
>> net/wireless/nl80211.c | 123 ++++++++++++++++++++++++++++++++++-
>> 3 files changed, 236 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 2e2be4fd2bb6..496a9d4956de 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -6142,7 +6142,11 @@ void wiphy_delayed_work_flush(struct wiphy *wiphy,
>> * unprotected beacon report
>> * @links: array of %IEEE80211_MLD_MAX_NUM_LINKS elements containing @addr
>> * @ap and @client for each link
>> + * @links.ap.bpcc: Bss param change count value for each link
>
> BSS
>
Sure, will correct this.
>> + * @links.ap.switch_count: CSA/BCCA count for each link
>
> Had to think about BCCA a bit too much ... maybe explain it? :)
>
> Also can someone actually be trying to do channel switch and color
> change at the same time?
>
Currently kernel doesn't allow another color change if one is already
active or if CSA is active. Similarly, during channel switch if there is
a color change in progress, it will get aborted.
>> + * @links.ap.critical_flag: Critical update flag for each link
>> * @valid_links: bitmap describing what elements of @links are valid
>> + * @critical_update: critical params updated on any wdev link
>
> Why have two critical indications? And they're even named differently?
> I'd probably rename the inner to 'critical_update' and remove the outer,
> it's easy to loop over (valid) links?
>
Sure, will remove the second one and iterate valid links for the same.
>> static inline const u8 *wdev_address(struct wireless_dev *wdev)
>> @@ -8340,6 +8348,7 @@ void cfg80211_conn_failed(struct net_device *dev, const u8 *mac_addr,
>> * @flags: flags, as defined in &enum nl80211_rxmgmt_flags
>> * @rx_tstamp: Hardware timestamp of frame RX in nanoseconds
>> * @ack_tstamp: Hardware timestamp of ack TX in nanoseconds
>> + * @critical_update: critical params updated for the received frame
>> */
>> struct cfg80211_rx_info {
>> int freq;
>> @@ -8351,6 +8360,7 @@ struct cfg80211_rx_info {
>> u32 flags;
>> u64 rx_tstamp;
>> u64 ack_tstamp;
>> + bool critical_update;
>
> Not sure what this means?
>
This flag will be set on mac80211 when critical parameters are received
on driver along with probe/assoc request frame receive.
>> + * @NL80211_ATTR_RXMGMT_CRITICAL_UPDATE: This is a nested attribute for driver
>> + * supporting critical update feature for AP MLD. When used with
>> + * %NL80211_CMD_FRAME it contains attribute defined in &enum nl80211_cu_attrs
>> + * to send critical update params for list of MLDs. Driver adds this attribute
>> + * only for probe, assoc and reassoc request frame. User-space can use these
>> + * params to update CU fields on corresponding response frame. This attribute
>> + * is needed only on beacon offload case and it is not needed on beacon
>> + * non-offload case since user space itself has these data.
>
> The wording of "beacon offload" vs. "beacon non-offload" seems, at best,
> imprecise? This should be about offloading partner link updates, or
> something like that?
>
Sure, will update this.
> Also I find this API a bit confusing, why is this sent in an RX frame
> notification, which is stateless today, but you only send it once on the
> first RX notification after it was set. Is that some kind of
> optimisation?
>
Yes, it is sent not only on first RX notification, it is sent later as
well if there is a change in the value received from driver when
compared to previous values.
> I'd think it would make more sense to send a separate notification of
> the data, and then you don't even need to store the data in the wdev.
>
> Maybe then if we find out that it updates too frequently, we can defer
> that separate notification (e.g. create the SKB for it, store a pointer
> to that, and send it only on the first frame RX).
>
CU parameters should be sent to user space either before or
along with probe or assoc request frame receive to ensure that user
space uses latest CU values and BPCC while generating response to the
received frames.
Do you recommend to send it as separate NL80211 command before handling
these management frame RX ? Please suggest.
>> @@ -3401,6 +3410,7 @@ enum nl80211_attrs {
>>
>> NL80211_ATTR_ASSOC_SPP_AMSDU,
>>
>> + NL80211_ATTR_RXMGMT_CRITICAL_UPDATE,
>> /* add attributes here, update the policy in nl80211.c */
>
> nit: blank line
>
Sure, will correct this.
>> + * @NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD: User space sets critical
>> + * information only on impacted link through @NL80211_CMD_SET_BEACON.
>> + * Driver/Device handles synchronization among all the links and update
>> + * critical information on partner link beacon in case of MLD.
>
> That's not really _quite_ what this implies today.
>
> I mean, yeah, that's the idea, but you're actually using it for
> something else. I don't even mind, but maybe it'd make sense to put this
> into a separate patch, that starts building out the infrastructure for
> handling all of this, simply with this for starters to say "not needed
> in this case"?
>
> (I think maybe I'm missing the broader picture a bit in these patches)
>
ok, will raise a separate patch for this feature flag.
And patch to add critical update information on impacted link during set
beacon template is still under review.
>> +/*
>> + * Critical update attribute length for a MLD list with two nested
>> + * attributes. Each nla_nest_start() reserves four bytes.
>> + */
>> +#define NL80211_CU_ATTR_MLDS_LEN 8
> ...
>> +#define NL80211_CU_ATTR_MLD_LEN 16
> ...
>> +#define NL80211_CU_ATTR_LINK_LEN 32
>
> Please don't do that, use nla_attr_size() or something?
>
> But maybe anyway we can find a better representation.
>
Sure, will check this.
>> +/**
>> + * nl80211_cu_attrs - critical update attributes
>> + *
>> + *
>
> nit: double blank line
>
Ok
>> + * @__NL80211_CU_ATTR_INVALID: invalid
>> + * @NL80211_CU_ATTR_MLDS: nested attribute specifying list of MLDs,
>> + * see &enum nl80211_cu_mld_attrs
>> + * @__NL80211_CU_ATTR_LAST: internal use
>> + * @NL80211_CU_ATTR_MAX: maximum critical update attribute
>> + */
>> +enum nl80211_cu_attrs {
>> + __NL80211_CU_ATTR_INVALID,
>> +
>> + NL80211_CU_ATTR_MLDS,
>> +
>> + /* keep last */
>> + __NL80211_CU_ATTR_LAST,
>> + NL80211_CU_ATTR_MAX = __NL80211_CU_ATTR_LAST - 1
>> +};
>
> But I'm not sure why you need this level anyway?
>
> Why would you need to represent multiple MLDs for a single interface?
> You've tagged it to RX (see above for comments on that) but that's per
> interface anyway, so certainly per MLD?
>
If multiple vaps are created on a radio, then on MBSSID IE for each Non
TX BSSID profile, we have to update CU bits & BPCC on Multi link
element. These non-TX vaps can be part of different MLDs, so we need to
pass the critical information for all MLDs instead of single MLD.
>> +++ b/net/wireless/nl80211.c
>> @@ -468,6 +468,25 @@ static const struct netlink_range_validation nl80211_punct_bitmap_range = {
>> .max = 0xffff,
>> };
>>
>> +static const struct nla_policy
>> +link_policy[NL80211_CU_MLD_LINK_ATTR_MAX + 1] = {
>> + [NL80211_CU_MLD_LINK_ATTR_ID] = { .type = NLA_U8 },
>> + [NL80211_CU_MLD_LINK_ATTR_CRITICAL_FLAG] = { .type = NLA_FLAG },
>> + [NL80211_CU_MLD_LINK_ATTR_BPCC] = { .type = NLA_U8 },
>> + [NL80211_CU_MLD_LINK_ATTR_SWITCH_COUNT] = { .type = NLA_U8 },
>> +};
>> +
>> +static const struct nla_policy
>> +mld_policy[NL80211_CU_MLD_ATTR_MAX + 1] = {
>> + [NL80211_CU_MLD_ATTR_IFINDEX] = { .type = NLA_U32 },
>> + [NL80211_CU_MLD_ATTR_LINKS] = NLA_POLICY_NESTED(link_policy),
>> +};
>> +
>> +static const struct nla_policy
>> +cu_policy[NL80211_CU_ATTR_MAX + 1] = {
>> + [NL80211_CU_ATTR_MLDS] = NLA_POLICY_NESTED(mld_policy),
>> +};
>> +
>> static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
>> [0] = { .strict_start_type = NL80211_ATTR_HE_OBSS_PD },
>> [NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
>> @@ -826,6 +845,7 @@ 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_RXMGMT_CRITICAL_UPDATE] = NLA_POLICY_NESTED(cu_policy),
>
> All of these are outgoing though, so probably shouldn't even have a
> policy (which implies NLA_REJECT, i.e. not used incoming)?
>
Sure, will remove this change.
>> - msg = nlmsg_new(100 + info->len, gfp);
>> + if (wiphy_ext_feature_isset(
>> + wdev->wiphy,
>> + NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD) &&
>> + info->critical_update)
>
> I don't even see how the flag check is needed here, since it relies on
> the driver setting info->critical_update?
>
> johannes
Sure, will remove this flag check.
On 5/3/2024 2:57 PM, Johannes Berg wrote:
> On Wed, 2024-04-03 at 21:52 +0530, Rathees Kumar R Chinannan wrote:
>> When a critical update occurs to any of elements inside beacon frame, AP
>> shall increment BSS Parameters Change Count(BPCC) subfield and set the
>> Critical Update flag subfield of the Capability Information to notify
>> client that the critical update occurred on AP. Refer section "35.3.10
>> BSS parameter critical update procedure" on IEEE P802.11be D4.0 for
>> details.
>>
>> On beacon offload case, change in CU parameters should be sent to user
>> space either before or along with probe or assoc request frame receive
>> to ensure that user space uses latest CU values and BPCC while generating
>> response to the received frames. So, add the critical update parameters
>> as a new attribute to existing NL80211_CMD_FRAME command instead of
>> sending this on a separate NL80211 event.
>>
>> Add an ieee80211_critical_update() API to send the parameters to cfg80211
>> and call it when event received from firmware to update critical
>> parameters to user space.
>
> Somewhat more conceptually, I wonder if we should really handle this
> hybrid approach? You're offloading the beacon updates, why not offload
> the probe/assoc response cases as well? Are they really _that_ much more
> complex? What does the hostapd code for this look like?
>
>
> Also, as we already discussed, this is fundamentally racy today, and
> that cannot be fixed unless you really put it all into the firmware,
> directly in the TX path, which is probably never going to happen.
>
> So under the assumption that it already *is* racy, I'm not entirely sure
> I see where this is needed at all?
>
> You're basically handling two (kinds of) values here:
>
> 1a) CSA counters: these are today handled in mac80211, and we've
> already decided that we need to handle them also in mac80211 (and
> get offsets to the partner links from hostapd a la
> NL80211_ATTR_CSA_C_OFFSETS_TX).
> Yeah this is still racy, but you can't fix that without offloading
> it all anyway.
>
> 1b) BCCA counters: these are missing today, but that should be fixed,
> and then it's just the same as 1a.
>
> 2) BSS parameter change count and critical update flag: are these
> really actually completely by the firmware? I'm not sure how that's
> necessary, since hostapd initiates all the relevant operations? So
> not sure why you need these at all, couldn't hostapd track this and
> you just copy it across to the other links?
>
> So for 1a/1b I don't even think we should push this to hostapd, it's not
> necessary and it will just cause more API fragmentation, because already
> *know* that we have to do things like NL80211_ATTR_CSA_C_OFFSETS_TX for
> other devices (and hwsim) for this, just more complex.
>
> Thus, I don't think this is right. We can handle your case the same way,
> and you don't even need the NL80211_EXT_FEATURE_CRITICAL_UPDATE_OFFLOAD
> flag right now, by just pushing the offsets into the kernel and handling
> the values coming from the firmware by filling in at the right offsets.
> Could perhaps get the offsets down into the driver and do it there, or
> just handle it in mac80211, not sure.
>
> But this then doesn't fragment the API here, because hostapd will just
> give all the offsets, and the below stack (mac80211/driver/fw/hwsim)
> will fill the values.
>
In case of MBSSID configuration, hostapd has to provide multiple offsets
to update these counters on BMLE per-STA profile for each non-TX MBSSID
profile when CSA/BCCA triggered on any of partner links.
To avoid providing multiple offsets, this approach is used to pass the
counter values to hostapd and use it while generating the probe/assoc
response frame.
> Not sure about the BPCC and critical update flag, but let's think about
> how that would be handled on other hardware? It feels to me right now
> that hostapd should already be in control and know this, and not need
> any indications (it does control all links), but maybe I'm wrong (or
> there's just some extra case the firmware might do) and it doesn't, but
Yes, firmware is handling some extra cases and maintain synchronization
among partner links and it maintains BPCC value and set/clear critical
flag based on TBTT timer. Hence we are passing these values also to user
space.
> then let's also consider how that would be handled in other hardware (or
> think about hwsim instead), and find API that doesn't fragment so much.
>
Sure, will check this.
> Yes we'll always fragment whether or not the partner link beacons need
> updating, but it shouldn't need all those different paths for everything
> else too.
>
> johannes