Re: [PATCH 3/8] station: add handling for new NETCONFIG state

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 1/3/24 12:46, James Prestwood wrote:
> There was an unhandled corner case if netconfig was running and
> multiple roam conditions happened in sequence, all before netconfig
> had completed. A single roam before netconfig was already handled
> (23f0f5717c) but this did not take into account any additional roam
> conditions.

So if netconfig hasn't completed, we're in the 'connecting' state.  Any 
subsequent roams should still be treated as if we are in 'connecting' state. 
Are we transitioning from connecting -> roaming at the D-Bus API level? We 
shouldn't be.

Another weirdness is that I think we're sending the d-bus reply after connecting 
to the AP, but before netconfig runs.

> 
> If IWD is in this state, having started netconfig, then roamed, and
> again restarted netconfig it is still in a roaming state which will
> prevent any further roams. IWD will remain "stuck" on the current
> BSS until netconfig completes or gets disconnected.

Makes sense, since roaming means netconfig isn't really doing anything.

> 
> To fix this a new internal station state was added (no changes to
> the DBus API) to distinguish between a purely WiFi connecting state
> (STATION_STATE_CONNECTING/AUTO) and netconfig
> (STATION_STATE_NETCONFIG). This allows IWD roam as needed if
> netconfig is still running.

Okay, but how would you distinguish between connecting -> netconfig and 
netconfig->roaming->netconfig?

> 
> The change is mainly just adding STATION_STATE_NETCONFIG anywhere
> that STATION_STATE_CONNECTING is to maintain the same behavior,
> except within the netconfig event handler. In this case we should
> never get here without being in either a NETCONFIG or ROAMING
> state.
> 
> For some background this scenario happens if the DHCP server goes
> down for an extended period, e.g. if its being upgraded/serviced.
> ---
>   src/station.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/src/station.c b/src/station.c
> index 57d22e91..8f310ec8 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -1768,6 +1768,7 @@ static void station_reset_connection_state(struct station *station)
>   	if (station->state == STATION_STATE_CONNECTED ||
>   			station->state == STATION_STATE_CONNECTING ||
>   			station->state == STATION_STATE_CONNECTING_AUTO ||
> +			station->state == STATION_STATE_NETCONFIG ||
>   			station_is_roaming(station))
>   		network_disconnected(network);
>   }
> @@ -2043,8 +2044,9 @@ static void station_netconfig_event_handler(enum netconfig_event event,
>   			dbus_pending_reply(&station->connect_pending, reply);
>   		}
>   
> -		if (L_IN_SET(station->state, STATION_STATE_CONNECTING,
> -				STATION_STATE_CONNECTING_AUTO))
> +		if (L_IN_SET(station->state, STATION_STATE_NETCONFIG,
> +				STATION_STATE_ROAMING, STATION_STATE_FT_ROAMING,
> +				STATION_STATE_FW_ROAMING))

I understand why NETCONFIG state is in the set, but why the others?

>   			network_connect_failed(station->connected_network,
>   						false);
>   
> @@ -2070,9 +2072,14 @@ static bool netconfig_after_roam(struct station *station)
>   					network_get_settings(network)))
>   		return false;
>   
> -	return netconfig_configure(station->netconfig,
> +	if (L_WARN_ON(!netconfig_configure(station->netconfig,
>   					station_netconfig_event_handler,
> -					station);
> +					station)))

You already have an L_WARN_ON in the single call site of netconfig_after_roam?

> +		return false;
> +
> +	station_enter_state(station, STATION_STATE_NETCONFIG);
> +
> +	return true;
>   }
>   
>   static void station_roamed(struct station *station)
> @@ -3255,6 +3262,8 @@ static void station_connect_ok(struct station *station)
>   						station_netconfig_event_handler,
>   						station)))
>   			return;
> +
> +		station_enter_state(station, STATION_STATE_NETCONFIG);
>   	} else
>   		station_enter_state(station, STATION_STATE_CONNECTED);
>   }
> @@ -4067,7 +4076,8 @@ static struct l_dbus_message *station_dbus_scan(struct l_dbus *dbus,
>   		return dbus_error_busy(message);
>   
>   	if (station->state == STATION_STATE_CONNECTING ||
> -			station->state == STATION_STATE_CONNECTING_AUTO)
> +			station->state == STATION_STATE_CONNECTING_AUTO ||
> +			station->state == STATION_STATE_NETCONFIG)

Might as well use L_IN_SET here

>   		return dbus_error_busy(message);
>   
>   	station->dbus_scan_subset_idx = 0;
> @@ -5025,7 +5035,8 @@ static struct l_dbus_message *station_debug_scan(struct l_dbus *dbus,
>   		return dbus_error_busy(message);
>   
>   	if (station->state == STATION_STATE_CONNECTING ||
> -			station->state == STATION_STATE_CONNECTING_AUTO)
> +			station->state == STATION_STATE_CONNECTING_AUTO ||
> +			station->state == STATION_STATE_NETCONFIG)

Also, shouldn't this also cover the roaming states?  And do we still need this 
check given wiphy_work?  Does netconfig use wiphy work to make sure nothing 
tries to scan or go off-channel?

>   		return dbus_error_busy(message);
>   
>   	if (!l_dbus_message_get_arguments(message, "aq", &iter))

Regards,
-Denis