LinuxLists.cc - [PATCH] RFC: Universal scan proposal

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On 22-11-2016 18:29, Dmitry Shmidt wrote:
> Hi Luca,
>
> On Mon, Nov 21, 2016 at 11:24 PM, Luca Coelho <[email protected]> wrote:
>> Hi Dmitry,
>> On Wed, 2016-11-16 at 22:47 +0000, [email protected] wrote:
>>> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
>>> From: Dmitry Shmidt <[email protected]>
>>> Date: Wed, 16 Nov 2016 14:27:26 -0800
>>> Subject: [PATCH] RFC: Universal scan proposal
>>>
>>> Currently we have sched scan with possibility of various
>>> intervals. We would like to extend it to support also
>>> different types of scan.
>>> In case of powerful wlan CPU, all this functionality
>>> can be offloaded.
>>> In general case FW processes additional scan requests
>>> and puts them into queue based on start time and interval.
>>> Once current request is fulfilled, FW adds it (if interval != 0)
>>> again to the queue with proper interval. If requests are
>>> overlapping, new request can be combined with either one before,
>>> or one after, assuming that requests are not mutually exclusive.
>>> Combining requests is done by combining scan channels, ssids,
>>> bssids and types of scan result. Once combined request was fulfilled
>>> it will be reinserted as two (or three) different requests based on
>>> their type and interval.
>>> Each request has attribute:
>>> Type: connectivity / location
>>> Report: none / batch / immediate
>>> Request may have priority and can be inserted into
>>> the head of the queue.
>>> Types of scans:
>>> - Normal scan
>>> - Scheduled scan
>>> - Hotlist (BSSID scan)
>>> - Roaming
>>> - AutoJoin
>>>
>>> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
>>> Signed-off-by: Dmitry Shmidt <[email protected]>
>>> ---
>>
>> I like the initiative and I think this is definitely something that can
>> improve concurrent scanning instances. But IMHO the most important is
>> to discuss the semantics of this change, such as which scans can be
>> combined, who makes the decisions of combining them, how priorities are
>> sorted out etc. I think the types of scan are not relevant in the
>> nl80211 API, but the characteristics of the scans are. For instance,
>> "urgent scan" (for initial connection), best-effort scan for roaming...
>> and latency requirements, such as low-latency for location and initial
>> connection and high-latency for scheduled scan. Then we decided, in
>> the kernel, how to combine and prioritize them according to their
>> characteristics, instead of having to map scan types to these
>> characteristics.
>>
>> What do you think?
>
> 1. Combining scans.
> There are two scenarios in general: combine scans that can be offload
> and scans that can not be offload due to "weak" FW / wlan SoC. In last
> case this approach maybe not attractive at all - non-mobile device
> may not need all these different types of scan.
> In case of offload - it will be FW code decision - I just wanted to propose
> the way how to do this efficiently.

I think Luca is looking at it as a way to deal with multiple user-space
entities request the device to perform a scan. Ignoring the scan types
on android it can be wifi-hal and wpa_supplicant.

> 2. Priority - very good point, we need to have it. I am just not sure
> that we need like scale priority - maybe just flag - urgent / not urgent.
> Urgent one will be inserted in queue as is and conflicting request
> should be postponed or combined.

What if we get another urgent one?

> 3. Scan types - I am not sure I fully understood your question, but
> if the idea is for kernel to decide about type of scan based on its
> characteristics instead of specific type request performance may
> cause confusion to wifi manager.
> However, it would definitely simplify kernel API. Still I am not sure
> if userspace wifi manager will "like" it.

Actually the idea is to hide the notion of scan type entirely from the
API and kernel code. If you add the scan type as an attribute in the
API, you still need to provide additional attributes for that scan type
so the question is what the scan type itself provides, ie. how does it
affect the scan itself.

> 4. There is an interesting question: to separate scan results for
> connection and location or not? The problem is how to "trust" them.
> I mean scan results are scan results, but for location we may decide
> to look for fewer channels and even maybe specific BSSIDs, i.e. not
> full scan results, and we may need to indicate that right now
> scan results are not full in order not to confuse wifi manager.

Regards,
Arend

>> --
>> Cheers,
>> Luca.

2016-11-17 20:56:49

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On 16-11-2016 23:47, [email protected] wrote:
> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
> From: Dmitry Shmidt <[email protected]>
> Date: Wed, 16 Nov 2016 14:27:26 -0800
> Subject: [PATCH] RFC: Universal scan proposal
>
> Currently we have sched scan with possibility of various
> intervals. We would like to extend it to support also
> different types of scan.
> In case of powerful wlan CPU, all this functionality
> can be offloaded.
> In general case FW processes additional scan requests
> and puts them into queue based on start time and interval.
> Once current request is fulfilled, FW adds it (if interval != 0)
> again to the queue with proper interval. If requests are
> overlapping, new request can be combined with either one before,
> or one after, assuming that requests are not mutually exclusive.
> Combining requests is done by combining scan channels, ssids,
> bssids and types of scan result. Once combined request was fulfilled
> it will be reinserted as two (or three) different requests based on
> their type and interval.
> Each request has attribute:
> Type: connectivity / location

Don't see this type in the patch below (guess it was a last minute
addition ;-) ).

> Report: none / batch / immediate
> Request may have priority and can be inserted into
> the head of the queue.
> Types of scans:
> - Normal scan
> - Scheduled scan
> - Hotlist (BSSID scan)
> - Roaming
> - AutoJoin

Don't see the last one mentioned in the patch below.

> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
> Signed-off-by: Dmitry Shmidt <[email protected]>
> ---
> include/net/cfg80211.h | 37 +++++++++++++++++++++++++++++++------
> 1 file changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 14b51d7..1ae2570 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1602,8 +1602,10 @@ struct cfg80211_sched_scan_plan {
> * cycle. The driver may ignore this parameter and start
> * immediately (or at any other time), if this feature is not
> * supported.

At the the top of this kernel doc section it probably still refers to
struct cfg80211_sched_scan_request.

> + * @uscan_type: uscan type (scan, sched_scan, hotlist, scan_history,
> roaming)
> + * @uscan_results: uscan results report (none/batch/immediate)

If you change the type to struct cfg80211_uscan_request it seems a bit
redundant to prefix the field names with 'uscan_'.

> */
> -struct cfg80211_sched_scan_request {
> +struct cfg80211_uscan_request {
> struct cfg80211_ssid *ssids;
> int n_ssids;
> u32 n_channels;
> @@ -1621,6 +1623,10 @@ struct cfg80211_sched_scan_request {
> u8 mac_addr[ETH_ALEN] __aligned(2);
> u8 mac_addr_mask[ETH_ALEN] __aligned(2);
>
> + /* universal scan fields */

As the struct name changed it seems to me all fields in the struct are
universal scan fields.

> + u32 uscan_type;
> + u32 uscan_results;
> +
> /* internal */
> struct wiphy *wiphy;
> struct net_device *dev;
> @@ -1633,6 +1639,21 @@ struct cfg80211_sched_scan_request {
> };
>
> /**
> + * struct cfg80211_uscan_capa - universal scan capabilities
> + *
> + * @uscan_type: uscan type (scan, sched_scan, hotlist, scan_history,
> roaming)
> + * @max_ssids: max amount of ssids in sched scan
> + * @max_hotlist: max amount of bssids in hotlist
> + * @max_history: max amount of records in history
> + */
> +struct cfg80211_uscan_capa {
> + u32 uscan_type;
> + u32 max_ssids;
> + u32 max_hotlist;
> + u32 max_history;
> +};
> +
> +/**
> * enum cfg80211_signal_type - signal type
> *
> * @CFG80211_SIGNAL_TYPE_NONE: no signal strength information available
> @@ -2898,10 +2919,13 @@ struct cfg80211_ops {
> int (*set_antenna)(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant);
> int (*get_antenna)(struct wiphy *wiphy, u32 *tx_ant, u32 *rx_ant);
>
> - int (*sched_scan_start)(struct wiphy *wiphy,
> + int (*uscan_get_capa)(struct wiphy *wiphy, struct net_device *dev,
> + struct cfg80211_uscan_capa *uscan_capa);

Not sure if people start worrying about the size of struct wiphy
already, but for static information like capabilities I think a callback
is overkill.

> + int (*uscan_start)(struct wiphy *wiphy,
> struct net_device *dev,
> - struct cfg80211_sched_scan_request *request);
> - int (*sched_scan_stop)(struct wiphy *wiphy, struct net_device
> *dev);
> + struct cfg80211_uscan_request *request);

So given the two extra fields, what different driver/device behaviour is
required, eg. if uscan_type == roaming what will be different compared
to a normal scan.

> + int (*uscan_stop)(struct wiphy *wiphy, struct net_device *dev,
> + int uscan_id);

The uscan_id is probably referring to a specific scan request started by
uscan_start. So who hands out that id (wiphy->cookie_counter?) and how
does the driver know about it. If cfg80211 determines the id it probably
need to be passed in the uscan request.

> int (*set_rekey_data)(struct wiphy *wiphy, struct net_device *dev,
> struct cfg80211_gtk_rekey_data *data);
> @@ -4305,11 +4329,12 @@ void cfg80211_scan_done(struct
> cfg80211_scan_request *request,
> struct cfg80211_scan_info *info);
>
> /**
> - * cfg80211_sched_scan_results - notify that new scan results are
> available
> + * cfg80211_uscan_results - notify that new uscan results are available
> *
> * @wiphy: the wiphy which got scheduled scan results
> + * @uscan_id: type of scan results

Confused now. What is uscan_id here? Same as in .uscan_stop() callback?

> */
> -void cfg80211_sched_scan_results(struct wiphy *wiphy);
> +void cfg80211_sched_scan_results(struct wiphy *wiphy, int uscan_id);

should this be renamed to cfg80211_uscan_results().

> /**
> * cfg80211_sched_scan_stopped - notify that the scheduled scan has
> stopped

Also change to cfg80211_uscan_stopped()? Does it need an additional
argument, ie. uscan_id.

Regards,
Arend

2016-11-28 19:25:52

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Wed, Nov 23, 2016 at 12:43 AM, Arend Van Spriel
<[email protected]> wrote:
> On 22-11-2016 21:54, Dmitry Shmidt wrote:
>> On Tue, Nov 22, 2016 at 12:41 PM, Arend Van Spriel
>> <[email protected]> wrote:
>>> On 22-11-2016 18:29, Dmitry Shmidt wrote:
>>>> Hi Luca,
>>>>
>>>> On Mon, Nov 21, 2016 at 11:24 PM, Luca Coelho <[email protected]> wrote:
>>>>> Hi Dmitry,
>>>>> On Wed, 2016-11-16 at 22:47 +0000, [email protected] wrote:
>>>>>> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
>>>>>> From: Dmitry Shmidt <[email protected]>
>>>>>> Date: Wed, 16 Nov 2016 14:27:26 -0800
>>>>>> Subject: [PATCH] RFC: Universal scan proposal
>>>>>>
>>>>>> Currently we have sched scan with possibility of various
>>>>>> intervals. We would like to extend it to support also
>>>>>> different types of scan.
>>>>>> In case of powerful wlan CPU, all this functionality
>>>>>> can be offloaded.
>>>>>> In general case FW processes additional scan requests
>>>>>> and puts them into queue based on start time and interval.
>>>>>> Once current request is fulfilled, FW adds it (if interval != 0)
>>>>>> again to the queue with proper interval. If requests are
>>>>>> overlapping, new request can be combined with either one before,
>>>>>> or one after, assuming that requests are not mutually exclusive.
>>>>>> Combining requests is done by combining scan channels, ssids,
>>>>>> bssids and types of scan result. Once combined request was fulfilled
>>>>>> it will be reinserted as two (or three) different requests based on
>>>>>> their type and interval.
>>>>>> Each request has attribute:
>>>>>> Type: connectivity / location
>>>>>> Report: none / batch / immediate
>>>>>> Request may have priority and can be inserted into
>>>>>> the head of the queue.
>>>>>> Types of scans:
>>>>>> - Normal scan
>>>>>> - Scheduled scan
>>>>>> - Hotlist (BSSID scan)
>>>>>> - Roaming
>>>>>> - AutoJoin
>>>>>>
>>>>>> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
>>>>>> Signed-off-by: Dmitry Shmidt <[email protected]>
>>>>>> ---
>>>>>
>>>>> I like the initiative and I think this is definitely something that can
>>>>> improve concurrent scanning instances. But IMHO the most important is
>>>>> to discuss the semantics of this change, such as which scans can be
>>>>> combined, who makes the decisions of combining them, how priorities are
>>>>> sorted out etc. I think the types of scan are not relevant in the
>>>>> nl80211 API, but the characteristics of the scans are. For instance,
>>>>> "urgent scan" (for initial connection), best-effort scan for roaming...
>>>>> and latency requirements, such as low-latency for location and initial
>>>>> connection and high-latency for scheduled scan. Then we decided, in
>>>>> the kernel, how to combine and prioritize them according to their
>>>>> characteristics, instead of having to map scan types to these
>>>>> characteristics.
>>>>>
>>>>> What do you think?
>>>>
>>>> 1. Combining scans.
>>>> There are two scenarios in general: combine scans that can be offload
>>>> and scans that can not be offload due to "weak" FW / wlan SoC. In last
>>>> case this approach maybe not attractive at all - non-mobile device
>>>> may not need all these different types of scan.
>>>> In case of offload - it will be FW code decision - I just wanted to propose
>>>> the way how to do this efficiently.
>>>
>>> I think Luca is looking at it as a way to deal with multiple user-space
>>> entities request the device to perform a scan. Ignoring the scan types
>>> on android it can be wifi-hal and wpa_supplicant.
>>>
>>>> 2. Priority - very good point, we need to have it. I am just not sure
>>>> that we need like scale priority - maybe just flag - urgent / not urgent.
>>>> Urgent one will be inserted in queue as is and conflicting request
>>>> should be postponed or combined.
>>>
>>> What if we get another urgent one?
>>
>> We may restrict it only to one urgent scan - what is the use case for
>> two requests?
>
> We don't know I guess so we can restrict to one for now. Just wondered
> reading this.
>
>>>> 3. Scan types - I am not sure I fully understood your question, but
>>>> if the idea is for kernel to decide about type of scan based on its
>>>> characteristics instead of specific type request performance may
>>>> cause confusion to wifi manager.
>>>> However, it would definitely simplify kernel API. Still I am not sure
>>>> if userspace wifi manager will "like" it.
>>>
>>> Actually the idea is to hide the notion of scan type entirely from the
>>> API and kernel code. If you add the scan type as an attribute in the
>>> API, you still need to provide additional attributes for that scan type
>>> so the question is what the scan type itself provides, ie. how does it
>>> affect the scan itself.
>>
>> Right, some scans types can be easily hidden behind scan parameters
>> like scan for SSID or BSSID or maybe even Roaming with list of
>> SSIDs or BSSIDs, or mix... However, scan history for example should
>> be separate, right?
>
> Not sure what scan history means. cfg80211 currently maintains the
> latest information for a given BSS. So I guess for a "scan history"
> request user-space can specify "history depth" parameter. It may become
> a bit memory intensive to retain all BSS information like that so we may
> need to identify the BSS attributes for which historic values are needed
> by user-space. Now what to do when a "scan history" is in progress and a
> "normal scan" is requested with flush flag attribute set?

Scan history is minimized scan results with time stamp usually restricted
to specific channels and maybe BSSIDs. It is definitely possible to
replace it with "scan depth" as you've suggested. I am not sure it is
worth it due to memory consumption. Also it is mostly offloaded feature.
Now, scan for history or scan for something else is just a request.
They can be combined. Results should be separated, but scan is scan.
This is the whole idea behind generic scan framework. If user wants
to flush current scan results - it is totally fine, it is not related
to history.

>>>> 4. There is an interesting question: to separate scan results for
>>>> connection and location or not? The problem is how to "trust" them.
>>>> I mean scan results are scan results, but for location we may decide
>>>> to look for fewer channels and even maybe specific BSSIDs, i.e. not
>>>> full scan results, and we may need to indicate that right now
>>>> scan results are not full in order not to confuse wifi manager.
>
> Another things is that when combining requests, we are combining results
> as well. So if requester A wants to look for SSIDs X and Y and requester
> B wants to look for SSID Z, both requester A and B will get results for
> X, Y, and Z when requests are combined.

We can use filtering attached to any request, so indeed we can combine
results, but we can answer proper values to request.

> Regards,
> Arend

2016-11-18 23:53:39

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Thu, Nov 17, 2016 at 12:56 PM, Arend Van Spriel
<[email protected]> wrote:
> On 16-11-2016 23:47, [email protected] wrote:
>> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
>> From: Dmitry Shmidt <[email protected]>
>> Date: Wed, 16 Nov 2016 14:27:26 -0800
>> Subject: [PATCH] RFC: Universal scan proposal
>>
>> Currently we have sched scan with possibility of various
>> intervals. We would like to extend it to support also
>> different types of scan.
>> In case of powerful wlan CPU, all this functionality
>> can be offloaded.
>> In general case FW processes additional scan requests
>> and puts them into queue based on start time and interval.
>> Once current request is fulfilled, FW adds it (if interval != 0)
>> again to the queue with proper interval. If requests are
>> overlapping, new request can be combined with either one before,
>> or one after, assuming that requests are not mutually exclusive.
>> Combining requests is done by combining scan channels, ssids,
>> bssids and types of scan result. Once combined request was fulfilled
>> it will be reinserted as two (or three) different requests based on
>> their type and interval.
>> Each request has attribute:
>> Type: connectivity / location
>
> Don't see this type in the patch below (guess it was a last minute
> addition ;-) ).
>
>> Report: none / batch / immediate
>> Request may have priority and can be inserted into
>> the head of the queue.
>> Types of scans:
>> - Normal scan
>> - Scheduled scan
>> - Hotlist (BSSID scan)
>> - Roaming
>> - AutoJoin
>
> Don't see the last one mentioned in the patch below.
>
>> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
>> Signed-off-by: Dmitry Shmidt <[email protected]>
>> ---
>> include/net/cfg80211.h | 37 +++++++++++++++++++++++++++++++------
>> 1 file changed, 31 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 14b51d7..1ae2570 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -1602,8 +1602,10 @@ struct cfg80211_sched_scan_plan {
>> * cycle. The driver may ignore this parameter and start
>> * immediately (or at any other time), if this feature is not
>> * supported.
>
> At the the top of this kernel doc section it probably still refers to
> struct cfg80211_sched_scan_request.
>
>> + * @uscan_type: uscan type (scan, sched_scan, hotlist, scan_history,
>> roaming)
>> + * @uscan_results: uscan results report (none/batch/immediate)
>
> If you change the type to struct cfg80211_uscan_request it seems a bit
> redundant to prefix the field names with 'uscan_'.
>
>> */
>> -struct cfg80211_sched_scan_request {
>> +struct cfg80211_uscan_request {
>> struct cfg80211_ssid *ssids;
>> int n_ssids;
>> u32 n_channels;
>> @@ -1621,6 +1623,10 @@ struct cfg80211_sched_scan_request {
>> u8 mac_addr[ETH_ALEN] __aligned(2);
>> u8 mac_addr_mask[ETH_ALEN] __aligned(2);
>>
>> + /* universal scan fields */
>
> As the struct name changed it seems to me all fields in the struct are
> universal scan fields.
>
>> + u32 uscan_type;
>> + u32 uscan_results;
>> +
>> /* internal */
>> struct wiphy *wiphy;
>> struct net_device *dev;
>> @@ -1633,6 +1639,21 @@ struct cfg80211_sched_scan_request {
>> };
>>
>> /**
>> + * struct cfg80211_uscan_capa - universal scan capabilities
>> + *
>> + * @uscan_type: uscan type (scan, sched_scan, hotlist, scan_history,
>> roaming)
>> + * @max_ssids: max amount of ssids in sched scan
>> + * @max_hotlist: max amount of bssids in hotlist
>> + * @max_history: max amount of records in history
>> + */
>> +struct cfg80211_uscan_capa {
>> + u32 uscan_type;
>> + u32 max_ssids;
>> + u32 max_hotlist;
>> + u32 max_history;
>> +};
>> +
>> +/**
>> * enum cfg80211_signal_type - signal type
>> *
>> * @CFG80211_SIGNAL_TYPE_NONE: no signal strength information available
>> @@ -2898,10 +2919,13 @@ struct cfg80211_ops {
>> int (*set_antenna)(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant);
>> int (*get_antenna)(struct wiphy *wiphy, u32 *tx_ant, u32 *rx_ant);
>>
>> - int (*sched_scan_start)(struct wiphy *wiphy,
>> + int (*uscan_get_capa)(struct wiphy *wiphy, struct net_device *dev,
>> + struct cfg80211_uscan_capa *uscan_capa);
>
> Not sure if people start worrying about the size of struct wiphy
> already, but for static information like capabilities I think a callback
> is overkill.
>
>> + int (*uscan_start)(struct wiphy *wiphy,
>> struct net_device *dev,
>> - struct cfg80211_sched_scan_request *request);
>> - int (*sched_scan_stop)(struct wiphy *wiphy, struct net_device
>> *dev);
>> + struct cfg80211_uscan_request *request);
>
> So given the two extra fields, what different driver/device behaviour is
> required, eg. if uscan_type == roaming what will be different compared
> to a normal scan.
>
>> + int (*uscan_stop)(struct wiphy *wiphy, struct net_device *dev,
>> + int uscan_id);
>
> The uscan_id is probably referring to a specific scan request started by
> uscan_start. So who hands out that id (wiphy->cookie_counter?) and how
> does the driver know about it. If cfg80211 determines the id it probably
> need to be passed in the uscan request.
>
>> int (*set_rekey_data)(struct wiphy *wiphy, struct net_device *dev,
>> struct cfg80211_gtk_rekey_data *data);
>> @@ -4305,11 +4329,12 @@ void cfg80211_scan_done(struct
>> cfg80211_scan_request *request,
>> struct cfg80211_scan_info *info);
>>
>> /**
>> - * cfg80211_sched_scan_results - notify that new scan results are
>> available
>> + * cfg80211_uscan_results - notify that new uscan results are available
>> *
>> * @wiphy: the wiphy which got scheduled scan results
>> + * @uscan_id: type of scan results
>
> Confused now. What is uscan_id here? Same as in .uscan_stop() callback?

Just to clarify - this is not a patch that is passing compilation.
It is something to discuss as a concept.
And we have several options - either have different set of functions for
different scans or to get id from start and use it for stop and notification.

>> */
>> -void cfg80211_sched_scan_results(struct wiphy *wiphy);
>> +void cfg80211_sched_scan_results(struct wiphy *wiphy, int uscan_id);
>
> should this be renamed to cfg80211_uscan_results().
>
>> /**
>> * cfg80211_sched_scan_stopped - notify that the scheduled scan has
>> stopped
>
> Also change to cfg80211_uscan_stopped()? Does it need an additional
> argument, ie. uscan_id.

Yes

> Regards,
> Arend

2016-11-22 20:54:22

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Tue, Nov 22, 2016 at 12:41 PM, Arend Van Spriel
<[email protected]> wrote:
> On 22-11-2016 18:29, Dmitry Shmidt wrote:
>> Hi Luca,
>>
>> On Mon, Nov 21, 2016 at 11:24 PM, Luca Coelho <[email protected]> wrote:
>>> Hi Dmitry,
>>> On Wed, 2016-11-16 at 22:47 +0000, [email protected] wrote:
>>>> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
>>>> From: Dmitry Shmidt <[email protected]>
>>>> Date: Wed, 16 Nov 2016 14:27:26 -0800
>>>> Subject: [PATCH] RFC: Universal scan proposal
>>>>
>>>> Currently we have sched scan with possibility of various
>>>> intervals. We would like to extend it to support also
>>>> different types of scan.
>>>> In case of powerful wlan CPU, all this functionality
>>>> can be offloaded.
>>>> In general case FW processes additional scan requests
>>>> and puts them into queue based on start time and interval.
>>>> Once current request is fulfilled, FW adds it (if interval != 0)
>>>> again to the queue with proper interval. If requests are
>>>> overlapping, new request can be combined with either one before,
>>>> or one after, assuming that requests are not mutually exclusive.
>>>> Combining requests is done by combining scan channels, ssids,
>>>> bssids and types of scan result. Once combined request was fulfilled
>>>> it will be reinserted as two (or three) different requests based on
>>>> their type and interval.
>>>> Each request has attribute:
>>>> Type: connectivity / location
>>>> Report: none / batch / immediate
>>>> Request may have priority and can be inserted into
>>>> the head of the queue.
>>>> Types of scans:
>>>> - Normal scan
>>>> - Scheduled scan
>>>> - Hotlist (BSSID scan)
>>>> - Roaming
>>>> - AutoJoin
>>>>
>>>> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
>>>> Signed-off-by: Dmitry Shmidt <[email protected]>
>>>> ---
>>>
>>> I like the initiative and I think this is definitely something that can
>>> improve concurrent scanning instances. But IMHO the most important is
>>> to discuss the semantics of this change, such as which scans can be
>>> combined, who makes the decisions of combining them, how priorities are
>>> sorted out etc. I think the types of scan are not relevant in the
>>> nl80211 API, but the characteristics of the scans are. For instance,
>>> "urgent scan" (for initial connection), best-effort scan for roaming...
>>> and latency requirements, such as low-latency for location and initial
>>> connection and high-latency for scheduled scan. Then we decided, in
>>> the kernel, how to combine and prioritize them according to their
>>> characteristics, instead of having to map scan types to these
>>> characteristics.
>>>
>>> What do you think?
>>
>> 1. Combining scans.
>> There are two scenarios in general: combine scans that can be offload
>> and scans that can not be offload due to "weak" FW / wlan SoC. In last
>> case this approach maybe not attractive at all - non-mobile device
>> may not need all these different types of scan.
>> In case of offload - it will be FW code decision - I just wanted to propose
>> the way how to do this efficiently.
>
> I think Luca is looking at it as a way to deal with multiple user-space
> entities request the device to perform a scan. Ignoring the scan types
> on android it can be wifi-hal and wpa_supplicant.
>
>> 2. Priority - very good point, we need to have it. I am just not sure
>> that we need like scale priority - maybe just flag - urgent / not urgent.
>> Urgent one will be inserted in queue as is and conflicting request
>> should be postponed or combined.
>
> What if we get another urgent one?

We may restrict it only to one urgent scan - what is the use case for
two requests?

>> 3. Scan types - I am not sure I fully understood your question, but
>> if the idea is for kernel to decide about type of scan based on its
>> characteristics instead of specific type request performance may
>> cause confusion to wifi manager.
>> However, it would definitely simplify kernel API. Still I am not sure
>> if userspace wifi manager will "like" it.
>
> Actually the idea is to hide the notion of scan type entirely from the
> API and kernel code. If you add the scan type as an attribute in the
> API, you still need to provide additional attributes for that scan type
> so the question is what the scan type itself provides, ie. how does it
> affect the scan itself.

Right, some scans types can be easily hidden behind scan parameters
like scan for SSID or BSSID or maybe even Roaming with list of
SSIDs or BSSIDs, or mix... However, scan history for example should
be separate, right?

>> 4. There is an interesting question: to separate scan results for
>> connection and location or not? The problem is how to "trust" them.
>> I mean scan results are scan results, but for location we may decide
>> to look for fewer channels and even maybe specific BSSIDs, i.e. not
>> full scan results, and we may need to indicate that right now
>> scan results are not full in order not to confuse wifi manager.
>
> Regards,
> Arend
>
>>> --
>>> Cheers,
>>> Luca.

2016-11-22 17:29:25

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

Hi Luca,

On Mon, Nov 21, 2016 at 11:24 PM, Luca Coelho <[email protected]> wrote:
> Hi Dmitry,
> On Wed, 2016-11-16 at 22:47 +0000, [email protected] wrote:
>> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
>> From: Dmitry Shmidt <[email protected]>
>> Date: Wed, 16 Nov 2016 14:27:26 -0800
>> Subject: [PATCH] RFC: Universal scan proposal
>>
>> Currently we have sched scan with possibility of various
>> intervals. We would like to extend it to support also
>> different types of scan.
>> In case of powerful wlan CPU, all this functionality
>> can be offloaded.
>> In general case FW processes additional scan requests
>> and puts them into queue based on start time and interval.
>> Once current request is fulfilled, FW adds it (if interval != 0)
>> again to the queue with proper interval. If requests are
>> overlapping, new request can be combined with either one before,
>> or one after, assuming that requests are not mutually exclusive.
>> Combining requests is done by combining scan channels, ssids,
>> bssids and types of scan result. Once combined request was fulfilled
>> it will be reinserted as two (or three) different requests based on
>> their type and interval.
>> Each request has attribute:
>> Type: connectivity / location
>> Report: none / batch / immediate
>> Request may have priority and can be inserted into
>> the head of the queue.
>> Types of scans:
>> - Normal scan
>> - Scheduled scan
>> - Hotlist (BSSID scan)
>> - Roaming
>> - AutoJoin
>>
>> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
>> Signed-off-by: Dmitry Shmidt <[email protected]>
>> ---
>
> I like the initiative and I think this is definitely something that can
> improve concurrent scanning instances. But IMHO the most important is
> to discuss the semantics of this change, such as which scans can be
> combined, who makes the decisions of combining them, how priorities are
> sorted out etc. I think the types of scan are not relevant in the
> nl80211 API, but the characteristics of the scans are. For instance,
> "urgent scan" (for initial connection), best-effort scan for roaming...
> and latency requirements, such as low-latency for location and initial
> connection and high-latency for scheduled scan. Then we decided, in
> the kernel, how to combine and prioritize them according to their
> characteristics, instead of having to map scan types to these
> characteristics.
>
> What do you think?

1. Combining scans.
There are two scenarios in general: combine scans that can be offload
and scans that can not be offload due to "weak" FW / wlan SoC. In last
case this approach maybe not attractive at all - non-mobile device
may not need all these different types of scan.
In case of offload - it will be FW code decision - I just wanted to propose
the way how to do this efficiently.

2. Priority - very good point, we need to have it. I am just not sure
that we need like scale priority - maybe just flag - urgent / not urgent.
Urgent one will be inserted in queue as is and conflicting request
should be postponed or combined.

3. Scan types - I am not sure I fully understood your question, but
if the idea is for kernel to decide about type of scan based on its
characteristics instead of specific type request performance may
cause confusion to wifi manager.
However, it would definitely simplify kernel API. Still I am not sure
if userspace wifi manager will "like" it.

4. There is an interesting question: to separate scan results for
connection and location or not? The problem is how to "trust" them.
I mean scan results are scan results, but for location we may decide
to look for fewer channels and even maybe specific BSSIDs, i.e. not
full scan results, and we may need to indicate that right now
scan results are not full in order not to confuse wifi manager.

> --
> Cheers,
> Luca.

2016-11-23 08:43:52

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On 22-11-2016 21:54, Dmitry Shmidt wrote:
> On Tue, Nov 22, 2016 at 12:41 PM, Arend Van Spriel
> <[email protected]> wrote:
>> On 22-11-2016 18:29, Dmitry Shmidt wrote:
>>> Hi Luca,
>>>
>>> On Mon, Nov 21, 2016 at 11:24 PM, Luca Coelho <[email protected]> wrote:
>>>> Hi Dmitry,
>>>> On Wed, 2016-11-16 at 22:47 +0000, [email protected] wrote:
>>>>> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
>>>>> From: Dmitry Shmidt <[email protected]>
>>>>> Date: Wed, 16 Nov 2016 14:27:26 -0800
>>>>> Subject: [PATCH] RFC: Universal scan proposal
>>>>>
>>>>> Currently we have sched scan with possibility of various
>>>>> intervals. We would like to extend it to support also
>>>>> different types of scan.
>>>>> In case of powerful wlan CPU, all this functionality
>>>>> can be offloaded.
>>>>> In general case FW processes additional scan requests
>>>>> and puts them into queue based on start time and interval.
>>>>> Once current request is fulfilled, FW adds it (if interval != 0)
>>>>> again to the queue with proper interval. If requests are
>>>>> overlapping, new request can be combined with either one before,
>>>>> or one after, assuming that requests are not mutually exclusive.
>>>>> Combining requests is done by combining scan channels, ssids,
>>>>> bssids and types of scan result. Once combined request was fulfilled
>>>>> it will be reinserted as two (or three) different requests based on
>>>>> their type and interval.
>>>>> Each request has attribute:
>>>>> Type: connectivity / location
>>>>> Report: none / batch / immediate
>>>>> Request may have priority and can be inserted into
>>>>> the head of the queue.
>>>>> Types of scans:
>>>>> - Normal scan
>>>>> - Scheduled scan
>>>>> - Hotlist (BSSID scan)
>>>>> - Roaming
>>>>> - AutoJoin
>>>>>
>>>>> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
>>>>> Signed-off-by: Dmitry Shmidt <[email protected]>
>>>>> ---
>>>>
>>>> I like the initiative and I think this is definitely something that can
>>>> improve concurrent scanning instances. But IMHO the most important is
>>>> to discuss the semantics of this change, such as which scans can be
>>>> combined, who makes the decisions of combining them, how priorities are
>>>> sorted out etc. I think the types of scan are not relevant in the
>>>> nl80211 API, but the characteristics of the scans are. For instance,
>>>> "urgent scan" (for initial connection), best-effort scan for roaming...
>>>> and latency requirements, such as low-latency for location and initial
>>>> connection and high-latency for scheduled scan. Then we decided, in
>>>> the kernel, how to combine and prioritize them according to their
>>>> characteristics, instead of having to map scan types to these
>>>> characteristics.
>>>>
>>>> What do you think?
>>>
>>> 1. Combining scans.
>>> There are two scenarios in general: combine scans that can be offload
>>> and scans that can not be offload due to "weak" FW / wlan SoC. In last
>>> case this approach maybe not attractive at all - non-mobile device
>>> may not need all these different types of scan.
>>> In case of offload - it will be FW code decision - I just wanted to propose
>>> the way how to do this efficiently.
>>
>> I think Luca is looking at it as a way to deal with multiple user-space
>> entities request the device to perform a scan. Ignoring the scan types
>> on android it can be wifi-hal and wpa_supplicant.
>>
>>> 2. Priority - very good point, we need to have it. I am just not sure
>>> that we need like scale priority - maybe just flag - urgent / not urgent.
>>> Urgent one will be inserted in queue as is and conflicting request
>>> should be postponed or combined.
>>
>> What if we get another urgent one?
>
> We may restrict it only to one urgent scan - what is the use case for
> two requests?

We don't know I guess so we can restrict to one for now. Just wondered
reading this.

>>> 3. Scan types - I am not sure I fully understood your question, but
>>> if the idea is for kernel to decide about type of scan based on its
>>> characteristics instead of specific type request performance may
>>> cause confusion to wifi manager.
>>> However, it would definitely simplify kernel API. Still I am not sure
>>> if userspace wifi manager will "like" it.
>>
>> Actually the idea is to hide the notion of scan type entirely from the
>> API and kernel code. If you add the scan type as an attribute in the
>> API, you still need to provide additional attributes for that scan type
>> so the question is what the scan type itself provides, ie. how does it
>> affect the scan itself.
>
> Right, some scans types can be easily hidden behind scan parameters
> like scan for SSID or BSSID or maybe even Roaming with list of
> SSIDs or BSSIDs, or mix... However, scan history for example should
> be separate, right?

Not sure what scan history means. cfg80211 currently maintains the
latest information for a given BSS. So I guess for a "scan history"
request user-space can specify "history depth" parameter. It may become
a bit memory intensive to retain all BSS information like that so we may
need to identify the BSS attributes for which historic values are needed
by user-space. Now what to do when a "scan history" is in progress and a
"normal scan" is requested with flush flag attribute set?

>>> 4. There is an interesting question: to separate scan results for
>>> connection and location or not? The problem is how to "trust" them.
>>> I mean scan results are scan results, but for location we may decide
>>> to look for fewer channels and even maybe specific BSSIDs, i.e. not
>>> full scan results, and we may need to indicate that right now
>>> scan results are not full in order not to confuse wifi manager.

Another things is that when combining requests, we are combining results
as well. So if requester A wants to look for SSIDs X and Y and requester
B wants to look for SSID Z, both requester A and B will get results for
X, Y, and Z when requests are combined.

Regards,
Arend

2016-11-22 07:24:15

by Luca Coelho

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

Hi Dmitry,
On Wed, 2016-11-16 at 22:47 +0000, [email protected] wrote:
> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
> From: Dmitry Shmidt <[email protected]>
> Date: Wed, 16 Nov 2016 14:27:26 -0800
> Subject: [PATCH] RFC: Universal scan proposal
>
> Currently we have sched scan with possibility of various
> intervals. We would like to extend it to support also
> different types of scan.
> In case of powerful wlan CPU, all this functionality
> can be offloaded.
> In general case FW processes additional scan requests
> and puts them into queue based on start time and interval.
> Once current request is fulfilled, FW adds it (if interval != 0)
> again to the queue with proper interval. If requests are
> overlapping, new request can be combined with either one before,
> or one after, assuming that requests are not mutually exclusive.
> Combining requests is done by combining scan channels, ssids,
> bssids and types of scan result. Once combined request was fulfilled
> it will be reinserted as two (or three) different requests based on
> their type and interval.
> Each request has attribute:
> Type: connectivity / location
> Report: none / batch / immediate
> Request may have priority and can be inserted into
> the head of the queue.
> Types of scans:
> - Normal scan
> - Scheduled scan
> - Hotlist (BSSID scan)
> - Roaming
> - AutoJoin
>
> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
> Signed-off-by: Dmitry Shmidt <[email protected]>
> ---

I like the initiative and I think this is definitely something that can
improve concurrent scanning instances. But IMHO the most important is
to discuss the semantics of this change, such as which scans can be
combined, who makes the decisions of combining them, how priorities are
sorted out etc. I think the types of scan are not relevant in the
nl80211 API, but the characteristics of the scans are. For instance,
"urgent scan" (for initial connection), best-effort scan for roaming...
and latency requirements, such as low-latency for location and initial
connection and high-latency for scheduled scan. Then we decided, in
the kernel, how to combine and prioritize them according to their
characteristics, instead of having to map scan types to these
characteristics.

What do you think?

--
Cheers,
Luca.

2016-12-07 18:39:37

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Tue, Dec 6, 2016 at 10:44 PM, Johannes Berg
<[email protected]> wrote:
>
>> Indeed, results are results. I just want to take care of two things:
>> 1) Memory consumption - we can clear stale scan results for
>> connection, but not for location if we are using history scan.
>
> Well eventually we also have to clear for location if we run out of
> memory, that usually means dumping them out to the host, no?

Being out of memory and consuming more memory are different
things, but I agree - maybe we don't need to worry about it.

>> 2) Use of insufficient results for connection - in case we had
>> history or hotlist scan only for very limited amount of channels,
>> then we may not have enough APs in our result for "sterling"
>> connection decision.
>
> I'm not entirely sure about this case - surely noticing "we can do
> better now" is still better than waiting for being able to make the
> perfect decision?

Maybe we can just keep flag saying that currently available results
were not received by usual full scan.

>> > > Report: none / batch / immediate
>> >
>> > Not sure I see much point in "none"??
>> >
>> > Can you define these more clearly? Do you think "batch" reporting
>> > should be like the gscan buckets? Or actually have full
>> > information?
>>
>> None - means that there is not need to report. It can be useful
>> in case of roaming scan, scheduling or hotlist scan - you didn't find
>> anything suitable - don't report that there is no scan results.
>
> But that seems more of a filtering thing, combined with "immediate" for
> anything passing the filter?

We can use this approach as well.

>> > > Request may have priority and can be inserted into
>> > > the head of the queue.
>> > > Types of scans:
>> > > - Normal scan
>> > > - Scheduled scan
>> > > - Hotlist (BSSID scan)
>> > > - Roaming
>> > > - AutoJoin
>> >
>> > I think somebody else said this but I didn't find it now - I think
>> > this would make more sense to define in terms of expected behaviour
>> > than use cases for each type of scan.
>>
>> I think Luca made this statement.
>
> Yeah - I just couldn't find it again on re-reading the thread :)
>
>> It is totally ok from SW point of
>> view - especially due to the fact that scan is scan. However,
>> I suspect it will be harder to handle from user experience. I mean
>> at the end wireless framework / driver / FW will convert special
>> scan type to usual scan with special params and response, but why
>> to put this burden on user?
>
> I just think it's more flexible and open-ended. The actual definition
> of the resulting parameters needs to be somewhere anyway - putting it
> into driver/firmware (vs. wifi framework or so) seems to duplicate it
> and certainly makes it harder to modify/extend in the future, no?

So, let's summarize:
Instead of creating new type of generic scan with special types,
we want to go with additional expansion of scheduled scan options and
parameters (in order not to "multiply entities"), including ability to send
new scheduled scan request without stopping previous one.

Is it Ok?

> johannes

2016-12-09 11:10:51

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On 8-12-2016 23:35, Dmitry Shmidt wrote:
> On Wed, Dec 7, 2016 at 12:51 PM, Arend Van Spriel
> <[email protected]> wrote:
>> On 7-12-2016 19:39, Dmitry Shmidt wrote:
>>> On Tue, Dec 6, 2016 at 10:44 PM, Johannes Berg
>>> <[email protected]> wrote:
>>>>
>>>>> Indeed, results are results. I just want to take care of two things:
>>>>> 1) Memory consumption - we can clear stale scan results for
>>>>> connection, but not for location if we are using history scan.
>>>>
>>>> Well eventually we also have to clear for location if we run out of
>>>> memory, that usually means dumping them out to the host, no?
>>>
>>> Being out of memory and consuming more memory are different
>>> things, but I agree - maybe we don't need to worry about it.

So does location use all information from the scan history or is it
interested in some specific information, eg. rssi.

>>>>> 2) Use of insufficient results for connection - in case we had
>>>>> history or hotlist scan only for very limited amount of channels,
>>>>> then we may not have enough APs in our result for "sterling"
>>>>> connection decision.
>>>>
>>>> I'm not entirely sure about this case - surely noticing "we can do
>>>> better now" is still better than waiting for being able to make the
>>>> perfect decision?
>>>
>>> Maybe we can just keep flag saying that currently available results
>>> were not received by usual full scan.
>>>
>>>>>>> Report: none / batch / immediate
>>>>>>
>>>>>> Not sure I see much point in "none"??
>>>>>>
>>>>>> Can you define these more clearly? Do you think "batch" reporting
>>>>>> should be like the gscan buckets? Or actually have full
>>>>>> information?
>>>>>
>>>>> None - means that there is not need to report. It can be useful
>>>>> in case of roaming scan, scheduling or hotlist scan - you didn't find
>>>>> anything suitable - don't report that there is no scan results.
>>>>
>>>> But that seems more of a filtering thing, combined with "immediate" for
>>>> anything passing the filter?
>>>
>>> We can use this approach as well.
>>>
>>>>>>> Request may have priority and can be inserted into
>>>>>>> the head of the queue.
>>>>>>> Types of scans:
>>>>>>> - Normal scan
>>>>>>> - Scheduled scan
>>>>>>> - Hotlist (BSSID scan)
>>>>>>> - Roaming
>>>>>>> - AutoJoin
>>>>>>
>>>>>> I think somebody else said this but I didn't find it now - I think
>>>>>> this would make more sense to define in terms of expected behaviour
>>>>>> than use cases for each type of scan.
>>>>>
>>>>> I think Luca made this statement.
>>>>
>>>> Yeah - I just couldn't find it again on re-reading the thread :)
>>>>
>>>>> It is totally ok from SW point of
>>>>> view - especially due to the fact that scan is scan. However,
>>>>> I suspect it will be harder to handle from user experience. I mean
>>>>> at the end wireless framework / driver / FW will convert special
>>>>> scan type to usual scan with special params and response, but why
>>>>> to put this burden on user?
>>
>> I don't think this will put burden on the user although it depends
>> who/what you mean by this. If you mean the mere mortal end-user I would
>> say no as indeed there must be some software entity (in user-space) that
>> will have to initiate a nl80211 command with appropriate attributes
>> according to whatever user-space is trying to accomplish.
>>
>>>> I just think it's more flexible and open-ended. The actual definition
>>>> of the resulting parameters needs to be somewhere anyway - putting it
>>>> into driver/firmware (vs. wifi framework or so) seems to duplicate it
>>>> and certainly makes it harder to modify/extend in the future, no?
>>>
>>> So, let's summarize:
>>> Instead of creating new type of generic scan with special types,
>>> we want to go with additional expansion of scheduled scan options and
>>> parameters (in order not to "multiply entities"), including ability to send
>>> new scheduled scan request without stopping previous one.
>>>
>>> Is it Ok?
>>
>> Sounds ok. To me a generic scan command with type attribute is
>> equivalent to having seperate commands for each type so there seems to
>> be no gain. Now whether this all can be accomplished by extending the
>> scheduled scan depends on the problem that you are trying to solve.
>>
>> Main purpose seems to be offloading different scanning tasks which could
>> make scheduled scan a good candidate. Now I want to add gscan to this
>> mix as it seems some concepts for that are in play in this discussion as
>> well, eg. hotlist. gscan is like scheduled scan, but it supports
>> multiple schedules. However, it is still a single request from a single
>> user-space process. I think Luca mentioned supporting requests from
>> different user-space processes. Is that also what you mean by "ability
>> to send new scheduled scan request without stopping previous one" or is
>> that still from a single user-space process. Do we need a limit to the
>> amount of scheduled scan requests that can be handled simultaneously.
>
> Supporting requests (or more precisely requests and results) differentiated
> by user-space entity can be tricky. Right now we are not checking current
> caller pid, right? Maybe it is also good idea - or maybe we can just
> make result filtering per user-space caller?

There is already struct cfg80211_sched_scan_request::owner_nlportid

* @owner_nlportid: netlink portid of owner (if this should is a request
* owned by a particular socket)

It is set in nl80211_start_sched_scan():

if (info->attrs[NL80211_ATTR_SOCKET_OWNER])
sched_scan_req->owner_nlportid = info->snd_portid;

It basically links the life-time of the request to the socket connection
if requested to do so. It is just not very useful with tools like iw.

>> A maybe more important aspect of gscan is user-space control of result
>> reporting and thus the frequency of waking up host and/or user-space. I
>> suspect this would be needed in the scheduled scan extension as well, right?
>
> It will be always up to user-space caller to make system more or less
> power-efficient, right?

That depends on the API, right? If the API provides the knobs, than
user-space can indeed make the decision.

Regards,
Arend

>> Regards,
>> Arend

2016-12-07 06:44:23

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

> Indeed, results are results. I just want to take care of two things:
> 1) Memory consumption - we can clear stale scan results for
> connection, but not for location if we are using history scan.

Well eventually we also have to clear for location if we run out of
memory, that usually means dumping them out to the host, no?

> 2) Use of insufficient results for connection - in case we had
> history or hotlist scan only for very limited amount of channels,
> then we may not have enough APs in our result for "sterling"
> connection decision.

I'm not entirely sure about this case - surely noticing "we can do
better now" is still better than waiting for being able to make the
perfect decision?

> > > Report: none / batch / immediate
> >
> > Not sure I see much point in "none"??
> >
> > Can you define these more clearly? Do you think "batch" reporting
> > should be like the gscan buckets? Or actually have full
> > information?
>
> None - means that there is not need to report. It can be useful
> in case of roaming scan, scheduling or hotlist scan - you didn't find
> anything suitable - don't report that there is no scan results.

But that seems more of a filtering thing, combined with "immediate" for
anything passing the filter?

> > > Request may have priority and can be inserted into
> > > the head of the queue.
> > > Types of scans:
> > > - Normal scan
> > > - Scheduled scan
> > > - Hotlist (BSSID scan)
> > > - Roaming
> > > - AutoJoin
> >
> > I think somebody else said this but I didn't find it now - I think
> > this would make more sense to define in terms of expected behaviour
> > than use cases for each type of scan.
>
> I think Luca made this statement.

Yeah - I just couldn't find it again on re-reading the thread :)

> It is totally ok from SW point of
> view - especially due to the fact that scan is scan. However,
> I suspect it will be harder to handle from user experience. I mean
> at the end wireless framework / driver / FW will convert special
> scan type to usual scan with special params and response, but why
> to put this burden on user?

I just think it's more flexible and open-ended. The actual definition
of the resulting parameters needs to be somewhere anyway - putting it
into driver/firmware (vs. wifi framework or so) seems to duplicate it
and certainly makes it harder to modify/extend in the future, no?

johannes

2016-12-08 22:35:05

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Wed, Dec 7, 2016 at 12:51 PM, Arend Van Spriel
<[email protected]> wrote:
> On 7-12-2016 19:39, Dmitry Shmidt wrote:
>> On Tue, Dec 6, 2016 at 10:44 PM, Johannes Berg
>> <[email protected]> wrote:
>>>
>>>> Indeed, results are results. I just want to take care of two things:
>>>> 1) Memory consumption - we can clear stale scan results for
>>>> connection, but not for location if we are using history scan.
>>>
>>> Well eventually we also have to clear for location if we run out of
>>> memory, that usually means dumping them out to the host, no?
>>
>> Being out of memory and consuming more memory are different
>> things, but I agree - maybe we don't need to worry about it.
>>
>>>> 2) Use of insufficient results for connection - in case we had
>>>> history or hotlist scan only for very limited amount of channels,
>>>> then we may not have enough APs in our result for "sterling"
>>>> connection decision.
>>>
>>> I'm not entirely sure about this case - surely noticing "we can do
>>> better now" is still better than waiting for being able to make the
>>> perfect decision?
>>
>> Maybe we can just keep flag saying that currently available results
>> were not received by usual full scan.
>>
>>>>>> Report: none / batch / immediate
>>>>>
>>>>> Not sure I see much point in "none"??
>>>>>
>>>>> Can you define these more clearly? Do you think "batch" reporting
>>>>> should be like the gscan buckets? Or actually have full
>>>>> information?
>>>>
>>>> None - means that there is not need to report. It can be useful
>>>> in case of roaming scan, scheduling or hotlist scan - you didn't find
>>>> anything suitable - don't report that there is no scan results.
>>>
>>> But that seems more of a filtering thing, combined with "immediate" for
>>> anything passing the filter?
>>
>> We can use this approach as well.
>>
>>>>>> Request may have priority and can be inserted into
>>>>>> the head of the queue.
>>>>>> Types of scans:
>>>>>> - Normal scan
>>>>>> - Scheduled scan
>>>>>> - Hotlist (BSSID scan)
>>>>>> - Roaming
>>>>>> - AutoJoin
>>>>>
>>>>> I think somebody else said this but I didn't find it now - I think
>>>>> this would make more sense to define in terms of expected behaviour
>>>>> than use cases for each type of scan.
>>>>
>>>> I think Luca made this statement.
>>>
>>> Yeah - I just couldn't find it again on re-reading the thread :)
>>>
>>>> It is totally ok from SW point of
>>>> view - especially due to the fact that scan is scan. However,
>>>> I suspect it will be harder to handle from user experience. I mean
>>>> at the end wireless framework / driver / FW will convert special
>>>> scan type to usual scan with special params and response, but why
>>>> to put this burden on user?
>
> I don't think this will put burden on the user although it depends
> who/what you mean by this. If you mean the mere mortal end-user I would
> say no as indeed there must be some software entity (in user-space) that
> will have to initiate a nl80211 command with appropriate attributes
> according to whatever user-space is trying to accomplish.
>
>>> I just think it's more flexible and open-ended. The actual definition
>>> of the resulting parameters needs to be somewhere anyway - putting it
>>> into driver/firmware (vs. wifi framework or so) seems to duplicate it
>>> and certainly makes it harder to modify/extend in the future, no?
>>
>> So, let's summarize:
>> Instead of creating new type of generic scan with special types,
>> we want to go with additional expansion of scheduled scan options and
>> parameters (in order not to "multiply entities"), including ability to send
>> new scheduled scan request without stopping previous one.
>>
>> Is it Ok?
>
> Sounds ok. To me a generic scan command with type attribute is
> equivalent to having seperate commands for each type so there seems to
> be no gain. Now whether this all can be accomplished by extending the
> scheduled scan depends on the problem that you are trying to solve.
>
> Main purpose seems to be offloading different scanning tasks which could
> make scheduled scan a good candidate. Now I want to add gscan to this
> mix as it seems some concepts for that are in play in this discussion as
> well, eg. hotlist. gscan is like scheduled scan, but it supports
> multiple schedules. However, it is still a single request from a single
> user-space process. I think Luca mentioned supporting requests from
> different user-space processes. Is that also what you mean by "ability
> to send new scheduled scan request without stopping previous one" or is
> that still from a single user-space process. Do we need a limit to the
> amount of scheduled scan requests that can be handled simultaneously.

Supporting requests (or more precisely requests and results) differentiated
by user-space entity can be tricky. Right now we are not checking current
caller pid, right? Maybe it is also good idea - or maybe we can just
make result filtering per user-space caller?

> A maybe more important aspect of gscan is user-space control of result
> reporting and thus the frequency of waking up host and/or user-space. I
> suspect this would be needed in the scheduled scan extension as well, right?

It will be always up to user-space caller to make system more or less
power-efficient, right?

> Regards,
> Arend

2016-12-13 16:04:33

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

> > Well eventually we also have to clear for location if we run out of
> > memory, that usually means dumping them out to the host, no?
>
> Being out of memory and consuming more memory are different
> things, but I agree - maybe we don't need to worry about it.

Well, reaching the limit of what we're willing to spend on it is
equivalent I guess :)

> > I'm not entirely sure about this case - surely noticing "we can do
> > better now" is still better than waiting for being able to make the
> > perfect decision?
>
> Maybe we can just keep flag saying that currently available results
> were not received by usual full scan.

Elsewhere we were planning per-channel results, and a cookie to filter
them - perhaps we could have a similar thing where you may even have to
request these scan results specifically with a certain cookie you got
from the scanning, or so. Or indicate the cookie there so you can tie
it back to the scan request somehow?

> So, let's summarize:
> Instead of creating new type of generic scan with special types,
> we want to go with additional expansion of scheduled scan options and
> parameters (in order not to "multiply entities"), including ability
> to send new scheduled scan request without stopping previous one.
>
> Is it Ok?

Sounds fine to me.

johannes

2016-12-21 10:20:40

[permalink] [raw]

Subject: [RFC] nl80211: allow multiple active scheduled scan requests

This patch implements the idea to have multiple scheduled scan requests
running concurrently. It mainly illustrates how to deal with the incoming
request from user-space in terms of backward compatibility. In order to
use multiple scheduled scans user-space needs to provide a flag attribute
NL80211_ATTR_SCHED_SCAN_MULTI to indicate support. If not the request is
treated as a legacy scan.

Cc: Dmitry Shmidt <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
Hi Johannes,

Did a bit of coding on the Universal/multi-scheduled scan idea. This
just deals with handling requests. Not sure if I got the RCU stuff
right so any remarks are welcome.

Regards,
Arend
---
include/net/cfg80211.h | 7 ++++
include/uapi/linux/nl80211.h | 12 ++++++-
net/wireless/core.c | 30 ++++++++++------
net/wireless/core.h | 10 +++++-
net/wireless/nl80211.c | 38 ++++++++++++++++++--
net/wireless/scan.c | 83 +++++++++++++++++++++++++++++++++++---------
6 files changed, 148 insertions(+), 32 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 814be4b..f5c0592 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1594,6 +1594,7 @@ struct cfg80211_sched_scan_plan {
/**
* struct cfg80211_sched_scan_request - scheduled scan request description
*
+ * @reqid: identifies this request.
* @ssids: SSIDs to scan for (passed in the probe_reqs in active scans)
* @n_ssids: number of SSIDs
* @n_channels: total number of channels to scan
@@ -1622,12 +1623,14 @@ struct cfg80211_sched_scan_plan {
* @rcu_head: RCU callback used to free the struct
* @owner_nlportid: netlink portid of owner (if this should is a request
* owned by a particular socket)
+ * @list: for keeping list of requests.
* @delay: delay in seconds to use before starting the first scan
* cycle. The driver may ignore this parameter and start
* immediately (or at any other time), if this feature is not
* supported.
*/
struct cfg80211_sched_scan_request {
+ u64 reqid;
struct cfg80211_ssid *ssids;
int n_ssids;
u32 n_channels;
@@ -1651,6 +1654,7 @@ struct cfg80211_sched_scan_request {
unsigned long scan_start;
struct rcu_head rcu_head;
u32 owner_nlportid;
+ struct list_head list;

/* keep last */
struct ieee80211_channel *channels[0];
@@ -3415,6 +3419,8 @@ struct wiphy_iftype_ext_capab {
* this variable determines its size
* @max_scan_ssids: maximum number of SSIDs the device can scan for in
* any given scan
+ * @max_sched_scan_reqs: maximum number of scheduled scan requests that
+ * the device can run concurrently.
* @max_sched_scan_ssids: maximum number of SSIDs the device can scan
* for in any given scheduled scan
* @max_match_sets: maximum number of match sets the device can handle
@@ -3547,6 +3553,7 @@ struct wiphy {

int bss_priv_size;
u8 max_scan_ssids;
+ u8 max_sched_scan_reqs;
u8 max_sched_scan_ssids;
u8 max_match_sets;
u16 max_scan_ie_len;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 6b76e3b..4045058 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -351,7 +351,9 @@
* are used. Extra IEs can also be passed from the userspace by
* using the %NL80211_ATTR_IE attribute. The first cycle of the
* scheduled scan can be delayed by %NL80211_ATTR_SCHED_SCAN_DELAY
- * is supplied.
+ * is supplied. If the device supports multiple concurrent scheduled
+ * scans, it will allow such when the caller provides the flag attribute
+ * %NL80211_ATTR_SCHED_SCAN_MULTI to indicate user-space support for it.
* @NL80211_CMD_STOP_SCHED_SCAN: stop a scheduled scan. Returns -ENOENT if
* scheduled scan is not running. The caller may assume that as soon
* as the call returns, it is safe to start a new scheduled scan again.
@@ -1980,6 +1982,11 @@ enum nl80211_commands {
* @NL80211_ATTR_BSSID: The BSSID of the AP. Note that %NL80211_ATTR_MAC is also
* used in various commands/events for specifying the BSSID.
*
+ * @NL80211_ATTR_SCHED_SCAN_MULTI: flag attribute which user-space shall use to
+ * indicate that it supports multiple active scheduled scan requests.
+ * @NL80211_ATTR_SCHED_SCAN_MAX_REQS: indicates maximum number of scheduled
+ * scan request that may be active for the device (u8).
+ *
* @NUM_NL80211_ATTR: total number of nl80211_attrs available
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2386,6 +2393,9 @@ enum nl80211_attrs {

NL80211_ATTR_BSSID,

+ NL80211_ATTR_SCHED_SCAN_MULTI,
+ NL80211_ATTR_SCHED_SCAN_MAX_REQS,
+
/* add attributes here, update the policy in nl80211.c */

__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 158c59e..1584234 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -346,13 +346,17 @@ static void cfg80211_destroy_iface_wk(struct work_struct *work)
static void cfg80211_sched_scan_stop_wk(struct work_struct *work)
{
struct cfg80211_registered_device *rdev;
+ struct cfg80211_sched_scan_request *pos, *tmp;

rdev = container_of(work, struct cfg80211_registered_device,
sched_scan_stop_wk);

rtnl_lock();

- __cfg80211_stop_sched_scan(rdev, false);
+ /* request gets removed from list so need safe iterator */
+ list_for_each_entry_safe(pos, tmp, &rdev->sched_scan_req_list, list) {
+ cfg80211_stop_sched_scan_req(rdev, pos, false);
+ }

rtnl_unlock();
}
@@ -436,6 +440,7 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
spin_lock_init(&rdev->beacon_registrations_lock);
spin_lock_init(&rdev->bss_lock);
INIT_LIST_HEAD(&rdev->bss_list);
+ INIT_LIST_HEAD(&rdev->sched_scan_req_list);
INIT_WORK(&rdev->scan_done_wk, __cfg80211_scan_done);
INIT_WORK(&rdev->sched_scan_results_wk, __cfg80211_sched_scan_results);
INIT_LIST_HEAD(&rdev->mlme_unreg);
@@ -690,6 +695,10 @@ int wiphy_register(struct wiphy *wiphy)
(wiphy->bss_select_support & ~(BIT(__NL80211_BSS_SELECT_ATTR_AFTER_LAST) - 2))))
return -EINVAL;

+ if ((wiphy->flags & WIPHY_FLAG_SUPPORTS_SCHED_SCAN) &&
+ !wiphy->max_sched_scan_reqs)
+ wiphy->max_sched_scan_reqs = 1;
+
if (wiphy->addresses)
memcpy(wiphy->perm_addr, wiphy->addresses[0].addr, ETH_ALEN);

@@ -1000,7 +1009,7 @@ void __cfg80211_leave(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev)
{
struct net_device *dev = wdev->netdev;
- struct cfg80211_sched_scan_request *sched_scan_req;
+ struct cfg80211_sched_scan_request *pos, *tmp;

ASSERT_RTNL();
ASSERT_WDEV_LOCK(wdev);
@@ -1011,9 +1020,10 @@ void __cfg80211_leave(struct cfg80211_registered_device *rdev,
break;
case NL80211_IFTYPE_P2P_CLIENT:
case NL80211_IFTYPE_STATION:
- sched_scan_req = rtnl_dereference(rdev->sched_scan_req);
- if (sched_scan_req && dev == sched_scan_req->dev)
- __cfg80211_stop_sched_scan(rdev, false);
+ list_for_each_entry_safe(pos, tmp, &rdev->sched_scan_req_list, list) {
+ if (dev == pos->dev)
+ cfg80211_stop_sched_scan_req(rdev, pos, false);
+ }

#ifdef CONFIG_CFG80211_WEXT
kfree(wdev->wext.ie);
@@ -1088,7 +1098,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
struct wireless_dev *wdev = dev->ieee80211_ptr;
struct cfg80211_registered_device *rdev;
- struct cfg80211_sched_scan_request *sched_scan_req;
+ struct cfg80211_sched_scan_request *pos, *tmp;

if (!wdev)
return NOTIFY_DONE;
@@ -1155,10 +1165,10 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
___cfg80211_scan_done(rdev, false);
}

- sched_scan_req = rtnl_dereference(rdev->sched_scan_req);
- if (WARN_ON(sched_scan_req &&
- sched_scan_req->dev == wdev->netdev)) {
- __cfg80211_stop_sched_scan(rdev, false);
+ list_for_each_entry_safe(pos, tmp,
+ &rdev->sched_scan_req_list, list) {
+ if (WARN_ON(pos && pos->dev == wdev->netdev))
+ cfg80211_stop_sched_scan_req(rdev, pos, false);
}

rdev->opencount--;
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 9820fa2..951954b 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -73,6 +73,8 @@ struct cfg80211_registered_device {
u32 bss_generation;
struct cfg80211_scan_request *scan_req; /* protected by RTNL */
struct sk_buff *scan_msg;
+ u8 sched_scan_req_count;
+ struct list_head sched_scan_req_list;
struct cfg80211_sched_scan_request __rcu *sched_scan_req;
unsigned long suspend_at;
struct work_struct scan_done_wk;
@@ -419,9 +421,15 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
void __cfg80211_scan_done(struct work_struct *wk);
void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev,
bool send_message);
+void cfg80211_add_sched_scan_req(struct cfg80211_registered_device *rdev,
+ struct cfg80211_sched_scan_request *req);
+bool cfg80211_legacy_sched_scan_active(struct cfg80211_registered_device *rdev);
void __cfg80211_sched_scan_results(struct work_struct *wk);
+int cfg80211_stop_sched_scan_req(struct cfg80211_registered_device *rdev,
+ struct cfg80211_sched_scan_request *req,
+ bool driver_initiated);
int __cfg80211_stop_sched_scan(struct cfg80211_registered_device *rdev,
- bool driver_initiated);
+ u64 reqid, bool driver_initiated);
void cfg80211_upload_connect_keys(struct wireless_dev *wdev);
int cfg80211_change_iface(struct cfg80211_registered_device *rdev,
struct net_device *dev, enum nl80211_iftype ntype,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 3df85a7..f1b253c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -405,6 +405,7 @@ enum nl80211_multicast_groups {
[NL80211_ATTR_FILS_NONCES] = { .len = 2 * FILS_NONCE_LEN },
[NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED] = { .type = NLA_FLAG, },
[NL80211_ATTR_BSSID] = { .len = ETH_ALEN },
+ [NL80211_ATTR_SCHED_SCAN_MULTI] = { .type = NLA_FLAG },
};

/* policy for the key attributes */
@@ -1463,6 +1464,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
rdev->wiphy.coverage_class) ||
nla_put_u8(msg, NL80211_ATTR_MAX_NUM_SCAN_SSIDS,
rdev->wiphy.max_scan_ssids) ||
+ nla_put_u8(msg, NL80211_ATTR_SCHED_SCAN_MAX_REQS,
+ rdev->wiphy.max_sched_scan_reqs) ||
nla_put_u8(msg, NL80211_ATTR_MAX_NUM_SCHED_SCAN_SSIDS,
rdev->wiphy.max_sched_scan_ssids) ||
nla_put_u16(msg, NL80211_ATTR_MAX_SCAN_IE_LEN,
@@ -7185,9 +7188,17 @@ static int nl80211_start_sched_scan(struct sk_buff *skb,
!rdev->ops->sched_scan_start)
return -EOPNOTSUPP;

- if (rdev->sched_scan_req)
+ /*
+ * allow only one legacy scheduled scan if user-space
+ * does not indicate multiple scheduled scan support.
+ */
+ if (!info->attrs[NL80211_ATTR_SCHED_SCAN_MULTI] &&
+ cfg80211_legacy_sched_scan_active(rdev))
return -EINPROGRESS;

+ if (rdev->sched_scan_req_count == rdev->wiphy.max_sched_scan_reqs)
+ return -ENOSPC;
+
sched_scan_req = nl80211_parse_sched_scan(&rdev->wiphy, wdev,
info->attrs);

@@ -7195,6 +7206,12 @@ static int nl80211_start_sched_scan(struct sk_buff *skb,
if (err)
goto out_err;

+ /* leave request id zero for legacy request */
+ if (info->attrs[NL80211_ATTR_SCHED_SCAN_MULTI]) {
+ while (!sched_scan_req->reqid)
+ sched_scan_req->reqid = rdev->wiphy.cookie_counter++;
+ }
+
err = rdev_sched_scan_start(rdev, dev, sched_scan_req);
if (err)
goto out_free;
@@ -7205,7 +7222,7 @@ static int nl80211_start_sched_scan(struct sk_buff *skb,
if (info->attrs[NL80211_ATTR_SOCKET_OWNER])
sched_scan_req->owner_nlportid = info->snd_portid;

- rcu_assign_pointer(rdev->sched_scan_req, sched_scan_req);
+ cfg80211_add_sched_scan_req(rdev, sched_scan_req);

nl80211_send_sched_scan(rdev, dev,
NL80211_CMD_START_SCHED_SCAN);
@@ -7220,13 +7237,28 @@ static int nl80211_start_sched_scan(struct sk_buff *skb,
static int nl80211_stop_sched_scan(struct sk_buff *skb,
struct genl_info *info)
{
+ struct cfg80211_sched_scan_request *req;
struct cfg80211_registered_device *rdev = info->user_ptr[0];
+ u64 cookie;

if (!(rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_SCHED_SCAN) ||
!rdev->ops->sched_scan_stop)
return -EOPNOTSUPP;

- return __cfg80211_stop_sched_scan(rdev, false);
+ if (info->attrs[NL80211_ATTR_COOKIE]) {
+ cookie = nla_get_u64(info->attrs[NL80211_ATTR_COOKIE]);
+ return __cfg80211_stop_sched_scan(rdev, cookie, false);
+ } else {
+ req = list_first_or_null_rcu(&rdev->sched_scan_req_list,
+ struct cfg80211_sched_scan_request,
+ list);
+ if (!req || req->reqid ||
+ (req->owner_nlportid &&
+ req->owner_nlportid != info->snd_portid))
+ return -ENOENT;
+
+ return cfg80211_stop_sched_scan_req(rdev, req, false);
+ }
}

static int nl80211_start_radar_detection(struct sk_buff *skb,
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index b5bd58d..942059e1 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -249,6 +249,46 @@ void cfg80211_scan_done(struct cfg80211_scan_request *request,
}
EXPORT_SYMBOL(cfg80211_scan_done);

+void cfg80211_add_sched_scan_req(struct cfg80211_registered_device *rdev,
+ struct cfg80211_sched_scan_request *req)
+{
+ if (req->reqid)
+ list_add_tail_rcu(&req->list, &rdev->sched_scan_req_list);
+ else
+ list_add_rcu(&req->list, &rdev->sched_scan_req_list);
+ rdev->sched_scan_req_count++;
+}
+
+static void cfg80211_del_sched_scan_req(struct cfg80211_registered_device *rdev,
+ struct cfg80211_sched_scan_request *req)
+{
+ list_del_rcu(&req->list);
+ kfree_rcu(req, rcu_head);
+ synchronize_rcu();
+ rdev->sched_scan_req_count--;
+}
+
+static struct cfg80211_sched_scan_request *
+cfg80211_find_sched_scan_req(struct cfg80211_registered_device *rdev, u64 reqid)
+{
+ struct cfg80211_sched_scan_request *pos;
+ list_for_each_entry(pos, &rdev->sched_scan_req_list, list) {
+ if (pos->reqid == reqid)
+ return pos;
+ }
+ return ERR_PTR(-ENOENT);
+}
+
+bool cfg80211_legacy_sched_scan_active(struct cfg80211_registered_device *rdev)
+{
+ struct cfg80211_sched_scan_request *req;
+
+ req = list_first_or_null_rcu(&rdev->sched_scan_req_list,
+ struct cfg80211_sched_scan_request, list);
+ /* request id 0 indicates legacy request in progress */
+ return req && !req->reqid;
+}
+
void __cfg80211_sched_scan_results(struct work_struct *wk)
{
struct cfg80211_registered_device *rdev;
@@ -295,7 +335,7 @@ void cfg80211_sched_scan_stopped_rtnl(struct wiphy *wiphy)

trace_cfg80211_sched_scan_stopped(wiphy);

- __cfg80211_stop_sched_scan(rdev, true);
+ __cfg80211_stop_sched_scan(rdev, 0, true);
}
EXPORT_SYMBOL(cfg80211_sched_scan_stopped_rtnl);

@@ -307,34 +347,43 @@ void cfg80211_sched_scan_stopped(struct wiphy *wiphy)
}
EXPORT_SYMBOL(cfg80211_sched_scan_stopped);

-int __cfg80211_stop_sched_scan(struct cfg80211_registered_device *rdev,
- bool driver_initiated)
+int cfg80211_stop_sched_scan_req(struct cfg80211_registered_device *rdev,
+ struct cfg80211_sched_scan_request *req,
+ bool driver_initiated)
{
- struct cfg80211_sched_scan_request *sched_scan_req;
- struct net_device *dev;
-
ASSERT_RTNL();

- if (!rdev->sched_scan_req)
- return -ENOENT;
-
- sched_scan_req = rtnl_dereference(rdev->sched_scan_req);
- dev = sched_scan_req->dev;
-
if (!driver_initiated) {
- int err = rdev_sched_scan_stop(rdev, dev);
+ int err = rdev_sched_scan_stop(rdev, req->dev);
if (err)
return err;
}

- nl80211_send_sched_scan(rdev, dev, NL80211_CMD_SCHED_SCAN_STOPPED);
+ nl80211_send_sched_scan(rdev, req->dev, NL80211_CMD_SCHED_SCAN_STOPPED);

- RCU_INIT_POINTER(rdev->sched_scan_req, NULL);
- kfree_rcu(sched_scan_req, rcu_head);
+ cfg80211_del_sched_scan_req(rdev, req);

return 0;
}

+int __cfg80211_stop_sched_scan(struct cfg80211_registered_device *rdev,
+ u64 reqid, bool driver_initiated)
+{
+ struct cfg80211_sched_scan_request *sched_scan_req;
+
+ ASSERT_RTNL();
+
+ if (!rdev->sched_scan_req_count)
+ return -ENOENT;
+
+ sched_scan_req = cfg80211_find_sched_scan_req(rdev, reqid);
+ if (IS_ERR(sched_scan_req))
+ return PTR_ERR(sched_scan_req);
+
+ return cfg80211_stop_sched_scan_req(rdev, sched_scan_req,
+ driver_initiated);
+}
+
void cfg80211_bss_age(struct cfg80211_registered_device *rdev,
unsigned long age_secs)
{
@@ -1078,7 +1127,7 @@ struct cfg80211_bss *
else
rcu_assign_pointer(tmp.pub.beacon_ies, ies);
rcu_assign_pointer(tmp.pub.ies, ies);
-
+
memcpy(tmp.pub.bssid, mgmt->bssid, ETH_ALEN);
tmp.pub.channel = channel;
tmp.pub.scan_width = data->scan_width;
--
1.9.1

2016-12-05 14:28:23

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

Hi Dmitry,

Sorry I didn't respond earlier.

>    Currently we have sched scan with possibility of various
> intervals. We would like to extend it to support also
> different types of scan.

"Different types of scan" is a bit misleading though, isn't it? I mean,
mostly they differ in the reporting modes - the scanning itself still
happens at "various intervals"?

>    In case of powerful wlan CPU, all this functionality
> can be offloaded.
>    In general case FW processes additional scan requests
> and puts them into queue based on start time and interval.
> Once current request is fulfilled, FW adds it (if interval != 0)
> again to the queue with proper interval. If requests are
> overlapping, new request can be combined with either one before,
> or one after, assuming that requests are not mutually exclusive.
>    Combining requests is done by combining scan channels, ssids,
> bssids and types of scan result. Once combined request was fulfilled
> it will be reinserted as two (or three) different requests based on
> their type and interval.
>    Each request has attribute:
> Type: connectivity / location

I'm not super happy with this - after all, in theory nothing precludes
using the results for one thing or the other, it's just about when and
how they are gathered, no?

You do have a point wrt. an incomplete scan triggering something wrt.
roaming or so in the connection manager, but I think that if it finds a
better result there than the current connection it would make sense to
pick it - even without full information.

So ultimately, I think this might boil down to reporting the scan
finished more accurately/precisely, rather than saying the type of
scan?

> Report: none / batch / immediate

Not sure I see much point in "none"??

Can you define these more clearly? Do you think "batch" reporting
should be like the gscan buckets? Or actually have full information?

>    Request may have priority and can be inserted into
> the head of the queue.
>    Types of scans:
> - Normal scan
> - Scheduled scan
> - Hotlist (BSSID scan)
> - Roaming
> - AutoJoin

I think somebody else said this but I didn't find it now - I think this
would make more sense to define in terms of expected behaviour than use
cases for each type of scan.

johannes

2016-12-13 16:06:47

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

> Supporting requests (or more precisely requests and results)
> differentiated by user-space entity can be tricky. Right now we are
> not checking current caller pid, right? Maybe it is also good idea -
> or maybe we can just make result filtering per user-space caller?

Could be done.

You seem to be very worried about the partial results - I'm not too
worried about that I guess, the connection manager itself will always
be able to wait for the full scan to finish before making a decision,
but it may not even want to (see the separate discussion on per-channel
"done" notifications etc.)

I'm much more worried about the "bucket reporting" since that doesn't
fit into the current full BSS reporting model at all. What's your
suggestion for this?

johannes

2016-12-05 18:32:48

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

Hi Johannes,

On Mon, Dec 5, 2016 at 6:28 AM, Johannes Berg <[email protected]> wrote:
> Hi Dmitry,
>
> Sorry I didn't respond earlier.
>
>> Currently we have sched scan with possibility of various
>> intervals. We would like to extend it to support also
>> different types of scan.
>
> "Different types of scan" is a bit misleading though, isn't it? I mean,
> mostly they differ in the reporting modes - the scanning itself still
> happens at "various intervals"?
>
>> In case of powerful wlan CPU, all this functionality
>> can be offloaded.
>> In general case FW processes additional scan requests
>> and puts them into queue based on start time and interval.
>> Once current request is fulfilled, FW adds it (if interval != 0)
>> again to the queue with proper interval. If requests are
>> overlapping, new request can be combined with either one before,
>> or one after, assuming that requests are not mutually exclusive.
>> Combining requests is done by combining scan channels, ssids,
>> bssids and types of scan result. Once combined request was fulfilled
>> it will be reinserted as two (or three) different requests based on
>> their type and interval.
>> Each request has attribute:
>> Type: connectivity / location
>
> I'm not super happy with this - after all, in theory nothing precludes
> using the results for one thing or the other, it's just about when and
> how they are gathered, no?

Indeed, results are results. I just want to take care of two things:
1) Memory consumption - we can clear stale scan results for connection,
but not for location if we are using history scan.
2) Use of insufficient results for connection - in case we had history
or hotlist scan only for very limited amount of channels, then we may not
have enough APs in our result for "sterling" connection decision.

> You do have a point wrt. an incomplete scan triggering something wrt.
> roaming or so in the connection manager, but I think that if it finds a
> better result there than the current connection it would make sense to
> pick it - even without full information.
>
> So ultimately, I think this might boil down to reporting the scan
> finished more accurately/precisely, rather than saying the type of
> scan?

This is intertwined with Luca's and yours point below - to use
scan by name or by description of the actions. Indeed maybe
this way is more generic and more useful.

>> Report: none / batch / immediate
>
> Not sure I see much point in "none"??
>
> Can you define these more clearly? Do you think "batch" reporting
> should be like the gscan buckets? Or actually have full information?

None - means that there is not need to report. It can be useful
in case of roaming scan, scheduling or hotlist scan - you didn't find
anything suitable - don't report that there is no scan results.
Batch - means to report only when buffer is full (or close to full) -
mostly for history scan or for example for case to report all scan
results together.

Immediate - is kind of self-explaining.

>> Request may have priority and can be inserted into
>> the head of the queue.
>> Types of scans:
>> - Normal scan
>> - Scheduled scan
>> - Hotlist (BSSID scan)
>> - Roaming
>> - AutoJoin
>
> I think somebody else said this but I didn't find it now - I think this
> would make more sense to define in terms of expected behaviour than use
> cases for each type of scan.

I think Luca made this statement. It is totally ok from SW point of
view - especially due to the fact that scan is scan. However,
I suspect it will be harder to handle from user experience. I mean
at the end wireless framework / driver / FW will convert special
scan type to usual scan with special params and response, but why
to put this burden on user?
Anyway, I am ok with this approach as well. If we see that it is
confusing we can use scan wrappers.

> johannes
>

2016-12-07 20:51:09

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On 7-12-2016 19:39, Dmitry Shmidt wrote:
> On Tue, Dec 6, 2016 at 10:44 PM, Johannes Berg
> <[email protected]> wrote:
>>
>>> Indeed, results are results. I just want to take care of two things:
>>> 1) Memory consumption - we can clear stale scan results for
>>> connection, but not for location if we are using history scan.
>>
>> Well eventually we also have to clear for location if we run out of
>> memory, that usually means dumping them out to the host, no?
>
> Being out of memory and consuming more memory are different
> things, but I agree - maybe we don't need to worry about it.
>
>>> 2) Use of insufficient results for connection - in case we had
>>> history or hotlist scan only for very limited amount of channels,
>>> then we may not have enough APs in our result for "sterling"
>>> connection decision.
>>
>> I'm not entirely sure about this case - surely noticing "we can do
>> better now" is still better than waiting for being able to make the
>> perfect decision?
>
> Maybe we can just keep flag saying that currently available results
> were not received by usual full scan.
>
>>>>> Report: none / batch / immediate
>>>>
>>>> Not sure I see much point in "none"??
>>>>
>>>> Can you define these more clearly? Do you think "batch" reporting
>>>> should be like the gscan buckets? Or actually have full
>>>> information?
>>>
>>> None - means that there is not need to report. It can be useful
>>> in case of roaming scan, scheduling or hotlist scan - you didn't find
>>> anything suitable - don't report that there is no scan results.
>>
>> But that seems more of a filtering thing, combined with "immediate" for
>> anything passing the filter?
>
> We can use this approach as well.
>
>>>>> Request may have priority and can be inserted into
>>>>> the head of the queue.
>>>>> Types of scans:
>>>>> - Normal scan
>>>>> - Scheduled scan
>>>>> - Hotlist (BSSID scan)
>>>>> - Roaming
>>>>> - AutoJoin
>>>>
>>>> I think somebody else said this but I didn't find it now - I think
>>>> this would make more sense to define in terms of expected behaviour
>>>> than use cases for each type of scan.
>>>
>>> I think Luca made this statement.
>>
>> Yeah - I just couldn't find it again on re-reading the thread :)
>>
>>> It is totally ok from SW point of
>>> view - especially due to the fact that scan is scan. However,
>>> I suspect it will be harder to handle from user experience. I mean
>>> at the end wireless framework / driver / FW will convert special
>>> scan type to usual scan with special params and response, but why
>>> to put this burden on user?

I don't think this will put burden on the user although it depends
who/what you mean by this. If you mean the mere mortal end-user I would
say no as indeed there must be some software entity (in user-space) that
will have to initiate a nl80211 command with appropriate attributes
according to whatever user-space is trying to accomplish.

>> I just think it's more flexible and open-ended. The actual definition
>> of the resulting parameters needs to be somewhere anyway - putting it
>> into driver/firmware (vs. wifi framework or so) seems to duplicate it
>> and certainly makes it harder to modify/extend in the future, no?
>
> So, let's summarize:
> Instead of creating new type of generic scan with special types,
> we want to go with additional expansion of scheduled scan options and
> parameters (in order not to "multiply entities"), including ability to send
> new scheduled scan request without stopping previous one.
>
> Is it Ok?

Sounds ok. To me a generic scan command with type attribute is
equivalent to having seperate commands for each type so there seems to
be no gain. Now whether this all can be accomplished by extending the
scheduled scan depends on the problem that you are trying to solve.

Main purpose seems to be offloading different scanning tasks which could
make scheduled scan a good candidate. Now I want to add gscan to this
mix as it seems some concepts for that are in play in this discussion as
well, eg. hotlist. gscan is like scheduled scan, but it supports
multiple schedules. However, it is still a single request from a single
user-space process. I think Luca mentioned supporting requests from
different user-space processes. Is that also what you mean by "ability
to send new scheduled scan request without stopping previous one" or is
that still from a single user-space process. Do we need a limit to the
amount of scheduled scan requests that can be handled simultaneously.

A maybe more important aspect of gscan is user-space control of result
reporting and thus the frequency of waking up host and/or user-space. I
suspect this would be needed in the scheduled scan extension as well, right?

Regards,
Arend

2017-01-04 20:39:37

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Wed, Jan 4, 2017 at 5:28 AM, Johannes Berg <[email protected]> wrote:
> On Tue, 2017-01-03 at 12:45 -0800, Dmitry Shmidt wrote:
>>
>> We can either use alternative structure in kernel wireless stack,
>> or alternative structure in userspace (in wpa_supplicant), and we
>> will most likely need special command for this case at least to
>> retrieve results.
>
> Yes, I tend to agree - we need to have some new structure (and netlink
> attributes etc.) to report the aggregated/partial results. But it seems
> to me that it'll have to be very obvious which way of obtaining results
> must be used for a given scan?

If we go with approach to use parameters and let FW or MAC80211
layer to decide what type of scan to do, then in general the only
difference between different types of scan is what to do with result:
- Normal scan: ssid list, channel list, dwell params, etc...
- Sched scan: ssid list, channel list, interval
- BSSID scan: bssid list, channel list, interval
Action: Report when suitable results are found (in case of Normal
scan it will be at the end of scan)

- Roaming / Autojoin: ssid list, channel list, interval
Action: Connect when suitable results are found

- History scan: bssid list, channel list, interval
Action: Report when buffer is full / almost full

So we can literally distinguish scan types by final
action. And for History scan we need new get_scan_results()
command.

Does it sound reasonable?

> johannes

2017-01-05 11:46:06

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

> If we go with approach to use parameters and let FW or MAC80211
> layer to decide what type of scan to do,

At that point though, is it even meaningful to ask "what type of scan
is this"? Or put another way - what does "scan type" even mean?

> then in general the only
> difference between different types of scan is what to do with result:
> - Normal scan: ssid list, channel list, dwell params, etc...
> - Sched scan: ssid list, channel list, interval
> - BSSID scan: bssid list, channel list, interval
> Action: Report when suitable results are found (in case of Normal
> scan it will be at the end of scan)
>
> - Roaming / Autojoin: ssid list, channel list, interval
> Action: Connect when suitable results are found
>
> - History scan: bssid list, channel list, interval
> Action: Report when buffer is full / almost full

Exactly. But the type of action is something set by the entity that
triggered the scan, right? normal and roam would be equivalent anyway,
no? wpa_supplicant would make a decision to connect - after the results
are coming in.

Oh, then again, maybe you're thinking of full-MAC devices - does a
roam/autojoin scan really already *imply* a new connection? And if so,
do we have to do it that way, or can we remove that type of action and
make a connection decision in higher layers, so it's really the same as
"report when suitable results are found"?

> So we can literally distinguish scan types by final action.

Actually I think I'm just misinterpreting your wording - you mean that
we can use the different final actions for the different scan types,
not that we should actually say - in driver/firmware/... - "this is a
history scan because the action is to report buffer full", right?

> And for History scan we need new get_scan_results() command.
>
> Does it sound reasonable?

I think it does.

There's a bit more complication wrt. the level of detail in results
though - sometimes the result may include all IEs (normal/sched scan),
sometimes it may not ("history scan") - are we sure we really only need
one new get_scan_results()?

johannes

2017-01-11 13:14:55

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

> > Well, we might not even need different commands. We need different
> > storage internally, but if you request the results for a given scan
> > ID then you might get a totally different result format? Though
> > that wouldn't lend itself well to query "everything you have" which
> > is also useful. But even then, it could be done by passing the
> > appropriate "report type" attribute to the dump command - we need
> > that anyway for trigger.
>
> True. With "report type" attribute you do not mean the actual
> report_type thing, right. Hopefully you mean the parameter attribute
> that implicitly relates to a "report type".

Right, I wasn't really thinking in terms of attributes while writing
this. OTOH, something like an attribute *would* be needed, no?

> The risk here is that it
> requires careful description of what user-space needs to look for if
> it gets a notification. I think having separate
> notification/retrieval commands lowers the risk of misinterpretation.

Yeah, fair point.

> Not sure if we're getting ahead of ourselves. Yes, we have to
> determine attributes for each scan "report type", but it is not a
> prerequisite for the other topic.

We'll also have to figure out which report types we need at all :)

> I guess to answer the question about the partial results attributes
> we need to know what the possible higher-level use-cases are. Other
> source of information would be to look what is done for g-scan in
> Android "M" or "N", but not sure if that is best approach as we may
> not consider all use-cases.

Right.

johannes

2017-01-09 11:19:32

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On 9-1-2017 11:45, Johannes Berg wrote:
> On Thu, 2017-01-05 at 12:45 -0800, Dmitry Shmidt wrote:
>>
>>> Oh, then again, maybe you're thinking of full-MAC devices - does a
>>> roam/autojoin scan really already *imply* a new connection? And if
>>> so, do we have to do it that way, or can we remove that type of
>>> action and make a connection decision in higher layers, so it's
>>> really the same as "report when suitable results are found"?
>>
>> We need to consider case when FW may do some actions like connection
>> during roaming/autojoin.
>
> Ok. I was unsure if that was happening. So you're saying that the scan
> parameters are determined by the host, and the scan is triggered from
> there, but the action (like roaming) is taken by the firmware?
>
> How does that differ to
>
> 1) the scan being started by the firmware, possibly based on the BSS
> selection configuration Arend added?

Currently, the BSS selection configuration is used in CMD_CONNECT. It
can probably be used for scanning as well. The presence of the attribute
would indicate the scan may result in a roam or connect event.

> 2) the scan result being reported to the host, and BSS selection done
> there?

Not sure if we need to consider this one, do we?

>> It depends how we want to make it flexible. For example we
>> may allow to FW to report even usual scan results not one by one
>> but as a chunk.
>
> Firmware can do that, but is there any point in doing that in the
> cfg80211 API? If it properly has full results, the driver can always
> unbundle them and call the report function for each BSS and everything
> should work just fine, no?

Agree.

>>> There's a bit more complication wrt. the level of detail in results
>>> though - sometimes the result may include all IEs (normal/sched
>>> scan), sometimes it may not ("history scan") - are we sure we
>>> really only need one new get_scan_results()?
>>
>> Maybe not - it is possible I missed something.
>
> I was hoping you could clarify the requirements :-)
>
>> Also looking at our
>> conversation I think we should consider separate command pair
>> for history scan.
>
> Perhaps, yes. Although perhaps having it triggered through the same
> (new or extended) command, but results reported depending on the
> "report type" would make sense. I think we need to clarify the exact
> requirements before we make that call.

Leaning towards single initiate command and a notification and results
command per "report type".

Regards,
Arend

2017-01-04 10:31:02

[permalink] [raw]

Subject: Re: [RFC] nl80211: allow multiple active scheduled scan requests

> However, we need to prefer something
> > -
> > always preferring the new sched scan could lead to bounces, so we
> > can
> > prefer (1) existing, (2) legacy-single type or (3) new-multi type,
> > but
> > not (4) new sched scan.
>
> Not sure I can follow. What is the difference between (1) and (2).

(1) would never cancel any existing sched scan, regardless of type
(legacy vs. multi-capable)

(2) would cancel an existing sched scan (in favour of a new one) if the
existing one is multi-capable

(3) would cancel an existing sched scan (in favour of a new one) if the
existing one is legacy type

> Also
> what do you consider (4) new sched scan. You mean the additional
> parameterization of the scheduled scan?

No, I just meant any new request.

> > I think preferring the existing would probably be best, i.e. refuse
> > legacy if any sched scan is running, and refuse multi if legacy is
> > running?
>
> Whatever the response above, I can understand this and it seems most
> straightforward. So I tend agree this is our best option although
> maybe for the wrong reason.

:)

johannes

2017-01-05 13:39:28

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

Merging my draft reply in this one. Hope it still makes sense :-p

On 5-1-2017 12:46, Johannes Berg wrote:
>
>> If we go with approach to use parameters and let FW or MAC80211
>> layer to decide what type of scan to do,
>
> At that point though, is it even meaningful to ask "what type of scan
> is this"? Or put another way - what does "scan type" even mean?
>
>> then in general the only
>> difference between different types of scan is what to do with result:
>> - Normal scan: ssid list, channel list, dwell params, etc...
>> - Sched scan: ssid list, channel list, interval
>> - BSSID scan: bssid list, channel list, interval
>> Action: Report when suitable results are found (in case of Normal
>> scan it will be at the end of scan)
>>
>> - Roaming / Autojoin: ssid list, channel list, interval
>> Action: Connect when suitable results are found
>>
>> - History scan: bssid list, channel list, interval
>> Action: Report when buffer is full / almost full
>
> Exactly. But the type of action is something set by the entity that
> triggered the scan, right? normal and roam would be equivalent anyway,
> no? wpa_supplicant would make a decision to connect - after the results
> are coming in.
>
> Oh, then again, maybe you're thinking of full-MAC devices - does a
> roam/autojoin scan really already *imply* a new connection? And if so,
> do we have to do it that way, or can we remove that type of action and
> make a connection decision in higher layers, so it's really the same as
> "report when suitable results are found"?

Today we already have roaming offload, right? Roaming scan would indeed
be the same if roaming offload is not used unless you would want
cfg80211 to make the decision, but then I would expect parameters for
making that decision in the request. Same/similar for autojoin.

>> So we can literally distinguish scan types by final action.
>
> Actually I think I'm just misinterpreting your wording - you mean that
> we can use the different final actions for the different scan types,
> not that we should actually say - in driver/firmware/... - "this is a
> history scan because the action is to report buffer full", right?

Do we care? The scan engine in our firmware does have use a scan type
concept, but that is hidden by the firmware api. I guess your point
would be that if user-space would prefer scan types and driver/firmware
uses that as well, we might do the same in cfg80211/mac80211. How can we
assure the scan types we come up with match those employed in any and
all wireless devices/firmwares.

>> And for History scan we need new get_scan_results() command.
>>
>> Does it sound reasonable?
>
> I think it does.

I think so too, but what I am missing in this is who is responsible for
the actions above. I mean do we want to consider option for offloading
from the start or add it later. We may use the new "history scan"
storage in cfg80211 only for devices that do not support offloading it
and provide a .get_scan_results() callback for those that do.

As Johannes points out it needs to be clear to user-space what its next
step should be in obtaining results. Does it make sense to have a
separate notification for the history scan results (not calling it that
of course :-p ) triggered by the action "Report when buffer is full /
almost full" or should user-space determine what to do based on request
id that would be included in the (scheduled) scan results notification.

> There's a bit more complication wrt. the level of detail in results
> though - sometimes the result may include all IEs (normal/sched scan),
> sometimes it may not ("history scan") - are we sure we really only need
> one new get_scan_results()?

So could the "all IEs" case not be handled by the existing BSS storage?
Is it still our intention to handle normal scan (no interval provided?)
as well in the universal scan approach.

Regards,
Arend

2017-01-04 10:35:00

[permalink] [raw]

Subject: Re: [RFC] nl80211: allow multiple active scheduled scan requests

On 4-1-2017 11:30, Johannes Berg wrote:
>> However, we need to prefer something
>>> -
>>> always preferring the new sched scan could lead to bounces, so we
>>> can
>>> prefer (1) existing, (2) legacy-single type or (3) new-multi type,
>>> but
>>> not (4) new sched scan.
>>
>> Not sure I can follow. What is the difference between (1) and (2).
>
> (1) would never cancel any existing sched scan, regardless of type
> (legacy vs. multi-capable)
>
> (2) would cancel an existing sched scan (in favour of a new one) if the
> existing one is multi-capable
>
> (3) would cancel an existing sched scan (in favour of a new one) if the
> existing one is legacy type

yes, yes. sinking in :-p

>> Also
>> what do you consider (4) new sched scan. You mean the additional
>> parameterization of the scheduled scan?
>
> No, I just meant any new request.

Yeah, so there is already an existing/active sched scan. Clear.

>>> I think preferring the existing would probably be best, i.e. refuse
>>> legacy if any sched scan is running, and refuse multi if legacy is
>>> running?
>>
>> Whatever the response above, I can understand this and it seems most
>> straightforward. So I tend agree this is our best option although
>> maybe for the wrong reason.
>
> :)

Thanks for clarifying it.

Gr. AvS

2017-01-05 20:00:00

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On 5-1-2017 14:44, Johannes Berg wrote:
> On Thu, 2017-01-05 at 14:39 +0100, Arend Van Spriel wrote:
>
>> Today we already have roaming offload, right?
>
> I guess - you defined the BSS selection stuff for it :)

Well I was referring to:
3047 WIPHY_FLAG_SUPPORTS_FW_ROAM = BIT(13),

>> Roaming scan would indeed
>> be the same if roaming offload is not used unless you would want
>> cfg80211 to make the decision, but then I would expect parameters for
>> making that decision in the request. Same/similar for autojoin.
>
> Right.
>
>>> Actually I think I'm just misinterpreting your wording - you mean
>>> that we can use the different final actions for the different scan
>>> types, not that we should actually say - in driver/firmware/... -
>>> "this is a history scan because the action is to report buffer
>>> full", right?
>>
>> Do we care? The scan engine in our firmware does have use a scan type
>> concept, but that is hidden by the firmware api. I guess your point
>> would be that if user-space would prefer scan types and
>> driver/firmware uses that as well, we might do the same in
>> cfg80211/mac80211. How can we assure the scan types we come up with
>> match those employed in any and all wireless devices/firmwares.
>
> To be clear: I *don't* like having scan types in the APIs. I think it
> muddies the waters and makes it less likely people will implement it
> precisely as requested, because they'll argue "this is only for roam,
> we'll implement our own magic there" etc.

To be clear: me neither ;-) On the same page here.

>> As Johannes points out it needs to be clear to user-space what its
>> next step should be in obtaining results. Does it make sense to have
>> a separate notification for the history scan results (not calling it
>> that of course :-p ) triggered by the action "Report when buffer is
>> full / almost full" or should user-space determine what to do based
>> on request id that would be included in the (scheduled) scan results
>> notification.
>
> Yeah, that's a good question - based on the current behaviours etc. I
> was assuming it'd be a new notification/result type.

fair enough.

>>> There's a bit more complication wrt. the level of detail in results
>>> though - sometimes the result may include all IEs (normal/sched
>>> scan),
>>> sometimes it may not ("history scan") - are we sure we really only
>>> need
>>> one new get_scan_results()?
>>
>> So could the "all IEs" case not be handled by the existing BSS
>> storage? Is it still our intention to handle normal scan (no interval
>> provided?) as well in the universal scan approach.
>
> Yes, the "all IEs" (essentially "all information") case is handled by
> the existing storage/dump methods/etc. but obviously the other cases
> can't be - so my question was if there's really only one more other
> case, or if we might have more new cases - or something that's flexible
> wrt. the data we get?

>From what Dmitry listed I guess there's really only one. Early on in the
thread Luca gave some other examples of scan extensions so may need to
consider notification/dump methods that are extensible. It seems awkward
to have a single "initiate" command and a couple of
"notification/retrieval" commands. It may not be so bad as long as it is
clear which retrieval command goes with a notification.

Regards,
Arend

2017-01-09 10:48:45

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Thu, 2017-01-05 at 20:59 +0100, Arend Van Spriel wrote:
>
> From what Dmitry listed I guess there's really only one.

Ok. I guess I need to go back to that then.

> Early on in the thread Luca gave some other examples of scan
> extensions so may need to consider notification/dump methods that are
> extensible. It seems awkward to have a single "initiate" command and
> a couple of "notification/retrieval" commands. It may not be so bad
> as long as it is clear which retrieval command goes with a
> notification.

Well, we might not even need different commands. We need different
storage internally, but if you request the results for a given scan ID
then you might get a totally different result format? Though that
wouldn't lend itself well to query "everything you have" which is also
useful. But even then, it could be done by passing the appropriate
"report type" attribute to the dump command - we need that anyway for
trigger.

I think with that discussion we're getting ahead of ourselves though -
do we really know that we just need the two result types

* full, and
* partial (for history scan)

and have we even defined the attributes we want in the partial one?

johannes

2017-01-09 10:45:31

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Thu, 2017-01-05 at 12:45 -0800, Dmitry Shmidt wrote:
>
> > Oh, then again, maybe you're thinking of full-MAC devices - does a
> > roam/autojoin scan really already *imply* a new connection? And if
> > so, do we have to do it that way, or can we remove that type of
> > action and make a connection decision in higher layers, so it's
> > really the same as "report when suitable results are found"?
>
> We need to consider case when FW may do some actions like connection
> during roaming/autojoin.

Ok. I was unsure if that was happening. So you're saying that the scan
parameters are determined by the host, and the scan is triggered from
there, but the action (like roaming) is taken by the firmware?

How does that differ to

1) the scan being started by the firmware, possibly based on the BSS
selection configuration Arend added?

2) the scan result being reported to the host, and BSS selection done
there?

> It depends how we want to make it flexible. For example we
> may allow to FW to report even usual scan results not one by one
> but as a chunk.

Firmware can do that, but is there any point in doing that in the
cfg80211 API? If it properly has full results, the driver can always
unbundle them and call the report function for each BSS and everything
should work just fine, no?

> > There's a bit more complication wrt. the level of detail in results
> > though - sometimes the result may include all IEs (normal/sched
> > scan), sometimes it may not ("history scan") - are we sure we
> > really only need one new get_scan_results()?
>
> Maybe not - it is possible I missed something.

I was hoping you could clarify the requirements :-)

> Also looking at our
> conversation I think we should consider separate command pair
> for history scan.

Perhaps, yes. Although perhaps having it triggered through the same
(new or extended) command, but results reported depending on the
"report type" would make sense. I think we need to clarify the exact
requirements before we make that call.

johannes

2017-01-02 10:44:04

[permalink] [raw]

Subject: Re: [RFC] nl80211: allow multiple active scheduled scan requests

> + /*
> + * allow only one legacy scheduled scan if user-space
> + * does not indicate multiple scheduled scan support.
> + */
> + if (!info->attrs[NL80211_ATTR_SCHED_SCAN_MULTI] &&
> + cfg80211_legacy_sched_scan_active(rdev))
> return -EINPROGRESS;

That probably doesn't go far enough - if legacy one is active then we
probably shouldn't allow a new MULTI one either (or abandon the legacy
one) so that older userspace doesn't get confused with multiple
notifications from sched scans it didn't start.

> + if (rdev->sched_scan_req_count == rdev->wiphy.max_sched_scan_reqs)
> + return -ENOSPC;

Do we really want to do the double-accounting, just to avoid counting
the list length here?

> + /* leave request id zero for legacy request */

why? The ID would be ignored, so why special-case it?

> +static void cfg80211_del_sched_scan_req(struct
> cfg80211_registered_device *rdev,
> + struct
> cfg80211_sched_scan_request *req)
> +{
> + list_del_rcu(&req->list);
> + kfree_rcu(req, rcu_head);
> + synchronize_rcu();
> + rdev->sched_scan_req_count--;
> +}

That's bogus - either you use kfree_rcu() or synchronize_rcu() (the
former is much better); combining both makes no sense.

> +bool cfg80211_legacy_sched_scan_active(struct
> cfg80211_registered_device *rdev)
> +{
> + struct cfg80211_sched_scan_request *req;
> +
> + req = list_first_or_null_rcu(&rdev->sched_scan_req_list,
> + struct
> cfg80211_sched_scan_request, list);
> + /* request id 0 indicates legacy request in progress */
> + return req && !req->reqid;
> +}

Ok, fair enough.

johannes

2017-01-09 12:07:49

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On 9-1-2017 11:48, Johannes Berg wrote:
> On Thu, 2017-01-05 at 20:59 +0100, Arend Van Spriel wrote:
>>
>> From what Dmitry listed I guess there's really only one.
>
> Ok. I guess I need to go back to that then.
>
>> Early on in the thread Luca gave some other examples of scan
>> extensions so may need to consider notification/dump methods that are
>> extensible. It seems awkward to have a single "initiate" command and
>> a couple of "notification/retrieval" commands. It may not be so bad
>> as long as it is clear which retrieval command goes with a
>> notification.
>
> Well, we might not even need different commands. We need different
> storage internally, but if you request the results for a given scan ID
> then you might get a totally different result format? Though that
> wouldn't lend itself well to query "everything you have" which is also
> useful. But even then, it could be done by passing the appropriate
> "report type" attribute to the dump command - we need that anyway for
> trigger.

True. With "report type" attribute you do not mean the actual
report_type thing, right. Hopefully you mean the parameter attribute
that implicitly relates to a "report type". The risk here is that it
requires careful description of what user-space needs to look for if it
gets a notification. I think having separate notification/retrieval
commands lowers the risk of misinterpretation.

> I think with that discussion we're getting ahead of ourselves though -
> do we really know that we just need the two result types
>
> * full, and
> * partial (for history scan)
>
> and have we even defined the attributes we want in the partial one?

Not sure if we're getting ahead of ourselves. Yes, we have to determine
attributes for each scan "report type", but it is not a prerequisite for
the other topic. I guess to answer the question about the partial
results attributes we need to know what the possible higher-level
use-cases are. Other source of information would be to look what is done
for g-scan in Android "M" or "N", but not sure if that is best approach
as we may not consider all use-cases.

Regards,
Arend

2017-01-05 20:45:33

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Thu, Jan 5, 2017 at 3:46 AM, Johannes Berg <[email protected]> wrote:
>
>> If we go with approach to use parameters and let FW or MAC80211
>> layer to decide what type of scan to do,
>
> At that point though, is it even meaningful to ask "what type of scan
> is this"? Or put another way - what does "scan type" even mean?

Probably not - if we focus on actions and results then type of
scan is meaningless - at the end it is scan + after actions.

>> then in general the only
>> difference between different types of scan is what to do with result:
>> - Normal scan: ssid list, channel list, dwell params, etc...
>> - Sched scan: ssid list, channel list, interval
>> - BSSID scan: bssid list, channel list, interval
>> Action: Report when suitable results are found (in case of Normal
>> scan it will be at the end of scan)
>>
>> - Roaming / Autojoin: ssid list, channel list, interval
>> Action: Connect when suitable results are found
>>
>> - History scan: bssid list, channel list, interval
>> Action: Report when buffer is full / almost full
>
> Exactly. But the type of action is something set by the entity that
> triggered the scan, right? normal and roam would be equivalent anyway,
> no? wpa_supplicant would make a decision to connect - after the results
> are coming in.

> Oh, then again, maybe you're thinking of full-MAC devices - does a
> roam/autojoin scan really already *imply* a new connection? And if so,
> do we have to do it that way, or can we remove that type of action and
> make a connection decision in higher layers, so it's really the same as
> "report when suitable results are found"?

We need to consider case when FW may do some actions like connection
during roaming/autojoin.

>> So we can literally distinguish scan types by final action.
>
> Actually I think I'm just misinterpreting your wording - you mean that
> we can use the different final actions for the different scan types,
> not that we should actually say - in driver/firmware/... - "this is a
> history scan because the action is to report buffer full", right?

It depends how we want to make it flexible. For example we
may allow to FW to report even usual scan results not one by one
but as a chunk.

>> And for History scan we need new get_scan_results() command.
>>
>> Does it sound reasonable?
>
> I think it does.
>
> There's a bit more complication wrt. the level of detail in results
> though - sometimes the result may include all IEs (normal/sched scan),
> sometimes it may not ("history scan") - are we sure we really only need
> one new get_scan_results()?

Maybe not - it is possible I missed something. Also looking at our
conversation I think we should consider separate command pair
for history scan.

> johannes

2017-01-04 10:00:42

[permalink] [raw]

Subject: Re: [RFC] nl80211: allow multiple active scheduled scan requests

On Tue, 2017-01-03 at 13:25 +0100, Arend Van Spriel wrote:
> On 2-1-2017 11:44, Johannes Berg wrote:
> >
> > > + /*
> > > + * allow only one legacy scheduled scan if user-space
> > > + * does not indicate multiple scheduled scan support.
> > > + */
> > > + if (!info->attrs[NL80211_ATTR_SCHED_SCAN_MULTI] &&
> > > + cfg80211_legacy_sched_scan_active(rdev))
> > > return -EINPROGRESS;
> >
> > That probably doesn't go far enough - if legacy one is active then
> > we
> > probably shouldn't allow a new MULTI one either (or abandon the
> > legacy
> > one) so that older userspace doesn't get confused with multiple
> > notifications from sched scans it didn't start.
>
> I considered that although not taking the notifications into account.
> Will change it. Abandoning the legacy one would be a behavioral
> change so probably not acceptable, right?

