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

Denis Kenzior
2024-07-24T14:30:18Z

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