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

[James Prestwood <prestwoj@gmail.com>]

Hi Denis,

On 10/29/23 3:04 PM, Denis Kenzior wrote:
> 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.

One thing I overlooked here was the fact that offchannel requests are at 
a higher priority than scanning, and since we "block" the work queue in 
DPP they will resume afterwards.

So these patches aren't very useful and instead I will remove 
periodic_scan_stop calls (to retain the autoconnect state) and change 
station_connect_network to be callable without a dbus message (to fix 
the state problem).

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

Yeah, this too.

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