Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Johannes Berg]

> Also I don't see the array issue. @relative_rssi_5g_pref with s8
> value seems same as @rssi_adjust with (band=5g, s8 value) packed
> together. Or am I missing something here.

Jouni is arguing that if you can specify which band to adjust, you
might want to adjust more than one band - and need an array of
(band,adjustment) rather than just a single such tuple.

But if we introduce this attribute with the tuple now, we can extend it
to an array of tuples later if we really need that.

johannes

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Johannes Berg]

On Tue, 2016-12-20 at 20:52 +, Malinen, Jouni wrote:

> > I think between that (unless we add that, we could technically
> > extend flag attributes to allow them being an int as well, or add a
> > new one) and the fact that the device may not support different
> > relative RSSI matches in different matchsets, I'm almost convinced
> > that adding new attributes is better.
> 
> I'm not completely sure how to interpret all this and also the last
> email from Arend in this thread. Could either (or both :) of you
> providemore detailed suggestion on what exactly you would like us to 
> change, if anything, in the attribute design now so that we can try
> to close on this?

I guess I'm thinking out loud too much :-)

Moving to the (band,adjustment) struct thing, like we have it in the
BSS select now, would be good I think - instead of the single value
that has the band hardcoded in the name. It's fairly useless with just
two bands since you can adjust one or the other to achieve the same
result, but for consistency it'd be good.

And then doing a match attribute we more or less discarded here, so
conceptually nothing else really changes.

johannes

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Arend Van Spriel]

On 20-12-2016 21:52, Malinen, Jouni wrote:
> On Fri, Dec 16, 2016 at 10:56:51AM +0100, Johannes Berg wrote:
>> On Thu, 2016-12-15 at 11:06 +, Malinen, Jouni wrote:
>>> Maybe the nl80211.h description was not clear enough, but the
>>> comments in cfg80211.h should be quite clear on how this was designed
>>> to work at the implementation level:
>>>
>>> "If the current connected BSS is in the 2.4 GHz band, other BSSs in
>>> the 2.4 GHz band to be reported should have better RSSI by
>>> @relative_rssi and other BSSs in the 5 GHz band to be reported should
>>> have better RSSI by (@relative_rssi - @relative_rssi_5g_pref).
>>> If the current connected BSS is in the 5 GHz band, other BSSs in the
>>> 2.4 GHz band to be reported should have better RSSI by
>>> (@relative_rssi + @relative_rssi_5g_pref) and other BSSs in the 5 GHz
>>> band to be reported should have better RSSI by by @relative_rssi."
>>
>> Oh, right. Should probably be in nl80211.h too, to set expectations
>> from userspace.
> 
> Sure, we can update that in the next revision.
> 
>>> At minimum, we would need to clearly document struct
>>> nl80211_bss_select_rssi_adjust, but even if we do, I'm not sure it
>>> really is ideal mechanism to move to now that I realized it is not an
>>> array, but a single band,delta pair.
>>
>> We can move to an array easily in the future by extending the attribute
>> length and advertising the number of array entries that are supported,
>> if that's your biggest concern? I don't see it as being very useful
>> right now since I don't think we'll see offloaded roaming between 2.4/5
>> and 60 GHz anytime soon. This may change when we add more bands later,
>> I suppose.
> 
> Hmm.. So do you want us to move to using this packed struct in the new
> attribute instead of using a signed 8-bit integer as the variable value?

That was my suggestion so it is more clear what user-space wants by
making it specify the band explicitly. So in the explanation above
reference to "5g" should be "specified band" etc. Whether you reuse the
packed struct nl80211_bss_select_rssi_adjust or come up with a new
(identical?) one is irrelevant to me.

Also I don't see the array issue. @relative_rssi_5g_pref with s8 value
seems same as @rssi_adjust with (band=5g, s8 value) packed together. Or
am I missing something here.

>>> If we are talking only about roaming within an ESS (a single SSID),
>>> that would sound clear, but which relative RSSI rules would apply if
>>> there are match sets for both the currently connected SSID and
>>> another SSID that the candidate BSS is for?
>>
>> Right, I see how this might become a problem. I generally see no issue
>> with supporting multiple matchsets with different SSIDs but all having
>> the "relative to connected BSS RSSI filter" (which would disregard the
>> SSID specified in the matchset), but this then would become a problem
>> when multiple matchsets are specified with *different* such RSSI
>> filters, e.g. one matchset would specify that you want a 5G preference
>> of 10dB, but the other would specify a preference of only 5dB.
>>
>> Conceptually in this approach, that would be supported, but firmware
>> likely would not be able to express this, I suppose?
> 
> That's certainly not at the level we were planning on implementing.. :)

Right. So having "relative rssi" matchset attribute is off the table as
far as I am concerned.

