| Message ID | 1479821515-13261-1-git-send-email-arend.vanspriel@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
> + if (wiphy->min_sched_scan_plan_interval && > + request->scan_plans[i].interval < > + wiphy->min_sched_scan_plan_interval) > + return -EINVAL; > I'm not sure we should break the API that way - just move it up if it's smaller? johannes
On 22-11-2016 14:38, Johannes Berg wrote: > >> + if (wiphy->min_sched_scan_plan_interval && >> + request->scan_plans[i].interval < >> + wiphy->min_sched_scan_plan_interval) >> + return -EINVAL; >> > I'm not sure we should break the API that way - just move it up if it's > smaller? Are we? Currently, the minimum is not checked in nl80211, but that does not say anything about the driver which might validate the interval as well and return an error. What made me start looking at this is that in brcmfmac the interval in the request was ignored and a fixed interval was provisioned in the device. I wanted to fix that but was not sure if I needed to check it against our firmware min..max range and what the appropriate error handling should be. If silently changing what user-space is requesting is fine for this, I am happy to make it so. Preferably in nl80211. Regards, Arend
Hi Arend, On Tue, 2016-11-22 at 13:31 +0000, Arend van Spriel wrote: > The interval for scheduled scan may have a minimum value for > the device. Allow drivers to specify a minimum value in the > struct wiphy so user-space interval values can be validated > against it. > > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > --- I'm not sure this is necessary, because the interval is a "soft" requirement, there being no guarantee on the accuracy of the intervals. The minimum interval a firmware can support is probably variable, depending on how long time it takes for a scan to complete. Let's say it takes 1 second to scan a certain amount of channels. In this case, the minimum interval is probably 1 second (i.e. you can start a new scan immediately after the first one completed). Now, if you're scanning more channels and it takes, say, 4 seconds to complete, the minimum interval would be 4 seconds. The iwlwifi firmware treats the interval as the amount of time to wait *after* one scan cycle completes before starting the cycle. Since sched scan is mostly used while the system is suspended, the interval can be quite large and the accuracy really doesn't matter. The absoulte time when the scan cycles occur will drift over time, but does it matter? So, for firmwares that really need to get a minimum value, I think bumping up to the minimum allowed if the provided value is too low, should be done in the driver. -- Cheers, Luca.
Sorry, forgot to reply to this until Luca's email bumped it up... On Tue, 2016-11-22 at 21:06 +0100, Arend Van Spriel wrote: > Are we? Currently, the minimum is not checked in nl80211, but that > does not say anything about the driver which might validate the > interval as well and return an error. Well, since drivers currently don't return an error (even if they ignore the value!) that *does* change the API. > What made me start looking at this is that in brcmfmac the interval > in the request was ignored and a fixed interval was provisioned in > the device. I wanted to fix that but was not sure if I needed to > check it against our firmware min..max range and what the appropriate > error handling should be. If silently changing what user-space is > requesting is fine for this, I am happy to make it so. Preferably in > nl80211. I think (agreeing with Luca) bumping it up is fine. johannes
On 11/25/2016 9:25 AM, Johannes Berg wrote: > Sorry, forgot to reply to this until Luca's email bumped it up... > > On Tue, 2016-11-22 at 21:06 +0100, Arend Van Spriel wrote: > >> Are we? Currently, the minimum is not checked in nl80211, but that >> does not say anything about the driver which might validate the >> interval as well and return an error. > > Well, since drivers currently don't return an error (even if they > ignore the value!) that *does* change the API. > >> What made me start looking at this is that in brcmfmac the interval >> in the request was ignored and a fixed interval was provisioned in >> the device. I wanted to fix that but was not sure if I needed to >> check it against our firmware min..max range and what the appropriate >> error handling should be. If silently changing what user-space is >> requesting is fine for this, I am happy to make it so. Preferably in >> nl80211. > > I think (agreeing with Luca) bumping it up is fine. Fine by me although the "drift over time" reason seems only more reason to have minimum validation mainly because nowhere is nl80211.h it is stated that the interval is a "soft" requirement. Now Luca proposes bumping to minimum should be done in the driver. What is your opinion? I will update the kernel doc to clarify what can be expected from the interval value. Regards, Arend
On Fri, 2016-11-25 at 11:06 +0100, Arend van Spriel wrote: > > On 11/25/2016 9:25 AM, Johannes Berg wrote: > > Sorry, forgot to reply to this until Luca's email bumped it up... > > > > On Tue, 2016-11-22 at 21:06 +0100, Arend Van Spriel wrote: > > > > > Are we? Currently, the minimum is not checked in nl80211, but that > > > does not say anything about the driver which might validate the > > > interval as well and return an error. > > > > Well, since drivers currently don't return an error (even if they > > ignore the value!) that *does* change the API. > > > > > What made me start looking at this is that in brcmfmac the interval > > > in the request was ignored and a fixed interval was provisioned in > > > the device. I wanted to fix that but was not sure if I needed to > > > check it against our firmware min..max range and what the appropriate > > > error handling should be. If silently changing what user-space is > > > requesting is fine for this, I am happy to make it so. Preferably in > > > nl80211. > > > > I think (agreeing with Luca) bumping it up is fine. > > Fine by me although the "drift over time" reason seems only more reason > to have minimum validation mainly because nowhere is nl80211.h it is > stated that the interval is a "soft" requirement. Now Luca proposes > bumping to minimum should be done in the driver. What is your opinion? > > I will update the kernel doc to clarify what can be expected from the > interval value. Yeah, I was almost sure there was a statement somewhere that the interval is "soft", but there isn't. I was confusing with the match logic, which is clearly documented as not-guaranteed: "...there is no guarantee that the reported BSSs are fully complying with the match sets and userspace needs to be able to ignore them by itself." A clarification in the documentation would be great. -- Luca.
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 2019310..ef5d6ab 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -3427,6 +3427,8 @@ struct wiphy_iftype_ext_capab { * scans * @max_sched_scan_plans: maximum number of scan plans (scan interval and number * of iterations) for scheduled scan supported by the device. + * @min_sched_scan_plan_interval: minimum interval (in seconds) for a + * single scan plan supported by the device. * @max_sched_scan_plan_interval: maximum interval (in seconds) for a * single scan plan supported by the device. * @max_sched_scan_plan_iterations: maximum number of iterations for a single @@ -3552,6 +3554,7 @@ struct wiphy { u16 max_scan_ie_len; u16 max_sched_scan_ie_len; u32 max_sched_scan_plans; + u32 min_sched_scan_plan_interval; u32 max_sched_scan_plan_interval; u32 max_sched_scan_plan_iterations; diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 259c9c7..77fc77a 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -2381,6 +2381,8 @@ enum nl80211_attrs { NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED, + NL80211_ATTR_MIN_SCAN_PLAN_INTERVAL, + /* add attributes here, update the policy in nl80211.c */ __NL80211_ATTR_AFTER_LAST, diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 24ab199..47aca56 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -1474,6 +1474,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, rdev->wiphy.max_sched_scan_plans) || nla_put_u32(msg, NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL, rdev->wiphy.max_sched_scan_plan_interval) || + nla_put_u32(msg, NL80211_ATTR_MIN_SCAN_PLAN_INTERVAL, + rdev->wiphy.min_sched_scan_plan_interval) || nla_put_u32(msg, NL80211_ATTR_MAX_SCAN_PLAN_ITERATIONS, rdev->wiphy.max_sched_scan_plan_iterations)) goto nla_put_failure; @@ -6777,6 +6779,12 @@ static int nl80211_abort_scan(struct sk_buff *skb, struct genl_info *info) if (!request->scan_plans[0].interval) return -EINVAL; + if (wiphy->min_sched_scan_plan_interval && + request->scan_plans[0].interval < + wiphy->min_sched_scan_plan_interval) + request->scan_plans[0].interval = + wiphy->min_sched_scan_plan_interval; + if (request->scan_plans[0].interval > wiphy->max_sched_scan_plan_interval) request->scan_plans[0].interval = @@ -6805,6 +6813,10 @@ static int nl80211_abort_scan(struct sk_buff *skb, struct genl_info *info) request->scan_plans[i].interval > wiphy->max_sched_scan_plan_interval) return -EINVAL; + if (wiphy->min_sched_scan_plan_interval && + request->scan_plans[i].interval < + wiphy->min_sched_scan_plan_interval) + return -EINVAL; if (plan[NL80211_SCHED_SCAN_PLAN_ITERATIONS]) { request->scan_plans[i].iterations =
The interval for scheduled scan may have a minimum value for the device. Allow drivers to specify a minimum value in the struct wiphy so user-space interval values can be validated against it. Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> --- include/net/cfg80211.h | 3 +++ include/uapi/linux/nl80211.h | 2 ++ net/wireless/nl80211.c | 12 ++++++++++++ 3 files changed, 17 insertions(+)