[RFC 0/2] nl80211: allow configuration of BSS selection

[RFC 0/2] nl80211: allow configuration of BSS selection

[Arend van Spriel]

This patch series adds support for configuration of BSS selection done by
the driver and/or firmware. It allows user-space to pass selection criteria
with the NL80211_CMD_CONNECT using NL80211_ATTR_BSS_SELECT. When user-space
does provide this attribute and no explicit BSSID is provided it will pass
the BSS selection criteria to the driver if the driver supports it.

When multiple BSS-es are found belonging to the same ESS, ie. Infra-BSS with
same SSID, the criteria can be used to select a BSS. The criteria defined are:

1. select BSS in specific band regardless RSSI level.
2. adjust RSSI level for BSS-es in specified band.

Open issue:
-----------
* NL80211_CMD_CONNECT support ATTR_MAC and ATTR_MAC_HINT. When ATTR_MAC is
  used BSS selection criteria are ignored. What about ATTR_MAC_HINT?

Arend van Spriel (2):
  nl80211: add extended feature for BSS selection support
  brcmfmac: add support for nl80211 BSS_SELECT feature

 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 47 ++++++++++++++++++++++
 drivers/net/wireless/brcm80211/brcmfmac/common.c   | 38 ++++++++++-------
 drivers/net/wireless/brcm80211/brcmfmac/core.h     |  1 +
 include/net/cfg80211.h                             | 17 ++++++++
 include/uapi/linux/nl80211.h                       | 35 ++++++++++++++++
 net/wireless/nl80211.c                             | 46 +++++++++++++++++++++
 6 files changed, 169 insertions(+), 15 deletions(-)

-- 
1.9.1

[RFC 1/2] nl80211: add extended feature for BSS selection support

[Arend van Spriel]

Introducing a new extended feature that the driver can support which
indicate the driver/firmware supports configuration of BSS selection
criteria upon CONNECT command.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Lei Zhang <leizh@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 include/net/cfg80211.h       | 17 ++++++++++++++++
 include/uapi/linux/nl80211.h | 35 +++++++++++++++++++++++++++++++++
 net/wireless/nl80211.c       | 46 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9bcaaf7..4001947 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1854,6 +1854,22 @@ struct cfg80211_ibss_params {
 };
 
 /**
+ * struct cfg80211_bss_selection - Connection parameters for BSS selection.
+ *
+ * @present: indicates whether parameters are set.
+ * @pref_band: preferred band.
+ * @rssi_adjust: adjustment for RSSI level of the preferred band.
+ * @ignore_rssi: indicates whether BSS in preferred band is to be selected
+ *	regardless its RSSI level.
+ */
+struct cfg80211_bss_selection {
+	bool present;
+	enum nl80211_band pref_band;
+	u8 rssi_adjust;
+	bool ignore_rssi;
+};
+
+/**
  * struct cfg80211_connect_params - Connection parameters
  *
  * This structure provides information needed to complete IEEE 802.11
@@ -1910,6 +1926,7 @@ struct cfg80211_connect_params {
 	struct ieee80211_ht_cap ht_capa_mask;
 	struct ieee80211_vht_cap vht_capa;
 	struct ieee80211_vht_cap vht_capa_mask;
+	struct cfg80211_bss_selection bss_select;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 5b7b5eb..6753468 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1789,6 +1789,9 @@ enum nl80211_commands {
  *	thus it must not specify the number of iterations, only the interval
  *	between scans. The scan plans are executed sequentially.
  *	Each scan plan is a nested attribute of &enum nl80211_sched_scan_plan.
+ * @NL80211_ATTR_BSS_SELECT: nested attribute used with %NL80211_CMD_CONNECT
+ *	containing criteria for BSS selection to be done by driver and/or
+ *	firmware.
  *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
@@ -2164,6 +2167,8 @@ enum nl80211_attrs {
 	NL80211_ATTR_MAX_SCAN_PLAN_ITERATIONS,
 	NL80211_ATTR_SCHED_SCAN_PLANS,
 
+	NL80211_ATTR_BSS_SELECT,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -4396,12 +4401,15 @@ enum nl80211_feature_flags {
 /**
  * enum nl80211_ext_feature_index - bit index of extended features.
  * @NL80211_EXT_FEATURE_VHT_IBSS: This driver supports IBSS with VHT datarates.
+ * @NL80211_EXT_FEATURE_BSS_SELECT: This driver supports BSS selection criteria
+ *	to be given upon %NL80211_CMD_CONNECT.
  *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
 enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_VHT_IBSS,
+	NL80211_EXT_FEATURE_BSS_SELECT,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
@@ -4651,4 +4659,31 @@ enum nl80211_sched_scan_plan {
 		__NL80211_SCHED_SCAN_PLAN_AFTER_LAST - 1
 };
 
+/**
+ * enum nl80211_attr_bss_select - attributes for bss selection.
+ *
+ * @__NL80211_ATTR_BSS_SELECT_INVALID: reserved.
+ * @NL80211_ATTR_BSS_SELECT_BAND_PREF: Required attribute indicating the
+ *	preferred band. The preference by itself still allows RSSI based
+ *	selection of BSS and as such is only a tie breaker. Value according
+ *	%enum nl80211_band.
+ * @NL80211_ATTR_BSS_SELECT_RSSI_ADJUST: When present the RSSI level for
+ *	the BSS in the preferred band is adjusted accordingly (u8).
+ * @NL80211_ATTR_BSS_SELECT_IGNORE_RSSI: flag attribute which can be used to
+ *	to have the BSS in the preferred band being selected regardless
+ *	of its RSSI level.
+ * @NL80211_ATTR_BSS_SELECT_MAX: highest bss select attribute number.
+ *@__NL80211_ATTR_BSS_SELECT_AFTER_LAST: internal use.
+ */
+enum nl80211_attr_bss_select {
+	__NL80211_ATTR_BSS_SELECT_INVALID,
+	NL80211_ATTR_BSS_SELECT_BAND_PREF,
+	NL80211_ATTR_BSS_SELECT_RSSI_ADJUST,
+	NL80211_ATTR_BSS_SELECT_IGNORE_RSSI,
+
+	/* keep last */
+	__NL80211_ATTR_BSS_SELECT_AFTER_LAST,
+	NL80211_ATTR_BSS_SELECT_MAX = __NL80211_ATTR_BSS_SELECT_AFTER_LAST - 1
+};
+
 #endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 72de698..850f27e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -485,6 +485,13 @@ nl80211_plan_policy[NL80211_SCHED_SCAN_PLAN_MAX + 1] = {
 	[NL80211_SCHED_SCAN_PLAN_ITERATIONS] = { .type = NLA_U32 },
 };
 
