Re: [RFC V3 03/11] nl80211: add support for gscan

[Arend Van Spriel]

On 16-12-2016 11:13, Johannes Berg wrote:
> On Wed, 2016-12-14 at 10:01 +0100, Arend Van Spriel wrote:
> 
>> Had to look for "> 16" ;-)
> 
> Sorry.
> 
>> Here an instance of the tab vs. space issue you mentioned. Will go
>> over the patch and fix that.
> 
> There were a few, not really interesting though - git would probably
> flag it anyway, or checkpatch :)
> 
>>> +   if (num_chans > 16)
>>> +   return -EINVAL;
>>
>> I suspect this is the restriction you were referring to. 
> 
> Yes.
> 
>> There is no
>> reason for this although the android wifi hal has max 16 channels in
>> a bucket so I might have picked that up. 
> 
> I thought I saw something with a u16 bitmap that seemed related, but I
> don't see that now so I'm probably just confused.
> 
>> So could a driver have a similar limit and should we add such to the
>> gscan capabilities? For instance our firmware api has a nasty
>> restriction of 64 channels for all buckets together, eg. can do 4
>> buckets of 16 channels each.
> 
> We do have a limit of the maximum scan buckets, which seems to be 16
> right now. We also have a limit on the number of channels per bucket,
> which is also 16, but no combined limit afaict (so 16x16 seems fine).
> 
> Maybe we do need some advertisement in that area then? Right now,
> wifihal seems to be able to read as capabilities the number of buckets
> (wifi_gscan_capabilities), but assumes the number of channels:
> 
> const unsigned MAX_CHANNELS= 16;
> const unsigned MAX_BUCKETS = 16;
> 
> I guess we took that and combined it, and you had more negotiation with
> Google ;-)

I was not so much involved with the initial gscan effort, but I guess
for brcm it might be true.

> We may then have to actually advertise the limit you have ("64 channels
> combined over all buckets"), unless you can get away with just
> advertising 4 buckets (and us saying 16 channels per bucket is enough?)
> 
> I'm a bit tempted to make this more forward compatible though and not
> hard-limit the number of channels per bucket in the code.

Indeed so I will remove it.

Regards,
Arend

Re: [RFC V3 03/11] nl80211: add support for gscan

[Johannes Berg]

On Wed, 2016-12-14 at 10:01 +0100, Arend Van Spriel wrote:

> Had to look for "> 16" ;-)

Sorry.

> Here an instance of the tab vs. space issue you mentioned. Will go
> over the patch and fix that.

There were a few, not really interesting though - git would probably
flag it anyway, or checkpatch :)

> > +   if (num_chans > 16)
> > +   return -EINVAL;
> 
> I suspect this is the restriction you were referring to. 

Yes.

> There is no
> reason for this although the android wifi hal has max 16 channels in
> a bucket so I might have picked that up. 

I thought I saw something with a u16 bitmap that seemed related, but I
don't see that now so I'm probably just confused.

> So could a driver have a similar limit and should we add such to the
> gscan capabilities? For instance our firmware api has a nasty
> restriction of 64 channels for all buckets together, eg. can do 4
> buckets of 16 channels each.

We do have a limit of the maximum scan buckets, which seems to be 16
right now. We also have a limit on the number of channels per bucket,
which is also 16, but no combined limit afaict (so 16x16 seems fine).

Maybe we do need some advertisement in that area then? Right now,
wifihal seems to be able to read as capabilities the number of buckets
(wifi_gscan_capabilities), but assumes the number of channels:

const unsigned MAX_CHANNELS= 16;
const unsigned MAX_BUCKETS = 16;

I guess we took that and combined it, and you had more negotiation with
Google ;-)

We may then have to actually advertise the limit you have ("64 channels
combined over all buckets"), unless you can get away with just
advertising 4 buckets (and us saying 16 channels per bucket is enough?)

I'm a bit tempted to make this more forward compatible though and not
hard-limit the number of channels per bucket in the code.

johannes

Re: [RFC V3 03/11] nl80211: add support for gscan

[Arend Van Spriel]

On 13-12-2016 23:29, Johannes Berg wrote:
> On Tue, 2016-12-13 at 21:09 +0100, Arend Van Spriel wrote:
>>  
>>> There's a bit of a weird hard-coded restriction to 16 channels too,
>>> that's due to the bucket map?
>>
>> Uhm. Is there? I will check, but if you can give me a pointer where
>> to look it is appreciated.
> 
> Just look for "< 16" or "<= 16" or so in the patch. I do think that's
> because the channel map is a u16 though, not sure we'd want to change
> that.

Had to look for "> 16" ;-)