Well, it would be acceptable since it's documented that it's possible
it might stop for any reason. However, we need to prefer something -
always preferring the new sched scan could lead to bounces, so we can
prefer (1) existing, (2) legacy-single type or (3) new-multi type, but
not (4) new sched scan.

I think preferring the existing would probably be best, i.e. refuse
legacy if any sched scan is running, and refuse multi if legacy is
running?

> I guess your remark means this clarifies your earlier question about
> the request id, right?

yeah.

johannes

2017-01-04 13:28:33

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Tue, 2017-01-03 at 12:45 -0800, Dmitry Shmidt wrote:
>
> We can either use alternative structure in kernel wireless stack,
> or alternative structure in userspace (in wpa_supplicant), and we
> will most likely need special command for this case at least to
> retrieve results.

Yes, I tend to agree - we need to have some new structure (and netlink
attributes etc.) to report the aggregated/partial results. But it seems
to me that it'll have to be very obvious which way of obtaining results
must be used for a given scan?

johannes

2017-01-03 12:25:40

[permalink] [raw]

Subject: Re: [RFC] nl80211: allow multiple active scheduled scan requests

On 2-1-2017 11:44, Johannes Berg wrote:
>
>> + /*
>> + * allow only one legacy scheduled scan if user-space
>> + * does not indicate multiple scheduled scan support.
>> + */
>> + if (!info->attrs[NL80211_ATTR_SCHED_SCAN_MULTI] &&
>> + cfg80211_legacy_sched_scan_active(rdev))
>> return -EINPROGRESS;
>
> That probably doesn't go far enough - if legacy one is active then we
> probably shouldn't allow a new MULTI one either (or abandon the legacy
> one) so that older userspace doesn't get confused with multiple
> notifications from sched scans it didn't start.