+static const struct nla_policy
+nl80211_bss_select_policy[NL80211_ATTR_BSS_SELECT_MAX + 1] = {
+	[NL80211_ATTR_BSS_SELECT_BAND_PREF] = { .type = NLA_U32 },
+	[NL80211_ATTR_BSS_SELECT_RSSI_ADJUST] = { .type = NLA_U8 },
+	[NL80211_ATTR_BSS_SELECT_IGNORE_RSSI] = { .type = NLA_FLAG },
+};
+
 static int nl80211_prepare_wdev_dump(struct sk_buff *skb,
 				     struct netlink_callback *cb,
 				     struct cfg80211_registered_device **rdev,
@@ -5753,6 +5760,33 @@ static int validate_scan_freqs(struct nlattr *freqs)
 	return n_channels;
 }
 
+static int parse_bss_select(struct nlattr *nla,
+			    struct cfg80211_bss_selection *bss_select)
+{
+	struct nlattr *attr[NL80211_ATTR_BSS_SELECT_MAX + 1];
+	int err;
+
+	if (!nla)
+		return 0;
+
+	err = nla_parse(attr, NL80211_ATTR_BSS_SELECT_MAX,
+			nla_data(nla), nla_len(nla), nl80211_bss_select_policy);
+	if (err)
+		return err;
+
+	bss_select->present = true;
+	if (!attr[NL80211_ATTR_BSS_SELECT_BAND_PREF])
+		return -EINVAL;
+
+	bss_select->pref_band =
+		nla_get_u32(attr[NL80211_ATTR_BSS_SELECT_BAND_PREF]);
+	bss_select->rssi_adjust =
+		nla_get_u8(attr[NL80211_ATTR_BSS_SELECT_RSSI_ADJUST]);
+	bss_select->ignore_rssi = !!attr[NL80211_ATTR_BSS_SELECT_IGNORE_RSSI];
+
+	return 0;
+}
+
 static int nl80211_parse_random_mac(struct nlattr **attrs,
 				    u8 *mac_addr, u8 *mac_addr_mask)
 {
@@ -7978,6 +8012,18 @@ static int nl80211_connect(struct sk_buff *skb, struct genl_info *info)
 		connect.flags |= ASSOC_REQ_USE_RRM;
 	}
 
+	/* only do bss selection when no BSSID is specified. */
+	if (!connect.bssid &&
+	    wiphy_ext_feature_isset(&rdev->wiphy,
+				    NL80211_EXT_FEATURE_BSS_SELECT)) {
+		err = parse_bss_select(info->attrs[NL80211_ATTR_BSS_SELECT],
+				       &connect.bss_select);
+		if (err) {
+			kzfree(connkeys);
+			return err;
+		}
+	}
+
 	wdev_lock(dev->ieee80211_ptr);
 	err = cfg80211_connect(rdev, dev, &connect, connkeys, NULL);
 	wdev_unlock(dev->ieee80211_ptr);
-- 
1.9.1

[RFC 2/2] brcmfmac: add support for nl80211 BSS_SELECT feature

[Arend van Spriel]

Announce support for extended nl80211 feature BSS_SELECT and
process BSS selection criteria provided in .connect() callback.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Lei Zhang <leizh@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 47 ++++++++++++++++++++++
 drivers/net/wireless/brcm80211/brcmfmac/common.c   | 38 ++++++++++-------
 drivers/net/wireless/brcm80211/brcmfmac/core.h     |  1 +
 3 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index deb5f78..9e99900 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -1722,6 +1722,50 @@ enum nl80211_auth_type brcmf_war_auth_type(struct brcmf_if *ifp,
 	return type;
 }
 