>>> I think I'm missing something here.. Where would the threshold value
>>> (how much better new BSS needs to be) be stored? And do we really
>>> want something like the combination of
>>> NL80211_BSS_SELECT_ATTR_BAND_PREF and
>>> NL80211_BSS_SELECT_ATTR_RSSI_ADJUST which seems to be two different
>>> ways of doing band preference (the former without specifying delta
>>> and the latter with specific delta)? Or am I still not understanding
>>> how NL80211_BSS_SELECT_ATTR_* really works?

It is documented here:

/**
 * enum nl80211_bss_select_attr - attributes for bss selection.
 *
[...]
 *
 * One and only one of these attributes are found within
%NL80211_ATTR_BSS_SELECT
 * for %NL80211_CMD_CONNECT. It specifies the required BSS selection
behaviour
 * which the driver shall use.
 */

It is checked in nl80211.c [1]

>> No, you're right, I missed the "better by" threshold.
>>
>> I think between that (unless we add that, we could technically extend
>> flag attributes to allow them being an int as well, or add a new one)
>> and the fact that the device may not support different relative RSSI
>> matches in different matchsets, I'm almost convinced that adding new
>> attributes is better.
> 
> I'm not completely sure how to interpret all this and also the last
> email from Arend in this thread. Could either (or both :) of you provide
> more detailed suggestion on what exactly you would like us to change, if
> anything, in the attribute design now so that we can try to close on
> this?

To summarize: 1) stick with the new attributes on request level (so not
matchset level), 2) use packed 

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Malinen, Jouni]

On Fri, Dec 16, 2016 at 10:56:51AM +0100, Johannes Berg wrote:
> On Thu, 2016-12-15 at 11:06 +, Malinen, Jouni wrote:
> > Maybe the nl80211.h description was not clear enough, but the
> > comments in cfg80211.h should be quite clear on how this was designed
> > to work at the implementation level:
> > 
> > "If the current connected BSS is in the 2.4 GHz band, other BSSs in
> > the 2.4 GHz band to be reported should have better RSSI by
> > @relative_rssi and other BSSs in the 5 GHz band to be reported should
> > have better RSSI by (@relative_rssi - @relative_rssi_5g_pref).
> > If the current connected BSS is in the 5 GHz band, other BSSs in the
> > 2.4 GHz band to be reported should have better RSSI by
> > (@relative_rssi + @relative_rssi_5g_pref) and other BSSs in the 5 GHz
> > band to be reported should have better RSSI by by @relative_rssi."
> 
> Oh, right. Should probably be in nl80211.h too, to set expectations
> from userspace.

Sure, we can update that in the next revision.

> > At minimum, we would need to clearly document struct
> > nl80211_bss_select_rssi_adjust, but even if we do, I'm not sure it
> > really is ideal mechanism to move to now that I realized it is not an
> > array, but a single band,delta pair.
> 
> We can move to an array easily in the future by extending the attribute
> length and advertising the number of array entries that are supported,
> if that's your biggest concern? I don't see it as being very useful
> right now since I don't think we'll see offloaded roaming between 2.4/5
> and 60 GHz anytime soon. This may change when we add more bands later,
> I suppose.

Hmm.. So do you want us to move to using this packed struct in the new
attribute instead of using a signed 8-bit integer as the variable value?

> > If we are talking only about roaming within an ESS (a single SSID),
> > that would sound clear, but which relative RSSI rules would apply if
> > there are match sets for both the currently connected SSID and
> > another SSID that the candidate BSS is for?
> 
> Right, I see how this might become a problem. I generally see no issue
> with supporting multiple matchsets with different SSIDs but all having
> the "relative to connected BSS RSSI filter" (which would disregard the
> SSID specified in the matchset), but this then would become a problem
> when multiple matchsets are specified with *different* such RSSI
> filters, e.g. one matchset would specify that you want a 5G preference
> of 10dB, but the other would specify a preference of only 5dB.
> 
> Conceptually in this approach, that would be supported, but firmware
> likely would not be able to express this, I suppose?

That's certainly not at the level we were planning on implementing.. :)

> > I think I'm missing something here.. Where would the threshold value
> > (how much better new BSS needs to be) be stored? And do we really
> > want something like the combination of
> > NL80211_BSS_SELECT_ATTR_BAND_PREF and
> > NL80211_BSS_SELECT_ATTR_RSSI_ADJUST which seems to be two different
> > ways of doing band preference (the former without specifying delta
> > and the latter with specific delta)? Or am I still not understanding
> > how NL80211_BSS_SELECT_ATTR_* really works?
> 
> No, you're right, I missed the "better by" threshold.
> 
> I think between that (unless we add that, we could technically extend
> flag attributes to allow them being an int as well, or add a new one)
> and the fact that the device may not support different relative RSSI
> matches in different matchsets, I'm almost convinced that adding new
> attributes is better.

