Re: [wireless-next v2 2/4] wifi: mac80211: support initialising an S1G short beaconing BSS

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2025-07-16 at 15:32 +1000, Lachlan Hodges wrote:
> 
> +++ b/include/net/mac80211.h
> @@ -365,6 +365,7 @@ struct ieee80211_vif_chanctx_switch {
>   * @BSS_CHANGED_MLD_VALID_LINKS: MLD valid links status changed.
>   * @BSS_CHANGED_MLD_TTLM: negotiated TID to link mapping was changed
>   * @BSS_CHANGED_TPE: transmit power envelope changed
> + * @BSS_CHANGED_S1G_SHORT_BEACON: S1G short beacon changed
>   */
>  enum ieee80211_bss_change {
>  	BSS_CHANGED_ASSOC		= 1<<0,
> @@ -402,6 +403,7 @@ enum ieee80211_bss_change {
>  	BSS_CHANGED_MLD_VALID_LINKS	= BIT_ULL(33),
>  	BSS_CHANGED_MLD_TTLM		= BIT_ULL(34),
>  	BSS_CHANGED_TPE			= BIT_ULL(35),
> +	BSS_CHANGED_S1G_SHORT_BEACON	= BIT_ULL(36),

I feel like at the moment the driver has no way of using this, is it
even needed? It's (currently) designed to really only work with drivers
that pull each individual beacon, since you have no "get short beacon
template" and such.

> + * @s1g_short_beaconing: determines if short beaconing is enabled for an S1G
> + *	BSS.
> + * @s1g_long_beacon_period: number of beacon intervals between each long
> + *	beacon transmission.
>   */
>  struct ieee80211_bss_conf {
>  	struct ieee80211_vif *vif;
> @@ -857,6 +863,9 @@ struct ieee80211_bss_conf {
>  
>  	u8 bss_param_ch_cnt;
>  	u8 bss_param_ch_cnt_link_id;
> +
> +	bool s1g_short_beaconing;
> +	u8 s1g_long_beacon_period;
>  };

Given that, should these even be visible to the driver?

And is s1g_short_beaconing needed at all, I don't see it ever being
read? Almost feels like it shouldn't - could get out of date vs. the
link->u.ap.s1g_short_beacon pointer, which internally indicates the
same.


> +static int
> +ieee80211_set_s1g_short_beacon(struct ieee80211_sub_if_data *sdata,
> +			       struct cfg80211_s1g_short_beacon *params,
> +			       struct ieee80211_link_data *link,
> +			       struct ieee80211_bss_conf *link_conf,
> +			       u64 *changed)
> +{
> +	struct s1g_short_beacon_data *new, *old;
> +	int new_head_len, new_tail_len, size;
> +
> +	if (!params->update)
> +		return 0;
> +
> +	old = sdata_dereference(link->u.ap.s1g_short_beacon, sdata);
> +	if (!params->short_head && !old)
> +		return -EINVAL;

Not sure I understand this logic. If there's no update, it returns
anyway.

This should probably be on the cfg80211 patch, but now that I'm writing
here ... If there is no new short beacon update cannot currently be set
to true, I think? And also, right now by the policy you can't set the
long_beacon_interval == 1 from userspace, but what if you actively want
to _remove_ the short beacon entirely?

Maybe that's not legal? Or maybe it should be allowed, and then I think
the easiest way of achieving it would be to allow the long beacon
interval to be set to 1, which effectively removes the short beacon?

Either way - I'm not sure I follow the "!new && !old" part here since
you can't actually have params->update && !params->head right now? And
even if you could, I'm not sure what the old matters.

> +	new_head_len = params->short_head ? params->short_head_len :
> +					    old->short_head_len;
> +	new_tail_len = (params->short_tail || !old) ? params->short_tail_len :
> +						      old->short_tail_len;

This seems similarly odd to me, if you set a new short head but no short
tail you surely don't want to reuse the old short tail?

Seems to me really this should never use "old" at all, since you want to
update everything (or even remove the short beacon if that's allowed per
above), but never mix things?

And if update is false nothing ever happens here anyway.

> +	if (old)
> +		kfree_rcu(old, rcu_head);

Though of course this is still needed.

johannes