Re: [PATCH v2 3/5] dpp: explicitly disconnect station if enrollee is started

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 7/22/24 1:29 PM, James Prestwood wrote:
> Prior to now the DPP state was required to be disconnected before
> DPP would start. This is inconvenient for the user since it requires
> extra state checking and/or DBus method calls. Instead model this
> case like WSC and issue a disconnect to station if DPP is requested
> to start.
> 
> The other conditions on stopping DPP are also preserved and no
> changes to the configurator role have been made, i.e. being
> disconnected while configuring still stops DPP. Similarly any
> connection made during enrolling will stop DPP.
> 
> It should also be noted that station's autoconfigure setting is also
> preserved and set back to its original value upon DPP completing.
> ---
>   src/dpp.c | 195 +++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 143 insertions(+), 52 deletions(-)
> 

<snip>

> @@ -3734,27 +3740,50 @@ static void dpp_frame_watch(struct dpp_sm *dpp, uint16_t frame_type,
>    *     DPP finishes.
>    *
>    * Other conditions shouldn't ever happen i.e. configuring and going into a
> - * connecting state or enrolling and going to a disconnected/roaming state.
> + * connecting state or enrolling and going to a roaming state.
>    */
>   static void dpp_station_state_watch(enum station_state state, void *user_data)
>   {
>   	struct dpp_sm *dpp = user_data;
>   
> -	if (dpp->state == DPP_STATE_NOTHING)
> +	if (dpp->state == DPP_STATE_NOTHING ||
> +					dpp->interface == DPP_INTERFACE_UNBOUND)

Is this check needed?  It looks like you create the state watch after setting 
state and interface accordingly?

>   		return;
>   

<snip>

> @@ -3927,6 +3947,64 @@ static void dpp_start_presence(struct dpp_sm *dpp, uint32_t *limit_freqs,
>   	dpp_start_offchannel(dpp, dpp->current_freq);
>   }
>   
> +static int dpp_disconnect_started(struct dpp_sm *dpp)
> +{
> +	int ret = 0;

Is initialization needed?

> +	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
> +
> +	/* Unusual case, but an enrollee could still be started */
> +	if (!station)
> +		return false;

Function signature returns int, but you're returning false.

> +
> +	dpp->autoconnect = station_get_autoconnect(station);
> +	station_set_autoconnect(station, false);
> +
> +	switch (station_get_state(station)) {
> +	case STATION_STATE_AUTOCONNECT_QUICK:
> +	case STATION_STATE_AUTOCONNECT_FULL:
> +		/* Should never happen since we just set autoconnect false */
> +		l_warn("Still in autoconnect state after setting false!");
> +
> +		/* fall through */
> +	case STATION_STATE_DISCONNECTED:
> +		ret = false;
> +		goto register_watch;
> +	case STATION_STATE_ROAMING:
> +	case STATION_STATE_FT_ROAMING:
> +	case STATION_STATE_FW_ROAMING:
> +	case STATION_STATE_CONNECTING:
> +	case STATION_STATE_CONNECTING_AUTO:
> +	case STATION_STATE_CONNECTED:
> +	case STATION_STATE_NETCONFIG:
> +		/*
> +		 * For any connected or connection in progress state, start a
> +		 * disconnect
> +		 */
> +		ret = station_disconnect(station);
> +		if (ret < 0)
> +			goto error;
> +
> +		/* fall through */
> +	case STATION_STATE_DISCONNECTING:
> +		l_debug("Delaying DPP start until after disconnect");
> +
> +		ret = true;
> +		goto register_watch;
> +	}
> +
> +error:
> +	l_warn("failed to start disconnecting (%d)", ret);
> +	station_set_autoconnect(station, dpp->autoconnect);
> +
> +	return ret;
> +
> +register_watch:
> +	dpp->station_watch = station_add_state_watch(station,
> +						dpp_station_state_watch,
> +						dpp, NULL);
> +	return ret;
> +}
> +
>   static void dpp_start_enrollee(struct dpp_sm *dpp)
>   {
>   	uint32_t freq = band_channel_to_freq(6, BAND_FREQ_2_4_GHZ);
> @@ -3955,27 +4033,27 @@ static struct l_dbus_message *dpp_dbus_start_enrollee(struct l_dbus *dbus,
>   						void *user_data)
>   {
>   	struct dpp_sm *dpp = user_data;
> -	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
> +	int ret;
>   
>   	if (dpp->state != DPP_STATE_NOTHING ||
>   				dpp->interface != DPP_INTERFACE_UNBOUND)
>   		return dbus_error_busy(message);
>   
> -	/*
> -	 * Station isn't actually required for DPP itself, although this will
> -	 * prevent connecting to the network once configured.
> -	 */
> -	if (station && station_get_connected_network(station)) {
> -		l_warn("cannot be enrollee while connected, please disconnect");
> -		return dbus_error_busy(message);
> -	} else if (!station)
> -		l_debug("No station device, continuing anyways...");
> -
>   	dpp->state = DPP_STATE_PRESENCE;
>   	dpp->role = DPP_CAPABILITY_ENROLLEE;
>   	dpp->interface = DPP_INTERFACE_DPP;
>   
> -	dpp_start_enrollee(dpp);
> +	ret = dpp_disconnect_started(dpp);
> +	if (ret < 0) {
> +		dpp_reset(dpp);
> +		return dbus_error_from_errno(ret, message);
> +	} else if (ret == false)
> +		dpp_start_enrollee(dpp);

I'm super confused here

> +
> +	/*
> +	 * If a disconnect was started/in progress the enrollee will start once
> +	 * that has finished
> +	 */
>   
>   	dpp->pending = l_dbus_message_ref(message);
>   

Regards,
-Denis