I'm not completely sure how to interpret all this and also the last
email from Arend in this thread. Could either (or both :) of you provide
more detailed suggestion on what exactly you would like us to change, if
anything, in the attribute design now so that we can try to close on
this?

-- 
Jouni MalinenPGP id EFC895FA

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Johannes Berg]

On Thu, 2016-12-15 at 11:06 +, Malinen, Jouni wrote:
> On Tue, Dec 13, 2016 at 04:56:55PM +0100, Johannes Berg wrote:
> > Regarding reusing attributes, we have (for the BSS selection thing)
> > the attribute NL80211_BSS_SELECT_ATTR_RSSI_ADJUST, which is really
> > quite similar to your
> > new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF since while
> > connected (which BSS_SELECT_ATTR_* assumes) the current BSS is
> > always part of the considered BSSes, I'd think.
> 
> It seems to have somewhat similar meaning, but it looks more generic
> by not being tied to just two bands (2.4 and 5). And now that I
> actually looked again at this, it does not look like
> NL80211_BSS_SELECT_ATTR_RSSI_ADJUST actually allows more than a
> single band,delta pair to be provided and as such, it actually would
> not work very well with more than two bands even if it might be a bit
> more generic by allowing band to be set to something else than 2.4 or
> 5. 

Agree, it wouldn't work well with more than 2 bands. Not that we have
them now (60GHz doesn't really count here).

> By the way, nl80211.h does not seem to document what values struct
> nl82011_bss_select_rssi_adjust band uses..  Is this enum
> nl80211_band?

I believe so, we should fix that.

> Neither does it say that delta is in dB (is it?).

We should also fix that, but let's assume so for the sake of this
discussion.

> > OTOH, the new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF doesn't
> > make this quite clear - is the current BSS to be adjusted before
> > comparing, if it's 5 GHz? If so, the semantics are equivalent. If
> > not, it doesn't actually make much sense ;-)
> 
> Maybe the nl80211.h description was not clear enough, but the
> comments in cfg80211.h should be quite clear on how this was designed
> to work at the implementation level:
> 
> "If the current connected BSS is in the 2.4 GHz band, other BSSs in
> the 2.4 GHz band to be reported should have better RSSI by
> @relative_rssi and other BSSs in the 5 GHz band to be reported should
> have better RSSI by (@relative_rssi - @relative_rssi_5g_pref).
> If the current connected BSS is in the 5 GHz band, other BSSs in the
> 2.4 GHz band to be reported should have better RSSI by
> (@relative_rssi + @relative_rssi_5g_pref) and other BSSs in the 5 GHz
> band to be reported should have better RSSI by by @relative_rssi."

Oh, right. Should probably be in nl80211.h too, to set expectations
from userspace.

> I'm not sure I'd describe this as adjusting the current BSS RSSI, but
> more like adjusting the RSSI threshold value if roaming would be from
> one band to another and doing that adjustment by adding or
> decrementing based on whether the roam would be from 2.4 to 5 or from
> 5 to 2.4.

It's functionally equivalent to doing the following adjustment

compare_rssi = current_bss_rssi
if current_bss_is_5ghz:
compare_rssi += relative_rssi_5g_pref

and then comparing all values (again adjusted for 5 GHz BSSes) to this.

(which is what I meant by "adjusting the current BSS RSSI").

> > So assuming that it is in fact taken into account after the same
> > adjustment, the two attributes are equivalent, and then perhaps it
> > would make sense to use struct nl80211_bss_select_rssi_adjust for
> > the new attribute. If a driver doesn't support arbitrary bands, but
> > just 5 GHz as in your example, it can just flip it around to 2.4
> > GHz by switching the sign.
> > 
> > Perhaps we should even consider doing that in cfg80211 and
> > adjusting the internal API for both that way?
> 
> I'm not completely sure I understood. One thing to note about
> differences here is that NL80211_BSS_SELECT_ATTR_* seems to be
> defining some preferences for BSS selection based on RSSI with an
> additional band preference, but it does not seem to define the
> threshold by how many dB the new candidate BSS should be better.

It's defining more than what we're talking about here, absolutely.
Clearly somebody thought a pure band preference might be useful (to
never connect to a 2.4GHz AP if a 5 GHz AP is available, regardless of
their relative RSSI), but that's not what we're looking at here.
(That could probably also be achieved by setting the adjustment to
90dB, but whatever)

> At minimum, we would need to clearly document struct
> nl80211_bss_select_rssi_adjust, but even if we do, I'm not sure it
> really is ideal mechanism to move to now that I realized it is not an
> array, but a single band,delta pair.

We can move to an array easily in the future by extending the attribute
length and advertising the number of array entries that are supported,
if that's your biggest concern? I don't see it as being very useful
right now since I don't think we'll see offloaded roaming between 2.4/5
and 60 GHz anytime soon. This may change when we add more bands later,
I suppose.

> Arend:
> > > I am not saying it should be avoided. Just looking at it
> > > conceptually
> > > the scheduled scan request holds so-called matchsets that specify
> > 

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Malinen, Jouni]

