diff mbox

[RFC,V3,04/11] nl80211: add driver api for gscan notifications

Message ID 1481543997-24624-5-git-send-email-arend.vanspriel@broadcom.com
State RFC
Headers show

Commit Message

Arend Van Spriel Dec. 12, 2016, 11:59 a.m. UTC
The driver can indicate gscan results are available or gscan operation
has stopped.

Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 include/net/cfg80211.h       | 28 +++++++++++++++++++++
 include/uapi/linux/nl80211.h |  2 ++
 net/wireless/core.c          |  2 ++
 net/wireless/core.h          |  2 ++
 net/wireless/nl80211.c       | 19 +++++++++++++++
 net/wireless/nl80211.h       |  2 ++
 net/wireless/scan.c          | 58 ++++++++++++++++++++++++++++++++++++++++++++
 net/wireless/trace.h         | 10 ++++++++
 8 files changed, 123 insertions(+)

--
1.9.1

Comments

Johannes Berg Dec. 13, 2016, 4:20 p.m. UTC | #1
On Mon, 2016-12-12 at 11:59 +0000, Arend van Spriel wrote:
> The driver can indicate gscan results are available or gscan
> operation has stopped.

This patch is renumbering the previous patches' nl80211 API, which is
best avoided, even if I do realize it doesn't matter now. :)

Even here it's not clear how things are reported though. Somehow I
thought that gscan was reporting only partial information through the
buckets, or is that not true?

johannes
Arend Van Spriel Dec. 14, 2016, 10:07 a.m. UTC | #2
On 13-12-2016 17:20, Johannes Berg wrote:
> On Mon, 2016-12-12 at 11:59 +0000, Arend van Spriel wrote:
>> The driver can indicate gscan results are available or gscan
>> operation has stopped.
> 
> This patch is renumbering the previous patches' nl80211 API, which is
> best avoided, even if I do realize it doesn't matter now. :)

Indeed. Will be more careful in upcoming revision(s).

> Even here it's not clear how things are reported though. Somehow I
> thought that gscan was reporting only partial information through the
> buckets, or is that not true?

Not sure what is meant by "through the buckets". Referring to your
remark/question in the "Unversal scan proposal" thread:

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

So this is exactly the dilemma I considered so I decided to stick with
the full BSS reporting model for gscan as well merely to get it
discussed so glad you brought it up ;-). The problem here is that
gscan is a vehicle that serves a number of use-cases. So ignoring
hotlists, ePNO, etc. the gscan configuration still hold several
notification types:

- report after completing M scans capturing N best APs or a
  percentage of (M * N).
- report after completing a scan include a specific bucket.
- report full scan results.

The first two notification trigger retrieval of gscan results which are
historic, ie. partial scan results for max M scans.

As said earlier the universal scan proposal has some similarities to
gscan. Guess you share that as you are using the term "bucket reporting"
in that discussion ;-). The historic results are needed for location (if
I am not mistaken) so the full BSS reporting model does not fit that.
Question is what particular attribute in the historic results is needed
for location (suspecting only rssi and possibly the timestamp, but would
like to see that confirmed). I was thinking about have historic storage
in cfg80211 so we do not need a per-driver solution.

Regards,
Arend
Johannes Berg Dec. 16, 2016, 10:02 a.m. UTC | #3
> Not sure what is meant by "through the buckets".

TBH, I was handwaving because I don't understand this part of gscan
well :-)

> Referring to your
> remark/question in the "Unversal scan proposal" thread:
> 
> """
> 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?
> """
> 
> So this is exactly the dilemma I considered so I decided to stick
> with the full BSS reporting model for gscan as well merely to get it
> discussed so glad you brought it up ;-).

Heh.

Ok, so I missed that. Somehow I thought hidden in the buckets was a
partial reporting :-)

> The problem here is that gscan is a vehicle that serves a number of
> use-cases. So ignoring hotlists, ePNO, etc. the gscan configuration
> still hold several notification types:
> 
> - report after completing M scans capturing N best APs or a
>   percentage of (M * N).
> - report after completing a scan include a specific bucket.
> - report full scan results.
> 
> The first two notification trigger retrieval of gscan results which
> are historic, ie. partial scan results for max M scans.
> 
> As said earlier the universal scan proposal has some similarities to
> gscan. Guess you share that as you are using the term "bucket
> reporting" in that discussion ;-). The historic results are needed
> for location (if I am not mistaken) so the full BSS reporting model
> does not fit that. Question is what particular attribute in the
> historic results is needed for location (suspecting only rssi and
> possibly the timestamp, but would like to see that confirmed). I was
> thinking about have historic storage in cfg80211 so we do not need a
> per-driver solution.