I considered that although not taking the notifications into account.
Will change it. Abandoning the legacy one would be a behavioral change
so probably not acceptable, right?

>> + if (rdev->sched_scan_req_count == rdev->wiphy.max_sched_scan_reqs)
>> + return -ENOSPC;
>
> Do we really want to do the double-accounting, just to avoid counting
> the list length here?

Ok. I have no strong preference.

>> + /* leave request id zero for legacy request */
>
> why? The ID would be ignored, so why special-case it?

It makes the function cfg80211_legacy_sched_scan_active() easier, ie.
not needing a is_legacy flag in struct cfg80211_sched_scan_request.

>> +static void cfg80211_del_sched_scan_req(struct
>> cfg80211_registered_device *rdev,
>> + struct
>> cfg80211_sched_scan_request *req)
>> +{
>> + list_del_rcu(&req->list);
>> + kfree_rcu(req, rcu_head);
>> + synchronize_rcu();
>> + rdev->sched_scan_req_count--;
>> +}
>
> That's bogus - either you use kfree_rcu() or synchronize_rcu() (the
> former is much better); combining both makes no sense.

Thanks. Both functions mentioned the rcu grace period so I was doubtful.
Will change it.

>> +bool cfg80211_legacy_sched_scan_active(struct
>> cfg80211_registered_device *rdev)
>> +{
>> + struct cfg80211_sched_scan_request *req;
>> +
>> + req = list_first_or_null_rcu(&rdev->sched_scan_req_list,
>> + struct
>> cfg80211_sched_scan_request, list);
>> + /* request id 0 indicates legacy request in progress */
>> + return req && !req->reqid;
>> +}
>
> Ok, fair enough.