+static void brcmf_set_join_pref(struct brcmf_if *ifp,
+				struct cfg80211_bss_selection *bss_select)
+{
+	struct brcmf_join_pref_params join_pref_params[2];
+	int err;
+
+	if (!bss_select->present) {
+		brcmf_c_set_joinpref_default(ifp);
+		return;
+	}
+
+	switch (bss_select->pref_band) {
+	case IEEE80211_BAND_2GHZ:
+		join_pref_params[0].band = WLC_BAND_2G;
+		break;
+	case IEEE80211_BAND_5GHZ:
+		join_pref_params[0].band = WLC_BAND_5G;
+		break;
+	default:
+		brcmf_err("unsupported band: %d, ignoring bss selection\n",
+			  bss_select->pref_band);
+		return;
+	}
+	if (bss_select->ignore_rssi) {
+		/* Setup join_pref to select 5GHz over 2.4Ghz */
+		join_pref_params[0].type = BRCMF_JOIN_PREF_BAND;
+		join_pref_params[0].len = 2;
+		join_pref_params[0].rssi_gain = 0;
+	} else {
+		/* Setup join_pref to select target by RSSI (boost on 5GHz) */
+		join_pref_params[0].type = BRCMF_JOIN_PREF_RSSI_DELTA;
+		join_pref_params[0].len = 2;
+		join_pref_params[0].rssi_gain = bss_select->rssi_adjust;
+	}
+	join_pref_params[1].type = BRCMF_JOIN_PREF_RSSI;
+	join_pref_params[1].len = 2;
+	join_pref_params[1].rssi_gain = 0;
+	join_pref_params[1].band = 0;
+	err = brcmf_fil_iovar_data_set(ifp, "join_pref", join_pref_params,
+				       sizeof(join_pref_params));
+	if (err)
+		brcmf_err("Set join_pref error (%d)\n", err);
+}
+
 static s32
 brcmf_cfg80211_connect(struct wiphy *wiphy, struct net_device *ndev,
 		       struct cfg80211_connect_params *sme)
@@ -1858,6 +1902,8 @@ brcmf_cfg80211_connect(struct wiphy *wiphy, struct net_device *ndev,
 	else
 		eth_broadcast_addr(ext_join_params->assoc_le.bssid);
 
+	brcmf_set_join_pref(ifp, &sme->bss_select);
+
 	if (cfg->channel) {
 		ext_join_params->assoc_le.chanspec_num = cpu_to_le32(1);
 
@@ -5920,6 +5966,7 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
 	wiphy->signal_type = CFG80211_SIGNAL_TYPE_MBM;
 	wiphy->cipher_suites = __wl_cipher_suites;
 	wiphy->n_cipher_suites = ARRAY_SIZE(__wl_cipher_suites);
+	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_BSS_SELECT);
 	wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
 			WIPHY_FLAG_OFFCHAN_TX |
 			WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/common.c b/drivers/net/wireless/brcm80211/brcmfmac/common.c
index fe54844..7c40979 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/common.c
@@ -33,14 +33,34 @@ const u8 ALLFFMAC[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 #define BRCMF_DEFAULT_SCAN_CHANNEL_TIME	40
 #define BRCMF_DEFAULT_SCAN_UNASSOC_TIME	40
 
-/* boost value for RSSI_DELTA in preferred join selection */
+/* default boost value for RSSI_DELTA in preferred join selection */
 #define BRCMF_JOIN_PREF_RSSI_BOOST	8
 
+void brcmf_c_set_joinpref_default(struct brcmf_if *ifp)
+{
+	struct brcmf_join_pref_params join_pref_params[2];
+	int err;
+
+	/* Setup join_pref to select target by RSSI (boost on 5GHz) */
+	join_pref_params[0].type = BRCMF_JOIN_PREF_RSSI_DELTA;
+	join_pref_params[0].len = 2;
+	join_pref_params[0].rssi_gain = BRCMF_JOIN_PREF_RSSI_BOOST;
+	join_pref_params[0].band = WLC_BAND_5G;
+
+	join_pref_params[1].type = BRCMF_JOIN_PREF_RSSI;
+	join_pref_params[1].len = 2;
+	join_pref_params[1].rssi_gain = 0;
+	join_pref_params[1].band = 0;
+	err = brcmf_fil_iovar_data_set(ifp, "join_pref", join_pref_params,
+				       sizeof(join_pref_params));
+	if (err)
+		brcmf_err("Set join_pref error (%d)\n", err);
+}
+
 int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
 {
 	s8 eventmask[BRCMF_EVENTING_MASK_LEN];
 	u8 buf[BRCMF_DCMD_SMLEN];
-	struct brcmf_join_pref_params join_pref_params[2];
 	struct brcmf_rev_info_le revinfo;
 	struct brcmf_rev_info *ri;
 	char *ptr;
@@ -127,19 +147,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
 		goto done;
 	}
 
-	/* Setup join_pref to select target by RSSI(with boost on 5GHz) */
-	join_pref_params[0].type = BRCMF_JOIN_PREF_RSSI_DELTA;
-	join_pref_params[0].len = 2;
-	join_pref_params[0].rssi_gain = BRCMF_JOIN_PREF_RSSI_BOOST;
-	join_pref_params[0].band = WLC_BAND_5G;
-	join_pref_params[1].type = BRCMF_JOIN_PREF_RSSI;
-	join_pref_params[1].len = 2;
-	join_pref_params[1].rssi_gain = 0;
-	join_pref_params[1].band = 0;
-	err = brcmf_fil_iovar_data_set(ifp, "join_pref", join_pref_params,
-				       sizeof(join_pref_params));
-	if (err)
-		brcmf_err("Set join_pref error (%d)\n", err);
+	brcmf_c_set_joinpref_default(ifp);
 
 	/* Setup event_msgs, enable E_IF */
 	err = brcmf_fil_iovar_data_get(ifp, "event_msgs", eventmask,
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/core.h b/drivers/net/wireless/brcm80211/brcmfmac/core.h
index 2f9101b..f8b4925 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/core.h
@@ -217,5 +217,6 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp,
 void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success);
 void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb);
 void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on);