> + /* ignore channels if band is specified */
> + if (band_select)
> + return 0;
> +
> +nla_for_each_nested(chan,
tb[NL80211_GSCAN_BUCKET_ATTR_CHANNELS], rem) {
> +num_chans++;
> +}

Here an instance of the tab vs. space issue you mentioned. Will go over
the patch and fix that.

> + if (num_chans > 16)
> + return -EINVAL;

I suspect this is the restriction you were referring to. There is no
reason for this although the android wifi hal has max 16 channels in a
bucket so I might have picked that up. So could a driver have a similar
limit and should we add such to the gscan capabilities? For instance our
firmware api has a nasty restriction of 64 channels for all buckets
together, eg. can do 4 buckets of 16 channels each.

Regards,
Arend

Re: [RFC V3 03/11] nl80211: add support for gscan

[Johannes Berg]

On Tue, 2016-12-13 at 21:09 +0100, Arend Van Spriel wrote:
> 
> > There's a bit of a weird hard-coded restriction to 16 channels too,
> > that's due to the bucket map?
> 
> Uhm. Is there? I will check, but if you can give me a pointer where
> to look it is appreciated.

Just look for "< 16" or "<= 16" or so in the patch. I do think that's
because the channel map is a u16 though, not sure we'd want to change
that.

johannes

Re: [RFC V3 03/11] nl80211: add support for gscan

[Arend Van Spriel]

On 13-12-2016 17:19, Johannes Berg wrote:
> On Mon, 2016-12-12 at 11:59 +, Arend van Spriel wrote:
>> This patch adds support for GScan which is a scan offload feature
>> used in Android.
> 
> Found a few places with spaces instead of tabs as indentation, and
> spurious braces around single-statement things, but other than that it
> looks fine from a patch/nl80211 POV.

I added a check in wiphy_register() in this patch which actually is more
appropriate with the "gscan capabilities" patch.

> Haven't really looked into the details of gscan itself now though,
> sorry.
> 
> There's a bit of a weird hard-coded restriction to 16 channels too,
> that's due to the bucket map?

Uhm. Is there? I will check, but if you can give me a pointer where to
look it is appreciated.

Regards,
Arend

Re: [RFC V3 03/11] nl80211: add support for gscan

[Johannes Berg]

On Mon, 2016-12-12 at 11:59 +, Arend van Spriel wrote:
> This patch adds support for GScan which is a scan offload feature
> used in Android.

Found a few places with spaces instead of tabs as indentation, and
spurious braces around single-statement things, but other than that it
looks fine from a patch/nl80211 POV.

Haven't really looked into the details of gscan itself now though,
sorry.

There's a bit of a weird hard-coded restriction to 16 channels too,
that's due to the bucket map?

johannes

Re: [RFC V3 03/11] nl80211: add support for gscan

[Arend Van Spriel]

