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

[Malinen, Jouni]

On Fri, Nov 25, 2016 at 11:21:15AM +0200, Luca Coelho wrote:
> On Thu, 2016-11-24 at 00:07 +0200, Jouni Malinen wrote:
> > From: vamsi krishna 
> > @@ -9670,6 +9689,15 @@ static int nl80211_send_wowlan_nd(struct sk_buff 
> > *msg,
> > if (nla_put_u32(msg, NL80211_ATTR_SCHED_SCAN_DELAY, req->delay))
> > return -ENOBUFS;
> >  
> > +   if (wiphy_ext_feature_isset(
> > +   wiphy, NL80211_EXT_FEATURE_SCHED_SCAN_BETTER_BSS) &&
> > +   (nla_put_u32(msg, NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI,
> > +req->relative_rssi) ||
> > +nla_put_u32(msg,
> > +NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF,
> > +req->relative_rssi_5g_pref)))
> > +   return -ENOBUFS;
> > +
> 
> Why did you add this to nl80211_send_wowlan_nd() function?

Rest of the feedback will be addressed in PATCH v2, but I'm not sure
what to do with this part. I don't think we have any use case for this,
i.e., the addition here for NL80211_CMD_GET_WOWLAN response attributes
is based on how other sched_scan attributes were already included in the
response. Any new attribute atted to nl80211_parse_sched_scan() (like
these two new attributes) get parsed into
rdev->wiphy.wowlan_config->nd_config in nl80211_wowlan_nd(), so they can
end up being set here when using NL80211_CMD_SET_WOWLAN.
 
-- 
Jouni MalinenPGP id EFC895FA

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

[Luca Coelho]

On Mon, 2016-11-28 at 15:20 +0100, Johannes Berg wrote:
> > It seems there has already
> > been taken a shot at this which may be used/extended [1].
> > 
> 
> That's a good point - it's somewhat similar.
> 
> This is obviously a different context - offloaded BSS selection vs.
> scheduled scan (for host BSS selection), but perhaps the attribute &
> definitions could be reused?

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.

--
Luca.

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

[Johannes Berg]

> It seems there has already
> been taken a shot at this which may be used/extended [1].
> 

That's a good point - it's somewhat similar.

This is obviously a different context - offloaded BSS selection vs.
scheduled scan (for host BSS selection), but perhaps the attribute &
definitions could be reused?

johannes

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

[Johannes Berg]

On Thu, 2016-11-24 at 00:07 +0200, Jouni Malinen wrote:
> 
> + int bbr;

bbr? Also, bool?
 
johannes

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

[Arend Van Spriel]

On 23-11-2016 23:07, 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 a significantly better RSSI than the current
> connected BSS.

I suppose the intent here is to be notified about roaming candidates and
hence the "connected state" prerequisite. It seems there has already
been taken a shot at this which may be used/extended [1].

Regards,
Arend

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

> Signed-off-by: vamsi krishna 
> Signed-off-by: Jouni Malinen 
> ---
>  include/net/cfg80211.h   | 17 +
>  include/uapi/linux/nl80211.h | 17 +
>  net/wireless/nl80211.c   | 32 ++--
>  3 files changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index ef42749..c62c42a 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1626,6 +1626,20 @@ 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 to restrict scan result reporting 
> in
> + *   connected state to cases where a matching BSS is determined to have a
> + *   significantly better RSSI than the current connected BSS.
> + * @relative_rssi_5g_pref: The amount of RSSI preference that is given to a
> + *   5 GHz BSS over 2.4 GHz BSS while looking for better BSSs in connected
> + *   state.
> + *   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 +1659,9 @@ struct cfg80211_sched_scan_request {
>   u8 mac_addr[ETH_ALEN] __aligned(2);
>   u8 mac_addr_mask[ETH_ALEN] __aligned(2);
>  
> + int relative_rssi;
> + int relative_rssi_5g_pref;

I suppose s8 would be sufficient here, right?

>   /* internal */
>   struct wiphy *wiphy;
>   struct net_device *dev;
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 984a35ac..34b16a4 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -1981,6 +1981,16 @@ enum nl80211_commands {
>   *   %NL80211_ATTR_MAC has also been 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.
> + *
> + * @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
> @@ -2387,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,
> @@ -4698,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_BETTER_BSS: 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.
> @@ -4713,6 +4729,7 @@ enum nl80211_ext_feature_index {
>   NL80211_EXT_FEATURE_BEACON_RATE_HT,
>   NL80211_EXT_FEATURE_BEACON_RATE_VHT,
>   

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

[Luca Coelho]

IMHO "report better BSSs" is vague in this context.  It would better to
use something more concrete like "add relative RSSI attribute to sched
scan".


On Thu, 2016-11-24 at 00:07 +0200, 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 a significantly better RSSI than the current

"Significantly" is dependent on the value you use. :)

> connected BSS.
> 
> Signed-off-by: vamsi krishna 
> Signed-off-by: Jouni Malinen 
> ---
>  include/net/cfg80211.h   | 17 +
>  include/uapi/linux/nl80211.h | 17 +
>  net/wireless/nl80211.c   | 32 ++--
>  3 files changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index ef42749..c62c42a 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1626,6 +1626,20 @@ 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 to restrict scan result
reporting in
> + *   connected state to cases where a matching BSS is
determined to have a
> + *   significantly better RSSI than the current connected BSS.

What happens with this attribute if we're not connected? Also, I think
you should specify the attribute type (int?) and the unit here.


> + * @relative_rssi_5g_pref: The amount of RSSI preference that is given to a
> + *   5 GHz BSS over 2.4 GHz BSS while looking for better BSSs in connected
> + *   state.
> + *   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.

Could there be cases where you want the opposite? Meaning,
relative_rssi_5g_pref being negative?

>   */
>  struct cfg80211_sched_scan_request {
>   struct cfg80211_ssid *ssids;
> @@ -1645,6 +1659,9 @@ struct cfg80211_sched_scan_request {
>   u8 mac_addr[ETH_ALEN] __aligned(2);
>   u8 mac_addr_mask[ETH_ALEN] __aligned(2);
>  
> + int relative_rssi;
> + int relative_rssi_5g_pref;

I'm not sure it was your intention, but for flexibility, it's good that
both these attributes are ints so that you can find relative values on
both sides (greater than and smaller than).

It doesn't apply to the "better BSS" case, but maybe people find a
different use for it in the future?


> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 984a35ac..34b16a4 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -1981,6 +1981,16 @@ enum nl80211_commands {
>   *   %NL80211_ATTR_MAC has also been 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.

The userspace can always do this, but the advantage here is that it
doesn't need to be woken up and check the results itself, since it
would only receive results that matter.


> + *
> + * @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
> @@ -2387,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,
> @@ -4698,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_BETTER_BSS: The driver supports sched_scan

[PATCH 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 a significantly better RSSI than the current
connected BSS.

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

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ef42749..c62c42a 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1626,6 +1626,20 @@ 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 to restrict scan result reporting in
+ * connected state to cases where a matching BSS is determined to have a
+ * significantly better RSSI than the current connected BSS.
+ * @relative_rssi_5g_pref: The amount of RSSI preference that is given to a
+ * 5 GHz BSS over 2.4 GHz BSS while looking for better BSSs in connected
+ * state.
+ * 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 +1659,9 @@ struct cfg80211_sched_scan_request {
u8 mac_addr[ETH_ALEN] __aligned(2);
u8 mac_addr_mask[ETH_ALEN] __aligned(2);
 
+   int relative_rssi;
+   int 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 984a35ac..34b16a4 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1981,6 +1981,16 @@ enum nl80211_commands {
  * %NL80211_ATTR_MAC has also been 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.
+ *
+ * @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
@@ -2387,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,
@@ -4698,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_BETTER_BSS: 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.
@@ -4713,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_BETTER_BSS,
 
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 8db5cb1..af018a5 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -405,6 +405,8 @@ enum nl80211_multicast_groups {
[NL80211_ATTR_FILS_NONCES] = { .len = 2 * FILS_NONCE_LEN },
[NL80211_ATTR_MULTICAST_TO_UNICAST_ENABLED] = { .type = NLA_FLAG, },
[NL80211_ATTR_BSSID] = { .len = ETH_ALEN },
+