+void brcmf_c_set_joinpref_default(struct brcmf_if *ifp);
 
 #endif /* BRCMFMAC_CORE_H */
-- 
1.9.1

Re: [RFC 0/2] nl80211: allow configuration of BSS selection

[Arend van Spriel]

On 24-12-15 13:19, Arend van Spriel wrote:
> This patch series adds support for configuration of BSS selection done by
> the driver and/or firmware. It allows user-space to pass selection criteria
> with the NL80211_CMD_CONNECT using NL80211_ATTR_BSS_SELECT. When user-space
> does provide this attribute and no explicit BSSID is provided it will pass
> the BSS selection criteria to the driver if the driver supports it.
> 
> When multiple BSS-es are found belonging to the same ESS, ie. Infra-BSS with
> same SSID, the criteria can be used to select a BSS. The criteria defined are:
> 
> 1. select BSS in specific band regardless RSSI level.

Not really regardless, but band would take precedence. So if there are
two BSS-es in the specified band it will still select the one with
highest RSSI.

> 2. adjust RSSI level for BSS-es in specified band.

I defined the attribute as unsigned, but maybe there are scenarios that
justify a signed value here. People may prefer using 5G band as it is
usually more quiet and channels are further apart. However, range can be
limited so people may use a penalty on RSSI, ie. a negative value.

Gr. AvS

> Open issue:
> -----------
> * NL80211_CMD_CONNECT support ATTR_MAC and ATTR_MAC_HINT. When ATTR_MAC is
>   used BSS selection criteria are ignored. What about ATTR_MAC_HINT?
> 
> Arend van Spriel (2):
>   nl80211: add extended feature for BSS selection support
>   brcmfmac: add support for nl80211 BSS_SELECT feature
> 
>  drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 47 ++++++++++++++++++++++
>  drivers/net/wireless/brcm80211/brcmfmac/common.c   | 38 ++++++++++-------
>  drivers/net/wireless/brcm80211/brcmfmac/core.h     |  1 +
>  include/net/cfg80211.h                             | 17 ++++++++
>  include/uapi/linux/nl80211.h                       | 35 ++++++++++++++++
>  net/wireless/nl80211.c                             | 46 +++++++++++++++++++++
>  6 files changed, 169 insertions(+), 15 deletions(-)
> 

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Johannes Berg]

On Thu, 2015-12-24 at 13:19 +0100, Arend van Spriel wrote:
> Introducing a new extended feature that the driver can support which
> indicate the driver/firmware supports configuration of BSS selection
> criteria upon CONNECT command.

Can you edit the commit message to include some of the 0/2 email here?

>  /**
> + * struct cfg80211_bss_selection - Connection parameters for BSS selection.
> + *
> + * @present: indicates whether parameters are set.
> + * @pref_band: preferred band.
> + * @rssi_adjust: adjustment for RSSI level of the preferred band.
> + * @ignore_rssi: indicates whether BSS in preferred band is to be selected
> + *> 	> regardless its RSSI level.
> + */
> +struct cfg80211_bss_selection {
> +> 	> bool present;
> +> 	> enum nl80211_band pref_band;
> +> 	> u8 rssi_adjust;
> +> 	> bool ignore_rssi;
> +};

Hm. Isn't it possible to specify *some* parameters of these? Or at
least, in the future (if we extend this), it would be?

Seems that 'present' might want to be a bitmap or so? Or perhaps be
done by using invalid values by default (e.g. NUM_BANDS for no band
preference, etc.)?