On 12-12-2016 18:43, Dan Williams wrote:
>> +
>> +/**
>> + * enum nl80211_bucket_band - GScan bucket band selection.
> Quite possibly this was already covered and somebody requested you to
> change this to the current name.  If that's the case, ignore this.

Nope. You are the first ;-)

> But shouldn't this enum and bucket_event_report include "gscan" in
> their name, like the other gscan specific stuff does?  Are these going
> to get used for something else too, and will that thing make sense with
> the word "bucket"?  Just "nl80211_bucket_band" is pretty generic.

I figured the term bucket was making it gscan specific, but you are
right. Will change it.

Thanks,
Arend

Re: [RFC V3 03/11] nl80211: add support for gscan

[Dan Williams]

On Mon, 2016-12-12 at 11:59 +, Arend van Spriel wrote:
> This patch adds support for GScan which is a scan offload feature
> used in Android.
> 
> Reviewed-by: Hante Meuleman 
> Reviewed-by: Pieter-Paul Giesberts  m>
> Reviewed-by: Franky Lin 
> Signed-off-by: Arend van Spriel 
> ---
> Changes:
>  V2
>   - remove pr_err() statement from nl80211.c
>   - get rid of #if 0 code.
>  V3
>   - change and document storage type of gscan attributes.
>   - remove base period attribute from nl80211.
>   - bucket periods are changed to be seconds.
>   - change NO_IR attribute to PASSIVE.
>   - check for NL80211_ATTR_MAC{,_MASK} if random mac support is
> requested.
>   - remove NL80211_SCAN_FLAG_IE_DATA.
> ---
>  include/net/cfg80211.h   |  91 +++
>  include/uapi/linux/nl80211.h | 146 ++
>  net/wireless/core.c  |  31 
>  net/wireless/core.h  |   4 +
>  net/wireless/nl80211.c   | 356
> ++-
>  net/wireless/rdev-ops.h  |  25 +++
>  net/wireless/scan.c  |  28 
>  net/wireless/trace.h |   9 ++
>  8 files changed, 685 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index b78377f..8bc8842 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2453,6 +2453,92 @@ struct cfg80211_nan_func {
>  };
> 
>  /**
> + * struct cfg80211_gscan_channel - GScan channel parameters.
> + *
> +
> + * @ch: specific channel.
> + * @dwell_time: hint for dwell time in milliseconds.
> + * @passive: indicates passive scan is requested.
> + */
> +struct cfg80211_gscan_channel {
> +struct ieee80211_channel *ch;
> +u8 dwell_time;
> +bool passive;
> +};
> +
> +/**
> + * struct cfg80211_gscan_bucket - GScan bucket parameters.
> + *
> + * @idx: unique bucket index.
> + * @band: bit flags for band(s) to use, see %enum
> nl80211_bucket_band.
> + * @report_events: This is a bit field according %enum
> nl80211_bucket_report_event.
> + * @period: period in which the bucket is scheduled to be scanned.
> If the
> + *   period is too small for driver it should not fail but
> report results
> + *   as fast as it can. For exponential backoff bucket this is
> the minimum
> + *   period.
> + * @max_period: used only for the exponential backoff bucket whose
> scan period
> + *   will grow exponentially to a maximum period of max_period.
> + * @exponent: used only for the exponential backoff bucket.
> + * @step_count: used only for the exponential backoff bucket.
> + * @n_channels: number of channels in @channels array.
> + * @channels: channels to scan which may include DFS channels.
> + */
> +struct cfg80211_gscan_bucket {
> + u32 idx;
> + u16 period;
> + u8 band;
> + u8 report_events;
> + u16 max_period;
> + u8 exponent;
> + u8 step_count;
> + u8 n_channels;
> + struct cfg80211_gscan_channel *channels;
> +};
> +
> +/**
> + * struct cfg80211_gscan_request - GScan request parameters.
> + *
> + * @flags: scan request flags according %enum nl80211_scan_flags.
> + * @base_period: base timer period in milliseconds.
> + * @max_ap_per_scan: number of APs to store in each scan entry in
> the BSSID/RSSI
> + *   history buffer (keep APS with highest RSSI).
> + * @report_threshold_percent: wake up system when scan buffer is
> filled to this
> + *   percentage.
> + * @report_threshold_num_scans: wake up system when this many scans
> are stored
> + *   in scan buffer.
> + * @mac: MAC address used for randomisation.
> + * @mac_mask: MAC address mask. bits that are 0 in the mask should
> be
> + *   randomised, bits that are 1 should be taken as is from
> @mac.
> + * @n_buckets: number of entries in @buckets array.
> + * @buckets: array of GScan buckets.
> + *
> + * @dev: net device for which GScan is requested.
> + * @rcu_head: RCU callback used to free the struct.
> + * @owner_nlportid: netlink port which initiated this request.
> + * @scan_start: start time of this scan in jiffies.
> + */
> +struct cfg80211_gscan_request {
> + u32 flags;
> + u16 base_period;
> + u8 max_ap_per_scan;
> + u8 report_threshold_percent;
> + u8 report_threshold_num_scans;
> + u8 mac[ETH_ALEN];
> + u8 mac_mask[ETH_ALEN];
> +
> + u8 n_buckets;
> +
> + /* internal */
> + struct net_device *dev;
> + struct rcu_head rcu_head;
> + u32 owner_nlportid;
> + unsigned long scan_start;
> +
> + /* keep last */
> + struct cfg80211_gscan_bucket buckets[0];
> +};
> +
> +/**
>   * struct cfg80211_ops - backend description for wireless
> configuration
>   *
>   * This struct is registered by fullmac card drivers and/or wireless
> stacks
> @@ -2764,6 +2850,8 @@ struct cfg80211_nan_func {
>   *   All other parameters must be ignored.
>   *
>   * @set_multicast_to_unicast: configure multicast to unicast
> 

[RFC V3 03/11] nl80211: add support for gscan

[Arend van Spriel]

This patch adds support for GScan which is a scan offload feature
used in Android.

Reviewed-by: Hante Meuleman 
Reviewed-by: Pieter-Paul Giesberts 
Reviewed-by: Franky Lin 
Signed-off-by: Arend van Spriel 
---
Changes:
 V2
  - remove pr_err() statement from nl80211.c
  - get rid of #if 0 code.
 V3
  - change and document storage type of gscan attributes.
  - remove base period attribute from nl80211.
  - bucket periods are changed to be seconds.
  - change NO_IR attribute to PASSIVE.
  - check for NL80211_ATTR_MAC{,_MASK} if random mac support is requested.
  - remove NL80211_SCAN_FLAG_IE_DATA.
---
 include/net/cfg80211.h   |  91 +++
 include/uapi/linux/nl80211.h | 146 ++
 net/wireless/core.c  |  31 
 net/wireless/core.h  |   4 +
 net/wireless/nl80211.c   | 356 ++-
 net/wireless/rdev-ops.h  |  25 +++
 net/wireless/scan.c  |  28 
 net/wireless/trace.h |   9 ++
 8 files changed, 685 insertions(+), 5 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index b78377f..8bc8842 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2453,6 +2453,92 @@ struct cfg80211_nan_func {
 };

 /**
+ * struct cfg80211_gscan_channel - GScan channel parameters.
+ *
+
+ * @ch: specific channel.
+ * @dwell_time: hint for dwell time in milliseconds.
+ * @passive: indicates passive scan is requested.
+ */
+struct cfg80211_gscan_channel {
+struct ieee80211_channel *ch;
+u8 dwell_time;
+bool passive;
+};
+
+/**
+ * struct cfg80211_gscan_bucket - GScan bucket parameters.
+ *
+ * @idx: unique bucket index.
+ * @band: bit flags for band(s) to use, see %enum nl80211_bucket_band.
+ * @report_events: This is a bit field according %enum 
nl80211_bucket_report_event.
+ * @period: period in which the bucket is scheduled to be scanned. If the
+ * period is too small for driver it should not fail but report results
+ * as fast as it can. For exponential backoff bucket this is the minimum
+ * period.
+ * @max_period: used only for the exponential backoff bucket whose scan period
+ * will grow exponentially to a maximum period of max_period.
+ * @exponent: used only for the exponential backoff bucket.
+ * @step_count: used only for the exponential backoff bucket.
+ * @n_channels: number of channels in @channels array.
+ * @channels: channels to scan which may include DFS channels.
+ */
+struct cfg80211_gscan_bucket {
+   u32 idx;
+   u16 period;
+   u8 band;
+   u8 report_events;
+   u16 max_period;
+   u8 exponent;
+   u8 step_count;
+   u8 n_channels;
+   struct cfg80211_gscan_channel *channels;
+};
+
+/**
+ * struct cfg80211_gscan_request - GScan request parameters.
+ *
+ * @flags: scan request flags according %enum nl80211_scan_flags.
+ * @base_period: base timer period in milliseconds.
+ * @max_ap_per_scan: number of APs to store in each scan entry in the 
BSSID/RSSI
+ * history buffer (keep APS with highest RSSI).
+ * @report_threshold_percent: wake up system when scan buffer is filled to this
+ * percentage.
+ * @report_threshold_num_scans: wake up system when this many scans are stored
+ * in scan buffer.
+ * @mac: MAC address used for randomisation.
+ * @mac_mask: MAC address mask. bits that are 0 in the mask should be
+ * randomised, bits that are 1 should be taken as is from @mac.
+ * @n_buckets: number of entries in @buckets array.
+ * @buckets: array of GScan buckets.
+ *
+ * @dev: net device for which GScan is requested.
+ * @rcu_head: RCU callback used to free the struct.
+ * @owner_nlportid: netlink port which initiated this request.
+ * @scan_start: start time of this scan in jiffies.
+ */
+struct cfg80211_gscan_request {
+   u32 flags;
+   u16 base_period;
+   u8 max_ap_per_scan;
+   u8 report_threshold_percent;
+   u8 report_threshold_num_scans;
+   u8 mac[ETH_ALEN];
+   u8 mac_mask[ETH_ALEN];
+
+   u8 n_buckets;
+
+   /* internal */
+   struct net_device *dev;
+   struct rcu_head rcu_head;
+   u32 owner_nlportid;
+   unsigned long scan_start;
+
+   /* keep last */
+   struct cfg80211_gscan_bucket buckets[0];
+};
+
+/**
  * struct cfg80211_ops - backend description for wireless configuration
  *
  * This struct is registered by fullmac card drivers and/or wireless stacks
@@ -2764,6 +2850,8 @@ struct cfg80211_nan_func {
  * All other parameters must be ignored.
  *
  * @set_multicast_to_unicast: configure multicast to unicast conversion for BSS
+ * @start_gscan: start the GSCAN scanning offload.
+ * @stop_gscan: stop the GSCAN scanning offload.
  */
 struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -3048,6 +3136,9 @@ struct cfg80211_ops {
int