Re: [PATCH v2 02/15] dpp: remove connect/scanning and resume periodic scans after DPP

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 10/26/23 15:26, James Prestwood wrote:
> When DPP is started periodic scans are stopped but never started
> again. This means if DPP fails IWD will never resume autoconnecting
> without some intervention.
> 

Okay, but why are you not removing scan_periodic_stop() calls then?  If the 
intent is to use station_set_autoconnect, then do that.  And this probably 
requires a separate patch.

> This also removes the internal scanning/connecting logic from DPP
> which was done for two reasons. First its unknown how long the
> DPP protocol took and its safest to explicitly scan to find the

Isn't this being done by invoking scan_active?

> target network/bss, and second the connect logic was flawed because
> station will not transition into a CONNECTING state since
> __station_connect_network shortcuts the state change. If DPP failed

Okay, so use station_connect_network?

> station would never resume autoconnecting, and if the post-DPP
> connection failed the state was set incorrectly so station would
> also not resume autoconnecting.
> 
> The downside of this is it takes slightly longer to connect after
> DPP since IWD must scan, but the DPP logic is simplified and keeps
> all connection logic in station.c where it belongs.

Well, the downside of this approach is that you're relying completely on 
autoconnect logic to pick the right network instead of having DPP connect to the 
network that was just configured.  So if autoconnect picks a different network 
you get results the user doesn't expect.

> ---
>   src/dpp.c | 76 ++++++++++++++++++++-----------------------------------
>   1 file changed, 27 insertions(+), 49 deletions(-)
> 

<snip>

> @@ -315,7 +313,17 @@ static void dpp_reset(struct dpp_sm *dpp)
>   		dpp->retry_timeout = NULL;
>   	}
>   
> +	/*
> +	 * Set station back to its original autoconnecting state if an
> +	 * enrollee and DPP failed
> +	 */
> +	if (station && dpp->role == DPP_CAPABILITY_ENROLLEE &&
> +			dpp->station_autoconnecting &&
> +			dpp->state != DPP_STATE_SUCCESS)
> +		station_set_autoconnect(station, true);

So what if autoconnect was false originally?

> @@ -830,16 +805,9 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame,
>   
>   	offchannel_cancel(dpp->wdev_id, dpp->offchannel_id);
>   
> -	if (network && bss)
> -		__station_connect_network(station, network, bss);
> -	else if (station) {
> -		dpp->connect_scan_id = scan_active(dpp->wdev_id, NULL, 0,
> -						dpp_scan_triggered,
> -						dpp_scan_results, dpp,
> -						dpp_scan_destroy);

Likely this needs to be a filtered scan (with SSID) as opposed to wildcard.

> -		if (dpp->connect_scan_id)
> -			return;
> -	}
> +	dpp->state = DPP_STATE_SUCCESS;
> +
> +	station_set_autoconnect(station, true);
>   
>   	dpp_reset(dpp);
>   }

Regards,
-Denis