Hi Johannes, Luca,
The gscan work made me look at scheduled scan and the implementation of
it in brcmfmac. The driver ignored the interval parameter from
user-space. Now I am fixing that. One thing is that our firmware has a
minimum interval which can not be indicated in struct wiphy. The other
issue is how the maximum interval is used in the nl80211.c.
In nl80211_parse_sched_scan_plans() it is used against value passed in
NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
For the first one it caps the value to the maximum, but for the second
one it returns -EINVAL. I suspect this is done because maximum interval
was introduced with schedule scan plans, but it feels inconsistent.
Thoughts?
Regards,
Arend
On Tue, 2016-11-22 at 10:10 +0100, Arend Van Spriel wrote:
> On 22-11-2016 6:59, Luca Coelho wrote:
> > Oh, I see. The problem is that the "max_sched_scan_plan_interval" was
> > introduced later. If the userspace passes
> > NL80211__ATTR_SCHED_SCAN_INTERVAL, it probably means that it doesn't
> > know about NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL (i.e. it's only using an
> > old API). If it is also, for some reason, passing a very large number,
> > we shouldn't suddenly make it fail with -EINVALID, because that would
> > be a break of UABI. And since we know the driver cannot support such a
> > large number, we cap it because it's the best we can do.
> >
> > Now, if the userspace uses NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL, it
> > means that it knows the new API (and was written after the new API was
> > introduced), so we can be stricter and assume it must have checked
> > NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL.
> >
> > Makes sense?
>
> Not really. As you say if user-space passes
> NL80211_ATTR_SCHED_SCAN_INTERVAL it is using old API. Otherwise it
> should pass NL80211_ATTR_SCHED_SCAN_PLANS.
Errr... I meant "if the userspace uses NL80211_ATTR_SCHED_SCAN_PLANS",
pasted the wrong thing. ;)
> If the driver doesn't set the max_sched_scan_plan_interval, mac80211's
> > default of MAX_U32 will be used:
> >
> > struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
> > const char *requested_name)
> > {
> > [...]
> > rdev->wiphy.max_sched_scan_plans = 1;
> > rdev->wiphy.max_sched_scan_plan_interval = U32_MAX;
> >
> > return &rdev->wiphy;
> > }
> > EXPORT_SYMBOL(wiphy_new_nm);
> >
> > ...so max_sched_scan_plan_interval will never be zero, unless the
> > driver explicitly sets it to zero.
>
> I think you are overlooking the cfg80211-based drivers here. According
> to lxr at least brcmfmac, mwifiex, and ath6kl are not specifying it.
> "Funny" detail is that scheduled scan support in mwifiex seems to be
> introduced after the scan plan API change.
You're right, I did overlook non-mac80211 drivers.
> With the old API nl80211 only assured it to be non-zero. The only one
> capping the value was iwlmvm. The only one setting the
> max_sched_scan_plan_interval (overriding mac80211 value) now is iwlmvm.
> So I think checking if it is set before capping it retains the old API
> behaviour.
Fair enough. The change you propose, to also check whether the value
is set (since 0 interval doesn't make sense), is valid. :)
--
Luca.
On 22-11-2016 10:19, Luca Coelho wrote:
> On Tue, 2016-11-22 at 10:10 +0100, Arend Van Spriel wrote:
>> On 22-11-2016 6:59, Luca Coelho wrote:
>>> Oh, I see. The problem is that the "max_sched_scan_plan_interval" was
>>> introduced later. If the userspace passes
>>> NL80211__ATTR_SCHED_SCAN_INTERVAL, it probably means that it doesn't
>>> know about NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL (i.e. it's only using an
>>> old API). If it is also, for some reason, passing a very large number,
>>> we shouldn't suddenly make it fail with -EINVALID, because that would
>>> be a break of UABI. And since we know the driver cannot support such a
>>> large number, we cap it because it's the best we can do.
>>>
>>> Now, if the userspace uses NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL, it
>>> means that it knows the new API (and was written after the new API was
>>> introduced), so we can be stricter and assume it must have checked
>>> NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL.
>>>
>>> Makes sense?
>>
>> Not really. As you say if user-space passes
>> NL80211_ATTR_SCHED_SCAN_INTERVAL it is using old API. Otherwise it
>> should pass NL80211_ATTR_SCHED_SCAN_PLANS.
>
> Errr... I meant "if the userspace uses NL80211_ATTR_SCHED_SCAN_PLANS",
> pasted the wrong thing. ;)
>
>
>> If the driver doesn't set the max_sched_scan_plan_interval, mac80211's
>>> default of MAX_U32 will be used:
>>>
>>> struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
>>> const char *requested_name)
>>> {
>>> [...]
>>> rdev->wiphy.max_sched_scan_plans = 1;
>>> rdev->wiphy.max_sched_scan_plan_interval = U32_MAX;
>>>
>>> return &rdev->wiphy;
>>> }
>>> EXPORT_SYMBOL(wiphy_new_nm);
>>>
>>> ...so max_sched_scan_plan_interval will never be zero, unless the
>>> driver explicitly sets it to zero.
>>
>> I think you are overlooking the cfg80211-based drivers here. According
>> to lxr at least brcmfmac, mwifiex, and ath6kl are not specifying it.
>> "Funny" detail is that scheduled scan support in mwifiex seems to be
>> introduced after the scan plan API change.
>
> You're right, I did overlook non-mac80211 drivers.
Actually, you are referring to wiphy_new_nm() which is obviously used
for cfg80211-based drivers as well. So I will ask to drop my patch.
Regards,
Arend
Hi Arend,
On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
> On 21-11-2016 12:30, Arend Van Spriel wrote:
> > On 21-11-2016 12:19, Arend Van Spriel wrote:
> > > Hi Johannes, Luca,
> > >
> > > The gscan work made me look at scheduled scan and the implementation of
> > > it in brcmfmac. The driver ignored the interval parameter from
> > > user-space. Now I am fixing that. One thing is that our firmware has a
> > > minimum interval which can not be indicated in struct wiphy. The other
> > > issue is how the maximum interval is used in the nl80211.c.
> > >
> > > In nl80211_parse_sched_scan_plans() it is used against value passed in
> > > NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
> > > For the first one it caps the value to the maximum, but for the second
> > > one it returns -EINVAL. I suspect this is done because maximum interval
> > > was introduced with schedule scan plans, but it feels inconsistent.
> >
> > It also maybe simply wrong to cap. At least brcmfmac does not set the
> > maximum so it will always get interval being zero. Maybe better to do:
> >
> > if (wiphy->max_sched_scan_plan_interval &&
> > request->scan_plans[0].interval >
> > wiphy->max_sched_scan_plan_interval)
> > return -EINVAL;
> >
> > > Thoughts?
>
> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
> struct sched_scan_request::interval was specified in milliseconds! Below
> the drivers that I see having scheduled scan support:
>
> iwlmvm: cap interval, convert to seconds.
> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
> wl12xx: no checking in driver, fw need milliseconds.
> wl18xx: no checking in driver, fw need milliseconds.
>
> The milliseconds conversion seems to be taken care of by multiplying
> with MSEC_PER_SEC in wl{12,18}xx drivers.
>
> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
> other drivers will get interval of zero which only ath6kl handles.
With the introduction of scheduled scan plans, we sort of deprecated
the "generic" scheduled scan interval. It doesn't make sense to have
both passed at the same time, so nl80211 forbids
NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
NL80211_ATTR_SCHED_SCAN_PLANS.
The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
which is silly because we can never get millisecond accuracy in this.
Thus, in the plans API, we decided to use seconds instead (because it
makes much more sense). Additionally, the interval is considered
"advisory", because the FW may not be able guarantee the exact
intervals (for instance, the iwlwifi driver actually starts the
interval timer after scan completion, so if you specify 10 seconds
intervals, in practice they'll be 13-14 seconds).
I'm not sure I'm answering your question, because I'm also not sure I
understood the question. :)
--
Cheers,
Luca.
On 21-11-2016 12:19, Arend Van Spriel wrote:
> Hi Johannes, Luca,
>
> The gscan work made me look at scheduled scan and the implementation of
> it in brcmfmac. The driver ignored the interval parameter from
> user-space. Now I am fixing that. One thing is that our firmware has a
> minimum interval which can not be indicated in struct wiphy. The other
> issue is how the maximum interval is used in the nl80211.c.
>
> In nl80211_parse_sched_scan_plans() it is used against value passed in
> NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
> For the first one it caps the value to the maximum, but for the second
> one it returns -EINVAL. I suspect this is done because maximum interval
> was introduced with schedule scan plans, but it feels inconsistent.
It also maybe simply wrong to cap. At least brcmfmac does not set the
maximum so it will always get interval being zero. Maybe better to do:
if (wiphy->max_sched_scan_plan_interval &&
request->scan_plans[0].interval >
wiphy->max_sched_scan_plan_interval)
return -EINVAL;
> Thoughts?
>
> Regards,
> Arend
>
On 22-11-2016 6:59, Luca Coelho wrote:
> Hi Arend,
> On Mon, 2016-11-21 at 20:34 +0100, Arend Van Spriel wrote:
>> On 21-11-2016 16:08, Luca Coelho wrote:
>>> Hi Arend,
>>>
>>> On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
>>>> On 21-11-2016 12:30, Arend Van Spriel wrote:
>>>>> On 21-11-2016 12:19, Arend Van Spriel wrote:
>>>>>> Hi Johannes, Luca,
>>>>>>
>>>>>> The gscan work made me look at scheduled scan and the implementation of
>>>>>> it in brcmfmac. The driver ignored the interval parameter from
>>>>>> user-space. Now I am fixing that. One thing is that our firmware has a
>>>>>> minimum interval which can not be indicated in struct wiphy. The other
>>>>>> issue is how the maximum interval is used in the nl80211.c.
>>>>>>
>>>>>> In nl80211_parse_sched_scan_plans() it is used against value passed in
>>>>>> NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
>>>>>> For the first one it caps the value to the maximum, but for the second
>>>>>> one it returns -EINVAL. I suspect this is done because maximum interval
>>>>>> was introduced with schedule scan plans, but it feels inconsistent.
>>>>>
>>>>> It also maybe simply wrong to cap. At least brcmfmac does not set the
>>>>> maximum so it will always get interval being zero. Maybe better to do:
>>>>>
>>>>> if (wiphy->max_sched_scan_plan_interval &&
>>>>> request->scan_plans[0].interval >
>>>>> wiphy->max_sched_scan_plan_interval)
>>>>> return -EINVAL;
>>>>>
>>>>>> Thoughts?
>>>>
>>>> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
>>>> struct sched_scan_request::interval was specified in milliseconds! Below
>>>> the drivers that I see having scheduled scan support:
>>>>
>>>> iwlmvm: cap interval, convert to seconds.
>>>> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
>>>> wl12xx: no checking in driver, fw need milliseconds.
>>>> wl18xx: no checking in driver, fw need milliseconds.
>>>>
>>>> The milliseconds conversion seems to be taken care of by multiplying
>>>> with MSEC_PER_SEC in wl{12,18}xx drivers.
>>>>
>>>> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
>>>> other drivers will get interval of zero which only ath6kl handles.
>>>
>>> With the introduction of scheduled scan plans, we sort of deprecated
>>> the "generic" scheduled scan interval. It doesn't make sense to have
>>> both passed at the same time, so nl80211 forbids
>>> NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
>>> NL80211_ATTR_SCHED_SCAN_PLANS.
>>
>> Indeed, but if no plans are passed it is still allowed.
>
> That's right. But the driver will get it as a single plan.
>
>
>>> The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
>>> which is silly because we can never get millisecond accuracy in this.
>>> Thus, in the plans API, we decided to use seconds instead (because it
>>> makes much more sense). Additionally, the interval is considered
>>> "advisory", because the FW may not be able guarantee the exact
>>> intervals (for instance, the iwlwifi driver actually starts the
>>> interval timer after scan completion, so if you specify 10 seconds
>>> intervals, in practice they'll be 13-14 seconds).
>>
>> Agree. Our firmware wants to have it in seconds as well.
>
> It was actually my mistake when I implemented it in my TI days (can't
> hide from git blame ;)). TI's firmware used msecs and I just blindly
> followed it.
>
>
>>> I'm not sure I'm answering your question, because I'm also not sure I
>>> understood the question. :)
>>
>> The question is this: Why is the interval capped at
>> max_sched_scan_plan_interval if it exceeds it and no plans are provided
>> (so continue to setup the scheduled scan request) whereas when plans are
>> provided and an interval exceeds max_sched_scan_plan_interval it aborts
>> with -EINVAL.
>
> Oh, I see. The problem is that the "max_sched_scan_plan_interval" was
> introduced later. If the userspace passes
> NL80211__ATTR_SCHED_SCAN_INTERVAL, it probably means that it doesn't
> know about NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL (i.e. it's only using an
> old API). If it is also, for some reason, passing a very large number,
> we shouldn't suddenly make it fail with -EINVALID, because that would
> be a break of UABI. And since we know the driver cannot support such a
> large number, we cap it because it's the best we can do.
>
> Now, if the userspace uses NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL, it
> means that it knows the new API (and was written after the new API was
> introduced), so we can be stricter and assume it must have checked
> NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL.
>
> Makes sense?
Not really. As you say if user-space passes
NL80211_ATTR_SCHED_SCAN_INTERVAL it is using old API. Otherwise it
should pass NL80211_ATTR_SCHED_SCAN_PLANS.
>> And a follow-up question for this snippet of code in
>> nl80211_parse_sched_scan_plans():
>>
>> if (!attrs[NL80211_ATTR_SCHED_SCAN_PLANS]) {
>> u32 interval;
>>
>> /*
>> * If scan plans are not specified,
>> * %NL80211_ATTR_SCHED_SCAN_INTERVAL must be specified. In this
>> * case one scan plan will be set with the specified scan
>> * interval and infinite number of iterations.
>> */
>> if (!attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL])
>> return -EINVAL;
>>
>> interval = nla_get_u32(attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL]);
>> if (!interval)
>> return -EINVAL;
>>
>> request->scan_plans[0].interval =
>> DIV_ROUND_UP(interval, MSEC_PER_SEC);
>> if (!request->scan_plans[0].interval)
>> return -EINVAL;
>>
>> if (request->scan_plans[0].interval >
>> wiphy->max_sched_scan_plan_interval)
>> request->scan_plans[0].interval =
>> wiphy->max_sched_scan_plan_interval;
>>
>> return 0;
>> }
>>
>> So in v4.3 the interval was only validated to be non-zero. Now the
>> interval is validated and capped to wiphy->max_sched_scan_plan_interval
>> but apart from iwlmvm there are no driver specifying that so
>> max_sched_scan_plan_interval = 0 for those and interval is unsigned int
>> not equal to zero. So the last if statement above is true setting the
>> interval to zero. I think the if statement should be extended to assure
>> max_sched_scan_interval is non-zero, ie. set explicitly by the driver:
>>
>> if (wiphy->max_sched_scan_plan_interval &&
>> request->scan_plans[0].interval >
>> wiphy->max_sched_scan_plan_interval)
>> request->scan_plans[0].interval =
>> wiphy->max_sched_scan_plan_interval;
>
> If the driver doesn't set the max_sched_scan_plan_interval, mac80211's
> default of MAX_U32 will be used:
>
> struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
> const char *requested_name)
> {
> [...]
> rdev->wiphy.max_sched_scan_plans = 1;
> rdev->wiphy.max_sched_scan_plan_interval = U32_MAX;
>
> return &rdev->wiphy;
> }
> EXPORT_SYMBOL(wiphy_new_nm);
>
> ...so max_sched_scan_plan_interval will never be zero, unless the
> driver explicitly sets it to zero.
I think you are overlooking the cfg80211-based drivers here. According
to lxr at least brcmfmac, mwifiex, and ath6kl are not specifying it.
"Funny" detail is that scheduled scan support in mwifiex seems to be
introduced after the scan plan API change.
With the old API nl80211 only assured it to be non-zero. The only one
capping the value was iwlmvm. The only one setting the
max_sched_scan_plan_interval (overriding mac80211 value) now is iwlmvm.
So I think checking if it is set before capping it retains the old API
behaviour.
Regards,
Arend
Hi Arend,
On Mon, 2016-11-21 at 20:34 +0100, Arend Van Spriel wrote:
> On 21-11-2016 16:08, Luca Coelho wrote:
> > Hi Arend,
> >
> > On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
> > > On 21-11-2016 12:30, Arend Van Spriel wrote:
> > > > On 21-11-2016 12:19, Arend Van Spriel wrote:
> > > > > Hi Johannes, Luca,
> > > > >
> > > > > The gscan work made me look at scheduled scan and the implementation of
> > > > > it in brcmfmac. The driver ignored the interval parameter from
> > > > > user-space. Now I am fixing that. One thing is that our firmware has a
> > > > > minimum interval which can not be indicated in struct wiphy. The other
> > > > > issue is how the maximum interval is used in the nl80211.c.
> > > > >
> > > > > In nl80211_parse_sched_scan_plans() it is used against value passed in
> > > > > NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
> > > > > For the first one it caps the value to the maximum, but for the second
> > > > > one it returns -EINVAL. I suspect this is done because maximum interval
> > > > > was introduced with schedule scan plans, but it feels inconsistent.
> > > >
> > > > It also maybe simply wrong to cap. At least brcmfmac does not set the
> > > > maximum so it will always get interval being zero. Maybe better to do:
> > > >
> > > > if (wiphy->max_sched_scan_plan_interval &&
> > > > request->scan_plans[0].interval >
> > > > wiphy->max_sched_scan_plan_interval)
> > > > return -EINVAL;
> > > >
> > > > > Thoughts?
> > >
> > > Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
> > > struct sched_scan_request::interval was specified in milliseconds! Below
> > > the drivers that I see having scheduled scan support:
> > >
> > > iwlmvm: cap interval, convert to seconds.
> > > ath6kl: cap to 1sec minimum, no max check, convert to seconds.
> > > wl12xx: no checking in driver, fw need milliseconds.
> > > wl18xx: no checking in driver, fw need milliseconds.
> > >
> > > The milliseconds conversion seems to be taken care of by multiplying
> > > with MSEC_PER_SEC in wl{12,18}xx drivers.
> > >
> > > It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
> > > other drivers will get interval of zero which only ath6kl handles.
> >
> > With the introduction of scheduled scan plans, we sort of deprecated
> > the "generic" scheduled scan interval. It doesn't make sense to have
> > both passed at the same time, so nl80211 forbids
> > NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
> > NL80211_ATTR_SCHED_SCAN_PLANS.
>
> Indeed, but if no plans are passed it is still allowed.
That's right. But the driver will get it as a single plan.
> > The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
> > which is silly because we can never get millisecond accuracy in this.
> > Thus, in the plans API, we decided to use seconds instead (because it
> > makes much more sense). Additionally, the interval is considered
> > "advisory", because the FW may not be able guarantee the exact
> > intervals (for instance, the iwlwifi driver actually starts the
> > interval timer after scan completion, so if you specify 10 seconds
> > intervals, in practice they'll be 13-14 seconds).
>
> Agree. Our firmware wants to have it in seconds as well.
It was actually my mistake when I implemented it in my TI days (can't
hide from git blame ;)). TI's firmware used msecs and I just blindly
followed it.
> > I'm not sure I'm answering your question, because I'm also not sure I
> > understood the question. :)
>
> The question is this: Why is the interval capped at
> max_sched_scan_plan_interval if it exceeds it and no plans are provided
> (so continue to setup the scheduled scan request) whereas when plans are
> provided and an interval exceeds max_sched_scan_plan_interval it aborts
> with -EINVAL.
Oh, I see. The problem is that the "max_sched_scan_plan_interval" was
introduced later. If the userspace passes
NL80211__ATTR_SCHED_SCAN_INTERVAL, it probably means that it doesn't
know about NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL (i.e. it's only using an
old API). If it is also, for some reason, passing a very large number,
we shouldn't suddenly make it fail with -EINVALID, because that would
be a break of UABI. And since we know the driver cannot support such a
large number, we cap it because it's the best we can do.
Now, if the userspace uses NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL, it
means that it knows the new API (and was written after the new API was
introduced), so we can be stricter and assume it must have checked
NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL.
Makes sense?
> And a follow-up question for this snippet of code in
> nl80211_parse_sched_scan_plans():
>
> if (!attrs[NL80211_ATTR_SCHED_SCAN_PLANS]) {
> u32 interval;
>
> /*
> * If scan plans are not specified,
> * %NL80211_ATTR_SCHED_SCAN_INTERVAL must be specified. In this
> * case one scan plan will be set with the specified scan
> * interval and infinite number of iterations.
> */
> if (!attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL])
> return -EINVAL;
>
> interval = nla_get_u32(attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL]);
> if (!interval)
> return -EINVAL;
>
> request->scan_plans[0].interval =
> DIV_ROUND_UP(interval, MSEC_PER_SEC);
> if (!request->scan_plans[0].interval)
> return -EINVAL;
>
> if (request->scan_plans[0].interval >
> wiphy->max_sched_scan_plan_interval)
> request->scan_plans[0].interval =
> wiphy->max_sched_scan_plan_interval;
>
> return 0;
> }
>
> So in v4.3 the interval was only validated to be non-zero. Now the
> interval is validated and capped to wiphy->max_sched_scan_plan_interval
> but apart from iwlmvm there are no driver specifying that so
> max_sched_scan_plan_interval = 0 for those and interval is unsigned int
> not equal to zero. So the last if statement above is true setting the
> interval to zero. I think the if statement should be extended to assure
> max_sched_scan_interval is non-zero, ie. set explicitly by the driver:
>
> if (wiphy->max_sched_scan_plan_interval &&
> request->scan_plans[0].interval >
> wiphy->max_sched_scan_plan_interval)
> request->scan_plans[0].interval =
> wiphy->max_sched_scan_plan_interval;
If the driver doesn't set the max_sched_scan_plan_interval, mac80211's
default of MAX_U32 will be used:
struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
const char *requested_name)
{
[...]
rdev->wiphy.max_sched_scan_plans = 1;
rdev->wiphy.max_sched_scan_plan_interval = U32_MAX;
return &rdev->wiphy;
}
EXPORT_SYMBOL(wiphy_new_nm);
...so max_sched_scan_plan_interval will never be zero, unless the
driver explicitly sets it to zero.
--
Cheers,
Luca.
On 21-11-2016 12:30, Arend Van Spriel wrote:
> On 21-11-2016 12:19, Arend Van Spriel wrote:
>> Hi Johannes, Luca,
>>
>> The gscan work made me look at scheduled scan and the implementation of
>> it in brcmfmac. The driver ignored the interval parameter from
>> user-space. Now I am fixing that. One thing is that our firmware has a
>> minimum interval which can not be indicated in struct wiphy. The other
>> issue is how the maximum interval is used in the nl80211.c.
>>
>> In nl80211_parse_sched_scan_plans() it is used against value passed in
>> NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
>> For the first one it caps the value to the maximum, but for the second
>> one it returns -EINVAL. I suspect this is done because maximum interval
>> was introduced with schedule scan plans, but it feels inconsistent.
>
> It also maybe simply wrong to cap. At least brcmfmac does not set the
> maximum so it will always get interval being zero. Maybe better to do:
>
> if (wiphy->max_sched_scan_plan_interval &&
> request->scan_plans[0].interval >
> wiphy->max_sched_scan_plan_interval)
> return -EINVAL;
>
>> Thoughts?
Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
struct sched_scan_request::interval was specified in milliseconds! Below
the drivers that I see having scheduled scan support:
iwlmvm: cap interval, convert to seconds.
ath6kl: cap to 1sec minimum, no max check, convert to seconds.
wl12xx: no checking in driver, fw need milliseconds.
wl18xx: no checking in driver, fw need milliseconds.
The milliseconds conversion seems to be taken care of by multiplying
with MSEC_PER_SEC in wl{12,18}xx drivers.
It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
other drivers will get interval of zero which only ath6kl handles.
Regards,
Arend
On 21-11-2016 16:08, Luca Coelho wrote:
> Hi Arend,
>
> On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
>> On 21-11-2016 12:30, Arend Van Spriel wrote:
>>> On 21-11-2016 12:19, Arend Van Spriel wrote:
>>>> Hi Johannes, Luca,
>>>>
>>>> The gscan work made me look at scheduled scan and the implementation of
>>>> it in brcmfmac. The driver ignored the interval parameter from
>>>> user-space. Now I am fixing that. One thing is that our firmware has a
>>>> minimum interval which can not be indicated in struct wiphy. The other
>>>> issue is how the maximum interval is used in the nl80211.c.
>>>>
>>>> In nl80211_parse_sched_scan_plans() it is used against value passed in
>>>> NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
>>>> For the first one it caps the value to the maximum, but for the second
>>>> one it returns -EINVAL. I suspect this is done because maximum interval
>>>> was introduced with schedule scan plans, but it feels inconsistent.
>>>
>>> It also maybe simply wrong to cap. At least brcmfmac does not set the
>>> maximum so it will always get interval being zero. Maybe better to do:
>>>
>>> if (wiphy->max_sched_scan_plan_interval &&
>>> request->scan_plans[0].interval >
>>> wiphy->max_sched_scan_plan_interval)
>>> return -EINVAL;
>>>
>>>> Thoughts?
>>
>> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
>> struct sched_scan_request::interval was specified in milliseconds! Below
>> the drivers that I see having scheduled scan support:
>>
>> iwlmvm: cap interval, convert to seconds.
>> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
>> wl12xx: no checking in driver, fw need milliseconds.
>> wl18xx: no checking in driver, fw need milliseconds.
>>
>> The milliseconds conversion seems to be taken care of by multiplying
>> with MSEC_PER_SEC in wl{12,18}xx drivers.
>>
>> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
>> other drivers will get interval of zero which only ath6kl handles.
>
> With the introduction of scheduled scan plans, we sort of deprecated
> the "generic" scheduled scan interval. It doesn't make sense to have
> both passed at the same time, so nl80211 forbids
> NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
> NL80211_ATTR_SCHED_SCAN_PLANS.
Indeed, but if no plans are passed it is still allowed.
> The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
> which is silly because we can never get millisecond accuracy in this.
> Thus, in the plans API, we decided to use seconds instead (because it
> makes much more sense). Additionally, the interval is considered
> "advisory", because the FW may not be able guarantee the exact
> intervals (for instance, the iwlwifi driver actually starts the
> interval timer after scan completion, so if you specify 10 seconds
> intervals, in practice they'll be 13-14 seconds).
Agree. Our firmware wants to have it in seconds as well.
> I'm not sure I'm answering your question, because I'm also not sure I
> understood the question. :)
The question is this: Why is the interval capped at
max_sched_scan_plan_interval if it exceeds it and no plans are provided
(so continue to setup the scheduled scan request) whereas when plans are
provided and an interval exceeds max_sched_scan_plan_interval it aborts
with -EINVAL.
And a follow-up question for this snippet of code in
nl80211_parse_sched_scan_plans():
if (!attrs[NL80211_ATTR_SCHED_SCAN_PLANS]) {
u32 interval;
/*
* If scan plans are not specified,
* %NL80211_ATTR_SCHED_SCAN_INTERVAL must be specified. In this
* case one scan plan will be set with the specified scan
* interval and infinite number of iterations.
*/
if (!attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL])
return -EINVAL;
interval = nla_get_u32(attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL]);
if (!interval)
return -EINVAL;
request->scan_plans[0].interval =
DIV_ROUND_UP(interval, MSEC_PER_SEC);
if (!request->scan_plans[0].interval)
return -EINVAL;
if (request->scan_plans[0].interval >
wiphy->max_sched_scan_plan_interval)
request->scan_plans[0].interval =
wiphy->max_sched_scan_plan_interval;
return 0;
}
So in v4.3 the interval was only validated to be non-zero. Now the
interval is validated and capped to wiphy->max_sched_scan_plan_interval
but apart from iwlmvm there are no driver specifying that so
max_sched_scan_plan_interval = 0 for those and interval is unsigned int
not equal to zero. So the last if statement above is true setting the
interval to zero. I think the if statement should be extended to assure
max_sched_scan_interval is non-zero, ie. set explicitly by the driver:
if (wiphy->max_sched_scan_plan_interval &&
request->scan_plans[0].interval >
wiphy->max_sched_scan_plan_interval)
request->scan_plans[0].interval =
wiphy->max_sched_scan_plan_interval;
Regards,
Arend
On 21-11-2016 17:59, Dave Taht wrote:
> On Mon, Nov 21, 2016 at 7:08 AM, Luca Coelho <[email protected]> wrote:
>> Hi Arend,
>>
>> On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
>>> On 21-11-2016 12:30, Arend Van Spriel wrote:
>>>> On 21-11-2016 12:19, Arend Van Spriel wrote:
>>>>> Hi Johannes, Luca,
>>>>>
>>>>> The gscan work made me look at scheduled scan and the implementation of
>>>>> it in brcmfmac. The driver ignored the interval parameter from
>>>>> user-space. Now I am fixing that. One thing is that our firmware has a
>>>>> minimum interval which can not be indicated in struct wiphy. The other
>>>>> issue is how the maximum interval is used in the nl80211.c.
>>>>>
>>>>> In nl80211_parse_sched_scan_plans() it is used against value passed in
>>>>> NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
>>>>> For the first one it caps the value to the maximum, but for the second
>>>>> one it returns -EINVAL. I suspect this is done because maximum interval
>>>>> was introduced with schedule scan plans, but it feels inconsistent.
>>>>
>>>> It also maybe simply wrong to cap. At least brcmfmac does not set the
>>>> maximum so it will always get interval being zero. Maybe better to do:
>>>>
>>>> if (wiphy->max_sched_scan_plan_interval &&
>>>> request->scan_plans[0].interval >
>>>> wiphy->max_sched_scan_plan_interval)
>>>> return -EINVAL;
>>>>
>>>>> Thoughts?
>>>
>>> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
>>> struct sched_scan_request::interval was specified in milliseconds! Below
>>> the drivers that I see having scheduled scan support:
>>>
>>> iwlmvm: cap interval, convert to seconds.
>>> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
>>> wl12xx: no checking in driver, fw need milliseconds.
>>> wl18xx: no checking in driver, fw need milliseconds.
>>>
>>> The milliseconds conversion seems to be taken care of by multiplying
>>> with MSEC_PER_SEC in wl{12,18}xx drivers.
>>>
>>> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
>>> other drivers will get interval of zero which only ath6kl handles.
>>
>> With the introduction of scheduled scan plans, we sort of deprecated
>> the "generic" scheduled scan interval. It doesn't make sense to have
>> both passed at the same time, so nl80211 forbids
>> NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
>> NL80211_ATTR_SCHED_SCAN_PLANS.
>>
>> The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
>> which is silly because we can never get millisecond accuracy in this.
>> Thus, in the plans API, we decided to use seconds instead (because it
>> makes much more sense). Additionally, the interval is considered
>> "advisory", because the FW may not be able guarantee the exact
>> intervals (for instance, the iwlwifi driver actually starts the
>> interval timer after scan completion, so if you specify 10 seconds
>> intervals, in practice they'll be 13-14 seconds).
>>
>> I'm not sure I'm answering your question, because I'm also not sure I
>> understood the question. :)
>
> I'm not sure if I understand the discussion and hooks myself, but
> recently fixes for doing channel scans saner from userspace ended up
> here, after some discussion.
scheduled scan is a scan-offload feature in which user-space requests
the firmware on the device to perform a period scan at a requested
interval. As such it is not different from NM or wpa_supplicant
performing a scan, but the host will only get informed about found SSIDs
which user-space has configured, eg. the networks that are configured in
wpa_supplicant. How much time the scan takes is determined by the
channels in the scheduled scan request. Worst case that means all
supported 2G and 5G channels. So it is just about offloading. There is
not necessarily reduced impact.
Regards,
Arend
> https://bugzilla.gnome.org/show_bug.cgi?id=766482
>
> Anything that can reduce the impact of this behavior, I'm for!
>
> http://www.taht.net/~d/channel_scans_destroying_latency_under_load_for_10s.png
>
>
>
>>
>> --
>> Cheers,
>> Luca.
>
>
>
On 22-11-2016 10:19, Luca Coelho wrote:
> On Tue, 2016-11-22 at 10:10 +0100, Arend Van Spriel wrote:
>> On 22-11-2016 6:59, Luca Coelho wrote:
>>> Oh, I see. The problem is that the "max_sched_scan_plan_interval" was
>>> introduced later. If the userspace passes
>>> NL80211__ATTR_SCHED_SCAN_INTERVAL, it probably means that it doesn't
>>> know about NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL (i.e. it's only using an
>>> old API). If it is also, for some reason, passing a very large number,
>>> we shouldn't suddenly make it fail with -EINVALID, because that would
>>> be a break of UABI. And since we know the driver cannot support such a
>>> large number, we cap it because it's the best we can do.
>>>
>>> Now, if the userspace uses NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL, it
>>> means that it knows the new API (and was written after the new API was
>>> introduced), so we can be stricter and assume it must have checked
>>> NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL.
>>>
>>> Makes sense?
>>
>> Not really. As you say if user-space passes
>> NL80211_ATTR_SCHED_SCAN_INTERVAL it is using old API. Otherwise it
>> should pass NL80211_ATTR_SCHED_SCAN_PLANS.
>
> Errr... I meant "if the userspace uses NL80211_ATTR_SCHED_SCAN_PLANS",
> pasted the wrong thing. ;)
>
>
>> If the driver doesn't set the max_sched_scan_plan_interval, mac80211's
>>> default of MAX_U32 will be used:
>>>
>>> struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
>>> const char *requested_name)
>>> {
>>> [...]
>>> rdev->wiphy.max_sched_scan_plans = 1;
>>> rdev->wiphy.max_sched_scan_plan_interval = U32_MAX;
>>>
>>> return &rdev->wiphy;
>>> }
>>> EXPORT_SYMBOL(wiphy_new_nm);
>>>
>>> ...so max_sched_scan_plan_interval will never be zero, unless the
>>> driver explicitly sets it to zero.
>>
>> I think you are overlooking the cfg80211-based drivers here. According
>> to lxr at least brcmfmac, mwifiex, and ath6kl are not specifying it.
>> "Funny" detail is that scheduled scan support in mwifiex seems to be
>> introduced after the scan plan API change.
>
> You're right, I did overlook non-mac80211 drivers.
>
>
>> With the old API nl80211 only assured it to be non-zero. The only one
>> capping the value was iwlmvm. The only one setting the
>> max_sched_scan_plan_interval (overriding mac80211 value) now is iwlmvm.
>> So I think checking if it is set before capping it retains the old API
>> behaviour.
>
> Fair enough. The change you propose, to also check whether the value
> is set (since 0 interval doesn't make sense), is valid. :)
Thanks. Patch coming up.
Gr. AvS
On Mon, Nov 21, 2016 at 7:08 AM, Luca Coelho <[email protected]> wrote:
> Hi Arend,
>
> On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
>> On 21-11-2016 12:30, Arend Van Spriel wrote:
>> > On 21-11-2016 12:19, Arend Van Spriel wrote:
>> > > Hi Johannes, Luca,
>> > >
>> > > The gscan work made me look at scheduled scan and the implementation=
of
>> > > it in brcmfmac. The driver ignored the interval parameter from
>> > > user-space. Now I am fixing that. One thing is that our firmware has=
a
>> > > minimum interval which can not be indicated in struct wiphy. The oth=
er
>> > > issue is how the maximum interval is used in the nl80211.c.
>> > >
>> > > In nl80211_parse_sched_scan_plans() it is used against value passed =
in
>> > > NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVA=
L.
>> > > For the first one it caps the value to the maximum, but for the seco=
nd
>> > > one it returns -EINVAL. I suspect this is done because maximum inter=
val
>> > > was introduced with schedule scan plans, but it feels inconsistent.
>> >
>> > It also maybe simply wrong to cap. At least brcmfmac does not set the
>> > maximum so it will always get interval being zero. Maybe better to do:
>> >
>> > if (wiphy->max_sched_scan_plan_interval &&
>> > request->scan_plans[0].interval >
>> > wiphy->max_sched_scan_plan_interval)
>> > return -EINVAL;
>> >
>> > > Thoughts?
>>
>> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
>> struct sched_scan_request::interval was specified in milliseconds! Below
>> the drivers that I see having scheduled scan support:
>>
>> iwlmvm: cap interval, convert to seconds.
>> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
>> wl12xx: no checking in driver, fw need milliseconds.
>> wl18xx: no checking in driver, fw need milliseconds.
>>
>> The milliseconds conversion seems to be taken care of by multiplying
>> with MSEC_PER_SEC in wl{12,18}xx drivers.
>>
>> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
>> other drivers will get interval of zero which only ath6kl handles.
>
> With the introduction of scheduled scan plans, we sort of deprecated
> the "generic" scheduled scan interval. It doesn't make sense to have
> both passed at the same time, so nl80211 forbids
> NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
> NL80211_ATTR_SCHED_SCAN_PLANS.
>
> The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
> which is silly because we can never get millisecond accuracy in this.
> Thus, in the plans API, we decided to use seconds instead (because it
> makes much more sense). Additionally, the interval is considered
> "advisory", because the FW may not be able guarantee the exact
> intervals (for instance, the iwlwifi driver actually starts the
> interval timer after scan completion, so if you specify 10 seconds
> intervals, in practice they'll be 13-14 seconds).
>
> I'm not sure I'm answering your question, because I'm also not sure I
> understood the question. :)
I'm not sure if I understand the discussion and hooks myself, but
recently fixes for doing channel scans saner from userspace ended up
here, after some discussion.
https://bugzilla.gnome.org/show_bug.cgi?id=3D766482
Anything that can reduce the impact of this behavior, I'm for!
http://www.taht.net/~d/channel_scans_destroying_latency_under_load_for_10s.=
png
>
> --
> Cheers,
> Luca.
--=20
Dave T=C3=A4ht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org
