[v2,1/2] station: start roam on beacon loss event

Commit Message

James Prestwood Nov. 7, 2023, 2:11 p.m. UTC

Beacon loss handling was removed in the past because it was
determined that this even always resulted in a disconnect. This
was short sighted and not always true. The default kernel behavior
waits for 7 lost beacons before emitting this event, then sends
either a few nullfuncs or probe requests to the BSS to determine
if its really gone. If these come back successfully the connection
will remain alive. This can give IWD some time to roam in some
cases so we should be handling this event.

Since beacon loss indicates a very poor connection the roam scan
is delayed by a few seconds in order to give the kernel a chance
to send the nullfuncs/probes or receive more beacons. This may
result in a disconnect, but it would have happened anyways.
Attempting a roam mainly handles the case when the connection can
be maintained after beacon loss, but is still poor.
---
 src/netdev.h  |  1 +
 src/station.c | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

v2:
 * Delay the roam to give the kernel a chance to salvage the
   connection

FYI, I did attempt to add back the beacon loss test without much
luck. Blocking beacons with hwsim did not result in a beacon loss
event which may have been one of the reasons it was removed in the
first place. Even blocking _all_ frames doesn't result in an event
but instead just a local disconnect.

After looking more into it I see that actually only a few drivers
even use ieee80211_beacon_loss/ieee80211_connection_loss:

iwlwifi,wl1251,intersil,mt76,wfx,ath10/11k,wcn36xx

Every other driver likely disconnects rather than attempts to
handle beacon loss. So probably for the majority of users this
actually didn't matter, but for these drivers it does make sense
to handle the event.

Comments

Denis Kenzior Nov. 7, 2023, 3:45 p.m. UTC | #1

Hi James,

On 11/7/23 08:11, James Prestwood wrote:
> Beacon loss handling was removed in the past because it was
> determined that this even always resulted in a disconnect. This
> was short sighted and not always true. The default kernel behavior
> waits for 7 lost beacons before emitting this event, then sends
> either a few nullfuncs or probe requests to the BSS to determine
> if its really gone. If these come back successfully the connection
> will remain alive. This can give IWD some time to roam in some
> cases so we should be handling this event.
> 
> Since beacon loss indicates a very poor connection the roam scan
> is delayed by a few seconds in order to give the kernel a chance
> to send the nullfuncs/probes or receive more beacons. This may
> result in a disconnect, but it would have happened anyways.
> Attempting a roam mainly handles the case when the connection can
> be maintained after beacon loss, but is still poor.
> ---
>   src/netdev.h  |  1 +
>   src/station.c | 18 ++++++++++++++++++
>   2 files changed, 19 insertions(+)
> 
> v2:
>   * Delay the roam to give the kernel a chance to salvage the
>     connection
> 
> FYI, I did attempt to add back the beacon loss test without much
> luck. Blocking beacons with hwsim did not result in a beacon loss
> event which may have been one of the reasons it was removed in the
> first place. Even blocking _all_ frames doesn't result in an event
> but instead just a local disconnect.

Good info, thanks for doing this.  Guess we'll have to rely on manual testing 
for this feature. :(

> 
> After looking more into it I see that actually only a few drivers
> even use ieee80211_beacon_loss/ieee80211_connection_loss:
> 
> iwlwifi,wl1251,intersil,mt76,wfx,ath10/11k,wcn36xx
> 
> Every other driver likely disconnects rather than attempts to
> handle beacon loss. So probably for the majority of users this
> actually didn't matter, but for these drivers it does make sense
> to handle the event.
> 

All applied, thanks.

Regards,
-Denis

Patch

diff --git a/src/netdev.h b/src/netdev.h
index 73d38c32..f27130f1 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -51,6 +51,7 @@  enum netdev_event {
 	NETDEV_EVENT_RSSI_LEVEL_NOTIFY,
 	NETDEV_EVENT_PACKET_LOSS_NOTIFY,
 	NETDEV_EVENT_FT_ROAMED,
+	NETDEV_EVENT_BEACON_LOSS_NOTIFY,
 };
 
 enum netdev_watch_event {
diff --git a/src/station.c b/src/station.c
index 5a202ebd..c3f26b5a 100644
--- a/src/station.c
+++ b/src/station.c
@@ -3370,6 +3370,21 @@  static void station_packets_lost(struct station *station, uint32_t num_pkts)
 	station_start_roam(station);
 }
 
+static void station_beacon_lost(struct station *station)
+{
+	l_debug("Beacon lost event");
+
+	if (station_cannot_roam(station))
+		return;
+
+	station_debug_event(station, "beacon-loss-roam");
+
+	if (station->roam_trigger_timeout)
+		return;
+
+	station_roam_timeout_rearm(station, LOSS_ROAM_RATE_LIMIT);
+}
+
 static void station_netdev_event(struct netdev *netdev, enum netdev_event event,
 					void *event_data, void *user_data)
 {
@@ -3414,6 +3429,9 @@  static void station_netdev_event(struct netdev *netdev, enum netdev_event event,
 
 		station_roamed(station);
 		break;
+	case NETDEV_EVENT_BEACON_LOSS_NOTIFY:
+		station_beacon_lost(station);
+		break;
 	}
 }