[PATCH v3 1/4] wifi: cfg80211: Add short_beacon_tail/head/period

[PATCH v3 1/4] wifi: cfg80211: Add short_beacon_tail/head/period

[Gilad Itzkovitch]

From: Kieran Frewen <kieran.frewen@morsemicro.com>

Support variables to handle short beacon period and adding a
separate tail/head for them.

Signed-off-by: Kieran Frewen <kieran.frewen@morsemicro.com>
Co-developed-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
---
 include/net/cfg80211.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 11a370e64143..99f28ef2323f 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1179,8 +1179,13 @@ struct cfg80211_mbssid_elems {
  *	or %NULL if not changed
  * @tail: tail portion of beacon (after TIM IE)
  *	or %NULL if not changed
+ * @short_head: head portion of short beacon or %NULL if not changed
+ * @short_tail: short tail portion of beacon (after TIM IE)
+	or %NULL if not changed
  * @head_len: length of @head
  * @tail_len: length of @tail
+ * @short_head_len: length of @short_head
+ * @short_tail_len: length of @short_tail
  * @beacon_ies: extra information element(s) to add into Beacon frames or %NULL
  * @beacon_ies_len: length of beacon_ies in octets
  * @proberesp_ies: extra information element(s) to add into Probe Response
@@ -1208,6 +1213,7 @@ struct cfg80211_beacon_data {
 	unsigned int link_id;
 
 	const u8 *head, *tail;
+	const u8 *short_head, *short_tail;
 	const u8 *beacon_ies;
 	const u8 *proberesp_ies;
 	const u8 *assocresp_ies;
@@ -1218,6 +1224,7 @@ struct cfg80211_beacon_data {
 	s8 ftm_responder;
 
 	size_t head_len, tail_len;
+	size_t short_head_len, short_tail_len;
 	size_t beacon_ies_len;
 	size_t proberesp_ies_len;
 	size_t assocresp_ies_len;
@@ -1328,7 +1335,7 @@ struct cfg80211_ap_settings {
 
 	struct cfg80211_beacon_data beacon;
 
-	int beacon_interval, dtim_period;
+	int beacon_interval, dtim_period, short_beacon_period;
 	const u8 *ssid;
 	size_t ssid_len;
 	enum nl80211_hidden_ssid hidden_ssid;

base-commit: 5c111ec204d15d1c7d00428b0afdda62ff118565
-- 
2.34.1

[PATCH v3 2/4] wifi: mac80211: S1G beacon/short beacon support

[Gilad Itzkovitch]

From: Kieran Frewen <kieran.frewen@morsemicro.com>

If configured, use the S1G short beacon format. The S1G short beacon
format includes a limited set of information elements.

Signed-off-by: Kieran Frewen <kieran.frewen@morsemicro.com>
Co-developed-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
---
 include/net/mac80211.h     |  1 +
 net/mac80211/cfg.c         |  1 +
 net/mac80211/ieee80211_i.h |  1 +
 net/mac80211/tx.c          | 14 +++++++++++++-
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 721c450a9ccd..39faafde109a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -674,6 +674,7 @@ struct ieee80211_bss_conf {
 	bool enable_beacon;
 	u8 dtim_period;
 	u16 beacon_int;
+	u8 short_beacon_period;
 	u16 assoc_capability;
 	u64 sync_tsf;
 	u32 sync_device_ts;
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index c848fe04dd44..a42abaa25273 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1308,6 +1308,7 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 	}
 
 	link_conf->dtim_period = params->dtim_period;
+	link_conf->short_beacon_period = params->short_beacon_period;
 	link_conf->enable_beacon = true;
 	link_conf->allow_p2p_go_ps = sdata->vif.p2p;
 	link_conf->twt_responder = params->twt_responder;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 63ff0d2524b6..2e1d829c548a 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -268,6 +268,7 @@ struct beacon_data {
 	struct ieee80211_meshconf_ie *meshconf;
 	u16 cntdwn_counter_offsets[IEEE80211_MAX_CNTDWN_COUNTERS_NUM];
 	u8 cntdwn_current_counter;
+	u8 long_beacon_count;
 	struct cfg80211_mbssid_elems *mbssid_ies;
 	struct rcu_head rcu_head;
 };
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index bb2e54610101..a12b32544a3a 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -5116,6 +5116,18 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,
 	struct sk_buff *skb = NULL;
 	u16 csa_off_base = 0;
 	int mbssid_len;
+	bool is_short = false;
+
+	if (vif->cfg.s1g) {
+		if (beacon->long_beacon_count == 0) {
+			is_short = false;
+			beacon->long_beacon_count =
+				vif->bss_conf.short_beacon_period - 1;
+		} else {
+			is_short = true;
+			beacon->long_beacon_count--;
+		}
+	}
 
 	if (beacon->cntdwn_counter_offsets[0]) {
 		if (!is_template)
@@ -5153,7 +5165,7 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,
 		csa_off_base = skb->len;
 	}
 
-	if (beacon->tail)
+	if (beacon->tail && !is_short)
 		skb_put_data(skb, beacon->tail, beacon->tail_len);
 
 	if (ieee80211_beacon_protect(skb, local, sdata, link) < 0)
-- 
2.34.1

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

[Gilad Itzkovitch]

From: Kieran Frewen <kieran.frewen@morsemicro.com>

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

Signed-off-by: Kieran Frewen <kieran.frewen@morsemicro.com>
Co-developed-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
---
 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

[PATCH v3 4/4] wifi: mac80211: support setting S1G short beacon period, head and tail

[Gilad Itzkovitch]

From: Kieran Frewen <kieran.frewen@morsemicro.com>

With the kernel able to send both short and long S1G beacons, include
the ability for setting the short beacon period.
It also adds the support for distinguish short beacon head and tail.

Signed-off-by: Kieran Frewen <kieran.frewen@morsemicro.com>
Co-developed-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
---
 net/mac80211/cfg.c         | 38 ++++++++++++++++++++++++++++++++++++--
 net/mac80211/ieee80211_i.h |  2 ++
 net/mac80211/tx.c          | 10 ++++++++--
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index a42abaa25273..9c4db453a9f1 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1092,6 +1092,7 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 	struct cfg80211_mbssid_elems *mbssid = NULL;
 	struct beacon_data *new, *old;
 	int new_head_len, new_tail_len;
+	int new_short_head_len, new_short_tail_len;
 	int size, err;
 	u32 changed = BSS_CHANGED_BEACON;
 	struct ieee80211_bss_conf *link_conf = link->conf;
@@ -1108,6 +1109,14 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 	else
 		new_head_len = old->head_len;
 
+	/* Short head is not a mandatory field */
+	new_short_head_len = 0;
+	if (params->short_head)
+		new_short_head_len = params->short_head_len;
+	else
+		if (old && old->short_head)
+			new_short_head_len = old->short_head_len;
+
 	/* new or old tail? */
 	if (params->tail || !old)
 		/* params->tail_len will be zero for !params->tail */
@@ -1115,7 +1124,14 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 	else
 		new_tail_len = old->tail_len;
 
-	size = sizeof(*new) + new_head_len + new_tail_len;
+	if (params->short_tail || !old)
+	       /* params->tail_len will be zero for !params->tail */
+		new_short_tail_len = params->short_tail_len;
+	else
+		new_short_tail_len = old->short_tail_len;
+
+	size = sizeof(*new) + new_head_len + new_tail_len +
+		new_short_head_len + new_short_tail_len;
 
 	/* new or old multiple BSSID elements? */
 	if (params->mbssid_ies) {
@@ -1140,11 +1156,16 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 	 */
 	new->head = ((u8 *) new) + sizeof(*new);
 	new->tail = new->head + new_head_len;
+	new->short_head = new->tail + new_tail_len;
+	new->short_tail = new->short_head + new_short_head_len;
 	new->head_len = new_head_len;
 	new->tail_len = new_tail_len;
+	new->short_head_len = new_short_head_len;
+	new->short_tail_len = new_short_tail_len;
+
 	/* copy in optional mbssid_ies */
 	if (mbssid) {
-		u8 *pos = new->tail + new->tail_len;
+		u8 *pos = new->short_tail + new->short_tail_len;
 
 		new->mbssid_ies = (void *)pos;
 		pos += struct_size(new->mbssid_ies, elem, mbssid->cnt);
@@ -1170,6 +1191,12 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 	else
 		memcpy(new->head, old->head, new_head_len);
 
+	if (params->short_head)
+		memcpy(new->short_head, params->short_head, new_short_head_len);
+	else
+		if (old && old->short_head)
+			memcpy(new->short_head, old->short_head, new_short_head_len);
+
 	/* copy in optional tail */
 	if (params->tail)
 		memcpy(new->tail, params->tail, new_tail_len);
@@ -1177,6 +1204,13 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 		if (old)
 			memcpy(new->tail, old->tail, new_tail_len);
 
+	/* copy in optional short tail */
+	if (params->short_tail)
+		memcpy(new->short_tail, params->short_tail, new_short_tail_len);
+	else
+		if (old)
+			memcpy(new->short_tail, old->short_tail, new_short_tail_len);
+
 	err = ieee80211_set_probe_resp(sdata, params->probe_resp,
 				       params->probe_resp_len, csa, cca, link);
 	if (err < 0) {
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2e1d829c548a..212457672d7f 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -264,7 +264,9 @@ struct ieee80211_color_change_settings {
 
 struct beacon_data {
 	u8 *head, *tail;
+	u8 *short_head, *short_tail;
 	int head_len, tail_len;
+	int short_head_len, short_tail_len;
 	struct ieee80211_meshconf_ie *meshconf;
 	u16 cntdwn_counter_offsets[IEEE80211_MAX_CNTDWN_COUNTERS_NUM];
 	u8 cntdwn_current_counter;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index a12b32544a3a..963596772de4 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -5147,7 +5147,11 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,
 		return NULL;
 
 	skb_reserve(skb, local->tx_headroom);
-	skb_put_data(skb, beacon->head, beacon->head_len);
+
+	if (is_short)
+		skb_put_data(skb, beacon->short_head, beacon->short_head_len);
+	else
+		skb_put_data(skb, beacon->head, beacon->head_len);
 
 	ieee80211_beacon_add_tim(sdata, link, &ap->ps, skb, is_template);
 
@@ -5165,7 +5169,9 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,
 		csa_off_base = skb->len;
 	}
 
-	if (beacon->tail && !is_short)
+	if (beacon->short_tail && is_short)
+		skb_put_data(skb, beacon->short_tail, beacon->short_tail_len);
+	else if (beacon->tail && !is_short)
 		skb_put_data(skb, beacon->tail, beacon->tail_len);
 
 	if (ieee80211_beacon_protect(skb, local, sdata, link) < 0)
-- 
2.34.1

Re: [PATCH v3 1/4] wifi: cfg80211: Add short_beacon_tail/head/period

[Kieran Frewen]

On Wed, Nov 16, 2022 at 3:08 PM Gilad Itzkovitch
<gilad.itzkovitch@morsemicro.com> wrote:
>
> From: Kieran Frewen <kieran.frewen@morsemicro.com>
>
> Support variables to handle short beacon period and adding a
> separate tail/head for them.
>
> Signed-off-by: Kieran Frewen <kieran.frewen@morsemicro.com>
> Co-developed-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
> Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
> ---
>  include/net/cfg80211.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 11a370e64143..99f28ef2323f 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1179,8 +1179,13 @@ struct cfg80211_mbssid_elems {
>   *     or %NULL if not changed
>   * @tail: tail portion of beacon (after TIM IE)
>   *     or %NULL if not changed
> + * @short_head: head portion of short beacon or %NULL if not changed
> + * @short_tail: short tail portion of beacon (after TIM IE)
> +       or %NULL if not changed
>   * @head_len: length of @head
>   * @tail_len: length of @tail
> + * @short_head_len: length of @short_head
> + * @short_tail_len: length of @short_tail
>   * @beacon_ies: extra information element(s) to add into Beacon frames or %NULL
>   * @beacon_ies_len: length of beacon_ies in octets
>   * @proberesp_ies: extra information element(s) to add into Probe Response
> @@ -1208,6 +1213,7 @@ struct cfg80211_beacon_data {
>         unsigned int link_id;
>
>         const u8 *head, *tail;
> +       const u8 *short_head, *short_tail;
>         const u8 *beacon_ies;
>         const u8 *proberesp_ies;
>         const u8 *assocresp_ies;
> @@ -1218,6 +1224,7 @@ struct cfg80211_beacon_data {
>         s8 ftm_responder;
>
>         size_t head_len, tail_len;
> +       size_t short_head_len, short_tail_len;
>         size_t beacon_ies_len;
>         size_t proberesp_ies_len;
>         size_t assocresp_ies_len;
> @@ -1328,7 +1335,7 @@ struct cfg80211_ap_settings {
>
>         struct cfg80211_beacon_data beacon;
>
> -       int beacon_interval, dtim_period;
> +       int beacon_interval, dtim_period, short_beacon_period;
>         const u8 *ssid;
>         size_t ssid_len;
>         enum nl80211_hidden_ssid hidden_ssid;
>
> base-commit: 5c111ec204d15d1c7d00428b0afdda62ff118565
> --
> 2.34.1
>

Hi Johannes,

Just following up from our end to see what the status of this patchset
is? We were wondering if there was anything we can add in terms of
comments or further clarifications to help progress this.

Thanks and kind regards,
Kieran

Re: [PATCH v3 1/4] wifi: cfg80211: Add short_beacon_tail/head/period

[Johannes Berg]

Hi,

Hmm. This was on my list forever, and somehow I always went past it.
Sorry.

> Support variables to handle short beacon period and adding a
> separate tail/head for them.
> 

I don't think this really needs to be a separate patch? Should be
squashed with the nl80211 one.

johannes

Re: [PATCH v3 2/4] wifi: mac80211: S1G beacon/short beacon support

[Johannes Berg]

On Wed, 2022-11-16 at 15:06 +1300, Gilad Itzkovitch wrote:
> From: Kieran Frewen <kieran.frewen@morsemicro.com>
> 
> If configured, use the S1G short beacon format. The S1G short beacon
> format includes a limited set of information elements.

I think you can squash this with the fourth patch so you just have two
overall.

johannes

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

[Johannes Berg]

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

Re: [PATCH v3 4/4] wifi: mac80211: support setting S1G short beacon period, head and tail

[Johannes Berg]

On Wed, 2022-11-16 at 15:07 +1300, Gilad Itzkovitch wrote:
> 
> +	/* Short head is not a mandatory field */
> +	new_short_head_len = 0;
> +	if (params->short_head)
> +		new_short_head_len = params->short_head_len;
> +	else
> +		if (old && old->short_head)
> +			new_short_head_len = old->short_head_len;


"else if (...)"

would seem nicer and save the indentation level

> +	if (params->short_head)
> +		memcpy(new->short_head, params->short_head, new_short_head_len);
> +	else
> +		if (old && old->short_head)
> +			memcpy(new->short_head, old->short_head, new_short_head_len);

same here

> +
>  	/* copy in optional tail */
>  	if (params->tail)
>  		memcpy(new->tail, params->tail, new_tail_len);
> @@ -1177,6 +1204,13 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
>  		if (old)
>  			memcpy(new->tail, old->tail, new_tail_len);
>  
> +	/* copy in optional short tail */
> +	if (params->short_tail)
> +		memcpy(new->short_tail, params->short_tail, new_short_tail_len);
> +	else
> +		if (old)
> +			memcpy(new->short_tail, old->short_tail, new_short_tail_len);

and here.


And also here I'm a bit worried about hitting all these code paths when
it's not S1G?


johannes