2022-11-16 02:11:23

by Gilad Itzkovitch

[permalink] [raw]
Subject: [PATCH v3 3/4] wifi: nl80211: update attributes netlink for S1G short beacon

From: Kieran Frewen <[email protected]>

Add short beacon period, head and tail attributes for user
configuration

Signed-off-by: Kieran Frewen <[email protected]>
Co-developed-by: Gilad Itzkovitch <[email protected]>
Signed-off-by: Gilad Itzkovitch <[email protected]>
---
include/uapi/linux/nl80211.h | 11 +++++++++++
net/wireless/nl80211.c | 32 +++++++++++++++++++++++++++++++-
2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index c14a91bbca7c..04bd39ee9d1d 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2751,6 +2751,11 @@ enum nl80211_commands {
* the incoming frame RX timestamp.
* @NL80211_ATTR_TD_BITMAP: Transition Disable bitmap, for subsequent
* (re)associations.
+ *
+ * @NL80211_ATTR_SHORT_BEACON_PERIOD: S1G short beacon period in TUs.
+ * @NL80211_ATTR_SHORT_BEACON_HEAD: portion of the short beacon before the TIM IE.
+ * @NL80211_ATTR_SHORT_BEACON_TAIL: portion of the short beacon after the TIM.
+ *
* @NUM_NL80211_ATTR: total number of nl80211_attrs available
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
@@ -3280,6 +3285,10 @@ enum nl80211_attrs {
NL80211_ATTR_RX_HW_TIMESTAMP,
NL80211_ATTR_TD_BITMAP,

+ NL80211_ATTR_SHORT_BEACON_PERIOD,
+ NL80211_ATTR_SHORT_BEACON_HEAD,
+ NL80211_ATTR_SHORT_BEACON_TAIL,
+
/* add attributes here, update the policy in nl80211.c */

__NL80211_ATTR_AFTER_LAST,
@@ -4963,6 +4972,7 @@ enum nl80211_bss_scan_width {
* @NL80211_BSS_FREQUENCY_OFFSET: frequency offset in KHz
* @NL80211_BSS_MLO_LINK_ID: MLO link ID of the BSS (u8).
* @NL80211_BSS_MLD_ADDR: MLD address of this BSS if connected to it.
+ * @NL80211_BSS_SHORT_BEACON_PERIOD: S1G short beacon period in TUs
* @__NL80211_BSS_AFTER_LAST: internal
* @NL80211_BSS_MAX: highest BSS attribute
*/
@@ -4990,6 +5000,7 @@ enum nl80211_bss {
NL80211_BSS_FREQUENCY_OFFSET,
NL80211_BSS_MLO_LINK_ID,
NL80211_BSS_MLD_ADDR,
+ NL80211_BSS_SHORT_BEACON_PERIOD,

/* keep last */
__NL80211_BSS_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 148f66edb015..fca6e223c2c7 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -231,12 +231,18 @@ static int validate_beacon_head(const struct nlattr *attr,
const struct ieee80211_mgmt *mgmt = (void *)data;
unsigned int fixedlen, hdrlen;
bool s1g_bcn;
+ bool s1g_short_bcn;

if (len < offsetofend(typeof(*mgmt), frame_control))
goto err;

s1g_bcn = ieee80211_is_s1g_beacon(mgmt->frame_control);
- if (s1g_bcn) {
+ s1g_short_bcn = ieee80211_is_s1g_short_beacon(mgmt->frame_control);
+ if (s1g_short_bcn) {
+ fixedlen = offsetof(struct ieee80211_ext,
+ u.s1g_short_beacon.variable);
+ hdrlen = offsetof(struct ieee80211_ext, u.s1g_short_beacon);
+ } else if (s1g_bcn) {
fixedlen = offsetof(struct ieee80211_ext,
u.s1g_beacon.variable);
hdrlen = offsetof(struct ieee80211_ext, u.s1g_beacon);
@@ -805,6 +811,14 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[NL80211_ATTR_MLD_ADDR] = NLA_POLICY_EXACT_LEN(ETH_ALEN),
[NL80211_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
[NL80211_ATTR_MAX_NUM_AKM_SUITES] = { .type = NLA_REJECT },
+ [NL80211_ATTR_SHORT_BEACON_PERIOD] = { .type = NLA_U16 },
+ [NL80211_ATTR_SHORT_BEACON_HEAD] =
+ NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_beacon_head,
+ IEEE80211_MAX_DATA_LEN),
+ [NL80211_ATTR_SHORT_BEACON_TAIL] =
+ NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_ie_attr,
+ IEEE80211_MAX_DATA_LEN),
+
};

/* policy for the key attributes */
@@ -5440,6 +5454,18 @@ static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
haveinfo = true;
}

+ if (attrs[NL80211_ATTR_SHORT_BEACON_HEAD]) {
+ bcn->short_head = nla_data(attrs[NL80211_ATTR_SHORT_BEACON_HEAD]);
+ bcn->short_head_len = nla_len(attrs[NL80211_ATTR_SHORT_BEACON_HEAD]);
+ haveinfo = true;
+ }
+
+ if (attrs[NL80211_ATTR_SHORT_BEACON_TAIL]) {
+ bcn->short_tail = nla_data(attrs[NL80211_ATTR_SHORT_BEACON_TAIL]);
+ bcn->short_tail_len = nla_len(attrs[NL80211_ATTR_SHORT_BEACON_TAIL]);
+ haveinfo = true;
+ }
+
if (!haveinfo)
return -EINVAL;

@@ -5804,6 +5830,10 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
nla_get_u32(info->attrs[NL80211_ATTR_BEACON_INTERVAL]);
params->dtim_period =
nla_get_u32(info->attrs[NL80211_ATTR_DTIM_PERIOD]);
+ if (info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD])
+ params->short_beacon_period =
+ nla_get_u32(info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD]) == 0 ?
+ 1 : nla_get_u32(info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD]);

err = cfg80211_validate_beacon_int(rdev, dev->ieee80211_ptr->iftype,
params->beacon_interval);
--
2.34.1



2023-03-30 10:00:18

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] wifi: nl80211: update attributes netlink for S1G short beacon

