[wireless-next 0/2] wifi: support S1G TIM encoding

[wireless-next 0/2] wifi: support S1G TIM encoding

[Lachlan Hodges]

An S1G PPDU formats the TIM differently compared to non-S1G PPDU's,
meaning in order for an S1G STA and AP to correctly coordinate
power save, the TIM PVB shall be encoded as per IEEE80211-2024
9.4.2.5.1 Figure 9-213.

An S1G PPDU uses the notion of "pages" where each page can represent
2048 AIDs, for a maximum theoretical limit of 8192 AIDs. However as
there is no page slicing support, we will be limiting the PVB to
a single page describing entire range of AIDs within the bitmap.
This is permitted as per IEEE80211-2024 9.4.2.5.1:

"If the Page Slice Number subfield is 31, then the entire page indicated
by the Page Index subfield value is encoded in the Partial Virtual Bitmap
field of the TIM elements with the same page index."

When this configuration is used, the PVB is broken down into a list of
encoded blocks. Each encoded block has a maximum length of 10 bytes and
can describe 64 AIDs. We currently only support block bitmap encoding.
Each encoded block contains a block control byte, which contains the
block offset. The block offset represents the block index the current
encoded block describes. Following the block control is the block
bitmap and between 0 and 8 subblocks. Each subblock is 1 byte and can
describe at most 8 AIDs, where the respective bit set in the block bitmap
indicates the presence of the subblock.

However, there are some further limitations. Firstly, the TIM element
has a maximum length of 255 bytes - and given that each encoded block
has a maximum length of 10 bytes, 32 * 10 = 320 this exceeds the maximum
TIM element length. As a comprimise until page slicing is implemented,
we set a maximum block count of 25 (25 * 10 + 1 + 1 + 1) meaning we can
describe 25 * 64 = 1600 AIDs. This fits within the existing mac80211 bitmap
and is more than enough for current implementations. In the future, when
page slicing support is added this will need to be modified.

Parsing support for the S1G PPDU PVB format is added on the STA side to
allow full power save support for S1G drivers.

Notes:

(1) I've run hwsim tests and (obviously) tested the S1G power save path,
    but the 2 hwsim power save tests only check multicast traffic and
    even then only 1 sta. So would be good for someone to confirm that this
    hasn't broken non-S1G tim encoding. Even though it's only code being
    shuffled around, that wouldn't be ideal :)

Trace Example:

Tag: Traffic Indication Map (TIM): DTIM 2 of 10 bitmap
    Tag Number: Traffic Indication Map (TIM) (5)
    Tag length: 6
    DTIM count: 2
    DTIM period: 10
    Bitmap Control: 0x3e
        .... ...0 = Traffic Indication: 0x0
        ..11 111. = Page Slice Number: 31
        00.. .... = Page Index: 0
    Partial Virtual Bitmap
        Encoded Block 0
            Block Control Byte: 0x00
            Block Bitmap
                Block Bitmap: 0x01, Subblock 1 present
                Subblock 0
                    .... ..1. = STA AID13:  0x1
		    .... .1.. = STA AID13:  0x2

Lachlan Hodges (2):
  wifi: mac80211: support encoding S1G TIM PVB
  wifi: mac80211: support parsing S1G TIM PVB

 drivers/net/wireless/ath/carl9170/rx.c        |   2 +-
 drivers/net/wireless/intersil/p54/txrx.c      |   2 +-
 .../net/wireless/ralink/rt2x00/rt2x00dev.c    |   2 +-
 drivers/net/wireless/realtek/rtlwifi/ps.c     |   2 +-
 include/linux/ieee80211.h                     | 110 ++++++++++-
 net/mac80211/mesh_ps.c                        |   2 +-
 net/mac80211/mlme.c                           |  15 +-
 net/mac80211/tx.c                             | 173 ++++++++++++++----
 8 files changed, 257 insertions(+), 51 deletions(-)

-- 
2.43.0

[wireless-next 1/2] wifi: mac80211: support encoding S1G TIM PVB

[Lachlan Hodges]

An S1G PPDU TIM PVB (Partial Virtual Bitmap) follows a different
format to non-S1G PPDU's. An S1G TIM PVB breaks down the contiguous
AID bitmap into encoded blocks and only sends an encoded block if
there is at least one AID with buffered unicast traffic.

An S1G PPDU TIM represents groups of AIDS with an array of encoded
blocks. Each encoded block represents 64 AIDs and is only present if
at least one AID within the group of 64 is present. Each encoded
block can contain 0 to 8 subblocks, where each subblock represents 8
AIDs. Similarly to the encoded blocks, sublocks are only added if there
is at least 1 AID set within that subblock. If a subblock is present,
the subblocks bit is set in the block bitmap which precedes the
sublock bitmap.

As page slicing is currently not supported by mac80211, we limit the
S1G max AID to 1600. This is due to the fact that the TIM element has
a maximum length of 255, such that we have DTIM count + DTIM period +
bitmap control = 3 bytes, leaving us with 252 bytes for encoded
blocks. Each encoded block has a maximum size of 10 bytes (assuming
every AID is set) leaving us with 25 * 10 + 3 = 253 bytes.

Correctly encode the S1G PPDU TIM PVB utilising the TIM bitmap.

Signed-off-by: Lachlan Hodges <lachlan.hodges@morsemicro.com>
---
 include/linux/ieee80211.h |  14 +++
 net/mac80211/tx.c         | 173 +++++++++++++++++++++++++++++++-------
 2 files changed, 156 insertions(+), 31 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index e5a2096e022e..2a73f9e4b0ac 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -220,6 +220,20 @@ static inline u16 ieee80211_sn_sub(u16 sn1, u16 sn2)
 #define IEEE80211_MAX_AID_S1G		8191
 #define IEEE80211_MAX_TIM_LEN		251
 #define IEEE80211_MAX_MESH_PEERINGS	63
+
+/*
+ * An S1G PPDU TIM PVB uses the notion of pages. Each page can reference
+ * 2048 AIDs, however since mac80211 does not support page slicing we
+ * are reusing the existing TIM bitmap, which supports up to 2008 AIDs.
+ * As the TIM element has a maximum length of 255 bytes, and each encoded
+ * block has a maximum length of 10 bytes at most we can support 25 blocks,
+ * as 1 + 1 + 1 + 25 * 10 = 253 bytes, leaving our maximum AID count for
+ * an S1G PPDU at 25 * 64 = 1600. If page slicing is introduced in the
+ * future, this will need to be modified.
+ */
+#define IEEE80211_MAX_AID_S1G_NO_PS	1600
+#define IEEE80211_MAX_S1G_TIM_BLOCKS	25
+
 /* Maximum size for the MA-UNITDATA primitive, 802.11 standard section
    6.2.1.1.2.
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 00671ae45b2f..26f45d32d4a1 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -4882,15 +4882,139 @@ void ieee80211_tx_pending(struct tasklet_struct *t)
 
 /* functions for drivers to get certain frames */
 
