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

On 2-1-2017 11:44, Johannes Berg wrote:

quoted

+	/*
+	 * 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?

quoted

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

quoted

+	/* 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.

quoted

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

quoted

+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