On Tue, Dec 13, 2016 at 04:56:55PM +0100, Johannes Berg wrote:
> Regarding reusing attributes, we have (for the BSS selection thing) the
> attribute NL80211_BSS_SELECT_ATTR_RSSI_ADJUST, which is really quite
> similar to your new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF since
> while connected (which BSS_SELECT_ATTR_* assumes) the current BSS is
> always part of the considered BSSes, I'd think.

It seems to have somewhat similar meaning, but it looks more generic by
not being tied to just two bands (2.4 and 5). And now that I actually
looked again at this, it does not look like
NL80211_BSS_SELECT_ATTR_RSSI_ADJUST actually allows more than a single
band,delta pair to be provided and as such, it actually would not work
very well with more than two bands even if it might be a bit more
generic by allowing band to be set to something else than 2.4 or 5. By
the way, nl80211.h does not seem to document what values struct
nl82011_bss_select_rssi_adjust band uses..  Is this enum nl80211_band?
Neither does it say that delta is in dB (is it?).

> OTOH, the new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF doesn't
> make this quite clear - is the current BSS to be adjusted before
> comparing, if it's 5 GHz? If so, the semantics are equivalent. If not,
> it doesn't actually make much sense ;-)

Maybe the nl80211.h description was not clear enough, but the comments
in cfg80211.h should be quite clear on how this was designed to work at
the implementation level:

"If the current connected BSS is in the 2.4 GHz band, other BSSs in the
2.4 GHz band to be reported should have better RSSI by @relative_rssi
and other BSSs in the 5 GHz band to be reported should have better RSSI
by (@relative_rssi - @relative_rssi_5g_pref).
If the current connected BSS is in the 5 GHz band, other BSSs in the
2.4 GHz band to be reported should have better RSSI by
(@relative_rssi + @relative_rssi_5g_pref) and other BSSs in the 5 GHz
band to be reported should have better RSSI by by @relative_rssi."

I'm not sure I'd describe this as adjusting the current BSS RSSI, but
more like adjusting the RSSI threshold value if roaming would be from
one band to another and doing that adjustment by adding or decrementing
based on whether the roam would be from 2.4 to 5 or from 5 to 2.4.

> So assuming that it is in fact taken into account after the same
> adjustment, the two attributes are equivalent, and then perhaps it
> would make sense to use struct nl80211_bss_select_rssi_adjust for the
> new attribute. If a driver doesn't support arbitrary bands, but just 5
> GHz as in your example, it can just flip it around to 2.4 GHz by
> switching the sign.
> 
> Perhaps we should even consider doing that in cfg80211 and adjusting
> the internal API for both that way?

I'm not completely sure I understood. One thing to note about
differences here is that NL80211_BSS_SELECT_ATTR_* seems to be defining
some preferences for BSS selection based on RSSI with an additional band
preference, but it does not seem to define the threshold by how many dB
the new candidate BSS should be better.

At minimum, we would need to clearly document struct
nl80211_bss_select_rssi_adjust, but even if we do, I'm not sure it
really is ideal mechanism to move to now that I realized it is not an
array, but a single band,delta pair.

Arend:
> > I am not saying it should be avoided. Just looking at it conceptually
> > the scheduled scan request holds so-called matchsets that specify the
> > constraints to determine whether a BSS was found that is worth
> > notifying the host/user-space about. As such I would expect the
> > relative RSSI attribute(s) to be part of the matchset. That way you
> > can specify it together with the currently connected SSID in a single
> > matchset.
> 
> I think this makes a lot of sense.

If we are talking only about roaming within an ESS (a single SSID), that
would sound clear, but which relative RSSI rules would apply if there
are match sets for both the currently connected SSID and another SSID
that the candidate BSS is for?

> We already have NL80211_SCHED_SCAN_MATCH_ATTR_RSSI, which asks to be
> reporting only networks that have an *absolute* RSSI value above the
> value of the attribute - a new attribute to make it relative to the
> current network instead would make sense.

When you say "current network", do you mean the current BSS? This gets
complex when thinking about multiple SSIDs (which some call "networks")
and there being NL80211_SCHED_SCAN_MATCH_ATTR_SSID and multiple
match sets..

> That would indeed be equivalent to NL80211_BSS_SELECT_ATTR_RSSI then.

NL80211_BSS_SELECT_ATTR_RSSI is a flag attribute.. BSS select mechanism
does not provide an absolute RSSI value or threshold; it just seems to
indicate use of RSSI-based selection mechanism without defining what
exactly is better. There is NL80211_BSS_SELECT_ATTR_BAND_PREF that gives
a preference to a specific band (without defining what that preference
is) and then the 

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Johannes Berg]

