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