On Fri, 2024-04-05 at 00:29 +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. It also
> updates the AP removal timer automatically and notifies once the counter
> is expired.
How do we distinguish those drivers from others that won't (be able to)
do this as we had discussed also for the CSA flows?
> +/**
> + * struct cfg80211_link_reconfig_removal_params - Contains params needed for
> + * link reconfig removal
nit: maybe indent the second line with a tab? is that possible? it's a
bit confusing to go into the parameters right away without any visual
separation, IMHO.
> + * @link_removal_cntdown: TBTT countdown value until which the beacon with ML
> + * reconfigure element will be sent.
> + * @reconfigure_elem: ML reconfigure element to be updated in beacon in the link going to be
> + * removed and in all affiliated links.
> + * @elem_len: ML reconfigure element length
> + * @link_id: Link id of the link to be removed.
> + */
> +struct cfg80211_link_reconfig_removal_params {
> + u16 link_removal_cntdown;
> + const u8 *reconfigure_elem;
> + size_t elem_len;
> + unsigned int link_id;
nit: I guess the size of this doesn't really matter, but putting a
pointer after a u16 always feels wrong because it adds 6 bytes of
padding on 64-bit :)
> +/**
> + * cfg80211_update_link_reconfig_remove_update - Inform userspace about
> + * the removal status of link which is scheduled for removal
here you did a tab btw :)
> + * This function is used to inform userspace about the ongoing link removal
> + * status. 'NL80211_CMD_LINK_REMOVAL_STARTED' is issued when the first beacon with
Please use %NL80211_CMD_LINK_REMOVAL_STARTED which would print it as a
constant, instead of '...'.
> + * ML reconfigure element is sent out. This event can be used by userspace to start
> + * the BTM in case of AP mode. And, NL80211_CMD_LINK_REMOVAL_COMPLETED is issued
same here
> + * when the last beacon is sent with ML reconfigure element. 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
"FEATURE FLAG" was ... some kind of placeholder?
Should this even return a value? What are you going to do with it?
You should also document the context this can be called in.
> + * @NL80211_ATTR_TSF: (u64) TSF value when the first beacon with reconfiguration
> + * MLE is sent.
What's that needed for, btw?
> +++ 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 },
No range? I guess 0 might be an issue?
> + [NL80211_ATTR_TSF] = { .type = NLA_U64 },
If the TSF is output only maybe it should not have a policy for input?
> @@ -16103,6 +16105,7 @@ 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 = {};
that can move into the if
> @@ -16115,6 +16118,30 @@ static int nl80211_remove_link(struct sk_buff *skb, struct genl_info *info)
> return -EINVAL;
> }
>
> + 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;
> +
> + /* 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;
> +
> + params.reconfigure_elem = nla_data(info->attrs[NL80211_ATTR_IE]);
> + params.elem_len = nla_len(info->attrs[NL80211_ATTR_IE]);
> + params.link_removal_cntdown =
> + nla_get_u16(info->attrs[NL80211_ATTR_AP_REMOVAL_COUNT]);
doesn't match the policy ... but policy also doesn't match
documentation, so I think policy is wrong?
> +int
> +cfg80211_update_link_reconfig_remove_update(struct net_device *netdev,
> + unsigned int link_id,
> + u8 tbtt_count, u64 tsf,
> + enum nl80211_commands cmd)
> +{
[...]
> + nla_put_failure:
> + genlmsg_cancel(msg, hdr);
> + nlmsg_free(msg);
nit: no need to cancel before free
> +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->elem_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->reconfigure_elem,
> + params->elem_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)
What's that frame argument?! Need to check it's long enough? But ... is
that worth it at all to try to print it here in this way? Maybe with
%*pH but then you don't get all of it either? Or just leave it, and
write a small plugin if needed?
> +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);
> +
> + 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)
> + return rdev_link_reconfig_remove(rdev, wdev->netdev, params);
> +
> + return -EINVAL;
Would kind of feel to better be
if (iftype != ...) return -EINVAL;
... normal flow
johannes