Ok... this is getting complicated :)

Regarding reusing attributes, we have (for the BSS selection thing) the
attribute NL80211_BSS_SELECT_ATTR_RSSI_ADJUST, which is really quite
similar to your new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF since
while connected (which BSS_SELECT_ATTR_* assumes) the current BSS is
always part of the considered BSSes, I'd think.

However, I tend to think now that reusing the attribute is perhaps not
the right thing to do - but defining them with the same semantics would
still make sense.

Assuming that the value defined in NL80211_BSS_SELECT_ATTR_RSSI_ADJUST
applies also to the *current* BSS, it's actually quite pointless to
define there the band to adjust - if you want to adjust 2.4 GHz
positively you might as well adjust 5 GHz negatively, and vice versa,
and both ways are supported.

OTOH, the new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF doesn't
make this quite clear - is the current BSS to be adjusted before
comparing, if it's 5 GHz? If so, the semantics are equivalent. If not,
it doesn't actually make much sense ;-)
So assuming that it is in fact taken into account after the same
adjustment, the two attributes are equivalent, and then perhaps it
would make sense to use struct nl80211_bss_select_rssi_adjust for the
new attribute. If a driver doesn't support arbitrary bands, but just 5
GHz as in your example, it can just flip it around to 2.4 GHz by
switching the sign.

Perhaps we should even consider doing that in cfg80211 and adjusting
the internal API for both that way?

> I am not saying it should be avoided. Just looking at it conceptually
> the scheduled scan request holds so-called matchsets that specify the
> constraints to determine whether a BSS was found that is worth
> notifying the host/user-space about. As such I would expect the
> relative RSSI attribute(s) to be part of the matchset. That way you
> can specify it together with the currently connected SSID in a single
> matchset.

I think this makes a lot of sense.

We already have NL80211_SCHED_SCAN_MATCH_ATTR_RSSI, which asks to be
reporting only networks that have an *absolute* RSSI value above the
value of the attribute - a new attribute to make it relative to the
current network instead would make sense.

That would indeed be equivalent to NL80211_BSS_SELECT_ATTR_RSSI then.

Now, if we consider this, NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI
actually is equivalent to NL80211_BSS_SELECT_ATTR_RSSI (a flag
attribute indicating whether or not RSSI-based selection/matching is
done) and NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF is equivalent
to NL80211_BSS_SELECT_ATTR_RSSI_ADJUST, both need to be given with the
flag and affect operation.

However, NL80211_BSS_SELECT_ATTR_BAND_PREF doesn't exist, and reusing
the BSS_SELECT namespace also doesn't make sense.


So, how about we move these into NL80211_SCHED_SCAN_MATCH_ATTR_* as
suggested by Arend, and define them with the same content as  the
corresponding NL80211_BSS_SELECT_ATTR_*?

If they're part of match attributes, we might even remove the feature
flag entirely - those were always defined to be optional, but it very
well be worthwhile for userspace to know if they're supported if it
wants to behave differently depending on whether they're supported or
not, I'll leave that up to you since presumably you know the userspace
implementation that you're planning to create.

johannes

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Arend Van Spriel]

On 8-12-2016 18:52, Malinen, Jouni wrote:
> On Wed, Dec 07, 2016 at 09:03:23PM +0100, Arend Van Spriel wrote:
>> On 7-12-2016 10:33, Vamsi, Krishna wrote:
 -Original Message-
 From: Johannes Berg [mailto:johan...@sipsolutions.net]
>>>
 What about Arend's comment regarding this functionality overlapping with 
 the
 BSS selection offload configuration?

 Do you think there's any ability to share attributes/functionality?
>>>
>>> Hi Johannes,
>>>
>>> I think the later comment from Luca on this which I pasted below is more 
>>> agreeable.
>>>
>>> Yes, similar but not quite the same.  The existing case is for finding BSSs 
>>> that are worth waking the host up for (while disconnected), so it needs to 
>>> be better than the RSSI passed (absolute number).  Now this is about 
>>> relative RSSI as compared to the current connection, so RELATIVE_RSSI is 
>>> different than RSSI and I think the same attribute should not be used, to 
>>> avoid confusion.
>>
>> I noticed the response from Luca, but did not get back on this. I know
>> it is not the same, but what I meant is whether we could extend it so it
>> also covers your scenario.
> 
> I'm not sure I see the point of trying to avoid the new RELATIVE_RSSI
> attribute. We need to clearly specify that this new constraint is indeed
> for relative comparison against the currently connected BSS.

Hi Jouni,