Ok, now I'm starting to understand this better, I guess.

As far as I can tell from our code, for cached results we're reporting
the following data:

 * some flags
 * a scan ID
 * and for each AP:
   * RSSI
   * timestamp
   * channel
   * BSSID
   * SSID (which internally we even have a separate table for and each
           AP just has an index to it, to save memory I guess)
   * beacon period
   * capability field

No IEs and similar things like differentiating probe response/beacon,
so we can't use the full reporting for this.

I have no problem introducing a common storage for this, if necessary
with some fields/nl attributes being optional, but I suspect this is
actually a necessary part of gscan, otherwise you're not able to report
all the necessary data?

johannes
Arend Van Spriel Dec. 16, 2016, 12:17 p.m. UTC | #4
On 16-12-2016 11:02, Johannes Berg wrote:
> 
>> Not sure what is meant by "through the buckets".
> 
> TBH, I was handwaving because I don't understand this part of gscan
> well :-)
> 
>> Referring to your
>> remark/question in the "Unversal scan proposal" thread:
>>
>> """
>> 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?
>> """
>>
>> So this is exactly the dilemma I considered so I decided to stick
>> with the full BSS reporting model for gscan as well merely to get it
>> discussed so glad you brought it up ;-).
> 
> Heh.
> 
> Ok, so I missed that. Somehow I thought hidden in the buckets was a
> partial reporting :-)
> 
>> The problem here is that gscan is a vehicle that serves a number of
>> use-cases. So ignoring hotlists, ePNO, etc. the gscan configuration
>> still hold several notification types:
>>
>> - report after completing M scans capturing N best APs or a
>>   percentage of (M * N).
>> - report after completing a scan include a specific bucket.
>> - report full scan results.
>>
>> The first two notification trigger retrieval of gscan results which
>> are historic, ie. partial scan results for max M scans.
>>
>> As said earlier the universal scan proposal has some similarities to
>> gscan. Guess you share that as you are using the term "bucket
>> reporting" in that discussion ;-). The historic results are needed
>> for location (if I am not mistaken) so the full BSS reporting model
>> does not fit that. Question is what particular attribute in the
>> historic results is needed for location (suspecting only rssi and
>> possibly the timestamp, but would like to see that confirmed). I was
>> thinking about have historic storage in cfg80211 so we do not need a
>> per-driver solution.
> 
> Ok, now I'm starting to understand this better, I guess.
> 
> As far as I can tell from our code, for cached results we're reporting
> the following data:
> 
>  * some flags
>  * a scan ID
>  * and for each AP:
>    * RSSI
>    * timestamp
>    * channel
>    * BSSID
>    * SSID (which internally we even have a separate table for and each
>            AP just has an index to it, to save memory I guess)
>    * beacon period
>    * capability field
> 
> No IEs and similar things like differentiating probe response/beacon,
> so we can't use the full reporting for this.
> 
> I have no problem introducing a common storage for this, if necessary
> with some fields/nl attributes being optional, but I suspect this is
> actually a necessary part of gscan, otherwise you're not able to report
> all the necessary data?

If you just look at the gscan api in wifihal then yes. I was just
wondering whether "all the necessary data" really comprises all these
from a use-case perspective. As an example the api also has rtt fields,
but both brcm and intel solutions do not report that.

Regards,
Arend
Johannes Berg Dec. 16, 2016, 12:36 p.m. UTC | #5
On Fri, 2016-12-16 at 13:17 +0100, Arend Van Spriel wrote:
> 
> > I have no problem introducing a common storage for this, if
> > necessary with some fields/nl attributes being optional, but I
> > suspect this is actually a necessary part of gscan, otherwise
> > you're not able to report all the necessary data?
> 
> If you just look at the gscan api in wifihal then yes. I was just
> wondering whether "all the necessary data" really comprises all these
> from a use-case perspective. As an example the api also has rtt
> fields, but both brcm and intel solutions do not report that.

Yeah, no idea. Sorry - my wording wasn't quite precise. By "this is
actually a necessary part of gscan" I meant the partial history
reporting itself, not any particular field thereof.

