Re: [RFC 0/5] wifi: S1G short beacon support

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

So just to say this up front I have very little interest per se in this
feature directly, so I'll mostly leave it to you :)

> Preface:
> 
> Previously, some work was done ~2 years ago to get short beaconing
> in the kernel but it was never successful. The patches can be found
> here:
> 
> 1/2: https://patchwork.kernel.org/project/linux-wireless/patch/20230810093556.33800-1-bassem@morsemicro.com/
> 2/2: https://patchwork.kernel.org/project/linux-wireless/patch/20230810093556.33800-2-bassem@morsemicro.com/

Heh, I didn't even remember.

> There is something to say regarding whether this is "correct" as
> after all they are extension frames and while we feel this is the best
> approach when all factors are taken into account - ultimately it is
> up to maintainers to decide.

Seems reasonable to me, and anyway probably necessary so that we don't
regress.

> 11.1.3.10.2:
> 
> "The value for the dot11ShortBeaconPeriod shall be such that
> dot11BeaconPeriod = n * dot11ShortBeaconPeriod, where n is a
> positive integer. This defines a series of TSBTTs exactly
> dot11ShortBeaconPeriod TUs apart"
> 
> the value for n here is what we are denoting as
> s1g_short_beacon_period (another deviation from the naming
> within the standard) which represents the number of short
> beacons between each long beacon.

That seems slightly confusing - I've have interpreted a 'period' either
as a span of time (as in the spec), or the number of steps between
events, so maybe that's rather 's1g_long_beacon_period'? But if the spec
uses period as a span of time, then perhaps 's1g_long_beacon_step' or
something would be easier to understand? Not sure ...

> To keep track of the current
> state, we introduce a new parameter sb_count within the
> struct ps_data structure to track the current index into this
> period. This is what is used to determine whether we send a
> long or short beacon on beacon retrieval where a value of 0
> indicates a long beacon (following the same cadence of
> decrementing the DTIM count).

Seems fair.

> (4) Beacon retrieval
> 
> There were essentially two options we could take with regards to
> retrieving the beacons:
> 
> 1.implement it in the traditional beacon path via
>   ieee80211_beacon_get_ap
> 
> 2.Implement an S1G (or maybe extension frame?) specific path
>   such as ieee80211_s1g_beacon_get_ap where the S1G specific
>   handling can be done in its own function, leaving the regular
>   beacon path untouched
> 
> We have opted to take method (1) but this really comes down to
> maintainers preference. New conditionals will be introduced
> in the beacon path regardless and we feel this is the best
> approach but are open to feedback.

That seems reasonable to me, why bother the driver with the difference,
it's likely simply going to transmit the frame anyway, regardless of
what it is.

> (5) Testing
> 
> This patchset has been tested on the following configurations:
> 
> 1. Multi-sta setup with real Morse Micro hardware.
> 2. S1G hwsim configuration with a single AP and > 10 STAs
> 3. 2.4GHz hwsim configuration to test for regressions along the
>    non-s1g path consisting of a single AP and STA.
> 4. hostapd hwsim tests were run to ensure no regression for
>    regular 2.4/5/6 radios.

Cool.

> (6) Other notes:
> 
> 1. The update mechanism is not really that nice. Though that may
>    stem from a misunderstanding with how FILS discovery and unsol
>    bcast update mechanisms work. Since short beacons rely on a BSS
>    to be configured we ensure that an update is only performed
>    when this is the case, and when the BSS has not been configured
>    we do not allow a set of parameters used for updating the beacon
>    to be used when the BSS has not been initialised. Would like some
>    feedback on our implementation here as Im not really convinved
>    it's the right way, but it seems this is how we currently handle
>    it. My understanding of an "update" is that we are updating an
>    existing interface. When an interface is being initialised, we
>    are not updating - we are setting... An option here is to just
>    not even allow updates since this is how S1G is done now and this
>    can be ammended in the future.

Noted. I'll go through the commits and shout if I find anything :)


johannes