I am not saying it should be avoided. Just looking at it conceptually
the scheduled scan request holds so-called matchsets that specify the
constraints to determine whether a BSS was found that is worth notifying
the host/user-space about. As such I would expect the relative RSSI
attribute(s) to be part of the matchset. That way you can specify it
together with the currently connected SSID in a single matchset.

> As far as your second email is concerned, it might make more sense to
> use the existing NL80211_ATTR_BSS_SELECT instead of the new
> NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF since they cover very
> similar purpose in defining RSSI preference between bands. We can take a
> look at doing so. One thing to be a careful about this is in what claims
> there are about using NL80211_ATTR_BSS_SELECT for capability indication
> in GET_WIPHY. I guess we can leave that as-is to apply for _CONNECT and
> the new EXT_FEATURE flag we are adding for sched_scan applies for this
> attribute in SCHED_SCAN.

The NL80211_ATTR_BSS_SELECT supports different methods for BSS
selection. One of them being RSSI_ADJUST which is similar to this. It
lets user-space specify the band and adjustment instead of having a band
implied in the attribute name. Agree that if reusing it for scheduled
scan you would still need the EXT_FEATURE flag.

Regards,
Arend

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Malinen, Jouni]

On Wed, Dec 07, 2016 at 09:03:23PM +0100, Arend Van Spriel wrote:
> On 7-12-2016 10:33, Vamsi, Krishna wrote:
> >> -Original Message-
> >> From: Johannes Berg [mailto:johan...@sipsolutions.net]
> > 
> >> What about Arend's comment regarding this functionality overlapping with 
> >> the
> >> BSS selection offload configuration?
> >>
> >> Do you think there's any ability to share attributes/functionality?
> > 
> > Hi Johannes,
> > 
> > I think the later comment from Luca on this which I pasted below is more 
> > agreeable.
> > 
> > Yes, similar but not quite the same.  The existing case is for finding BSSs 
> > that are worth waking the host up for (while disconnected), so it needs to 
> > be better than the RSSI passed (absolute number).  Now this is about 
> > relative RSSI as compared to the current connection, so RELATIVE_RSSI is 
> > different than RSSI and I think the same attribute should not be used, to 
> > avoid confusion.
> 
> I noticed the response from Luca, but did not get back on this. I know
> it is not the same, but what I meant is whether we could extend it so it
> also covers your scenario.

I'm not sure I see the point of trying to avoid the new RELATIVE_RSSI
attribute. We need to clearly specify that this new constraint is indeed
for relative comparison against the currently connected BSS.

As far as your second email is concerned, it might make more sense to
use the existing NL80211_ATTR_BSS_SELECT instead of the new
NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF since they cover very
similar purpose in defining RSSI preference between bands. We can take a
look at doing so. One thing to be a careful about this is in what claims
there are about using NL80211_ATTR_BSS_SELECT for capability indication
in GET_WIPHY. I guess we can leave that as-is to apply for _CONNECT and
the new EXT_FEATURE flag we are adding for sched_scan applies for this
attribute in SCHED_SCAN.

-- 
Jouni MalinenPGP id EFC895FA

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Arend Van Spriel]

