Re: [PATCH 0/4] Packet/beacon loss roaming improvements

[James Prestwood <prestwoj@gmail.com>]

Hi Denis,

On 10/30/23 8:00 AM, Denis Kenzior wrote:
> Hi James,
> 
>>
>> We were seeing beacon loss events not resulting in an immediate
>> disconnnect (as I have always expected), still eventually but after
> 
> If I recall correctly, Lost Beacon is sent after several beacons in 
> succession were lost.  You are right that this could just be bad luck 
> and doesn't actually mean that no packets are getting through.  However, 
> in practice mac80211 almost always disconnected us soon after.  Didn't 
> we test this pretty thoroughly?

Yes, it appears mac80211 by default waits for 7 missed beacons before 
sending the event. It then probes the AP (either nullfunc or probe 
request) so apparently the connection could be recovered if the AP 
responded. Unfortunately we don't get any notification in userspace if 
the AP responded or not...

I can't remember what hardware was tested. But there really wasn't a 
consistent way to test this. The testing involved me disabling roaming 
and walking away from the AP until I got disconnected. Sometimes this 
was due to beacon loss, sometimes the AP disconnected explicitly. But 
what I do remember is when beacon loss occurred, a local disconnect 
followed near immediately. This is why (I think) we thought there was no 
reason to handle this event.

> 
> My memory is fuzzy here, but I seem to recall that power save has an 
> effect on how lost beacon events are treated by mac80211.  Maybe your 
> recent power save patches had an effect?

 From what I can tell in mac80211 power save doesn't change handling. 
Its the driver that tells mac80211 of the beacon loss but maybe the 
driver (or firmware) could handle it differently depending on power save.

When I was watching this device power save was disabled.

> 
>> plenty of time to roam. I initially added handling for
>> beacon loss identical to packet loss (try and find a better BSS) but
>> noticed that if IWD did not find a better candidate it resulted in a
>> disconnect 100% of the time. I watched a client for a full day and
>> whenever beacon loss events arrived they were followed by a
>> disconnect within ~5-6 seconds if IWD did not roam. This led me to
>> believe that at least on ath10k a beacon loss is still very much a
>> sign that a disconnect is going to come, we just have a bit more time
>> than other drivers. This was the motivation behind re-using/re-naming
>> the 'ap_directed_roam' flag in order to force IWD off the BSS.
>>
> 
> ath10k is still a mac80211 driver, no?  Given that we did test Lost 
> Beacon event behavior before, I would like some more data points before 
> being convinced it is a driver behavior change.
> 
>> Again, this is just one driver. Maybe other drivers can
>> handle/recover from beacon loss. If we instead want to keep the
>> behavior the same as packet loss I'm ok with that (I can keep the
>> patch locally), or put the forced roam behavior behind a user
>> option e.g. [Roam].ForceRoamOnBeaconLoss
> 
> If this is a driver behavior quirk, then this belongs in src/wiphy.c 
> driver_infos table somehow.  I'd really rather not add a bazillion 
> config options that address the bug-of-the-day.

Yeah, adding a driver specific quirk doesn't seem like the right route.

I think for now there is no harm in trying to roam on beacon loss, 
basically the same handling as packet loss. If a disconnect comes 
immediately the scan would be canceled. Otherwise maybe we get lucky and 
be able to roam.

Since our specific hardware/use case seems to benefit from the forced 
roam I can keep that change out of tree (just set ap_directed_roaming), 
at least until more testing can be done, or if others report similar 
behavior.

Thanks,
James

> 
>>
>> James Prestwood (4):
>>    station: rename ap_directed_roam to force_roam
>>    station: start roam on beacon loss event
>>    netdev: handle/send beacon loss event
>>    station: rate limit packet loss roam scans
>>
>>   src/netdev.c  |  6 ++++-
>>   src/netdev.h  |  1 +
>>   src/station.c | 61 +++++++++++++++++++++++++++++++++++++++++++--------
>>   3 files changed, 58 insertions(+), 10 deletions(-)
>>
> 
> Regards,
> -Denis