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
