Re: [PATCH 1/4] station: treat netconfig failures as connection failures

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 5/28/25 9:27 AM, James Prestwood wrote:
> Currently the netconfig functionality is somewhat separated from
> IWD's "connection" process. Regardless if netconfig is enabled
> the method return from Connect() will happen after IWD has completed
> the 4-way handshake. If netconfig fails its shows externally as an
> IWD state change rather than part of the method return from
> Connect(). Overall this doesn't pose a significant problem since a

It looks like you're fixing this as part of this commit to send the DBus reply 
after the netconfig has been completed.  Can you separate this change into a 
separate commit and document it for posterity?  Not sure if NM relies on this 
behavior (hopefully not), but we should have a Fixes tag as well.

Maybe 72e7d3ceb83d ("station: Handle NETCONFIG_EVENT_FAILED") ?

> netconfig failure essentially appears as if IWD became disconnected
> but more critical is that IWD will not iteratively try more BSS's if
> netconfig fails.
> 
> For example a BSS may be misconfigured, or not able to communicate to
> the DHCP server. IWD could connect to it, and fail netconfig thereby
> restarting the autoconnect logic. IWD will then choose the "best" BSS
> which may be the one it just failed on. This would then repeat
> indefinitely or until a better BSS comes around which could be never.
> 
> To improve this netconfig has been adopted into the IWD's BSS retry
> logic. If netconfig fails this will not result in IWD transitioning
> to a disconnected state, and instead the BSS will be network
> blacklisted and the next will be tried. Only once all BSS's have been
> tried will IWD go into a disconnected state and start autoconnect
> over.
> ---
>   src/station.c | 104 +++++++++++++++++++++++++-------------------------
>   1 file changed, 51 insertions(+), 53 deletions(-)
> 
> diff --git a/src/station.c b/src/station.c
> index 2b6a18f8..81c1e595 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -1795,6 +1795,13 @@ static void station_enter_state(struct station *station,
>   		periodic_scan_stop(station);
>   		break;
>   	case STATION_STATE_CONNECTED:
> +		if (station->connect_pending) {
> +			struct l_dbus_message *reply =
> +				l_dbus_message_new_method_return(
> +						station->connect_pending);
> +			dbus_pending_reply(&station->connect_pending, reply);
> +		}
> +

Looks like this part should be in a separate commit per above comment.

>   		l_dbus_object_add_interface(dbus,
>   					netdev_get_path(station->netdev),
>   					IWD_STATION_DIAGNOSTIC_INTERFACE,
> @@ -2214,6 +2221,26 @@ static void station_early_neighbor_report_cb(struct netdev *netdev, int err,
>   				&station->roam_freqs);
>   }
>   
> +static bool station_try_next_bss(struct station *station)
> +{
> +	struct scan_bss *next;
> +	int ret;
> +
> +	next = network_bss_select(station->connected_network, false);
> +
> +	if (!next)
> +		return false;
> +
> +	ret = __station_connect_network(station, station->connected_network,
> +						next, station->state);
> +	if (ret < 0)
> +		return false;
> +
> +	l_debug("Attempting to connect to next BSS "MAC, MAC_STR(next->addr));
> +
> +	return true;
> +}
> +
>   static bool station_can_fast_transition(struct station *station,
>   					struct handshake_state *hs,
>   					struct scan_bss *bss)
> @@ -2256,28 +2283,28 @@ static bool station_can_fast_transition(struct station *station,
>   	return true;
>   }
>   
> -static void station_disconnect_on_error_cb(struct netdev *netdev, bool success,
> -					void *user_data)
> +static void station_disconnect_on_netconfig_failed(struct netdev *netdev,
> +							bool success,
> +							void *user_data)
>   {
>   	struct station *station = user_data;
> -	bool continue_autoconnect;
>   
> -	station_enter_state(station, STATION_STATE_DISCONNECTED);
> +	if (station_try_next_bss(station))
> +		return;
>   
> -	continue_autoconnect = station->state == STATION_STATE_CONNECTING_AUTO;
> +	network_disconnected(station->connected_network);

Hmm, why is this needed? station_reset_connection_state() already invokes this.

>   
> -	if (continue_autoconnect) {
> -		if (station_autoconnect_next(station) < 0) {
> -			l_debug("Nothing left on autoconnect list");
> -			station_enter_state(station,
> -					STATION_STATE_AUTOCONNECT_FULL);
> -		}
> +	if (station->connect_pending) {
> +		struct l_dbus_message *reply = dbus_error_failed(
> +						station->connect_pending);
>   
> -		return;
> +		dbus_pending_reply(&station->connect_pending, reply);
>   	}
>   
> -	if (station->autoconnect)
> -		station_enter_state(station, STATION_STATE_AUTOCONNECT_QUICK);
> +	station_reset_connection_state(station);
> +
> +	station_enter_state(station, STATION_STATE_DISCONNECTED);
> +	station_enter_state(station, STATION_STATE_AUTOCONNECT_FULL);
>   }
>   
>   static void station_netconfig_event_handler(enum netconfig_event event,
> @@ -2287,26 +2314,24 @@ static void station_netconfig_event_handler(enum netconfig_event event,
>   
>   	switch (event) {
>   	case NETCONFIG_EVENT_CONNECTED:
> +		network_connected(station->connected_network);
>   		station_enter_state(station, STATION_STATE_CONNECTED);
>   		break;
>   	case NETCONFIG_EVENT_FAILED:
> -		if (station->connect_pending) {
> -			struct l_dbus_message *reply = dbus_error_failed(
> -						station->connect_pending);
> +		station_debug_event(station, "netconfig-failed");
>   
> -			dbus_pending_reply(&station->connect_pending, reply);
> -		}
> +		netconfig_reset(station->netconfig);
>   
>   		if (station->state == STATION_STATE_NETCONFIG)
>   			network_connect_failed(station->connected_network,
>   						false);
>   
> +		network_blacklist_add(station->connected_network,
> +						station->connected_bss);
> +
>   		netdev_disconnect(station->netdev,
> -					station_disconnect_on_error_cb,
> +					station_disconnect_on_netconfig_failed,
>   					station);
> -		station_reset_connection_state(station);
> -
> -		station_enter_state(station, STATION_STATE_DISCONNECTING);
>   		break;
>   	default:
>   		l_error("station: Unsupported netconfig event: %d.", event);
> @@ -3409,26 +3434,6 @@ static void station_event_channel_switched(struct station *station,
>   	network_bss_update(network, station->connected_bss);
>   }
>   
> -static bool station_try_next_bss(struct station *station)
> -{
> -	struct scan_bss *next;
> -	int ret;
> -
> -	next = network_bss_select(station->connected_network, false);
> -
> -	if (!next)
> -		return false;
> -
> -	ret = __station_connect_network(station, station->connected_network,
> -						next, station->state);
> -	if (ret < 0)
> -		return false;
> -
> -	l_debug("Attempting to connect to next BSS "MAC, MAC_STR(next->addr));
> -
> -	return true;
> -}
> -
>   static bool station_retry_owe_default_group(struct station *station)
>   {
>   	/*
> @@ -3581,13 +3586,6 @@ static void station_connect_ok(struct station *station)
>   
>   	l_debug("");
>   
> -	if (station->connect_pending) {
> -		struct l_dbus_message *reply =
> -			l_dbus_message_new_method_return(
> -						station->connect_pending);
> -		dbus_pending_reply(&station->connect_pending, reply);
> -	}
> -
>   	/*
>   	 * Get a neighbor report now so future roams can avoid waiting for
>   	 * a report at that time

And this chunk belongs in a separate commit as well.

> @@ -3598,8 +3596,6 @@ static void station_connect_ok(struct station *station)
>   			l_warn("Could not request neighbor report");
>   	}
>   
> -	network_connected(station->connected_network);
> -

I'm not so sure about this part.  network_connected() is mainly about syncing 
the PSK to disk.  Since we connected successfully, that should still happen, 
regardless of whether netconfig succeeds or not.

>   	if (station->netconfig) {
>   		if (hs->fils_ip_req_ie && hs->fils_ip_resp_ie) {
>   			struct ie_fils_ip_addr_response_info info;
> @@ -3639,8 +3635,10 @@ static void station_connect_ok(struct station *station)
>   			return;
>   
>   		station_enter_state(station, STATION_STATE_NETCONFIG);
> -	} else
> +	} else {
> +		network_connected(station->connected_network);
>   		station_enter_state(station, STATION_STATE_CONNECTED);
> +	}
>   }
>   
>   static void station_connect_cb(struct netdev *netdev, enum netdev_result result,

Regards,
-Denis