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

[Arend Van Spriel <arend.vanspriel@broadcom.com>]

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 +0000, 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 struct for @relative_rssi_5g_pref.

Regards,
Arend

[1] http://lxr.free-electrons.com/source/net/wireless/nl80211.c#L6382