On 2-12-2016 22:59, Jouni Malinen wrote:
> From: vamsi krishna 
> 
> Enhance sched scan to support option of finding a better BSS while in
> connected state. Firmware scans the medium and reports when it finds a
> known BSS which has better RSSI than the current connected BSS. New
> attributes to specify the relative RSSI (compared to the current BSS)
> are added to the sched scan to implement this.
> 
> Signed-off-by: vamsi krishna 
> Signed-off-by: Jouni Malinen 
> ---
>  include/net/cfg80211.h   | 19 +++
>  include/uapi/linux/nl80211.h | 18 ++
>  net/wireless/nl80211.c   | 29 +++--
>  3 files changed, 64 insertions(+), 2 deletions(-)
> 
> v2: address comments from Luca, Arend, and Johannes
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index ef42749..dcdd0c4 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1626,6 +1626,22 @@ struct cfg80211_sched_scan_plan {
>   *   cycle.  The driver may ignore this parameter and start
>   *   immediately (or at any other time), if this feature is not
>   *   supported.
> + * @relative_rssi: Relative RSSI threshold in dB to restrict scan result
> + *   reporting in connected state to cases where a matching BSS is determined
> + *   to have better RSSI than the current connected BSS. The relative RSSI
> + *   threshold values are ignored in disconnected state.
> + * @relative_rssi_5g_pref: The amount of RSSI preference in dB that is given 
> to
> + *   a 5 GHz BSS over 2.4 GHz BSS while looking for better BSSs in connected
> + *   state. A negative value can be passed if 2.4 GHz band should be given
> + *   priority to 5 GHz band.
> + *   If the current connected BSS is in the 2.4 GHz band, other BSSs in the
> + *   2.4 GHz band to be reported should have better RSSI by @relative_rssi
> + *   and other BSSs in the 5 GHz band to be reported should have better RSSI
> + *   by (@relative_rssi - @relative_rssi_5g_pref).
> + *   If the current connected BSS is in the 5 GHz band, other BSSs in the
> + *   2.4 GHz band to be reported should have better RSSI by
> + *   (@relative_rssi + @relative_rssi_5g_pref) and other BSSs in the 5 GHz
> + *   band to be reported should have better RSSI by by @relative_rssi.

The choice of these attributes makes the implicit assumption that the
BSS-es in 5G are preferred. The relative_rssi_5g_pref is actually more a
bonus or penalty is negative value is used. I guess for speed junkies
that want their 11ac card maxed out that is true, but if you need to
cross a couple of concrete floors you might want to stick with 2.4G.

I introduced a similar attribute to be provided in the
NL80211_CMD_CONNECT (see [1]).

Regards,
Arend

[1] http://lxr.free-electrons.com/source/include/uapi/linux/nl80211.h#L1815

>   */
>  struct cfg80211_sched_scan_request {
>   struct cfg80211_ssid *ssids;
> @@ -1645,6 +1661,9 @@ struct cfg80211_sched_scan_request {
>   u8 mac_addr[ETH_ALEN] __aligned(2);
>   u8 mac_addr_mask[ETH_ALEN] __aligned(2);
>  
> + s8 relative_rssi;
> + s8 relative_rssi_5g_pref;
> +
>   /* internal */
>   struct wiphy *wiphy;
>   struct net_device *dev;
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 6b76e3b..fc29bdb 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -1980,6 +1980,17 @@ enum nl80211_commands {
>   * @NL80211_ATTR_BSSID: The BSSID of the AP. Note that %NL80211_ATTR_MAC is 
> also
>   *   used in various commands/events for specifying the BSSID.
>   *
> + * @NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI: Relative RSSI threshold by which
> + *   other BSSs has to be better than the current connected BSS so that they
> + *   get reported to user space. This will give an opportunity to userspace
> + *   to consider connecting to other matching BSSs which have better RSSI
> + *   than the current connected BSS by using an offloaded operation to avoid
> + *   unnecessary wakeups.
> + *
> + * @NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF: The amount of RSSI 
> preference
> + *   to be given to 5 GHz APs over 2.4 GHz APs while searching for better
> + *   BSSs than the current connected BSS.
> + *
>   * @NUM_NL80211_ATTR: total number of nl80211_attrs available
>   * @NL80211_ATTR_MAX: highest attribute number currently defined
>   * @__NL80211_ATTR_AFTER_LAST: internal use
> @@ -2386,6 +2397,9 @@ enum nl80211_attrs {
>  
>   NL80211_ATTR_BSSID,
>  
> + NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI,
> + NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF,
> +
>   /* add attributes here, update the policy in nl80211.c */
>  
>   __NL80211_ATTR_AFTER_LAST,
> @@ -4697,6 +4711,9 @@ enum nl80211_feature_flags {
>   *   configuration (AP/mesh) with VHT rates.
>   * @NL80211_EXT_FEATURE_FILS_STA: This driver supports Fast Initial Link 
> Setup
>   *   with user 

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Arend Van Spriel]

On 7-12-2016 10:33, Vamsi, Krishna wrote:
>> -Original Message-
>> From: Johannes Berg [mailto:johan...@sipsolutions.net]
> 
>> What about Arend's comment regarding this functionality overlapping with the
>> BSS selection offload configuration?
>>
>> Do you think there's any ability to share attributes/functionality?
> 
> Hi Johannes,
> 
> I think the later comment from Luca on this which I pasted below is more 
> agreeable.
> 
> Yes, similar but not quite the same.  The existing case is for finding BSSs 
> that are worth waking the host up for (while disconnected), so it needs to be 
> better than the RSSI passed (absolute number).  Now this is about relative 
> RSSI as compared to the current connection, so RELATIVE_RSSI is different 
> than RSSI and I think the same attribute should not be used, to avoid 
> confusion.

I noticed the response from Luca, but did not get back on this. I know
it is not the same, but what I meant is whether we could extend it so it
also covers your scenario.

Regards,
Arend

RE: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Vamsi, Krishna]

> -Original Message-
> From: Johannes Berg [mailto:johan...@sipsolutions.net]

> What about Arend's comment regarding this functionality overlapping with the
> BSS selection offload configuration?
> 
> Do you think there's any ability to share attributes/functionality?

Hi Johannes,

I think the later comment from Luca on this which I pasted below is more 
agreeable.

Yes, similar but not quite the same.  The existing case is for finding BSSs 
that are worth waking the host up for (while disconnected), so it needs to be 
better than the RSSI passed (absolute number).  Now this is about relative RSSI 
as compared to the current connection, so RELATIVE_RSSI is different than RSSI 
and I think the same attribute should not be used, to avoid confusion.

Thanks,
Vamsi

Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Johannes Berg]

> v2: address comments from Luca, Arend, and Johannes

What about Arend's comment regarding this functionality overlapping
with the BSS selection offload configuration?

Do you think there's any ability to share attributes/functionality?

johannes

[PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs

[Jouni Malinen]

From: vamsi krishna 

Enhance sched scan to support option of finding a better BSS while in
connected state. Firmware scans the medium and reports when it finds a
known BSS which has better RSSI than the current connected BSS. New
attributes to specify the relative RSSI (compared to the current BSS)
are added to the sched scan to implement this.

Signed-off-by: vamsi krishna 
Signed-off-by: Jouni Malinen 
---
 include/net/cfg80211.h   | 19 +++
 include/uapi/linux/nl80211.h | 18 ++
 net/wireless/nl80211.c   | 29 +++--
 3 files changed, 64 insertions(+), 2 deletions(-)

v2: address comments from Luca, Arend, and Johannes

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ef42749..dcdd0c4 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1626,6 +1626,22 @@ struct cfg80211_sched_scan_plan {
  * cycle.  The driver may ignore this parameter and start
  * immediately (or at any other time), if this feature is not
  * supported.
+ * @relative_rssi: Relative RSSI threshold in dB to restrict scan result
+ * reporting in connected state to cases where a matching BSS is determined
+ * to have better RSSI than the current connected BSS. The relative RSSI
+ * threshold values are ignored in disconnected state.
+ * @relative_rssi_5g_pref: The amount of RSSI preference in dB that is given to
+ * a 5 GHz BSS over 2.4 GHz BSS while looking for better BSSs in connected
+ * state. A negative value can be passed if 2.4 GHz band should be given
+ * priority to 5 GHz band.
+ * If the current connected BSS is in the 2.4 GHz band, other BSSs in the
+ * 2.4 GHz band to be reported should have better RSSI by @relative_rssi
+ * and other BSSs in the 5 GHz band to be reported should have better RSSI
+ * by (@relative_rssi - @relative_rssi_5g_pref).
+ * If the current connected BSS is in the 5 GHz band, other BSSs in the
+ * 2.4 GHz band to be reported should have better RSSI by
+ * (@relative_rssi + @relative_rssi_5g_pref) and other BSSs in the 5 GHz
+ * band to be reported should have better RSSI by by @relative_rssi.
  */
 struct cfg80211_sched_scan_request {
struct cfg80211_ssid *ssids;
@@ -1645,6 +1661,9 @@ struct cfg80211_sched_scan_request {
u8 mac_addr[ETH_ALEN] __aligned(2);
u8 mac_addr_mask[ETH_ALEN] __aligned(2);
 
+   s8 relative_rssi;
+   s8 relative_rssi_5g_pref;
+
/* internal */
struct wiphy *wiphy;
struct net_device *dev;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 6b76e3b..fc29bdb 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1980,6 +1980,17 @@ enum nl80211_commands {
  * @NL80211_ATTR_BSSID: The BSSID of the AP. Note that %NL80211_ATTR_MAC is 
also
  * used in various commands/events for specifying the BSSID.
  *
+ * @NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI: Relative RSSI threshold by which
+ * other BSSs has to be better than the current connected BSS so that they
+ * get reported to user space. This will give an opportunity to userspace
+ * to consider connecting to other matching BSSs which have better RSSI
+ * than the current connected BSS by using an offloaded operation to avoid
+ * unnecessary wakeups.
+ *
+ * @NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF: The amount of RSSI 
preference
+ * to be given to 5 GHz APs over 2.4 GHz APs while searching for better
+ * BSSs than the current connected BSS.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2386,6 +2397,9 @@ enum nl80211_attrs {
 
NL80211_ATTR_BSSID,
 
+   NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI,
+   NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF,
+
/* add attributes here, update the policy in nl80211.c */
 
__NL80211_ATTR_AFTER_LAST,
@@ -4697,6 +4711,9 @@ enum nl80211_feature_flags {
  * configuration (AP/mesh) with VHT rates.
  * @NL80211_EXT_FEATURE_FILS_STA: This driver supports Fast Initial Link Setup
  * with user space SME (NL80211_CMD_AUTHENTICATE) in station mode.
+ * @NL80211_EXT_FEATURE_SCHED_SCAN_RELATIVE_RSSI: The driver supports 
sched_scan
+ * for reporting BSSs with better RSSI than the current connected BSS
+ * (%NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI).
  *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -4712,6 +4729,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_BEACON_RATE_HT,
NL80211_EXT_FEATURE_BEACON_RATE_VHT,
NL80211_EXT_FEATURE_FILS_STA,
+   NL80211_EXT_FEATURE_SCHED_SCAN_RELATIVE_RSSI,
 
/* add new features before the definition below */