I guess your remark means this clarifies your earlier question about the
request id, right?

Regards,
Arend

2017-01-03 20:46:01

[permalink] [raw]

Subject: Re: [PATCH] RFC: Universal scan proposal

On Tue, Dec 13, 2016 at 8:06 AM, Johannes Berg
<[email protected]> wrote:
>
>> Supporting requests (or more precisely requests and results)
>> differentiated by user-space entity can be tricky. Right now we are
>> not checking current caller pid, right? Maybe it is also good idea -
>> or maybe we can just make result filtering per user-space caller?
>
> Could be done.
>
> You seem to be very worried about the partial results - I'm not too
> worried about that I guess, the connection manager itself will always
> be able to wait for the full scan to finish before making a decision,
> but it may not even want to (see the separate discussion on per-channel
> "done" notifications etc.)

Probably you are right.

> I'm much more worried about the "bucket reporting" since that doesn't
> fit into the current full BSS reporting model at all. What's your
> suggestion for this?
>
> johannes

We can either use alternative structure in kernel wireless stack,
or alternative structure in userspace (in wpa_supplicant), and we will
most likely need special command for this case at least to retrieve
results.

2017-01-04 10:20:53

[permalink] [raw]

Subject: Re: [RFC] nl80211: allow multiple active scheduled scan requests