johannes
diff mbox

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 8bc8842..4b02585 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4552,6 +4552,34 @@  void cfg80211_scan_done(struct cfg80211_scan_request *request,
 void cfg80211_sched_scan_stopped_rtnl(struct wiphy *wiphy);

 /**
+ * cfg80211_gscan_results - notify that new scan results are available
+ *
+ * @wiphy: the wiphy which got GScan results
+ */
+void cfg80211_gscan_results(struct wiphy *wiphy);
+
+/**
+ * cfg80211_gscan_stopped - notify that the GScan has stopped
+ *
+ * @wiphy: the wiphy on which the GScan stopped.
+ *
+ * The driver can call this function to inform cfg80211 that the
+ * GScan had to be stopped, for whatever reason.
+ */
+void cfg80211_gscan_stopped(struct wiphy *wiphy);
+
+/**
+ * cfg80211_gscan_stopped_rtnl - notify that the GScan has stopped
+ *
+ * @wiphy: the wiphy on which the GScan stopped.
+ *
+ * The driver can call this function to inform cfg80211 that the
+ * GScan had to be stopped, for whatever reason.
+ * This function should be called with rtnl locked.
+ */
+void cfg80211_gscan_stopped_rtnl(struct wiphy *wiphy);
+
+/**
  * cfg80211_inform_bss_frame_data - inform cfg80211 of a received BSS frame
  * @wiphy: the wiphy reporting the BSS
  * @data: the BSS metadata
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 5e42383..fd7ccd2 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -896,6 +896,7 @@ 
  *
  * @NL80211_CMD_START_GSCAN: start GScan.
  * @NL80211_CMD_STOP_GSCAN: request to stop current GScan.
+ * @NL80211_CMD_GSCAN_RESULTS: indicates that there GScan results available.
  * @NL80211_CMD_GSCAN_STOPPED: indicates that the currently running GScan
  *	has stopped. This event is generated upon @NL80211_CMD_STOP_GSCAN and
  *	the driver may issue this event at any time when a GScan is running.
@@ -1101,6 +1102,7 @@  enum nl80211_commands {

 	NL80211_CMD_START_GSCAN,
 	NL80211_CMD_STOP_GSCAN,
+	NL80211_CMD_GSCAN_RESULTS,
 	NL80211_CMD_GSCAN_STOPPED,

 	/* add new commands above here */
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 760a2fb..69eea4c 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -453,6 +453,7 @@  struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
 	INIT_LIST_HEAD(&rdev->bss_list);
 	INIT_WORK(&rdev->scan_done_wk, __cfg80211_scan_done);
 	INIT_WORK(&rdev->sched_scan_results_wk, __cfg80211_sched_scan_results);
+	INIT_WORK(&rdev->gscan_results_wk, __cfg80211_gscan_results);
 	INIT_LIST_HEAD(&rdev->mlme_unreg);
 	spin_lock_init(&rdev->mlme_unreg_lock);
 	INIT_WORK(&rdev->mlme_unreg_wk, cfg80211_mlme_unreg_wk);
@@ -935,6 +936,7 @@  void wiphy_unregister(struct wiphy *wiphy)
 	cancel_delayed_work_sync(&rdev->dfs_update_channels_wk);
 	flush_work(&rdev->destroy_work);
 	flush_work(&rdev->sched_scan_stop_wk);
+	flush_work(&rdev->gscan_stop_wk);
 	flush_work(&rdev->mlme_unreg_wk);

 #ifdef CONFIG_PM
diff --git a/net/wireless/core.h b/net/wireless/core.h
index ee1d162..89e934f 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -79,6 +79,7 @@  struct cfg80211_registered_device {
 	unsigned long suspend_at;
 	struct work_struct scan_done_wk;
 	struct work_struct sched_scan_results_wk;
+	struct work_struct gscan_results_wk;

 	struct genl_info *cur_cmd_info;

@@ -424,6 +425,7 @@  void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev,
 void __cfg80211_sched_scan_results(struct work_struct *wk);
 int __cfg80211_stop_sched_scan(struct cfg80211_registered_device *rdev,
 			       bool driver_initiated);
+void __cfg80211_gscan_results(struct work_struct *wk);
 int __cfg80211_stop_gscan(struct cfg80211_registered_device *rdev,
 			  bool driver_initiated);
 void cfg80211_upload_connect_keys(struct wireless_dev *wdev);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 4186ece..7699b16 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13302,6 +13302,25 @@  void nl80211_send_sched_scan_results(struct cfg80211_registered_device *rdev,
 				NL80211_MCGRP_SCAN, GFP_KERNEL);
 }

+void nl80211_send_gscan_results(struct cfg80211_registered_device *rdev,
+				struct net_device *netdev)
+{
+	struct sk_buff *msg;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return;
+
+	if (nl80211_send_scan_event_msg(msg, rdev, netdev, 0, 0, 0,
+					NL80211_CMD_GSCAN_RESULTS) < 0) {
+		nlmsg_free(msg);
+		return;
+	}
+
+	genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+				NL80211_MCGRP_SCAN, GFP_KERNEL);
+}
+
 void nl80211_send_scan_event(struct cfg80211_registered_device *rdev,
 			     struct net_device *netdev, u32 cmd)
 {
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index fb304ce9..4eec856 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -20,6 +20,8 @@  void nl80211_send_scan_event(struct cfg80211_registered_device *rdev,
 			     struct net_device *netdev, u32 cmd);
 void nl80211_send_sched_scan_results(struct cfg80211_registered_device *rdev,
 				     struct net_device *netdev);
+void nl80211_send_gscan_results(struct cfg80211_registered_device *rdev,
+				     struct net_device *netdev);
 void nl80211_common_reg_change_event(enum nl80211_commands cmd_id,
 				     struct regulatory_request *request);

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 8c141c2..cc4f7c7 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -338,6 +338,44 @@  void cfg80211_sched_scan_results(struct wiphy *wiphy)
 }
 EXPORT_SYMBOL(cfg80211_sched_scan_results);

