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