On 4-1-2017 10:59, Johannes Berg wrote:
> On Tue, 2017-01-03 at 13:25 +0100, Arend Van Spriel wrote:
>> On 2-1-2017 11:44, Johannes Berg wrote:
>>>
>>>> + /*
>>>> + * allow only one legacy scheduled scan if user-space
>>>> + * does not indicate multiple scheduled scan support.
>>>> + */
>>>> + if (!info->attrs[NL80211_ATTR_SCHED_SCAN_MULTI] &&
>>>> + cfg80211_legacy_sched_scan_active(rdev))
>>>> return -EINPROGRESS;
>>>
>>> That probably doesn't go far enough - if legacy one is active then
>>> we
>>> probably shouldn't allow a new MULTI one either (or abandon the
>>> legacy
>>> one) so that older userspace doesn't get confused with multiple
>>> notifications from sched scans it didn't start.
>>
>> I considered that although not taking the notifications into account.
>> Will change it. Abandoning the legacy one would be a behavioral
>> change so probably not acceptable, right?
>
> Well, it would be acceptable since it's documented that it's possible
> it might stop for any reason. However, we need to prefer something -
> always preferring the new sched scan could lead to bounces, so we can
> prefer (1) existing, (2) legacy-single type or (3) new-multi type, but
> not (4) new sched scan.

Not sure I can follow. What is the difference between (1) and (2). Also
what do you consider (4) new sched scan. You mean the additional
parameterization of the scheduled scan?

> I think preferring the existing would probably be best, i.e. refuse
> legacy if any sched scan is running, and refuse multi if legacy is
> running?

Whatever the response above, I can understand this and it seems most
straightforward. So I tend agree this is our best option although maybe
for the wrong reason.

>> I guess your remark means this clarifies your earlier question about
>> the request id, right?
>
> yeah.

Thanks,
Arend

2017-01-05 13:46:07