+void __cfg80211_gscan_results(struct work_struct *wk)
+{
+	struct cfg80211_registered_device *rdev;
+	struct cfg80211_gscan_request *request;
+
+	rdev = container_of(wk, struct cfg80211_registered_device,
+			    gscan_results_wk);
+
+	rtnl_lock();
+
+	request = rtnl_dereference(rdev->gscan_req);
+
+	/* we don't have request anymore if the scan is stopping */
+	if (request) {
+		if (request->flags & NL80211_SCAN_FLAG_FLUSH) {
+			/* flush entries from previous scans */
+			spin_lock_bh(&rdev->bss_lock);
+			__cfg80211_bss_expire(rdev, request->scan_start);
+			spin_unlock_bh(&rdev->bss_lock);
+			request->scan_start = jiffies;
+		}
+		nl80211_send_gscan_results(rdev, request->dev);
+	}
+
+	rtnl_unlock();
+}
+
+void cfg80211_gscan_results(struct wiphy *wiphy)
+{
+	trace_cfg80211_gscan_results(wiphy);
+	/* ignore if we're not scanning */
+
+	if (rcu_access_pointer(wiphy_to_rdev(wiphy)->gscan_req))
+		queue_work(cfg80211_wq,
+			   &wiphy_to_rdev(wiphy)->gscan_results_wk);
+}
+EXPORT_SYMBOL(cfg80211_gscan_results);
+
 void cfg80211_sched_scan_stopped_rtnl(struct wiphy *wiphy)
 {
 	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
@@ -358,6 +396,26 @@  void cfg80211_sched_scan_stopped(struct wiphy *wiphy)
 }
 EXPORT_SYMBOL(cfg80211_sched_scan_stopped);

+void cfg80211_gscan_stopped_rtnl(struct wiphy *wiphy)
+{
+	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
+
+	ASSERT_RTNL();
+
+	trace_cfg80211_gscan_stopped(wiphy);
+
+	__cfg80211_stop_gscan(rdev, true);
+}
+EXPORT_SYMBOL(cfg80211_gscan_stopped_rtnl);
+
+void cfg80211_gscan_stopped(struct wiphy *wiphy)
+{
+	rtnl_lock();
+	cfg80211_gscan_stopped_rtnl(wiphy);
+	rtnl_unlock();
+}
+EXPORT_SYMBOL(cfg80211_gscan_stopped);
+
 int __cfg80211_stop_sched_scan(struct cfg80211_registered_device *rdev,
 			       bool driver_initiated)
 {
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 1d0fde9..f3f5d82 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -2799,6 +2799,16 @@ 
 	TP_ARGS(wiphy)
 );

+DEFINE_EVENT(wiphy_only_evt, cfg80211_gscan_results,
+	TP_PROTO(struct wiphy *wiphy),
+	TP_ARGS(wiphy)
+);
+
+DEFINE_EVENT(wiphy_only_evt, cfg80211_gscan_stopped,
+	TP_PROTO(struct wiphy *wiphy),
+	TP_ARGS(wiphy)
+);
+
 TRACE_EVENT(cfg80211_get_bss,
 	TP_PROTO(struct wiphy *wiphy, struct ieee80211_channel *channel,
 		 const u8 *bssid, const u8 *ssid, size_t ssid_len,