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

[James Prestwood <prestwoj@gmail.com>]

Hi Denis,

On 7/24/24 7:30 AM, Denis Kenzior wrote:
> 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.

Yep this is intended.

true for a disconnect has started or is ongoing.
false for already disconnected
< 0 for a fatal error (i.e. station_disconnect() failed)

If this isn't desirable we could do:

-EALREADY for a disconnect is started/ongoing
0 for already disconnected
< 0 for a fatal error

We do a somewhat similar technique with wiphy_radio_work_is_running. But 
if we want to keep the return strictly integers/error codes I'm fine 
with that.

>
>> +
>> +    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
>