On Wed, Jul 16, 2025 at 09:55:58AM +0200, Johannes Berg wrote:
> 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.
>
This is correct and an oversight on my part. Looking at our driver we
retrieve the beacon each time, so theres no reason to be notified for
a short beacon update. Will fix this.
> > + * @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.
>
An interface can't be modified such that we can disable short beaconing
without tearing it down. This is how I've reworked the initial
configuration within the first 2 patches. As for whether it needs to be
exposed to the driver, it does not. I kept it included as I find it
easier to read then validaing against the link->u.ap.s1g_short_beacon
pointer though I have no opposition to doing what you suggest (though
maybe in an struct not exposed to the driver). I suppose since this is
only checked twice (once here, and once when we check if we need to return
a short beacon SKB) we can just check the link->u.ap.s1g_short_beacon.
>
> > +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?
>
I initially did think so that this should be in a cfg80211 targetted
patch but since cfg.c since within mac80211 I did such. Can do either.
> 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.
>
So you are correct, It's not legal to disable short beaconing without
tearing the interface down (as mentioned above). Thats why within
ieee80211_change_beacon() we check if we are short beaconing first
and if theres an update proceed with said update. This function should
really just be setting the new pointers and discarding the old pointers
if they exist i.e during an update.
> > + 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.
>
Yea.. this seems to be a case of me stealing the beacon change code...
:). We essentially just want to be setting the new pointers, freeing
the old and that is it. A quick rewrite to something like this:
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 s1g_short_beacon_data *new;
struct s1g_short_beacon_data *old =
sdata_dereference(link->u.ap.s1g_short_beacon, sdata);
size_t new_len =
sizeof(*new) + params->short_head_len + params->short_tail_len;
if (!params->update)
return 0;
if (!params->short_head)
return -EINVAL;
new = kzalloc(new_len, GFP_KERNEL);
if (!new)
return -ENOMEM;
/* Memory layout: | struct | head | tail | */
new->short_head = ((u8 *)new) + sizeof(*new);
new->short_head_len = params->short_head_len;
memcpy(new->short_head, params->short_head, params->short_head_len);
if (params->short_tail) {
new->short_tail = new->short_head + params->short_head_len;
new->short_tail_len = params->short_tail_len;
memcpy(new->short_tail, params->short_tail,
params->short_tail_len);
}
rcu_assign_pointer(link->u.ap.s1g_short_beacon, new);
if (old)
kfree_rcu(old, rcu_head);
return 0;
}
seems more robust, where we simply update the new beacon given
the parameters and discard the old, else if theres no update
we do nothing.
lachlan
