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 NL80211_BSS_SELECT_ATTR_RSSI_ADJUST that can actually give a specific RSSI adjustment value (in dB?). > 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. Hmm.. So you did notice it is a flag attribute.. So how would this match NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI which provides the threshold value for how many dB better the new BSS needs to be for it to be reported? > 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_*? 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? > 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. The main concern I have for optional features with sched_scan is in whether the device ends up being woken up constantly if the driver does not understand a constraint that user space is trying to use to avoid being notified all the time. -- Jouni Malinen PGP id EFC895FA
