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

[Johannes Berg <johannes@sipsolutions.net>]

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