Re: [PATCH] wifi: ath9k: fix beacon slot selection once TSF exceeds 2^39 us

["Toke Høiland-Jørgensen" <toke@toke.dk>]

"Felix K." <fk@mutemail.de> writes:

> ath9k_beacon_choose_slot() derives the staggered beacon slot from the
> TSF as
>
> 	tsftu = TSF_TO_TU((tsf * ATH_BCBUF) >> 32, tsf * ATH_BCBUF);
> 	slot = (tsftu % (intval * ATH_BCBUF)) / intval;
>
> TSF_TO_TU() truncates its result to 32 bits, so tsftu is
> (tsf / 128) mod 2^32 and wraps every 2^39 us (~6.36 days). 2^32 is not
> a multiple of intval * ATH_BCBUF (800 for the default beacon interval
> of 100 TU, 2^32 mod 800 = 96), so every wrap shifts the computed
> position within the beacon interval by 96/100 of a slot.
>
> After the first wrap the SWBA handler therefore picks the vif of the
> previous slot: every BSS sends its beacon one slot (12.8 ms) late,
> while its timestamp is still corrected by the tsf_adjust of its own
> slot, so clients see the beacon 12.8 ms after their expected TBTT.
> After n wraps the beacons are n slots late (up to 89.6 ms), until the
> error wraps around after 8 wraps (~51 days). This affects every AP or
> mesh setup, also with a single vif, since 8 slots are always used.
>
> Internal resets (e.g. on stuck beacon) preserve the TSF, so the state
> only recovers when the interface is restarted and ath9k_start() resets
> the TSF.
>
> Compute the offset within the beacon interval from the 64-bit TSF
> instead. For TSF values below 2^39 us the result is identical to the
> old calculation.
>
> Observed on a TP-Link Archer C7 v2 (QCA9558, AHB) running OpenWrt
> 25.12.5 (backports 6.18.26) with three AP interfaces on 2.4 GHz. The
> TSF was set via debugfs (regidx/regval 0x804c/0x8050) followed by
> "echo 1 > ath9k/reset", and beacons were captured with a second device
> in monitor mode. Beacon timestamp modulo beacon interval, identical for
> all three BSS:
>
>   TSF                       offset
>   3.40 days  (0 wraps)        384 us
>   6.36 days  (1 wrap)       13184 us
>   25.45 days (4 wraps)      51584 us
>   44.54 days (7 wraps)      89984 us
>   50.90 days (8 wraps)        384 us
>
> With the beacons 89.6 ms late, a power saving IoT client (EV charger
> controller) went from 40 ms to 1.4 s average ping RTT (max 7.2 s) with
> 1.9% packet loss within 9 minutes. Most other clients were not visibly
> affected in that time.
>
> Tested with this patch applied on the same device, using the same
> debugfs TSF method: the beacons are no longer delayed. The offset stays
> at 384 us for all three BSS with the TSF set to 0, 1, 4 and 7 wraps,
> and with the TSF set to 7 wraps the EV charger controller stayed at
> 40 ms average ping RTT (max 117 ms, no loss) over 5 minutes.

Thank you for the patch, and the detailed writeup! A few comments below:

> ath9k_htc_choose_bslot() in htc_drv_beacon.c uses a similar 32-bit
> TU conversion (wrapping every 2^42 us, ~50.9 days); it is not changed
> here as I cannot test it.

Care to send a (separate) patch for ath9k_htc? We can get others to test
it before applying.

> Fixes: dd347f2fb2dd ("ath9k: fix beacon timer handling issues")
> Cc: stable@vger.kernel.org
> Signed-off-by: Felix K. <fk@mutemail.de>
> ---
>  drivers/net/wireless/ath/ath9k/beacon.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
> index 4a27e3753..8a9bd23d3 100644
> --- a/drivers/net/wireless/ath/ath9k/beacon.c
> +++ b/drivers/net/wireless/ath/ath9k/beacon.c
> @@ -308,7 +308,7 @@ static int ath9k_beacon_choose_slot(struct ath_softc *sc)
>  	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>  	struct ath_beacon_config *cur_conf = &sc->cur_chan->beacon;
>  	u16 intval;
> -	u32 tsftu;
> +	u32 offset;
>  	u64 tsf;
>  	int slot;
>  
> @@ -322,11 +322,11 @@ static int ath9k_beacon_choose_slot(struct ath_softc *sc)
>  	intval = cur_conf->beacon_interval ? : ATH_DEFAULT_BINTVAL;
>  	tsf = ath9k_hw_gettsf64(sc->sc_ah);
>  	tsf += TU_TO_USEC(sc->sc_ah->config.sw_beacon_response_time);
> -	tsftu = TSF_TO_TU((tsf * ATH_BCBUF) >>32, tsf * ATH_BCBUF);

AFAICT, this is the only usage of the TSF_TO_TU macro defined in ath9k.h
(htc has its own in htc.h). So please remove the definition while you're
at it :)

-Toke