+static void ieee80211_beacon_add_tim_pvb(struct ps_data *ps,
+					 struct sk_buff *skb, u8 aid0, u8 *tim)
+{
+	int i, n1 = 0, n2;
+
+	/*
+	 * Find largest even number N1 so that bits numbered 1 through
+	 * (N1 x 8) - 1 in the bitmap are 0 and number N2 so that bits
+	 * (N2 + 1) x 8 through 2007 are 0.
+	 */
+	for (i = 0; i < IEEE80211_MAX_TIM_LEN; i++) {
+		if (ps->tim[i]) {
+			n1 = i & 0xfe;
+			break;
+		}
+	}
+	n2 = n1;
+	for (i = IEEE80211_MAX_TIM_LEN - 1; i >= n1; i--) {
+		if (ps->tim[i]) {
+			n2 = i;
+			break;
+		}
+	}
+
+	/* Bitmap control */
+	skb_put_u8(skb, n1 | aid0);
+	/* Part Virt Bitmap */
+	skb_put_data(skb, ps->tim + n1, n2 - n1 + 1);
+
+	tim[1] = n2 - n1 + 4;
+}
+
+/*
+ * Unlike the non-S1G TIM, which encodes the contiguous range n_first‥n_last,
+ * an S1G TIM transmits the bitmap as a sequence of encoded blocks. Each
+ * encoded block contains the following:
+ *  - one block-control byte,
+ *  - one block-bitmap byte,
+ *  - zero to eight sub-block bytes (one per set bit in the bitmap).
+ *
+ * Each encoded block covers 64 AIDs (8 * 8). mac80211 does not implement
+ * page slicing, so we treat the whole PVB as belonging to a single page
+ * (bitmap-control: page-slice 31, page-index 0) and support at most
+ * 1600 AIDs (25 blocks * 64). We do this to reuse the existing bitmap
+ * and allow for the worst case scenario where every single AID is set to
+ * fit within the TIM maximum length of 255. If page slicing is to be added
+ * a separate bitmap will need to be added to allow up to 8192 AIDs.
+ *
+ * Only encoding-mode 0 (block bitmap, non-inverse) is generated or
+ * recognised; other encoding modes are not supported.
+ */
+static void ieee80211_s1g_beacon_add_tim_pvb(struct ps_data *ps,
+					     struct sk_buff *skb, u8 aid0,
+					     u8 *tim)
+{
+	int blk;
+
+	/*
+	 * Emit a bitmap control block with a page slice number of 31 and a
+	 * page index of 0 which indicates as per IEEE80211-2024 9.4.2.5.1
+	 * that the entire page (2048 bits) indicated by the page index
+	 * is encoded in the partial virtual bitmap.
+	 */
+	skb_put_u8(skb, aid0 | (31 << 1));
+	tim[1]++;
+
+	/* Emit an encoded block for each non-zero sub-block */
+	for (blk = 0; blk < IEEE80211_MAX_S1G_TIM_BLOCKS; blk++) {
+		u8 blk_bmap = 0;
+		int sblk, subcnt = 0;
+
+		for (sblk = 0; sblk < 8; sblk++) {
+			int idx = blk * 8 + sblk;
+
+			if (idx >= IEEE80211_MAX_AID_S1G_NO_PS)
+				break;
+
+			/*
+			 * If the current subblock is non-zero, increase the
+			 * number of subblocks to emit for the current block.
+			 */
+			if (ps->tim[idx]) {
+				blk_bmap |= BIT(sblk);
+				subcnt++;
+			}
+		}
+
+		/* If the current block contains no non-zero sublocks */
+		if (!blk_bmap)
+			continue;
+
+		/*
+		 * Emit a block control byte for the current encoded block
+		 * with an encoding mode of block bitmap (0x0), not inverse
+		 * (0x0) and the current block offset (5 bits)
+		 */
+		skb_put_u8(skb, blk << 3);
+
+		/*
+		 * Emit the block bitmap for the current encoded block which
+		 * contains the present subblocks.
+		 */
+		skb_put_u8(skb, blk_bmap);
+
+		/* Emit the present subblocks */
+		for (sblk = 0; sblk < 8; sblk++) {
+			int idx = blk * 8 + sblk;
+
+			if (!(blk_bmap & BIT(sblk)))
+				continue;
+
+			skb_put_u8(skb, ps->tim[idx]);
+		}
+
+		/*
+		 * Increase the tim length by the current encoded block
+		 * length by block control + block bitmap + n_subblocks where
+		 * n_subblocks represents the number of subblock bytes to emit
+		 * for the current block.
+		 */
+		tim[1] += 1 + 1 + subcnt;
+	}
+}
+
 static void __ieee80211_beacon_add_tim(struct ieee80211_sub_if_data *sdata,
 				       struct ieee80211_link_data *link,
 				       struct ps_data *ps, struct sk_buff *skb,
 				       bool is_template)
 {
 	u8 *pos, *tim;
-	int aid0 = 0;
-	int i, have_bits = 0, n1, n2;
+	int aid0 = 0, have_bits = 0;
 	struct ieee80211_bss_conf *link_conf = link->conf;
+	bool s1g = ieee80211_get_link_sband(link)->band == NL80211_BAND_S1GHZ;
 
 	/* Generate bitmap for TIM only if there are any STAs in power save
 	 * mode. */
@@ -4906,9 +5030,10 @@ static void __ieee80211_beacon_add_tim(struct ieee80211_sub_if_data *sdata,
 			ps->dtim_count--;
 	}
 
-	tim = pos = skb_put(skb, 5);
+	pos = skb_put(skb, 4);
+	tim = pos;
 	*pos++ = WLAN_EID_TIM;
-	*pos++ = 3;
+	*pos++ = 2;
 	*pos++ = ps->dtim_count;
 	*pos++ = link_conf->dtim_period;
 
@@ -4918,34 +5043,20 @@ static void __ieee80211_beacon_add_tim(struct ieee80211_sub_if_data *sdata,
 	ps->dtim_bc_mc = aid0 == 1;
 
 	if (have_bits) {
-		/* Find largest even number N1 so that bits numbered 1 through
-		 * (N1 x 8) - 1 in the bitmap are 0 and number N2 so that bits
-		 * (N2 + 1) x 8 through 2007 are 0. */
-		n1 = 0;
-		for (i = 0; i < IEEE80211_MAX_TIM_LEN; i++) {
-			if (ps->tim[i]) {
-				n1 = i & 0xfe;
-				break;
-			}
-		}
-		n2 = n1;
-		for (i = IEEE80211_MAX_TIM_LEN - 1; i >= n1; i--) {
-			if (ps->tim[i]) {
-				n2 = i;
-				break;
-			}
-		}
-
-		/* Bitmap control */
-		*pos++ = n1 | aid0;
-		/* Part Virt Bitmap */
-		skb_put_data(skb, ps->tim + n1, n2 - n1 + 1);
-
-		tim[1] = n2 - n1 + 4;
+		if (s1g)
+			ieee80211_s1g_beacon_add_tim_pvb(ps, skb, aid0, tim);
+		else
+			ieee80211_beacon_add_tim_pvb(ps, skb, aid0, tim);
 	} else {
-		*pos++ = aid0; /* Bitmap control */
-
-		if (ieee80211_get_link_sband(link)->band != NL80211_BAND_S1GHZ) {
+		/*
+		 * If there is no buffered unicast traffic for an S1G
+		 * interface, we can exclude the bitmap control. This is in
+		 * contrast to other phy types as they do include the bitmap
+		 * control and pvb even when there is no buffered traffic.
+		 */
+		if (!s1g) {
+			/* Bitmap control */
+			skb_put_u8(skb, aid0);
 			tim[1] = 4;
 			/* Part Virt Bitmap */
 			skb_put_u8(skb, 0);
-- 
2.43.0

[wireless-next 2/2] wifi: mac80211: support parsing S1G TIM PVB

[Lachlan Hodges]

When parsing a TIM element from an S1G AP, ensure we correctly
parse based on the S1G PPDU TIM PVB format, allowing an S1G STA
to correctly enter/exit power save states to receive buffered
unicast traffic. Also make sure we don't allocate over the
mac80211 supported S1G PPDU AID count.

Signed-off-by: Lachlan Hodges <lachlan.hodges@morsemicro.com>
---
 drivers/net/wireless/ath/carl9170/rx.c        |  2 +-
 drivers/net/wireless/intersil/p54/txrx.c      |  2 +-
 .../net/wireless/ralink/rt2x00/rt2x00dev.c    |  2 +-
 drivers/net/wireless/realtek/rtlwifi/ps.c     |  2 +-
 include/linux/ieee80211.h                     | 96 +++++++++++++++++--
 net/mac80211/mesh_ps.c                        |  2 +-
 net/mac80211/mlme.c                           | 15 +--
 7 files changed, 101 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/rx.c b/drivers/net/wireless/ath/carl9170/rx.c
index 908c4c8b7f82..6833430130f4 100644
--- a/drivers/net/wireless/ath/carl9170/rx.c
+++ b/drivers/net/wireless/ath/carl9170/rx.c
@@ -555,7 +555,7 @@ static void carl9170_ps_beacon(struct ar9170 *ar, void *data, unsigned int len)
 	/* Check whenever the PHY can be turned off again. */
 
 	/* 1. What about buffered unicast traffic for our AID? */
-	cam = ieee80211_check_tim(tim_ie, tim_len, ar->common.curaid);
+	cam = ieee80211_check_tim(tim_ie, tim_len, ar->common.curaid, false);
 
 	/* 2. Maybe the AP wants to send multicast/broadcast data? */
 	cam |= !!(tim_ie->bitmap_ctrl & 0x01);
diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c
index 2deb1bb54f24..1294a1d6528e 100644
--- a/drivers/net/wireless/intersil/p54/txrx.c
+++ b/drivers/net/wireless/intersil/p54/txrx.c
@@ -317,7 +317,7 @@ static void p54_pspoll_workaround(struct p54_common *priv, struct sk_buff *skb)
 	tim_len = tim[1];
 	tim_ie = (struct ieee80211_tim_ie *) &tim[2];
 
-	new_psm = ieee80211_check_tim(tim_ie, tim_len, priv->aid);
+	new_psm = ieee80211_check_tim(tim_ie, tim_len, priv->aid, false);
 	if (new_psm != priv->powersave_override) {
 		priv->powersave_override = new_psm;
 		p54_set_ps(priv);
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index 432ddfac2c33..963c1e0af8b8 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -679,7 +679,7 @@ static void rt2x00lib_rxdone_check_ps(struct rt2x00_dev *rt2x00dev,
 	/* Check whenever the PHY can be turned off again. */
 
 	/* 1. What about buffered unicast traffic for our AID? */
-	cam = ieee80211_check_tim(tim_ie, tim_len, rt2x00dev->aid);
+	cam = ieee80211_check_tim(tim_ie, tim_len, rt2x00dev->aid, false);
 
 	/* 2. Maybe the AP wants to send multicast/broadcast data? */
 	cam |= (tim_ie->bitmap_ctrl & 0x01);
diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c b/drivers/net/wireless/realtek/rtlwifi/ps.c
index 6241e4fed4f6..bcab12c3b4c1 100644
--- a/drivers/net/wireless/realtek/rtlwifi/ps.c
+++ b/drivers/net/wireless/realtek/rtlwifi/ps.c
@@ -519,7 +519,7 @@ void rtl_swlps_beacon(struct ieee80211_hw *hw, void *data, unsigned int len)
 
 	/* 1. What about buffered unicast traffic for our AID? */
 	u_buffed = ieee80211_check_tim(tim_ie, tim_len,
-				       rtlpriv->mac80211.assoc_id);
+				       rtlpriv->mac80211.assoc_id, false);
 
 	/* 2. Maybe the AP wants to send multicast/broadcast data? */
 	m_buffed = tim_ie->bitmap_ctrl & 0x01;
diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 2a73f9e4b0ac..acf59ae631a7 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -234,6 +234,9 @@ static inline u16 ieee80211_sn_sub(u16 sn1, u16 sn2)
 #define IEEE80211_MAX_AID_S1G_NO_PS	1600
 #define IEEE80211_MAX_S1G_TIM_BLOCKS	25
 
+/* IEEE80211-2024 Block Control Encoding */
+#define IEEE80211_S1G_TIM_ENC_MODE_BLOCK	0x00
+
 /* Maximum size for the MA-UNITDATA primitive, 802.11 standard section
    6.2.1.1.2.
 
@@ -4771,15 +4774,8 @@ static inline unsigned long ieee80211_tu_to_usec(unsigned long tu)
 	return 1024 * tu;
 }
 
-/**
- * ieee80211_check_tim - check if AID bit is set in TIM
- * @tim: the TIM IE
- * @tim_len: length of the TIM IE
- * @aid: the AID to look for
- * Return: whether or not traffic is indicated in the TIM for the given AID
- */
-static inline bool ieee80211_check_tim(const struct ieee80211_tim_ie *tim,
-				       u8 tim_len, u16 aid)
+static inline bool __ieee80211_check_tim(const struct ieee80211_tim_ie *tim,
+					 u8 tim_len, u16 aid)
 {
 	u8 mask;
 	u8 index, indexn1, indexn2;
@@ -4802,6 +4798,88 @@ static inline bool ieee80211_check_tim(const struct ieee80211_tim_ie *tim,
 	return !!(tim->virtual_map[index] & mask);
 }
 
+/* See ieee80211_s1g_beacon_add_tim_pvb for implementation documentation. */
+static inline bool ieee80211_s1g_check_tim(const struct ieee80211_tim_ie *tim,
+					   u8 tim_len, u16 aid)
+{
+	u8 bit_idx = aid & 0x7;
+	u8 sub_idx = (aid >> 3) & 0x7;
+	u8 blk_idx_target = (aid >> 6) & 0x1f;
+	u8 blk_bmap = 0;
+	const u8 *ptr = tim->virtual_map;
+	const u8 *end = (const u8 *)tim + tim_len + 2;
+
+	/*
+	 * When an S1G AP has no buffered unicast traffic, bitmap control and
+	 * PVB are not present.
+	 */
+	if (tim_len < 3)
+		return false;
+
+	/*
+	 * Enumerate each encoded block, for which we need at least block
+	 * control and block bitmap.
+	 */
+	while (ptr + 2 <= end) {
+		u8 blk_ctrl = *ptr++;
+		u8 enc_mode = blk_ctrl & 0x3;
+		u8 blk_idx = blk_ctrl >> 3;
+
+		/* If we are past our target block index, exit */
+		if (blk_idx > blk_idx_target)
+			break;
+
+		/* mac80211 only supports block bitmap encoding */
+		if (enc_mode != IEEE80211_S1G_TIM_ENC_MODE_BLOCK)
+			break;
+
+		/*
+		 * If our target block doesn't exist within the block
+		 * that the current encoded block represents, move to the
+		 * next encoded block.
+		 */
+		blk_bmap = *ptr++;
+		if (blk_idx < blk_idx_target) {
+			ptr += hweight8(blk_bmap);
+			continue;
+		}
+
+		/* Ensure our subblock is present */
+		if (!(blk_bmap & BIT(sub_idx)))
+			break;
+
+		/*
+		 * Count the number of subblocks that appear before our target
+		 * subblock, increment ptr by number of set subblocks.
+		 */
+		if (sub_idx)
+			ptr += hweight8(blk_bmap & GENMASK(sub_idx - 1, 0));
+
+		if (ptr >= end)
+			break;
+
+		/* Check our AID is present in the subblock */
+		return *ptr & BIT(bit_idx);
+	}
+
+	return false;
+}
+
+/**
+ * ieee80211_check_tim - check if AID bit is set in TIM
+ * @tim: the TIM IE
+ * @tim_len: length of the TIM IE
+ * @aid: the AID to look for
+ * @s1g: whether the TIM is from an S1G PPDU
+ * Return: whether or not traffic is indicated in the TIM for the given AID
+ */
+static inline bool ieee80211_check_tim(const struct ieee80211_tim_ie *tim,
+				       u8 tim_len, u16 aid, bool s1g)
+{
+	return s1g ? ieee80211_s1g_check_tim(tim, tim_len, aid) :
+		     __ieee80211_check_tim(tim, tim_len, aid);
+}
+
 /**
  * ieee80211_get_tdls_action - get TDLS action code
  * @skb: the skb containing the frame, length will not be checked
diff --git a/net/mac80211/mesh_ps.c b/net/mac80211/mesh_ps.c
index 20e022a03933..ebab1f0a0138 100644
--- a/net/mac80211/mesh_ps.c
+++ b/net/mac80211/mesh_ps.c
@@ -586,7 +586,7 @@ void ieee80211_mps_frame_release(struct sta_info *sta,
 
 	if (sta->mesh->plink_state == NL80211_PLINK_ESTAB)
 		has_buffered = ieee80211_check_tim(elems->tim, elems->tim_len,
-						   sta->mesh->aid);
+						   sta->mesh->aid, false);
 
 	if (has_buffered)
 		mps_dbg(sta->sdata, "%pM indicates buffered frames\n",
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index b4b7ea52c65e..2f893fb6d188 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -6348,6 +6348,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 	};
 	u8 ap_mld_addr[ETH_ALEN] __aligned(2);
 	unsigned int link_id;
+	u16 max_aid = IEEE80211_MAX_AID;
 
 	lockdep_assert_wiphy(sdata->local->hw.wiphy);
 
@@ -6374,10 +6375,12 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 	reassoc = ieee80211_is_reassoc_resp(mgmt->frame_control);
 	capab_info = le16_to_cpu(mgmt->u.assoc_resp.capab_info);
 	status_code = le16_to_cpu(mgmt->u.assoc_resp.status_code);
-	if (assoc_data->s1g)
+	if (assoc_data->s1g) {
 		elem_start = mgmt->u.s1g_assoc_resp.variable;
-	else
+		max_aid = IEEE80211_MAX_AID_S1G_NO_PS;
+	} else {
 		elem_start = mgmt->u.assoc_resp.variable;
+	}
 
 	/*
 	 * Note: this may not be perfect, AP might misbehave - if
@@ -6401,8 +6404,6 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 
 	if (elems->aid_resp)
 		aid = le16_to_cpu(elems->aid_resp->aid);
-	else if (assoc_data->s1g)
-		aid = 0; /* TODO */
 	else
 		aid = le16_to_cpu(mgmt->u.assoc_resp.aid);
 
@@ -6447,7 +6448,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 		event.u.mlme.reason = status_code;
 		drv_event_callback(sdata->local, sdata, &event);
 	} else {
-		if (aid == 0 || aid > IEEE80211_MAX_AID) {
+		if (aid == 0 || aid > max_aid) {
 			sdata_info(sdata,
 				   "invalid AID value %d (out of range), turn off PS\n",
 				   aid);
@@ -6485,6 +6486,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 		}
 
 		sdata->vif.cfg.aid = aid;
+		sdata->vif.cfg.s1g = assoc_data->s1g;
 
 		if (!ieee80211_assoc_success(sdata, mgmt, elems,
 					     elem_start, elem_len)) {
@@ -7432,7 +7434,8 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_link_data *link,
 	ncrc = elems->crc;
 
 	if (ieee80211_hw_check(&local->hw, PS_NULLFUNC_STACK) &&
-	    ieee80211_check_tim(elems->tim, elems->tim_len, vif_cfg->aid)) {
+	    ieee80211_check_tim(elems->tim, elems->tim_len, vif_cfg->aid,
+				vif_cfg->s1g)) {
 		if (local->hw.conf.dynamic_ps_timeout > 0) {
 			if (local->hw.conf.flags & IEEE80211_CONF_PS) {
 				local->hw.conf.flags &= ~IEEE80211_CONF_PS;
-- 
2.43.0

Re: [wireless-next 0/2] wifi: support S1G TIM encoding

[Lachlan Hodges]

On Tue, Jul 22, 2025 at 05:16:38PM +1000, Lachlan Hodges wrote:
> (1) I've run hwsim tests and (obviously) tested the S1G power save path,
>     but the 2 hwsim power save tests only check multicast traffic and
>     even then only 1 sta. So would be good for someone to confirm that this
>     hasn't broken non-S1G tim encoding. Even though it's only code being
>     shuffled around, that wouldn't be ideal :)

Something I forgot to mention (my file didn't save ._.) is that I aim to get
some S1G hwsim tests up and running soon as S1G is almost fully functional
within mac80211 (1 maybe 2 more patchsets left) such that there is some
standardised testing utilising hwsim.

lachlan

Re: [wireless-next 0/2] wifi: support S1G TIM encoding

[Johannes Berg]

On Tue, 2025-07-22 at 17:22 +1000, Lachlan Hodges wrote:
> On Tue, Jul 22, 2025 at 05:16:38PM +1000, Lachlan Hodges wrote:
> > (1) I've run hwsim tests and (obviously) tested the S1G power save path,
> >     but the 2 hwsim power save tests only check multicast traffic and
> >     even then only 1 sta. So would be good for someone to confirm that this
> >     hasn't broken non-S1G tim encoding. Even though it's only code being
> >     shuffled around, that wouldn't be ideal :)
> 
> Something I forgot to mention (my file didn't save ._.) is that I aim to get
> some S1G hwsim tests up and running soon as S1G is almost fully functional
> within mac80211 (1 maybe 2 more patchsets left) such that there is some
> standardised testing utilising hwsim.

That's awesome :-)

A question on that topic: In your experience - since surely you also
work with real hardware - is it plausible that hardware has both S1G and
"regular" WiFi support (2.4/5/6 GHz)? Because hwsim does that and it's
caused some issues with the large number of channels and such before, so
I wonder if we shouldn't be more realistic and at least make the default
not be that, unless we ever expect real HW to exist that does all of
S1G/2.4/5/6 combined?

johannes

Re: [wireless-next 1/2] wifi: mac80211: support encoding S1G TIM PVB

[Johannes Berg]

On Tue, 2025-07-22 at 17:16 +1000, Lachlan Hodges wrote:
> An S1G PPDU TIM PVB (Partial Virtual Bitmap) follows a different
> format to non-S1G PPDU's. An S1G TIM PVB breaks down the contiguous
> AID bitmap into encoded blocks and only sends an encoded block if
> there is at least one AID with buffered unicast traffic.
> 
> An S1G PPDU TIM represents groups of AIDS with an array of encoded
> blocks. Each encoded block represents 64 AIDs and is only present if
> at least one AID within the group of 64 is present. Each encoded
> block can contain 0 to 8 subblocks, where each subblock represents 8
> AIDs. Similarly to the encoded blocks, sublocks are only added if there
> is at least 1 AID set within that subblock. If a subblock is present,
> the subblocks bit is set in the block bitmap which precedes the
> sublock bitmap.
> 
> As page slicing is currently not supported by mac80211, we limit the
> S1G max AID to 1600. This is due to the fact that the TIM element has
> a maximum length of 255, such that we have DTIM count + DTIM period +
> bitmap control = 3 bytes, leaving us with 252 bytes for encoded
> blocks. Each encoded block has a maximum size of 10 bytes (assuming
> every AID is set) leaving us with 25 * 10 + 3 = 253 bytes.

Looking at the spec, this seems like a bit of a simplification, since it
also allows for "Single AID" and "OLB" modes, where I think this only
covers "Block Bitmap" mode?

> Correctly encode the S1G PPDU TIM PVB utilising the TIM bitmap.

No argument there though, the transmitter doesn't _have_ to use the most
space-efficient encoding.

johannes

Re: [wireless-next 2/2] wifi: mac80211: support parsing S1G TIM PVB

[Johannes Berg]

On Tue, 2025-07-22 at 17:16 +1000, Lachlan Hodges wrote:
> When parsing a TIM element from an S1G AP, ensure we correctly
> parse based on the S1G PPDU TIM PVB format, allowing an S1G STA
> to correctly enter/exit power save states to receive buffered
> unicast traffic.

I think this isn't quite correct.

> Also make sure we don't allocate over the
> mac80211 supported S1G PPDU AID count.

But this specifically caught my eye - how can the client ensure it
doesn't allocate over, since it doesn't even allocate? First I thought
this really belongs into the AP side code (i.e. the other patch), but
you actually did something here:

> @@ -6374,10 +6375,12 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
>  	reassoc = ieee80211_is_reassoc_resp(mgmt->frame_control);
>  	capab_info = le16_to_cpu(mgmt->u.assoc_resp.capab_info);
>  	status_code = le16_to_cpu(mgmt->u.assoc_resp.status_code);
> -	if (assoc_data->s1g)
> +	if (assoc_data->s1g) {
>  		elem_start = mgmt->u.s1g_assoc_resp.variable;
> -	else
> +		max_aid = IEEE80211_MAX_AID_S1G_NO_PS;
> +	} else {
>  		elem_start = mgmt->u.assoc_resp.variable;
> +	}
>  
>  	/*
>  	 * Note: this may not be perfect, AP might misbehave - if
> @@ -6401,8 +6404,6 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
>  
>  	if (elems->aid_resp)
>  		aid = le16_to_cpu(elems->aid_resp->aid);
> -	else if (assoc_data->s1g)
> -		aid = 0; /* TODO */
>  	else
>  		aid = le16_to_cpu(mgmt->u.assoc_resp.aid);
>  
> @@ -6447,7 +6448,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
>  		event.u.mlme.reason = status_code;
>  		drv_event_callback(sdata->local, sdata, &event);
>  	} else {
> -		if (aid == 0 || aid > IEEE80211_MAX_AID) {
> +		if (aid == 0 || aid > max_aid) {
>  			sdata_info(sdata,
>  				   "invalid AID value %d (out of range), turn off PS\n",
>  				   aid);

Which ... seems questionable? At least in terms of message it's not
actually _invalid_ if it's out of this range, in theory S1G allows up to
8191.

And you also forgot to change the masking - S1G says 3 bits are
reserved, where we actually check the top two bits are 0b11 I think?

Also the parsing for S1G only parses a subset of the valid formats,
notably the format that mac80211 can generate. That seems questionable,
unless you expect there never to be another implementation (or the
mac80211 one to never be extended) and you can basically make up your
own standard?

And even if you _do_ get an AID that's <1600 there's still no guarantee
that the encoding will be in the bitmap format, if only two stations
have traffic the AP might well decide to use the "Single AID" mode?

So I'm overall a bit confused how all this is meant to work - even if
only partially - because AID<1600 is no guarantee that you can parse the
bitmap, so turning off powersave only for AID>=1600 will not really be
sufficient?


Which also means that

> +#define IEEE80211_MAX_AID_S1G_NO_PS	1600

really doesn't belong into ieee80211.h, if it should exist at all then
as part of mac80211 (but above I'm arguing it shouldn't.)

johannes

Re: [wireless-next 0/2] wifi: support S1G TIM encoding

[Johannes Berg]

On Tue, 2025-07-22 at 17:22 +1000, Lachlan Hodges wrote:
> 
> Something I forgot to mention (my file didn't save ._.) is that I aim to get
> some S1G hwsim tests up and running soon as S1G is almost fully functional
> within mac80211 (1 maybe 2 more patchsets left) such that there is some
> standardised testing utilising hwsim.

After reviewing the bit twiddling code here a bit I'd like to point out
that perhaps some testing in kunit might also be useful. Not saying you
_must_ do that, but I've found that even for development it helps, and
we could drop examples from Annex L into it too.

johannes

Re: [wireless-next 2/2] wifi: mac80211: support parsing S1G TIM PVB

[Lachlan Hodges]

On Tue, Jul 22, 2025 at 01:25:50PM +0200, Johannes Berg wrote:
> On Tue, 2025-07-22 at 17:16 +1000, Lachlan Hodges wrote:
> > When parsing a TIM element from an S1G AP, ensure we correctly
> > parse based on the S1G PPDU TIM PVB format, allowing an S1G STA
> > to correctly enter/exit power save states to receive buffered
> > unicast traffic.
> 
> I think this isn't quite correct.
> 
> > Also make sure we don't allocate over the
> > mac80211 supported S1G PPDU AID count.
> 
> But this specifically caught my eye - how can the client ensure it
> doesn't allocate over, since it doesn't even allocate? First I thought
> this really belongs into the AP side code (i.e. the other patch), but
> you actually did something here:
> 
> [...]
>
> Which ... seems questionable? At least in terms of message it's not
> actually _invalid_ if it's out of this range, in theory S1G allows up to
> 8191.

Yes, from a client side it is. See comments below.

> Also the parsing for S1G only parses a subset of the valid formats,
> notably the format that mac80211 can generate. That seems questionable,
> unless you expect there never to be another implementation (or the
> mac80211 one to never be extended) and you can basically make up your
> own standard?
> 
> And even if you _do_ get an AID that's <1600 there's still no guarantee
> that the encoding will be in the bitmap format, if only two stations
> have traffic the AP might well decide to use the "Single AID" mode?
> 
> So I'm overall a bit confused how all this is meant to work - even if
> only partially - because AID<1600 is no guarantee that you can parse the
> bitmap, so turning off powersave only for AID>=1600 will not really be
> sufficient?

This is something we discussed internally, and in retrospect mac80211
should be able to parse all types, even though it may only encode a
single type. So actually my reasoning for this is not very good as it
was essentially "nobody uses the other encoding modes" but in the future
who knows if that will remain true... 

One of the things I was concerned with was adding a new bitmap
specifically for S1G, but in actuality its probably more important
to be "correct" and we could probably just use an anonymous union.

After verifying with the standard and based on your feedback here is
what I propose:

For a first implementation, we still won't support page slicing - but
this is fine as it is an advertised capability. A new bitmap
should be added for S1G (as mentioned, probably a union of some sort
with the existing bitmap) that allows us to use the _actual_ S1G AID
count, this ensures correct interoperability into the future and is
safe due to page slicing being a discrete capabilitity. 

So as per figure 9-214, we can still use a page slice number of 31 which
states that the entire page indicated by the page index is encoded in
the PVB, but given we now have a correctly sized AID bitmap we would
correctly determine the page index. On the AP side, we would encode
using block bitmap but correctly indicate the page index. On the STA
side, we'd properly decode the PVB with the ability to decode all possible
formats. So this would consist of block bitmap, single AID, OLB and ADE
alongside their equivalent inverts.

One concern I have is that without page slicing, we are limited to a TIM
of size 255. A single page represents 2048 AIDs (page index = 2 bits),
using block bitmap encoding we can overflow given worst case scenario.
Now the easy answer is to probably encode until we hit the maximum
length, allow STAs to clear their bit after the beacon and repeat.
Though maybe this isn't the nicest? Not sure. Obviously this is an
extreme case and would (hopefully) never happen in the real wordl but
still needs to be considered. Internally, we default to block bitmap
encoding and this isn't modified dynamically, same goes for most
(if not all) vendor implementations - but as mentioned the client needs
the ability to decode these other formats.

Let me know your thoughts.

lachlan

Re: [wireless-next 0/2] wifi: support S1G TIM encoding

[Lachlan Hodges]

On Tue, Jul 22, 2025 at 01:35:10PM +0200, Johannes Berg wrote:
> On Tue, 2025-07-22 at 17:22 +1000, Lachlan Hodges wrote:
> > 
> > Something I forgot to mention (my file didn't save ._.) is that I aim to get
> > some S1G hwsim tests up and running soon as S1G is almost fully functional
> > within mac80211 (1 maybe 2 more patchsets left) such that there is some
> > standardised testing utilising hwsim.
> 
> After reviewing the bit twiddling code here a bit I'd like to point out
> that perhaps some testing in kunit might also be useful. Not saying you
> _must_ do that, but I've found that even for development it helps, and
> we could drop examples from Annex L into it too.

Sure Ill look into it. Especially now if we are adding a variety of
encoding types.

Re: [wireless-next 2/2] wifi: mac80211: support parsing S1G TIM PVB

[Johannes Berg]

Hi Lachlan,

First: I missed the whole page slicing with its extra element and the
capability when I read through it earlier. But then of course we can
simply decide not to implement that on either side, i.e. client doesn't
support it, and AP just doesn't take advantage of it (and thus is
limited to fewer STAs even if some of them implement page slicing.)

(So we have dot11PageSlicingImplemented = false and
dot11PageSlicingActivated = false as well.)


> > Which ... seems questionable? At least in terms of message it's not
> > actually _invalid_ if it's out of this range, in theory S1G allows up to
> > 8191.
> 
> Yes, from a client side it is. See comments below.

Maybe I'm confused, but what do you mean by invalid from the client
side"? I've been looking now and didn't find any restrictions on the AID
for STAs that e.g. don't implement page slicing, though of course an AP
might want to e.g. reserve lower AIDs for them and always use higher
AIDs for STAs with page slicing even if lower AIDs are free.


> > Also the parsing for S1G only parses a subset of the valid formats,
> > notably the format that mac80211 can generate. That seems questionable,
> > unless you expect there never to be another implementation (or the
> > mac80211 one to never be extended) and you can basically make up your
> > own standard?
> > 
> > And even if you _do_ get an AID that's <1600 there's still no guarantee
> > that the encoding will be in the bitmap format, if only two stations
> > have traffic the AP might well decide to use the "Single AID" mode?
> > 
> > So I'm overall a bit confused how all this is meant to work - even if
> > only partially - because AID<1600 is no guarantee that you can parse the
> > bitmap, so turning off powersave only for AID>=1600 will not really be
> > sufficient?
> 
> This is something we discussed internally, and in retrospect mac80211
> should be able to parse all types, even though it may only encode a
> single type. So actually my reasoning for this is not very good as it
> was essentially "nobody uses the other encoding modes" but in the future
> who knows if that will remain true... 

Yeah OK, that's fair, I'm really not going to push you too hard push
into "must implement the spec faithfully" territory, and you know better
than me what might be deployed or not matter, and lifetime of systems
being built now possibly factors into it etc.

Mostly I'm asking because I didn't really understand what it was trying
to do, so even clarifying that only part of it is implemented might be
reasonable.

> After verifying with the standard and based on your feedback here is
> what I propose:
> 
> For a first implementation, we still won't support page slicing - but
> this is fine as it is an advertised capability.

Right.

> A new bitmap
> should be added for S1G (as mentioned, probably a union of some sort
> with the existing bitmap) that allows us to use the _actual_ S1G AID
> count, this ensures correct interoperability into the future and is
> safe due to page slicing being a discrete capabilitity. 

OK I don't even follow here ... I feel confused between the AP side and
non-AP STA side now.

If you talk about a union, do you literally mean in a struct or
something? I'm not sure that really matters for either side? The AP side
has the ps->tim bitmap, but that could even remain as is, even if it
limits the # of STAs to 2008 right now. That's not a big deal, it just
limits the AIDs to that, but that's an implementation choice. It just
won't be able to have more STAs connected, but that's OK?

(In principle, we don't even really _need_ the ps->tim bitmap as a
bitmap, we could iterate the stations instead or something, but that's
more expensive and so the ps->tim bitmap is an optimisation, at least I
see it that way.)

Obviously the AP side then for S1G can't just put the partial virtual
bitmap from the ps->tim bitmap into the frame, but that's ultimately
just a question of bit twiddling while building the frame too. So not
sure where a union is needed for the AP side :)


For the client side, similarly, all it has to do is be able to walk
through the element (elements, if later implementing page slicing), and
be able to identify whether or not its own bit is set, so it never
really needs a holistic view of the bitmap.

> So as per figure 9-214, we can still use a page slice number of 31 which
> states that the entire page indicated by the page index is encoded in
> the PVB, but given we now have a correctly sized AID bitmap we would
> correctly determine the page index. On the AP side, we would encode
> using block bitmap but correctly indicate the page index. On the STA
> side, we'd properly decode the PVB with the ability to decode all possible
> formats. So this would consist of block bitmap, single AID, OLB and ADE
> alongside their equivalent inverts.

Yeah, that seems right.

(And kunit tests for this parsing would probably be a good idea :) )

> One concern I have is that without page slicing, we are limited to a TIM
> of size 255. A single page represents 2048 AIDs (page index = 2 bits),
> using block bitmap encoding we can overflow given worst case scenario.
> Now the easy answer is to probably encode until we hit the maximum
> length, allow STAs to clear their bit after the beacon and repeat.
> Though maybe this isn't the nicest? Not sure. Obviously this is an
> extreme case and would (hopefully) never happen in the real wordl but
> still needs to be considered. Internally, we default to block bitmap
> encoding and this isn't modified dynamically, same goes for most
> (if not all) vendor implementations - but as mentioned the client needs
> the ability to decode these other formats.

If I'm reading the spec correctly, then the worst case for only encoding
with block bitmaps would be having a large number of scattered AIDs
indicate presence, which would require

 - DTIM count [1 octet]
 - DTIM period [1 octet]
 - Bitmap Control [1 octet]
    - B0 (depends on traffic)
    - B1-B5: 31 (special page slice)
    - B6-B7: 0b00 (page index 0)
 - followed by a number B of blocks, numbered b=0..B-1, each:
    - Block Control [1 octet]
       - encoding mode: block bitmap
       - inverse: 0
       - block offset: b * 64
    - Block Bitmap: 0xFF [1 octet, assuming worst case]
    - subblocks [8 octets]

so out of 255 octets, 3 are overhead, leaving 252 = 25*10+2 for up to 25
blocks, for up to 64*25=1600 AIDs? I think? Which matches your 1600, so
whew, maybe I did this right ;-)

From that I'd argue the AP side should limit itself to AIDs 1..1599, I
guess? Or actually maybe 64..1663 and use the block offset 1 higher, but
probably not worth the added work just for 1 more AID.

Once you get to/above 1600 you have a trade-off to make, you cannot
indicate the absolute worst case without paging any more. You could hope
that STAs will retrieve their data and then you get free blocks, some
Block Bitmaps won't need to be 0xFF, and then you have more space, even
without resorting to other encodings.

Overall it's also an interesting optimisation problem how to best encode
this ... if you have clustered bits then a block bitmap is better, if
there are sparse bits then Single AID could be better. I can't easily
discern the cases right now where OLB or ADE would be better.

But I think for an initial implementation you could just leave that
aside, limit AIDs to <1600, implement only block encoding and accept
that it's just not optimal in many cases, but at least you won't have to
worry about not being able to include some data?

johannes

Re: [wireless-next 0/2] wifi: support S1G TIM encoding

[Arien Judge]

On Tue, Jul 22, 2025 at 09:34:30AM +0200, Johannes Berg wrote:
> A question on that topic: In your experience - since surely you also
> work with real hardware - is it plausible that hardware has both S1G and
> "regular" WiFi support (2.4/5/6 GHz)? Because hwsim does that and it's
> caused some issues with the large number of channels and such before, so
> I wonder if we shouldn't be more realistic and at least make the default
> not be that, unless we ever expect real HW to exist that does all of
> S1G/2.4/5/6 combined?

Chiming in here. I think it is plausible that a single chip with both S1G
and at least one other radio type would exist in the future. This probably
won't happen for some time though; much longer for all radios combined! 

Cheers,
Arien

Re: [wireless-next 2/2] wifi: mac80211: support parsing S1G TIM PVB

[Lachlan Hodges]

On Tue, Jul 22, 2025 at 03:55:19PM +0200, Johannes Berg wrote:
> Hi Lachlan,
> 
> First: I missed the whole page slicing with its extra element and the
> capability when I read through it earlier. But then of course we can
> simply decide not to implement that on either side, i.e. client doesn't
> support it, and AP just doesn't take advantage of it (and thus is
> limited to fewer STAs even if some of them implement page slicing.)
> 
> (So we have dot11PageSlicingImplemented = false and
> dot11PageSlicingActivated = false as well.)

Hi Johannes, yes that would be correct. For an AP advertising page
slicing support, we could validate that within ieee80211_alloc_hw() and
reject those trying to do so? I see there is _some_ validation for
various HT configuration so this may make sense?
 
> > > Which ... seems questionable? At least in terms of message it's not
> > > actually _invalid_ if it's out of this range, in theory S1G allows up to
> > > 8191.
> > 
> > Yes, from a client side it is. See comments below.
> 
> Maybe I'm confused, but what do you mean by invalid from the client
> side"? I've been looking now and didn't find any restrictions on the AID
> for STAs that e.g. don't implement page slicing, though of course an AP
> might want to e.g. reserve lower AIDs for them and always use higher
> AIDs for STAs with page slicing even if lower AIDs are free.

Sorry, I could have worded this better. I was talking from the
perspective of the implementation of this patchset - where we strictly
limit the number of AIDs based on limitations of the TIM length in worst
case scenarios (where that 1600 number came from). From the standards
perspective the limit is 8192 - regardless of whether page slicing is active.  
 
> > > Also the parsing for S1G only parses a subset of the valid formats,
> > > notably the format that mac80211 can generate. That seems questionable,
> > > unless you expect there never to be another implementation (or the
> > > mac80211 one to never be extended) and you can basically make up your
> > > own standard?
> > > 
> > > And even if you _do_ get an AID that's <1600 there's still no guarantee
> > > that the encoding will be in the bitmap format, if only two stations
> > > have traffic the AP might well decide to use the "Single AID" mode?
> > > 
> > > So I'm overall a bit confused how all this is meant to work - even if
> > > only partially - because AID<1600 is no guarantee that you can parse the
> > > bitmap, so turning off powersave only for AID>=1600 will not really be
> > > sufficient?
> > 
> > This is something we discussed internally, and in retrospect mac80211
> > should be able to parse all types, even though it may only encode a
> > single type. So actually my reasoning for this is not very good as it
> > was essentially "nobody uses the other encoding modes" but in the future
> > who knows if that will remain true... 
> 
> Yeah OK, that's fair, I'm really not going to push you too hard push
> into "must implement the spec faithfully" territory, and you know better
> than me what might be deployed or not matter, and lifetime of systems
> being built now possibly factors into it etc.
> 
> Mostly I'm asking because I didn't really understand what it was trying
> to do, so even clarifying that only part of it is implemented might be
> reasonable.

So after some discussion internally, we agree that even if mac80211 only
supports a single encoding mode, it should be able to decode all
formats. This ensures complete interoperability on the STA side and on
the AP side well the AP dictates it so shouldn't be any issues there.
Obviously the matter of optimisation - but for an initial implementation
block bitmap should suffice :) A little bit more work upfront but
probably worth it in the long run - and unit tests shall help.

> > A new bitmap
> > should be added for S1G (as mentioned, probably a union of some sort
> > with the existing bitmap) that allows us to use the _actual_ S1G AID
> > count, this ensures correct interoperability into the future and is
> > safe due to page slicing being a discrete capabilitity. 
> 
> OK I don't even follow here ... I feel confused between the AP side and
> non-AP STA side now.
> 
> If you talk about a union, do you literally mean in a struct or
> something? I'm not sure that really matters for either side? The AP side
> has the ps->tim bitmap, but that could even remain as is, even if it
> limits the # of STAs to 2008 right now. That's not a big deal, it just
> limits the AIDs to that, but that's an implementation choice. It just
> won't be able to have more STAs connected, but that's OK?
> 
> (In principle, we don't even really _need_ the ps->tim bitmap as a
> bitmap, we could iterate the stations instead or something, but that's
> more expensive and so the ps->tim bitmap is an optimisation, at least I
> see it that way.)
> 
> Obviously the AP side then for S1G can't just put the partial virtual
> bitmap from the ps->tim bitmap into the frame, but that's ultimately
> just a question of bit twiddling while building the frame too. So not
> sure where a union is needed for the AP side :)

Sorry I did mean literal C union i.e:

union {
	u8 tim[IEEE80211_MAX_AID];
	u8 s1g_tim[IEEE80211_S1G_MAX_AID];
};

of some sorts. Realistically however, nobody right now can get over 2008
STAs associated, so I have no issue (and would be easier) to simply use
the existing AID count for S1G (basically, remove all that AID
validation stuff I added.. plus wasn't even done correct anyway). With
this in mind however, we need to account for the TIM length which you
alluded to below. Ill discuss that below, but I think from our side we
have no issues limiting the AID count to 2008 or potentially lower and
as you state, this can be an implementation choice.
 
> For the client side, similarly, all it has to do is be able to walk
> through the element (elements, if later implementing page slicing), and
> be able to identify whether or not its own bit is set, so it never
> really needs a holistic view of the bitmap.

Correct.

> > So as per figure 9-214, we can still use a page slice number of 31 which
> > states that the entire page indicated by the page index is encoded in
> > the PVB, but given we now have a correctly sized AID bitmap we would
> > correctly determine the page index. On the AP side, we would encode
> > using block bitmap but correctly indicate the page index. On the STA
> > side, we'd properly decode the PVB with the ability to decode all possible
> > formats. So this would consist of block bitmap, single AID, OLB and ADE
> > alongside their equivalent inverts.
> 
> Yeah, that seems right.
> 
> (And kunit tests for this parsing would probably be a good idea :) )

I assume when I implement these kunit tests they will simply be apart of
the patchset? i.e go through the wireless-next tree? But yes as you
alluded to I think adding kunit tests with all samples for Annex L would
be beneficial so Ill ensure to get that included with the next patchset.

> > One concern I have is that without page slicing, we are limited to a TIM
> > of size 255. A single page represents 2048 AIDs (page index = 2 bits),
> > using block bitmap encoding we can overflow given worst case scenario.
> > Now the easy answer is to probably encode until we hit the maximum
> > length, allow STAs to clear their bit after the beacon and repeat.
> > Though maybe this isn't the nicest? Not sure. Obviously this is an
> > extreme case and would (hopefully) never happen in the real wordl but
> > still needs to be considered. Internally, we default to block bitmap
> > encoding and this isn't modified dynamically, same goes for most
> > (if not all) vendor implementations - but as mentioned the client needs
> > the ability to decode these other formats.
> 
> If I'm reading the spec correctly, then the worst case for only encoding
> with block bitmaps would be having a large number of scattered AIDs
> indicate presence, which would require
> 
>  - DTIM count [1 octet]
>  - DTIM period [1 octet]
>  - Bitmap Control [1 octet]
>     - B0 (depends on traffic)
>     - B1-B5: 31 (special page slice)
>     - B6-B7: 0b00 (page index 0)
>  - followed by a number B of blocks, numbered b=0..B-1, each:
>     - Block Control [1 octet]
>        - encoding mode: block bitmap
>        - inverse: 0
>        - block offset: b * 64
>     - Block Bitmap: 0xFF [1 octet, assuming worst case]
>     - subblocks [8 octets]
> 
> so out of 255 octets, 3 are overhead, leaving 252 = 25*10+2 for up to 25
> blocks, for up to 64*25=1600 AIDs? I think? Which matches your 1600, so
> whew, maybe I did this right ;-)

:D
 
> From that I'd argue the AP side should limit itself to AIDs 1..1599, I
> guess? Or actually maybe 64..1663 and use the block offset 1 higher, but
> probably not worth the added work just for 1 more AID.
> 
> Once you get to/above 1600 you have a trade-off to make, you cannot
> indicate the absolute worst case without paging any more. You could hope
> that STAs will retrieve their data and then you get free blocks, some
> Block Bitmaps won't need to be 0xFF, and then you have more space, even
> without resorting to other encodings.

Correct. The choice I decided on was simply limiting the the AID count
to 1600 (albeit, I think incorrectly given I did it on the STA side).
Thinking about this more though, if we keep the existing max AID count
of 2008, which would leave all validation the same (always a good thing :))  
is we could then simply stop adding blocks _if_ we are about to exceed
the maximum length, let those STAs clear their bit and on the next
beacon proceed? That way you'd have the block offsets increase and those
earlier AIDs get cleared and so on.

Obviously we are discussing a case that should _never_ happen, but still
need to account for it. I am leaning more to the case of utilising the
max AID count of 2008 and simply preventing TIM length overflow (this is
actually what I originally did) but you may see some issues with that or
not. Let me know.

> Overall it's also an interesting optimisation problem how to best encode
> this ... if you have clustered bits then a block bitmap is better, if
> there are sparse bits then Single AID could be better. I can't easily
> discern the cases right now where OLB or ADE would be better.
> 
> But I think for an initial implementation you could just leave that
> aside, limit AIDs to <1600, implement only block encoding and accept
> that it's just not optimal in many cases, but at least you won't have to
> worry about not being able to include some data?

Ha, now my mind is changed >:). It seems the error from my patch (and
probably a source of confusion for you) is that I was only validating
the maximum AID on the client side, whereas (and, do correct me if im
wrong) on the AP side the AID is passed down from hostap via
NL80211_ATTR_PEER_AID, which is then validated using the
NLA_POLICY_RANGE where the max is IEEE80211_MAX_AID. So if we were to
limit the AID to 1600 for S1G, we'd need some way to validate it within
cfg80211. Don't think this would be too hard, since we can confirm we
have an S1G station via the presence of the S1G capabilities within
nl80211_set_station_tdls():

...
	if (info->attrs[NL80211_ATTR_S1G_CAPABILITY])
		params->link_sta_params.s1g_capa =
			nla_data(info->attrs[NL80211_ATTR_S1G_CAPABILITY]);
...

where we could maybe perform some extra validation on the AID? I know
its maybe not the prettiest since we aren't technically using the
explicit nl80211 intrinsic validation - but I would say it gets the job
done.

I think what I'll do from here, is wait for your response to my
comments, mainly on the maximum AID count and any other comments you may 
have, and work on v2 which would roughly consist of the following:

1. On the AP side, we simply support block bitmap encoding. Patch 1 
   would more or less be the same (with the addition potentially of
   new AID validation) though I'd add some details in there and
   change some comments etc.
2. Support decoding the other encoding modes on the STA side. I think
   while a bit more work upfront, worth it in the long run.
3. Add in some kunit tests to tests both the encoding of block bitmap
   and also the decoding of all the formats from Annex L.
4. Fix up the AID stuff depending on what you think, If we go with the
   hard limit of 1600, ensure on the AP side we perform that validation.

Let me know if I've missed anything.

Whew. Thought I became a software engineer to write code, not essays :)
(though Im guessing you are very used to that...). Detailed response is
much appreciated, and I think saves us both time from having to submit
and review many patchsets.

lachlan

Re: [wireless-next 2/2] wifi: mac80211: support parsing S1G TIM PVB

[Johannes Berg]

Hi Lachlan,

On Wed, 2025-07-23 at 12:29 +1000, Lachlan Hodges wrote:
> Hi Johannes, yes that would be correct. For an AP advertising page
> slicing support, we could validate that within ieee80211_alloc_hw() and
> reject those trying to do so? I see there is _some_ validation for
> various HT configuration so this may make sense?

I'm not sure it matters that much one way or the other. We do that in
some cases, but also not in others, and I think someone who implements a
driver really needs to be aware of what's happening?

Up to you, add whatever you think is unlikely to be immediately obvious
perhaps? Off the top of my head I don't even know where/how the feature
advertisement works.

> So after some discussion internally, we agree that even if mac80211 only
> supports a single encoding mode, it should be able to decode all
> formats. This ensures complete interoperability on the STA side and on
> the AP side well the AP dictates it so shouldn't be any issues there.

Right. Makes sense.

> > Obviously the AP side then for S1G can't just put the partial virtual
> > bitmap from the ps->tim bitmap into the frame, but that's ultimately
> > just a question of bit twiddling while building the frame too. So not
> > sure where a union is needed for the AP side :)
> 
> Sorry I did mean literal C union i.e:
> 
> union {
> 	u8 tim[IEEE80211_MAX_AID];
> 	u8 s1g_tim[IEEE80211_S1G_MAX_AID];
> };
> 
> of some sorts.

Oh, OK. I always just thought of the tim bitmap as just a bitmap,
regardless of its size, but OK, understood.

> Realistically however, nobody right now can get over 2008
> STAs associated, so I have no issue (and would be easier) to simply use
> the existing AID count for S1G (basically, remove all that AID
> validation stuff I added.. plus wasn't even done correct anyway). With
> this in mind however, we need to account for the TIM length which you
> alluded to below. Ill discuss that below, but I think from our side we
> have no issues limiting the AID count to 2008 or potentially lower and
> as you state, this can be an implementation choice.

Right.

> > (And kunit tests for this parsing would probably be a good idea :) )
> 
> I assume when I implement these kunit tests they will simply be apart of
> the patchset? i.e go through the wireless-next tree?

Yes, they just go to net/mac80211/tests/. You can put something in the
same patch or if you feel that gets too big into a separate patch in the
series, I don't really care too much.

> But yes as you
> alluded to I think adding kunit tests with all samples for Annex L would
> be beneficial so Ill ensure to get that included with the next patchset.

Thanks much!

> > Once you get to/above 1600 you have a trade-off to make, you cannot
> > indicate the absolute worst case without paging any more. You could hope
> > that STAs will retrieve their data and then you get free blocks, some
> > Block Bitmaps won't need to be 0xFF, and then you have more space, even
> > without resorting to other encodings.
> 
> Correct. The choice I decided on was simply limiting the the AID count
> to 1600 (albeit, I think incorrectly given I did it on the STA side).

Yeah I understand now that was in some way incorrect, before I was just
confused :-)

> Thinking about this more though, if we keep the existing max AID count
> of 2008, which would leave all validation the same (always a good thing :))  
> is we could then simply stop adding blocks _if_ we are about to exceed
> the maximum length, let those STAs clear their bit and on the next
> beacon proceed? That way you'd have the block offsets increase and those
> earlier AIDs get cleared and so on.

Right, you can of course easily remove zeroes from all the blocks and
not set the bit in the block bitmap when the whole subblock is 0, and
also exclude the entire block when the whole 64 bits are 0.

That way, only a pathological case of having at least one out of every 8
stations have pending traffic will ever drop some information at the
end.

> Obviously we are discussing a case that should _never_ happen, but still
> need to account for it. I am leaning more to the case of utilising the
> max AID count of 2008 and simply preventing TIM length overflow (this is
> actually what I originally did) but you may see some issues with that or
> not. Let me know.

Sure, it's a bit of a trade-off in the encoding code, making sure it
cannot run over the element length vs. knowing a priori that it cannot.
Clearly it's a corner case, so it doesn't really matter.

> > But I think for an initial implementation you could just leave that
> > aside, limit AIDs to <1600, implement only block encoding and accept
> > that it's just not optimal in many cases, but at least you won't have to
> > worry about not being able to include some data?
> 
> Ha, now my mind is changed >:).

Hah, well, I don't really think it's an issue either way.

> It seems the error from my patch (and
> probably a source of confusion for you) is that I was only validating
> the maximum AID on the client side,

Yeah, that seemed to serve no practical purpose since even with an AID
<1600 the AP could still encode it in any kind of block, and so I just
really got confused about it.

> whereas (and, do correct me if im
> wrong) on the AP side the AID is passed down from hostap via
> NL80211_ATTR_PEER_AID, which is then validated using the
> NLA_POLICY_RANGE where the max is IEEE80211_MAX_AID. So if we were to
> limit the AID to 1600 for S1G, we'd need some way to validate it within
> cfg80211. Don't think this would be too hard, since we can confirm we
> have an S1G station via the presence of the S1G capabilities within
> nl80211_set_station_tdls():
> 
> ...
> 	if (info->attrs[NL80211_ATTR_S1G_CAPABILITY])
> 		params->link_sta_params.s1g_capa =
> 			nla_data(info->attrs[NL80211_ATTR_S1G_CAPABILITY]);
> ...
> 
> where we could maybe perform some extra validation on the AID? I know
> its maybe not the prettiest since we aren't technically using the
> explicit nl80211 intrinsic validation - but I would say it gets the job
> done.

Ohh! Now I see where you're coming from, I was completely handwaving
away the _practicalities_ of checking the limit in my head.

I mean, sure, we currently use a policy ... Using the S1G_CAPABILITY
attribute wouldn't work if hostapd isn't including it at that time, but
mac80211 can always also do the validation. Worst case we can even
expose a new max-AID attribute, per interface type.

If we wanted to extend it later to up to 8191 then we'd need to remove
the IEEE80211_MAX_AID policy anyway since it'd be limiting to much less.

So I don't really see this as too much an issue, I'd probably just have
added a comment in nl80211.h that it's practically 1600 for S1G for now,
and added validation in mac80211. Then if _later_ we extend it, we can
add a feature flag/feature attribute for "now we support bigger AIDs for
S1G".


> I think what I'll do from here, is wait for your response to my
> comments, mainly on the maximum AID count and any other comments you may 
> have, and work on v2 which would roughly consist of the following:
> 
> 1. On the AP side, we simply support block bitmap encoding. Patch 1 
>    would more or less be the same (with the addition potentially of
>    new AID validation) though I'd add some details in there and
>    change some comments etc.

Sure. I write in a comment on the other patch that
IEEE80211_MAX_AID_S1G_NO_PS should probably not be in ieee80211.h, but I
guess with the changes to validation etc. you'd renamed that anyway and
put it in a different place. Or remove it.

> 2. Support decoding the other encoding modes on the STA side. I think
>    while a bit more work upfront, worth it in the long run.
> 3. Add in some kunit tests to tests both the encoding of block bitmap
>    and also the decoding of all the formats from Annex L.
> 4. Fix up the AID stuff depending on what you think, If we go with the
>    hard limit of 1600, ensure on the AP side we perform that validation.

I really don't mind either way. Like I said above, it's a bit of a
question of complexity in the AP encoding implementation. Maybe just
start updating the AP encoding implementation and see how hard the TIM
element length validation would be to add to it, vs. a priori knowledge
that it'll always fit?

With all this newfound knowledge I'll take another look at the patches,
but won't send more email if I don't see anything else :)

> Whew. Thought I became a software engineer to write code, not essays :)
> (though Im guessing you are very used to that...). Detailed response is
> much appreciated, and I think saves us both time from having to submit
> and review many patchsets.

:-D

johannes

Re: [wireless-next 1/2] wifi: mac80211: support encoding S1G TIM PVB

[Johannes Berg]

On Tue, 2025-07-22 at 17:16 +1000, Lachlan Hodges wrote:

> +/*
> + * An S1G PPDU TIM PVB uses the notion of pages. Each page can reference
> + * 2048 AIDs, however since mac80211 does not support page slicing we
> + * are reusing the existing TIM bitmap, which supports up to 2008 AIDs.
> + * As the TIM element has a maximum length of 255 bytes, and each encoded
> + * block has a maximum length of 10 bytes at most we can support 25 blocks,
> + * as 1 + 1 + 1 + 25 * 10 = 253 bytes, leaving our maximum AID count for
> + * an S1G PPDU at 25 * 64 = 1600. If page slicing is introduced in the
> + * future, this will need to be modified.
> + */
> +#define IEEE80211_MAX_AID_S1G_NO_PS	1600
> +#define IEEE80211_MAX_S1G_TIM_BLOCKS	25

Come to think of it, neither of these really makes sense in the
ieee80211.h header file since they're implementation related limits due
to the encoding. And IEEE80211_MAX_S1G_TIM_BLOCKS is questionable too
(see below), spec wise the limit would maybe be 32 (you cannot encode
higher than block 0..31 in the 5 bit block index.)


> +static void ieee80211_beacon_add_tim_pvb(struct ps_data *ps,
> +					 struct sk_buff *skb, u8 aid0, u8 *tim)

Maybe use 'struct element' for the tim pointer? that way tim[1] = ...
becomes tim->datalen = ... which seems easier to understand.

I know the code didn't use that (it predates the existence of 'struct
element') but it could also originally put the WLAN_EID_TIM that way,
for example.

And maybe aid0 could be 'bool mcast_traffic' or 'bool aid0_traffic' (if
mcast is too specific, not sure what might get encoded there now/s1g.)

(mostly I'm thinking it should be bool, 'bool aid0' is also fine)

> +static void ieee80211_s1g_beacon_add_tim_pvb(struct ps_data *ps,
> +					     struct sk_buff *skb, u8 aid0,
> +					     u8 *tim)
[...]
> +	/* Emit an encoded block for each non-zero sub-block */
> +	for (blk = 0; blk < IEEE80211_MAX_S1G_TIM_BLOCKS; blk++) {

So this only makes sense if you actually limit the AIDs to 1600... Maybe
then that's what you want to do :)

Otherwise even if you have no traffic for AIDs 1..1000 you would still
never indicate the traffic for AIDs >=1600 (or so.)

If you don't want to limit to 1600 then I think encoding wise we're
limit to 2048 (but in practice to 2008 with the AID limit in nl80211),
but this code would then probably attempt to access 2048 so we need to
ensure the ps->tim bitmap is actually slightly larger. Not that I really
see an issue with that. Or add a check for the actual idx below, similar
to the one you have there, but with a subblock index of 2008/8 == 251
instead.

> +		u8 blk_bmap = 0;
> +		int sblk, subcnt = 0;
> +
> +		for (sblk = 0; sblk < 8; sblk++) {
> +			int idx = blk * 8 + sblk;
> +
> +			if (idx >= IEEE80211_MAX_AID_S1G_NO_PS)
> +				break;

I think you'll want to remove this condition. If you _do_ limit the AIDs
anyway it's not needed, and if you _don't_ limit the AIDs then it should
be replaced by some "does it fit" condition, and/or checking for the
2008 value.

> +			/*
> +			 * If the current subblock is non-zero, increase the
> +			 * number of subblocks to emit for the current block.
> +			 */
> +			if (ps->tim[idx]) {

Also ... this line and the idx>= cannot simultaneously be right ... Here
it's basically a subblock index into the bitmap, whereas the >=MAX_AID
means it would be an AID, i.e. they're different units by a factor of 8.

It actually looks like it's a subblock index, but then the AID condition
makes no sense anyway.

Maybe rename the variable ;-)

> +				blk_bmap |= BIT(sblk);
> +				subcnt++;

I'd be tempted to remove the subcnt variable and use hweight8(blk_bmap)
in its place? I don't think it's _that_ much more expensive, and avoids
having to maintain the same information twice, basically?

> +		/*
> +		 * Increase the tim length by the current encoded block
> +		 * length by block control + block bitmap + n_subblocks where
> +		 * n_subblocks represents the number of subblock bytes to emit
> +		 * for the current block.
> +		 */
> +		tim[1] += 1 + 1 + subcnt;

Or you could just remove all of this tim[1] code from _both_ functions
in the first place, and just have the _caller_ update it:

struct element *tim;

pos = skb_put(skb, 4);
tim = (void *)pos;
// add all the stuff including PVB
tim->datalen = skb_tail_pointer(skb) - &tim->data;

or so. I could be off by one I guess :)

Then (going back down the stack of thoughts) you don't need the 'subcnt'
at all.

johannes

Re: [wireless-next 1/2] wifi: mac80211: support encoding S1G TIM PVB

[Johannes Berg]

On Wed, 2025-07-23 at 10:04 +0200, Johannes Berg wrote:
> 
> If you don't want to limit to 1600 then I think encoding wise we're
> limit to 2048 (but in practice to 2008 with the AID limit in nl80211),

Actually I guess without paging anyway to 2048.

> struct element *tim;
> 
> pos = skb_put(skb, 4);
> tim = (void *)pos;
> // add all the stuff including PVB
> tim->datalen = skb_tail_pointer(skb) - &tim->data;

tim->data is already a pointer, so the & there is wrong, sorry.

johannes

Re: [wireless-next 2/2] wifi: mac80211: support parsing S1G TIM PVB

[Lachlan Hodges]

On Wed, Jul 23, 2025 at 09:39:47AM +0200, Johannes Berg wrote: 
> Yes, they just go to net/mac80211/tests/. You can put something in the
> same patch or if you feel that gets too big into a separate patch in the
> series, I don't really care too much.

I am playing around with them now, so will include them in the next
patchset :)
 
> > whereas (and, do correct me if im
> > wrong) on the AP side the AID is passed down from hostap via
> > NL80211_ATTR_PEER_AID, which is then validated using the
> > NLA_POLICY_RANGE where the max is IEEE80211_MAX_AID. So if we were to
> > limit the AID to 1600 for S1G, we'd need some way to validate it within
> > cfg80211. Don't think this would be too hard, since we can confirm we
> > have an S1G station via the presence of the S1G capabilities within
> > nl80211_set_station_tdls():
> > 
> > ...
> > 	if (info->attrs[NL80211_ATTR_S1G_CAPABILITY])
> > 		params->link_sta_params.s1g_capa =
> > 			nla_data(info->attrs[NL80211_ATTR_S1G_CAPABILITY]);
> > ...
> > 
> > where we could maybe perform some extra validation on the AID? I know
> > its maybe not the prettiest since we aren't technically using the
> > explicit nl80211 intrinsic validation - but I would say it gets the job
> > done.
> 
> Ohh! Now I see where you're coming from, I was completely handwaving
> away the _practicalities_ of checking the limit in my head.
> 
> I mean, sure, we currently use a policy ... Using the S1G_CAPABILITY
> attribute wouldn't work if hostapd isn't including it at that time, but
> mac80211 can always also do the validation. Worst case we can even
> expose a new max-AID attribute, per interface type.
> 
> If we wanted to extend it later to up to 8191 then we'd need to remove
> the IEEE80211_MAX_AID policy anyway since it'd be limiting to much less.
> 
> So I don't really see this as too much an issue, I'd probably just have
> added a comment in nl80211.h that it's practically 1600 for S1G for now,
> and added validation in mac80211. Then if _later_ we extend it, we can
> add a feature flag/feature attribute for "now we support bigger AIDs for
> S1G".

Sure
 
> > I think what I'll do from here, is wait for your response to my
> > comments, mainly on the maximum AID count and any other comments you may 
> > have, and work on v2 which would roughly consist of the following:
> > 
> > 1. On the AP side, we simply support block bitmap encoding. Patch 1 
> >    would more or less be the same (with the addition potentially of
> >    new AID validation) though I'd add some details in there and
> >    change some comments etc.
> 
> Sure. I write in a comment on the other patch that
> IEEE80211_MAX_AID_S1G_NO_PS should probably not be in ieee80211.h, but I
> guess with the changes to validation etc. you'd renamed that anyway and
> put it in a different place. Or remove it.

Yep sorry I did read that (and will respond after this email).

> > 2. Support decoding the other encoding modes on the STA side. I think
> >    while a bit more work upfront, worth it in the long run.
> > 3. Add in some kunit tests to tests both the encoding of block bitmap
> >    and also the decoding of all the formats from Annex L.
> > 4. Fix up the AID stuff depending on what you think, If we go with the
> >    hard limit of 1600, ensure on the AP side we perform that validation.
> 
> I really don't mind either way. Like I said above, it's a bit of a
> question of complexity in the AP encoding implementation. Maybe just
> start updating the AP encoding implementation and see how hard the TIM
> element length validation would be to add to it, vs. a priori knowledge
> that it'll always fit?

Based on our discussion now (going to read patch #1 email after this) 
Ill stick to 1600 limit, Ill leave that for response on patch #1 since
seems to be more relevent there.

lachlan

Re: [wireless-next 1/2] wifi: mac80211: support encoding S1G TIM PVB

[Lachlan Hodges]

On Wed, Jul 23, 2025 at 10:04:06AM +0200, Johannes Berg wrote:
Hi Johannes

> On Tue, 2025-07-22 at 17:16 +1000, Lachlan Hodges wrote:
> 
> > +/*
> > + * An S1G PPDU TIM PVB uses the notion of pages. Each page can reference
> > + * 2048 AIDs, however since mac80211 does not support page slicing we
> > + * are reusing the existing TIM bitmap, which supports up to 2008 AIDs.
> > + * As the TIM element has a maximum length of 255 bytes, and each encoded
> > + * block has a maximum length of 10 bytes at most we can support 25 blocks,
> > + * as 1 + 1 + 1 + 25 * 10 = 253 bytes, leaving our maximum AID count for
> > + * an S1G PPDU at 25 * 64 = 1600. If page slicing is introduced in the
> > + * future, this will need to be modified.
> > + */
> > +#define IEEE80211_MAX_AID_S1G_NO_PS	1600
> > +#define IEEE80211_MAX_S1G_TIM_BLOCKS	25
> 
> Come to think of it, neither of these really makes sense in the
> ieee80211.h header file since they're implementation related limits due
> to the encoding. And IEEE80211_MAX_S1G_TIM_BLOCKS is questionable too
> (see below), spec wise the limit would maybe be 32 (you cannot encode
> higher than block 0..31 in the 5 bit block index.)

Yup you are right, since this is an implementation detail rather then
the actual spec. Will move these somewhere else for v2.

> > +static void ieee80211_beacon_add_tim_pvb(struct ps_data *ps,
> > +					 struct sk_buff *skb, u8 aid0, u8 *tim)
> 
> Maybe use 'struct element' for the tim pointer? that way tim[1] = ...
> becomes tim->datalen = ... which seems easier to understand.
> 
> I know the code didn't use that (it predates the existence of 'struct
> element') but it could also originally put the WLAN_EID_TIM that way,
> for example.
> 
> And maybe aid0 could be 'bool mcast_traffic' or 'bool aid0_traffic' (if
> mcast is too specific, not sure what might get encoded there now/s1g.)
> 
> (mostly I'm thinking it should be bool, 'bool aid0' is also fine)

Yup I guess If im shuffling this code around I should clean up the
normal TIM function, since it definitely looks like it was written a
long time ago :)

> > +static void ieee80211_s1g_beacon_add_tim_pvb(struct ps_data *ps,
> > +					     struct sk_buff *skb, u8 aid0,
> > +					     u8 *tim)
> [...]
> > +	/* Emit an encoded block for each non-zero sub-block */
> > +	for (blk = 0; blk < IEEE80211_MAX_S1G_TIM_BLOCKS; blk++) {
> 
> So this only makes sense if you actually limit the AIDs to 1600... Maybe
> then that's what you want to do :)
> 
> Otherwise even if you have no traffic for AIDs 1..1000 you would still
> never indicate the traffic for AIDs >=1600 (or so.)
> 
> If you don't want to limit to 1600 then I think encoding wise we're
> limit to 2048 (but in practice to 2008 with the AID limit in nl80211),
> but this code would then probably attempt to access 2048 so we need to
> ensure the ps->tim bitmap is actually slightly larger. Not that I really
> see an issue with that. Or add a check for the actual idx below, similar
> to the one you have there, but with a subblock index of 2008/8 == 251
> instead.

As mentioned in other email, I'll stick with the 1600 limit as an
"Implementation detail" for now. 

> > +		u8 blk_bmap = 0;
> > +		int sblk, subcnt = 0;
> > +
> > +		for (sblk = 0; sblk < 8; sblk++) {
> > +			int idx = blk * 8 + sblk;
> > +
> > +			if (idx >= IEEE80211_MAX_AID_S1G_NO_PS)
> > +				break;
> 
> I think you'll want to remove this condition. If you _do_ limit the AIDs
> anyway it's not needed, and if you _don't_ limit the AIDs then it should
> be replaced by some "does it fit" condition, and/or checking for the
> 2008 value.

Ah yep its redundant due to the outer loop. Will fix.

> 
> > +			/*
> > +			 * If the current subblock is non-zero, increase the
> > +			 * number of subblocks to emit for the current block.
> > +			 */
> > +			if (ps->tim[idx]) {
> 
> Also ... this line and the idx>= cannot simultaneously be right ... Here
> it's basically a subblock index into the bitmap, whereas the >=MAX_AID
> means it would be an AID, i.e. they're different units by a factor of 8.
> 
> It actually looks like it's a subblock index, but then the AID condition
> makes no sense anyway.
> 
> Maybe rename the variable ;-)

Yep will tidy this up.

> > +				blk_bmap |= BIT(sblk);
> > +				subcnt++;
> 
> I'd be tempted to remove the subcnt variable and use hweight8(blk_bmap)
> in its place? I don't think it's _that_ much more expensive, and avoids
> having to maintain the same information twice, basically?

Yep I see - we can just use hweight8 rather then tracking it
separately. I think anytime we can make these bit twiddling functions
simpler is always good :)

> > +		/*
> > +		 * Increase the tim length by the current encoded block
> > +		 * length by block control + block bitmap + n_subblocks where
> > +		 * n_subblocks represents the number of subblock bytes to emit
> > +		 * for the current block.
> > +		 */
> > +		tim[1] += 1 + 1 + subcnt;
> 
> Or you could just remove all of this tim[1] code from _both_ functions
> in the first place, and just have the _caller_ update it:
> 
> struct element *tim;
> 
> pos = skb_put(skb, 4);
> tim = (void *)pos;
> // add all the stuff including PVB
> tim->datalen = skb_tail_pointer(skb) - &tim->data;
> 
> or so. I could be off by one I guess :)
> 
> Then (going back down the stack of thoughts) you don't need the 'subcnt'
> at all.

That is probably cleaner overall, will probably do something like that
in v2.

lachlan