On Wed, 2022-11-16 at 15:06 +1300, Gilad Itzkovitch wrote:
>
> + * @NL80211_ATTR_SHORT_BEACON_HEAD: portion of the short beacon before the TIM IE.
> + * @NL80211_ATTR_SHORT_BEACON_TAIL: portion of the short beacon after the TIM.

I would tend to say "TIM element" these days, since the spec changed
this, but I guess it doesn't matter much.

> + * @NL80211_BSS_SHORT_BEACON_PERIOD: S1G short beacon period in TUs
> * @__NL80211_BSS_AFTER_LAST: internal
> * @NL80211_BSS_MAX: highest BSS attribute
> */
> @@ -4990,6 +5000,7 @@ enum nl80211_bss {
> NL80211_BSS_FREQUENCY_OFFSET,
> NL80211_BSS_MLO_LINK_ID,
> NL80211_BSS_MLD_ADDR,
> + NL80211_BSS_SHORT_BEACON_PERIOD,

You don't use this.

> +++ b/net/wireless/nl80211.c
> @@ -231,12 +231,18 @@ static int validate_beacon_head(const struct nlattr *attr,
> const struct ieee80211_mgmt *mgmt = (void *)data;
> unsigned int fixedlen, hdrlen;
> bool s1g_bcn;
> + bool s1g_short_bcn;
>
> if (len < offsetofend(typeof(*mgmt), frame_control))
> goto err;
>
> s1g_bcn = ieee80211_is_s1g_beacon(mgmt->frame_control);
> - if (s1g_bcn) {
> + s1g_short_bcn = ieee80211_is_s1g_short_beacon(mgmt->frame_control);
> + if (s1g_short_bcn) {
> + fixedlen = offsetof(struct ieee80211_ext,
> + u.s1g_short_beacon.variable);
> + hdrlen = offsetof(struct ieee80211_ext, u.s1g_short_beacon);
> + } else if (s1g_bcn) {
> fixedlen = offsetof(struct ieee80211_ext,
> u.s1g_beacon.variable);
> hdrlen = offsetof(struct ieee80211_ext, u.s1g_beacon);

Even the old code here had me worried a bit, and the new code doesn't
make this better - what if you try to set an S1G (short or not) beacon
when the interface isn't using S1G really?

> + [NL80211_ATTR_SHORT_BEACON_PERIOD] = { .type = NLA_U16 },

You can add a better range validation here, and then you don't need the
extra validation in the code later.

> + [NL80211_ATTR_SHORT_BEACON_HEAD] =
> + NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_beacon_head,
> + IEEE80211_MAX_DATA_LEN),
> + [NL80211_ATTR_SHORT_BEACON_TAIL] =
> + NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_ie_attr,
> + IEEE80211_MAX_DATA_LEN),
> +

nit: unnecessary blank line

> + if (info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD])
> + params->short_beacon_period =
> + nla_get_u32(info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD]) == 0 ?
> + 1 : nla_get_u32(info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD]);

i.e. you don't need the == 0 check if you just reject == 0.

Also, you're confusing the types - using NLA_U16 above but nla_get_u32()
here.

johannes