> @@ -1910,6 +1926,7 @@ struct cfg80211_connect_params {
>  > 	> struct ieee80211_ht_cap ht_capa_mask;
>  > 	> struct ieee80211_vht_cap vht_capa;
>  > 	> struct ieee80211_vht_cap vht_capa_mask;
> +	struct cfg80211_bss_selection bss_select;

No documentation here?

> +/**
> + * enum nl80211_attr_bss_select - attributes for bss selection.
> + *
> + * @__NL80211_ATTR_BSS_SELECT_INVALID: reserved.
> + * @NL80211_ATTR_BSS_SELECT_BAND_PREF: Required attribute indicating the
> + *> 	> preferred band. The preference by itself still allows RSSI based
> + *> 	> selection of BSS and as such is only a tie breaker. Value according
> + *> 	> %enum nl80211_band.
> + * @NL80211_ATTR_BSS_SELECT_RSSI_ADJUST: When present the RSSI level for
> + *	the BSS in the preferred band is adjusted accordingly (u8).

"adjusted" is a bit vague - should specify more precisely what happens?

> + * @NL80211_ATTR_BSS_SELECT_IGNORE_RSSI: flag attribute which can be used to
> + *> 	> to have the BSS in the preferred band being selected regardless
> + *	of its RSSI level.

Can't that be done by a huge adjustment? Like, say, 255 dB adjustment?

> +> 	> err = nla_parse(attr, NL80211_ATTR_BSS_SELECT_MAX,
> +> 	> 	> 	> nla_data(nla), nla_len(nla), nl80211_bss_select_policy);
> +> 	> if (err)
> +> 	> 	> return err;
> +
> +> 	> bss_select->present = true;
> +> 	> if (!attr[NL80211_ATTR_BSS_SELECT_BAND_PREF])
> +> 	> 	> return -EINVAL;
> +
> +> 	> bss_select->pref_band =
> +> 	> 	> nla_get_u32(attr[NL80211_ATTR_BSS_SELECT_BAND_PREF]);
> +> 	> bss_select->rssi_adjust =
> +		nla_get_u8(attr[NL80211_ATTR_BSS_SELECT_RSSI_ADJUST]);

Need to check the attribute exists, no?


It could be worthwhile to have a "demo" implementation for the code in
cfg80211 that's used when you use CONNECT command with mac80211, maybe?

johannes

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Arend van Spriel]

On 01/05/2016 10:25 AM, Johannes Berg wrote:
> On Thu, 2015-12-24 at 13:19 +0100, Arend van Spriel wrote:
>> Introducing a new extended feature that the driver can support which
>> indicate the driver/firmware supports configuration of BSS selection
>> criteria upon CONNECT command.
>
> Can you edit the commit message to include some of the 0/2 email here?

Sure I can and will.

>>   /**
>> + * struct cfg80211_bss_selection - Connection parameters for BSS selection.
>> + *
>> + * @present: indicates whether parameters are set.
>> + * @pref_band: preferred band.
>> + * @rssi_adjust: adjustment for RSSI level of the preferred band.
>> + * @ignore_rssi: indicates whether BSS in preferred band is to be selected
>> + *> 	> regardless its RSSI level.
>> + */
>> +struct cfg80211_bss_selection {
>> +> 	> bool present;
>> +> 	> enum nl80211_band pref_band;
>> +> 	> u8 rssi_adjust;
>> +> 	> bool ignore_rssi;
>> +};
>
> Hm. Isn't it possible to specify *some* parameters of these? Or at
> least, in the future (if we extend this), it would be?
>
> Seems that 'present' might want to be a bitmap or so? Or perhaps be
> done by using invalid values by default (e.g. NUM_BANDS for no band
> preference, etc.)?

Ok. I was not sure how to go about this. Our firmware uses an ordered 
list of selection "keys" with the first being the primary selection key 
and so on. So there are three "key" types: band, rssi, and rssi_adjust. 
The latter is not really a selection key, but will do rssi adjustment 
for BSSes in the specified band.

One of the questions I have is whether the order of a nested list 
attribute is retained.

>> @@ -1910,6 +1926,7 @@ struct cfg80211_connect_params {
>>   > 	> struct ieee80211_ht_cap ht_capa_mask;
>>   > 	> struct ieee80211_vht_cap vht_capa;
>>   > 	> struct ieee80211_vht_cap vht_capa_mask;
>> +	struct cfg80211_bss_selection bss_select;
>
> No documentation here?

Obviously, but will add it ;-)

>> +/**
>> + * enum nl80211_attr_bss_select - attributes for bss selection.
>> + *
>> + * @__NL80211_ATTR_BSS_SELECT_INVALID: reserved.
>> + * @NL80211_ATTR_BSS_SELECT_BAND_PREF: Required attribute indicating the
>> + *> 	> preferred band. The preference by itself still allows RSSI based
>> + *> 	> selection of BSS and as such is only a tie breaker. Value according
>> + *> 	> %enum nl80211_band.
>> + * @NL80211_ATTR_BSS_SELECT_RSSI_ADJUST: When present the RSSI level for
>> + *	the BSS in the preferred band is adjusted accordingly (u8).
>
> "adjusted" is a bit vague - should specify more precisely what happens?

Ok. Will elaborate. In follow-up email I raise question whether this 
could/should be a signed value. Any opinion on this?

>> + * @NL80211_ATTR_BSS_SELECT_IGNORE_RSSI: flag attribute which can be used to
>> + *> 	> to have the BSS in the preferred band being selected regardless
>> + *	of its RSSI level.
>
> Can't that be done by a huge adjustment? Like, say, 255 dB adjustment?

Probably, but this may go away depending on the "key" type thing 
mentioned above.

>> +> 	> err = nla_parse(attr, NL80211_ATTR_BSS_SELECT_MAX,
>> +> 	> 	> 	> nla_data(nla), nla_len(nla), nl80211_bss_select_policy);
>> +> 	> if (err)
>> +> 	> 	> return err;
>> +
>> +> 	> bss_select->present = true;
>> +> 	> if (!attr[NL80211_ATTR_BSS_SELECT_BAND_PREF])
>> +> 	> 	> return -EINVAL;
>> +
>> +> 	> bss_select->pref_band =
>> +> 	> 	> nla_get_u32(attr[NL80211_ATTR_BSS_SELECT_BAND_PREF]);
>> +> 	> bss_select->rssi_adjust =
>> +		nla_get_u8(attr[NL80211_ATTR_BSS_SELECT_RSSI_ADJUST]);
>
> Need to check the attribute exists, no?

Was assuming nla_get_u8 would return 0 so it would not matter, but it 
seems to be recipe for disaster [1].

>
> It could be worthwhile to have a "demo" implementation for the code in
> cfg80211 that's used when you use CONNECT command with mac80211, maybe?

Sure. Sorry for being single-minded here ;-)

Regards,
Arend

[1] http://lxr.free-electrons.com/source/include/net/netlink.h#L1037

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Johannes Berg]

On Tue, 2016-01-05 at 10:50 +0100, Arend van Spriel wrote:

> > > +struct cfg80211_bss_selection {
> > > +> 	> bool present;
> > > +> 	> enum nl80211_band pref_band;
> > > +> 	> u8 rssi_adjust;
> > > +> 	> bool ignore_rssi;
> > > +};
> > 
> > Hm. Isn't it possible to specify *some* parameters of these? Or at
> > least, in the future (if we extend this), it would be?
> > 
> > Seems that 'present' might want to be a bitmap or so? Or perhaps be
> > done by using invalid values by default (e.g. NUM_BANDS for no band
> > preference, etc.)?
> 
> Ok. I was not sure how to go about this. Our firmware uses an ordered
> list of selection "keys" with the first being the primary selection
> key and so on. So there are three "key" types: band, rssi, and
> rssi_adjust. 
> The latter is not really a selection key, but will do rssi adjustment
> for BSSes in the specified band.

Ok.

> One of the questions I have is whether the order of a nested list 
> attribute is retained.

It is if you parse it right, but it's not typically something that we
rely on and take advantage of, so I wouldn't want to do it that way.
Also, I'm not really sure it'd really be what we wanted to do anyway?

It seems though that we might need to allow for other drivers having
other selection criteria, and having a validity flag for each? That
could go some of the way.

To really fully replicate your firmware's capabilities seems difficult,
though I also don't really see much point, or are you saying you could
put "rssi" first? But the way you described it in nl80211, with "band"
being a "tie breaker", it sounds like really "rssi" comes first,
usually, followed by rssi_adjust and band?

The other way - band first - could also be done with a huge rssi_adjust
though (as I said before), so I don't really see much value in having
all this complexity to start with?

> Ok. Will elaborate. In follow-up email I raise question whether this 
> could/should be a signed value. Any opinion on this?

I didn't see that, but yeah - good question. Would it be supported by
firmware?

But logically - does it even make sense? If you already prefer that
band, why give it a boost still? Just disable RSSI? Hmm.


johannes

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Arend van Spriel]

On 01/05/2016 05:31 PM, Johannes Berg wrote:
> On Tue, 2016-01-05 at 10:50 +0100, Arend van Spriel wrote:
>
>>>> +struct cfg80211_bss_selection {
>>>> +> 	> bool present;
>>>> +> 	> enum nl80211_band pref_band;
>>>> +> 	> u8 rssi_adjust;
>>>> +> 	> bool ignore_rssi;
>>>> +};
>>>
>>> Hm. Isn't it possible to specify *some* parameters of these? Or at
>>> least, in the future (if we extend this), it would be?
>>>
>>> Seems that 'present' might want to be a bitmap or so? Or perhaps be
>>> done by using invalid values by default (e.g. NUM_BANDS for no band
>>> preference, etc.)?
>>
>> Ok. I was not sure how to go about this. Our firmware uses an ordered
>> list of selection "keys" with the first being the primary selection
>> key and so on. So there are three "key" types: band, rssi, and
>> rssi_adjust.
>> The latter is not really a selection key, but will do rssi adjustment
>> for BSSes in the specified band.
>
> Ok.
>
>> One of the questions I have is whether the order of a nested list
>> attribute is retained.
>
> It is if you parse it right, but it's not typically something that we
> rely on and take advantage of, so I wouldn't want to do it that way.
> Also, I'm not really sure it'd really be what we wanted to do anyway?

ok

> It seems though that we might need to allow for other drivers having
> other selection criteria, and having a validity flag for each? That
> could go some of the way.

So do we want want a dedicated "bss selection capability" flag iso 
extended feature in which the driver can indicate the supported 
selection criteria to user-space? Guess so.

> To really fully replicate your firmware's capabilities seems difficult,
> though I also don't really see much point, or are you saying you could
> put "rssi" first? But the way you described it in nl80211, with "band"
> being a "tie breaker", it sounds like really "rssi" comes first,
> usually, followed by rssi_adjust and band?

The firmware api is pretty flexible, but I did not want to introduce 
that in nl80211. So I limited it to the two use-cases I know of 1) 
"band, rssi", and 2) "rssi_adjust, rssi". In terms of netlink attributes 
I played a trick in reusing ATTR_BSS_SELECT_BAND_PREF. When 
ATTR_BSS_SELECT_RSSI_ADJUST is passed the ATTR_BSS_SELECT_BAND_PREF is 
used to determine in which band the rssi is adjusted. So "band" and 
"rssi_adjust" are mutual exclusive.

> The other way - band first - could also be done with a huge rssi_adjust
> though (as I said before), so I don't really see much value in having
> all this complexity to start with?
>
>> Ok. Will elaborate. In follow-up email I raise question whether this
>> could/should be a signed value. Any opinion on this?
>
> I didn't see that, but yeah - good question. Would it be supported by
> firmware?

To be honest I had to check, but yes it does.

> But logically - does it even make sense? If you already prefer that
> band, why give it a boost still? Just disable RSSI? Hmm.

I hope the use-cases mentioned clarify this.

Regards,
Arend

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Johannes Berg]

On Wed, 2016-01-06 at 11:16 +0100, Arend van Spriel wrote:

> So do we want want a dedicated "bss selection capability" flag iso 
> extended feature in which the driver can indicate the supported 
> selection criteria to user-space? Guess so.

Frankly, I'm not really quite sure.

The alternative is to just treat all of this as advisory and not worry,
so that userspace can specify all it wants and the driver will use all
it can.

That seems mostly reasonable as well.

> I played a trick in reusing ATTR_BSS_SELECT_BAND_PREF. When 
> ATTR_BSS_SELECT_RSSI_ADJUST is passed the ATTR_BSS_SELECT_BAND_PREF
> is 
> used to determine in which band the rssi is adjusted. So "band" and 
> "rssi_adjust" are mutual exclusive.

Yeah, OK, I think that might have confused me a bit :)

> > But logically - does it even make sense? If you already prefer that
> > band, why give it a boost still? Just disable RSSI? Hmm.
> 
> I hope the use-cases mentioned clarify this.
> 

Right. So realistically, writing this a bit more verbosely, you have

1) rssi_preference, band_preference(band)
2) rssi_adjust(band, delta), rssi_preference

and perhaps

3) rssi_preference

as the default?

As for 1), you said it was "band, rssi" but it seems you really meant
the other way around since before you said "band" was a tie-breaker.


Perhaps then, the API should just expose the two "primitives"
 * band_preference(band)
 * rssi_adjust(band, delta)?

johannes

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Johannes Berg]

On Wed, 2016-01-06 at 15:36 +0100, Johannes Berg wrote:

> Right. So realistically, writing this a bit more verbosely, you have
> 
> 1) rssi_preference, band_preference(band)
> 2) rssi_adjust(band, delta), rssi_preference
> 
> and perhaps
> 
> 3) rssi_preference
> 
> as the default?
> 
> As for 1), you said it was "band, rssi" but it seems you really meant
> the other way around since before you said "band" was a tie-breaker.
> 
> 
> Perhaps then, the API should just expose the two "primitives"
>  * band_preference(band)
>  * rssi_adjust(band, delta)?
> 

Which would basically map to cases (1) and (2) then.

johannes

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Arend van Spriel]

On 01/06/2016 03:36 PM, Johannes Berg wrote:
> On Wed, 2016-01-06 at 11:16 +0100, Arend van Spriel wrote:
>
>> So do we want want a dedicated "bss selection capability" flag iso
>> extended feature in which the driver can indicate the supported
>> selection criteria to user-space? Guess so.
>
> Frankly, I'm not really quite sure.
>
> The alternative is to just treat all of this as advisory and not worry,
> so that userspace can specify all it wants and the driver will use all
> it can.
>
> That seems mostly reasonable as well.

Agree. However, ne of the reasons I added the ext_feature is because 
people gave feedback they wanted it for testing purposes so they know 
what to expect from the driver. That same argument could used for the 
supported selection criteria. I have no strong opinion though.

>> I played a trick in reusing ATTR_BSS_SELECT_BAND_PREF. When
>> ATTR_BSS_SELECT_RSSI_ADJUST is passed the ATTR_BSS_SELECT_BAND_PREF
>> is
>> used to determine in which band the rssi is adjusted. So "band" and
>> "rssi_adjust" are mutual exclusive.
>
> Yeah, OK, I think that might have confused me a bit :)
>
>>> But logically - does it even make sense? If you already prefer that
>>> band, why give it a boost still? Just disable RSSI? Hmm.
>>
>> I hope the use-cases mentioned clarify this.
>>
>
> Right. So realistically, writing this a bit more verbosely, you have
>
> 1) rssi_preference, band_preference(band)
> 2) rssi_adjust(band, delta), rssi_preference
>
> and perhaps
>
> 3) rssi_preference
>
> as the default?

Good point.

> As for 1), you said it was "band, rssi" but it seems you really meant
> the other way around since before you said "band" was a tie-breaker.

My bad. When using "band_preference()" the selected BSS is in specified 
band even though the BSS in other band would be stronger. If there is no 
proper BSS in the specified band the preference is obviously discarded.

> Perhaps then, the API should just expose the two "primitives"
>   * band_preference(band)
>   * rssi_adjust(band, delta)?

And for use-case number #3:

   * rssi_preference(void)

Gr. AvS

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Johannes Berg]

On Thu, 2016-01-07 at 13:52 +0100, Arend van Spriel wrote:
> 
> Agree. However, one of the reasons I added the ext_feature is because
> people gave feedback they wanted it for testing purposes so they know
> what to expect from the driver. That same argument could used for the
> supported selection criteria. I have no strong opinion though.

Yeah, that's true - if we play the "advisory" card then we don't really
need to add it at all... Not sure what to do really. I'd love to
specify it very clearly if we need to at all.

Perhaps for features we can add NL80211_ATTR_BSS_SELECT to the wiphy
information, and within that add each of the supported
ATTR_BSS_SELECT_* as an NLA_FLAG? That way at least we don't need to
define all kinds of new flags in the API.

> > Right. So realistically, writing this a bit more verbosely, you
> > have
> > 
> > 1) rssi_preference, band_preference(band)
> > 2) rssi_adjust(band, delta), rssi_preference
> > 
> > and perhaps
> > 
> > 3) rssi_preference
> > 
> > as the default?
> 
> Good point.
> 
> > As for 1), you said it was "band, rssi" but it seems you really
> > meant
> > the other way around since before you said "band" was a tie-
> > breaker.
> 
> My bad. When using "band_preference()" the selected BSS is in specified 
> band even though the BSS in other band would be stronger. If there is no 
> proper BSS in the specified band the preference is obviously
> discarded.

So 1) really becomes just
 1) band_preference(band)

without any RSSI at all? Ah, I get it. You have to kind of reword that
in terms of the logic used:

 1) select_highest_rssi_in_band(band), select_highest_rssi()
 2) adjust_rssi_if_in_band(band, delta), select_highest_rssi()
 3) select_highest_rssi()

But if you look at that, then "select_highest_rssi_in_band(band)" is
really the same as "adjust_rssi_if_in_band(band, 1000)" since you can't
have real RSSI that high, which leaves us with just a single primitive
and perhaps a standard value for that "1000"?


> > Perhaps then, the API should just expose the two "primitives"
> >   * band_preference(band)
> >   * rssi_adjust(band, delta)?

I guess it makes even more sense as I just worded it.

> > And for use-case number #3:
> 
>    * rssi_preference(void)

Yeah although you'd assume that's the default anyway?

johannes

Re: [RFC 1/2] nl80211: add extended feature for BSS selection support

[Arend van Spriel]

On 01/07/2016 03:41 PM, Johannes Berg wrote:
> On Thu, 2016-01-07 at 13:52 +0100, Arend van Spriel wrote:
>>
>> Agree. However, one of the reasons I added the ext_feature is because
>> people gave feedback they wanted it for testing purposes so they know
>> what to expect from the driver. That same argument could used for the
>> supported selection criteria. I have no strong opinion though.
>
> Yeah, that's true - if we play the "advisory" card then we don't really
> need to add it at all... Not sure what to do really. I'd love to
> specify it very clearly if we need to at all.

Yes. If we add something like this it should be a specific as possible.

> Perhaps for features we can add NL80211_ATTR_BSS_SELECT to the wiphy
> information, and within that add each of the supported
> ATTR_BSS_SELECT_* as an NLA_FLAG? That way at least we don't need to
> define all kinds of new flags in the API.

That sounds like a plan.

>>> Right. So realistically, writing this a bit more verbosely, you
>>> have
>>>
>>> 1) rssi_preference, band_preference(band)
>>> 2) rssi_adjust(band, delta), rssi_preference
>>>
>>> and perhaps
>>>
>>> 3) rssi_preference
>>>
>>> as the default?
>>
>> Good point.
>>
>>> As for 1), you said it was "band, rssi" but it seems you really
>>> meant
>>> the other way around since before you said "band" was a tie-
>>> breaker.
>>
>> My bad. When using "band_preference()" the selected BSS is in specified
>> band even though the BSS in other band would be stronger. If there is no
>> proper BSS in the specified band the preference is obviously
>> discarded.
>
> So 1) really becomes just
>   1) band_preference(band)
>
> without any RSSI at all? Ah, I get it. You have to kind of reword that
> in terms of the logic used:
>
>   1) select_highest_rssi_in_band(band), select_highest_rssi()
>   2) adjust_rssi_if_in_band(band, delta), select_highest_rssi()
>   3) select_highest_rssi()
>
> But if you look at that, then "select_highest_rssi_in_band(band)" is
> really the same as "adjust_rssi_if_in_band(band, 1000)" since you can't
> have real RSSI that high, which leaves us with just a single primitive
> and perhaps a standard value for that "1000"?

True. Although I may have a firmware issue with that as RSSI is probably 
typed as s8 there. At least the adjustment is so can do up to 127 there, 
which would probably be enough as well. I think I prefer an explicit 
primitive though.

>>> Perhaps then, the API should just expose the two "primitives"
>>>    * band_preference(band)
>>>    * rssi_adjust(band, delta)?
>
> I guess it makes even more sense as I just worded it.
>
>>> And for use-case number #3:
>>
>>     * rssi_preference(void)
>
> Yeah although you'd assume that's the default anyway?

That kinda depends on the driver I guess. Currently our driver defaults 
to rssi_adjust(5g, 8) and we restore to that when no BSS_SELECT 
attribute is provided in the .connect() callback.

Regards,
Arend