2016-01-26 13:06:37

by Arend van Spriel

[permalink] [raw]
Subject: [RFC V4 0/2] nl80211: allow configuration of BSS selection

This patch series adds support for configuration of BSS selection done by
the driver and/or firmware. It allows user-space to pass selection behaviour
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 behaviour 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, this can be used to select a BSS. The behaviours defined are:

1. RSSI based selection.
2. Band selection, RSSI based selection.
3. RSSI adjustment in given band, RSSI based selection.

V2:
- incorporate comments from Johannes Berg (may have missed some).
- split nl80211 patch in two separate patches.

V3:
- remove enum nl80211_bss_select_primitive.
- reword documentation of nl80211_bss_select_attr enum values.

V4:
- remove documentation reference to removed primitive.

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

.../broadcom/brcm80211/brcmfmac/cfg80211.c | 63 +++++++++++++++++-
.../wireless/broadcom/brcm80211/brcmfmac/common.c | 38 ++++++-----
.../wireless/broadcom/brcm80211/brcmfmac/core.h | 1 +
include/net/cfg80211.h | 34 ++++++++++
include/uapi/linux/nl80211.h | 42 ++++++++++++
net/wireless/core.c | 4 ++
net/wireless/nl80211.c | 76 ++++++++++++++++++++++
7 files changed, 240 insertions(+), 18 deletions(-)

--
1.9.1



2016-01-26 13:06:37

by Arend van Spriel

[permalink] [raw]
Subject: [RFC V4 1/2] nl80211: add feature for BSS selection support

Introducing a new feature that the driver can use to
indicate the driver/firmware supports configuration of BSS
selection criteria upon CONNECT command. This can be useful
when multiple BSS-es are found belonging to the same ESS,
ie. Infra-BSS with same SSID. The criteria can then be used to
offload selection of a preferred BSS.

Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Franky (Zhenhui) Lin <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Lei Zhang <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
include/net/cfg80211.h | 34 ++++++++++++++++++++
include/uapi/linux/nl80211.h | 42 ++++++++++++++++++++++++
net/wireless/core.c | 4 +++
net/wireless/nl80211.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 156 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9bcaaf7..f571f4a 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1854,6 +1854,33 @@ struct cfg80211_ibss_params {
};

/**
+ * struct cfg80211_bss_select_adjust - BSS selection with RSSI adjustment.
+ *
+ * @band: band of BSS which should match for RSSI level adjustment.
+ * @delta: value of RSSI level adjustment.
+ */
+struct cfg80211_bss_select_adjust {
+ enum nl80211_band band;
+ s8 delta;
+};
+
+/**
+ * struct cfg80211_bss_selection - connection parameters for BSS selection.
+ *
+ * @behaviour: requested BSS selection behaviour.
+ * @param: parameters for requestion behaviour.
+ * @band_pref: preferred band for %NL80211_BSS_SELECT_ATTR_BAND_PREF.
+ * @adjust: parameters for %NL80211_BSS_SELECT_ATTR_RSSI_ADJUST.
+ */
+struct cfg80211_bss_selection {
+ enum nl80211_bss_select_attr behaviour;
+ union {
+ enum nl80211_band band_pref;
+ struct cfg80211_bss_select_adjust adjust;
+ } param;
+};
+
+/**
* struct cfg80211_connect_params - Connection parameters
*
* This structure provides information needed to complete IEEE 802.11
@@ -1888,6 +1915,7 @@ struct cfg80211_ibss_params {
* @ht_capa_mask: The bits of ht_capa which are to be used.
* @vht_capa: VHT Capability overrides
* @vht_capa_mask: The bits of vht_capa which are to be used.
+ * @bss_select: criteria to be used for BSS selection.
*/
struct cfg80211_connect_params {
struct ieee80211_channel *channel;
@@ -1910,6 +1938,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;
};

/**
@@ -3178,6 +3207,9 @@ struct wiphy_vendor_command {
* low rssi when a frame is heard on different channel, then it should set
* this variable to the maximal offset for which it can compensate.
* This value should be set in MHz.
+ * @bss_select_support: bitmask indicating the BSS selection criteria supported
+ * by the driver in the .connect() callback. The bit position maps to the
+ * attribute indices defined in &enum nl80211_bss_select_attr.
*/
struct wiphy {
/* assign these fields before you register the wiphy */
@@ -3300,6 +3332,8 @@ struct wiphy {
u8 max_num_csa_counters;
u8 max_adj_channel_rssi_comp;

+ u32 bss_select_support;
+
char priv[0] __aligned(NETDEV_ALIGN);
};

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 5b7b5eb..f3ece95 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1789,6 +1789,12 @@ 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 for driver supporting the
+ * BSS selection feature. When used with %NL80211_CMD_GET_WIPHY it contains
+ * NLA_FLAG type according &enum nl80211_bss_select_attr to indicate what
+ * BSS selection behaviours are supported. When used with %NL80211_CMD_CONNECT
+ * it contains the behaviour and parameters 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 +2170,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,
@@ -4651,4 +4659,38 @@ enum nl80211_sched_scan_plan {
__NL80211_SCHED_SCAN_PLAN_AFTER_LAST - 1
};

+/**
+ * enum nl80211_bss_select_attr - attributes for bss selection.
+ *
+ * @__NL80211_BSS_SELECT_ATTR_INVALID: reserved.
+ * @NL80211_BSS_SELECT_ATTR_RSSI: Flag indicating only RSSI-based BSS selection
+ * is requested.
+ * @NL80211_BSS_SELECT_ATTR_BAND_PREF: attribute indicating BSS
+ * selection should be done such that the specified band is preferred.
+ * When there are multiple BSS-es in the preferred band, the driver
+ * shall use RSSI-based BSS selection as a second step. The value of
+ * this attribute is according to &enum nl80211_band (u32).
+ * @NL80211_BSS_SELECT_ATTR_RSSI_ADJUST: When present the RSSI level for
+ * BSS-es in the specified band is to be adjusted before doing
+ * RSSI-based BSS selection. The attribute value is a packed two-byte
+ * value. The lower byte contains the adjustment value (s8) and the
+ * high byte contains the band according &enum nl80211_band.
+ * @NL80211_BSS_SELECT_ATTR_MAX: highest bss select attribute number.
+ *@__NL80211_BSS_SELECT_ATTR_AFTER_LAST: internal use.
+ *
+ * These attributes are found within %NL80211_ATTR_BSS_SELECT and
+ * indicate the required BSS selection behaviour which the driver
+ * should use.
+ */
+enum nl80211_bss_select_attr {
+ __NL80211_BSS_SELECT_ATTR_INVALID,
+ NL80211_BSS_SELECT_ATTR_RSSI,
+ NL80211_BSS_SELECT_ATTR_BAND_PREF,
+ NL80211_BSS_SELECT_ATTR_RSSI_ADJUST,
+
+ /* keep last */
+ __NL80211_BSS_SELECT_ATTR_AFTER_LAST,
+ NL80211_BSS_SELECT_ATTR_MAX = __NL80211_BSS_SELECT_ATTR_AFTER_LAST - 1
+};
+
#endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 3a9c41b..f6b2976 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -626,6 +626,10 @@ int wiphy_register(struct wiphy *wiphy)
!rdev->ops->set_mac_acl)))
return -EINVAL;

+ if (WARN_ON(wiphy->bss_select_support &&
+ (wiphy->bss_select_support & ~(BIT(NL80211_BSS_SELECT_ATTR_MAX) - 1))))
+ return -EINVAL;
+
if (wiphy->addresses)
memcpy(wiphy->perm_addr, wiphy->addresses[0].addr, ETH_ALEN);

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d4786f2..e17044d 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_BSS_SELECT_ATTR_MAX + 1] = {
+ [NL80211_BSS_SELECT_ATTR_RSSI] = { .type = NLA_FLAG },
+ [NL80211_BSS_SELECT_ATTR_BAND_PREF] = { .type = NLA_U32 },
+ [NL80211_BSS_SELECT_ATTR_RSSI_ADJUST] = { .type = NLA_U8 },
+};
+
static int nl80211_prepare_wdev_dump(struct sk_buff *skb,
struct netlink_callback *cb,
struct cfg80211_registered_device **rdev,
@@ -1730,6 +1737,24 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
rdev->wiphy.ext_features))
goto nla_put_failure;

+ if (rdev->wiphy.bss_select_support) {
+ struct nlattr *nested;
+ u32 bss_select_support = rdev->wiphy.bss_select_support;
+
+ nested = nla_nest_start(msg, NL80211_ATTR_BSS_SELECT);
+ if (!nested)
+ goto nla_put_failure;
+
+ i = 0;
+ while (bss_select_support) {
+ if ((bss_select_support & 1) &&
+ nla_put_flag(msg, i))
+ goto nla_put_failure;
+ i++;
+ bss_select_support >>= 1;
+ }
+ nla_nest_end(msg, nested);
+ }
/* done */
state->split_start = 0;
break;
@@ -5753,6 +5778,42 @@ 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_BSS_SELECT_ATTR_MAX + 1];
+ u16 band_delta;
+ int err;
+
+ err = nla_parse(attr, NL80211_BSS_SELECT_ATTR_MAX,
+ nla_data(nla), nla_len(nla), nl80211_bss_select_policy);
+ if (err)
+ return err;
+
+ bss_select->behaviour = __NL80211_BSS_SELECT_ATTR_INVALID;
+
+ if (attr[NL80211_BSS_SELECT_ATTR_RSSI])
+ bss_select->behaviour = NL80211_BSS_SELECT_ATTR_RSSI;
+
+ if (attr[NL80211_BSS_SELECT_ATTR_BAND_PREF]) {
+ bss_select->behaviour = NL80211_BSS_SELECT_ATTR_BAND_PREF;
+ bss_select->param.band_pref =
+ nla_get_u32(attr[NL80211_BSS_SELECT_ATTR_BAND_PREF]);
+ }
+ if (attr[NL80211_BSS_SELECT_ATTR_RSSI_ADJUST]) {
+ bss_select->behaviour = NL80211_BSS_SELECT_ATTR_RSSI_ADJUST;
+ band_delta =
+ nla_get_u16(attr[NL80211_BSS_SELECT_ATTR_RSSI_ADJUST]);
+ bss_select->param.adjust.delta = (s8)(band_delta & 0xFF);
+ bss_select->param.adjust.band = band_delta >> 8;
+ }
+
+ if (bss_select->behaviour == __NL80211_BSS_SELECT_ATTR_INVALID)
+ return -EINVAL;
+
+ return 0;
+}
+
static int nl80211_parse_random_mac(struct nlattr **attrs,
u8 *mac_addr, u8 *mac_addr_mask)
{
@@ -7980,6 +8041,21 @@ 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 && rdev->wiphy.bss_select_support &&
+ info->attrs[NL80211_ATTR_BSS_SELECT]) {
+ err = parse_bss_select(info->attrs[NL80211_ATTR_BSS_SELECT],
+ &connect.bss_select);
+ if (err) {
+ kzfree(connkeys);
+ return err;
+ }
+ if (!(rdev->wiphy.bss_select_support & BIT(connect.bss_select.behaviour))) {
+ kzfree(connkeys);
+ return -EINVAL;
+ }
+ }
+
wdev_lock(dev->ieee80211_ptr);
err = cfg80211_connect(rdev, dev, &connect, connkeys, NULL);
wdev_unlock(dev->ieee80211_ptr);
--
1.9.1


2016-01-26 13:06:59

by Arend van Spriel

[permalink] [raw]
Subject: [RFC V4 2/2] brcmfmac: add support for nl80211 BSS_SELECT feature

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

Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Franky (Zhenhui) Lin <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Lei Zhang <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 63 ++++++++++++++++++++--
.../wireless/broadcom/brcm80211/brcmfmac/common.c | 38 +++++++------
.../wireless/broadcom/brcm80211/brcmfmac/core.h | 1 +
3 files changed, 84 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 7b01e4d..d90378e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -238,6 +238,20 @@ struct parsed_vndr_ies {
struct parsed_vndr_ie_info ie_info[VNDR_IE_PARSE_LIMIT];
};

+static u8 nl80211_band_to_fwil(enum nl80211_band band)
+{
+ switch (band) {
+ case NL80211_BAND_2GHZ:
+ return WLC_BAND_2G;
+ case NL80211_BAND_5GHZ:
+ return WLC_BAND_5G;
+ default:
+ WARN_ON(1);
+ break;
+ }
+ return 0;
+}
+
static u16 chandef_to_chanspec(struct brcmu_d11inf *d11inf,
struct cfg80211_chan_def *ch)
{
@@ -1737,6 +1751,43 @@ 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];
+ enum nl80211_band band;
+ int err, i = 0;
+
+ join_pref_params[i].len = 2;
+ join_pref_params[i].rssi_gain = 0;
+ switch (bss_select->behaviour) {
+ case NL80211_BSS_SELECT_ATTR_BAND_PREF:
+ join_pref_params[i].type = BRCMF_JOIN_PREF_BAND;
+ band = bss_select->param.band_pref;
+ join_pref_params[i].band = nl80211_band_to_fwil(band);
+ i++;
+ break;
+ case NL80211_BSS_SELECT_ATTR_RSSI_ADJUST:
+ join_pref_params[i].type = BRCMF_JOIN_PREF_RSSI_DELTA;
+ band = bss_select->param.adjust.band;
+ join_pref_params[i].band = nl80211_band_to_fwil(band);
+ join_pref_params[i].rssi_gain = bss_select->param.adjust.delta;
+ i++;
+ break;
+ case NL80211_BSS_SELECT_ATTR_RSSI:
+ default:
+ break;
+ }
+ join_pref_params[i].type = BRCMF_JOIN_PREF_RSSI;
+ join_pref_params[i].len = 2;
+ join_pref_params[i].rssi_gain = 0;
+ join_pref_params[i].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)
@@ -1862,11 +1913,13 @@ brcmf_cfg80211_connect(struct wiphy *wiphy, struct net_device *ndev,
ext_join_params->scan_le.scan_type = -1;
ext_join_params->scan_le.home_time = cpu_to_le32(-1);

- if (sme->bssid)
+ if (sme->bssid) {
memcpy(&ext_join_params->assoc_le.bssid, sme->bssid, ETH_ALEN);
- else
+ brcmf_c_set_joinpref_default(ifp);
+ } 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);

@@ -6179,6 +6232,10 @@ 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->bss_select_support = BIT(NL80211_BSS_SELECT_ATTR_RSSI) |
+ BIT(NL80211_BSS_SELECT_ATTR_BAND_PREF) |
+ BIT(NL80211_BSS_SELECT_ATTR_RSSI_ADJUST);
+
wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT |
WIPHY_FLAG_OFFCHAN_TX |
WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 4265b50..b9e81fb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -32,7 +32,7 @@ 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

#define BRCMF_DEFAULT_TXGLOM_SIZE 32 /* max tx frames in glom chain */
@@ -76,11 +76,31 @@ MODULE_PARM_DESC(ignore_probe_fail, "always succeed probe for debugging");

struct brcmf_mp_global_t brcmf_mp_global;

+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;
@@ -147,19 +167,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/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index 8f39435..c9a1a13 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -220,5 +220,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


2016-01-26 13:56:37

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC V4 1/2] nl80211: add feature for BSS selection support

On Tue, 2016-01-26 at 14:06 +0100, Arend van Spriel wrote:

> + * @behaviour: requested BSS selection behaviour.
> + * @param: parameters for requestion behaviour.
> + * @band_pref: preferred band for
> %NL80211_BSS_SELECT_ATTR_BAND_PREF.
> + * @adjust: parameters for %NL80211_BSS_SELECT_ATTR_RSSI_ADJUST.

Sadly, I don't think this works with kernel-doc. You'd have to split it
out into a named union to get this working properly.

> +/**
> + * enum nl80211_bss_select_attr - attributes for bss selection.
> + *
> + * @__NL80211_BSS_SELECT_ATTR_INVALID: reserved.
> + * @NL80211_BSS_SELECT_ATTR_RSSI: Flag indicating only RSSI-based BSS selection
> + * is requested.
> + * @NL80211_BSS_SELECT_ATTR_BAND_PREF: attribute indicating BSS
> + *> > selection should be done such that the specified band is preferred.
> + *> > When there are multiple BSS-es in the preferred band, the driver
> + *> > shall use RSSI-based BSS selection as a second step. The value of
> + *> > this attribute is according to &enum nl80211_band (u32).
> + * @NL80211_BSS_SELECT_ATTR_RSSI_ADJUST: When present the RSSI level for
> + *> > BSS-es in the specified band is to be adjusted before doing
> + *> > RSSI-based BSS selection. The attribute value is a packed two-byte
> + *> > value. The lower byte contains the adjustment value (s8) and the
> + * high byte contains the band according &enum nl80211_band.

I think it might be nicer to define an explicit struct for this, then
you don't have to use u8 for the band in one attribute and u32 for the
band in the other attribute either.

As long as there's no u64 in the struct that's pretty much safe - if
u64 is needed use compat_u64 :)

> + * @NL80211_BSS_SELECT_ATTR_MAX: highest bss select attribute number.
> + *@__NL80211_BSS_SELECT_ATTR_AFTER_LAST: internal use.
> + *
> + * These attributes are found within %NL80211_ATTR_BSS_SELECT and
> + * indicate the required BSS selection behaviour which the driver
> + * should use.

You should probably indicate that only a single one can ever be
specified?

> +static const struct nla_policy
> +nl80211_bss_select_policy[NL80211_BSS_SELECT_ATTR_MAX + 1] = {
> + [NL80211_BSS_SELECT_ATTR_RSSI] = { .type = NLA_FLAG },
> + [NL80211_BSS_SELECT_ATTR_BAND_PREF] = { .type = NLA_U32 },
> + [NL80211_BSS_SELECT_ATTR_RSSI_ADJUST] = { .type = NLA_U8 },

The RSSI_ADJUST here seems wrong in any case? Should've been NLA_U16
now?

> @@ -5753,6 +5778,42 @@ 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_BSS_SELECT_ATTR_MAX + 1];
> + u16 band_delta;
> + int err;

This should perhaps reject specification of multiple attributes, since
otherwise the order of the code here dictates which one "wins".


But these are small things - looks good!

johannes

2016-01-28 09:50:03

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC V4 1/2] nl80211: add feature for BSS selection support

On Wed, 2016-01-27 at 22:53 +0100, Arend van Spriel wrote:

> So you mean mapping the explicit structure over the nla_data()?

Yes, see "struct nl80211_sta_flag_update" for example.

> > The RSSI_ADJUST here seems wrong in any case? Should've been
> > NLA_U16
> > now?
>
> It should have, yes.

Now of course it will have to change for again for the struct :)


johannes

2016-01-27 21:53:44

by Arend van Spriel

[permalink] [raw]
Subject: Re: [RFC V4 1/2] nl80211: add feature for BSS selection support



On 26-1-2016 14:56, Johannes Berg wrote:
> On Tue, 2016-01-26 at 14:06 +0100, Arend van Spriel wrote:
>>
>> + * @behaviour: requested BSS selection behaviour.
>> + * @param: parameters for requestion behaviour.
>> + * @band_pref: preferred band for
>> %NL80211_BSS_SELECT_ATTR_BAND_PREF.
>> + * @adjust: parameters for %NL80211_BSS_SELECT_ATTR_RSSI_ADJUST.
>
> Sadly, I don't think this works with kernel-doc. You'd have to split it
> out into a named union to get this working properly.

Yeah. I did not run kernel-doc. Will look into it.

>> +/**
>> + * enum nl80211_bss_select_attr - attributes for bss selection.
>> + *
>> + * @__NL80211_BSS_SELECT_ATTR_INVALID: reserved.
>> + * @NL80211_BSS_SELECT_ATTR_RSSI: Flag indicating only RSSI-based BSS selection
>> + * is requested.
>> + * @NL80211_BSS_SELECT_ATTR_BAND_PREF: attribute indicating BSS
>> + *> > selection should be done such that the specified band is preferred.
>> + *> > When there are multiple BSS-es in the preferred band, the driver
>> + *> > shall use RSSI-based BSS selection as a second step. The value of
>> + *> > this attribute is according to &enum nl80211_band (u32).
>> + * @NL80211_BSS_SELECT_ATTR_RSSI_ADJUST: When present the RSSI level for
>> + *> > BSS-es in the specified band is to be adjusted before doing
>> + *> > RSSI-based BSS selection. The attribute value is a packed two-byte
>> + *> > value. The lower byte contains the adjustment value (s8) and the
>> + * high byte contains the band according &enum nl80211_band.
>
> I think it might be nicer to define an explicit struct for this, then
> you don't have to use u8 for the band in one attribute and u32 for the
> band in the other attribute either.
>
> As long as there's no u64 in the struct that's pretty much safe - if
> u64 is needed use compat_u64 :)

So you mean mapping the explicit structure over the nla_data()?

>> + * @NL80211_BSS_SELECT_ATTR_MAX: highest bss select attribute number.
>> + *@__NL80211_BSS_SELECT_ATTR_AFTER_LAST: internal use.
>> + *
>> + * These attributes are found within %NL80211_ATTR_BSS_SELECT and
>> + * indicate the required BSS selection behaviour which the driver
>> + * should use.
>
> You should probably indicate that only a single one can ever be
> specified?

Realized that was missing indeed. Will add it.

>> +static const struct nla_policy
>> +nl80211_bss_select_policy[NL80211_BSS_SELECT_ATTR_MAX + 1] = {
>> + [NL80211_BSS_SELECT_ATTR_RSSI] = { .type = NLA_FLAG },
>> + [NL80211_BSS_SELECT_ATTR_BAND_PREF] = { .type = NLA_U32 },
>> + [NL80211_BSS_SELECT_ATTR_RSSI_ADJUST] = { .type = NLA_U8 },
>
> The RSSI_ADJUST here seems wrong in any case? Should've been NLA_U16
> now?

It should have, yes.

>> @@ -5753,6 +5778,42 @@ 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_BSS_SELECT_ATTR_MAX + 1];
>> + u16 band_delta;
>> + int err;
>
> This should perhaps reject specification of multiple attributes, since
> otherwise the order of the code here dictates which one "wins".

I was waiting for your opinion on this as it did not feel right to me
either.

> But these are small things - looks good!

Thanks. Will work on final patch (famous last words).

Regards,
Arend

2016-01-26 14:02:38

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC V4 2/2] brcmfmac: add support for nl80211 BSS_SELECT feature

On Tue, 2016-01-26 at 14:06 +0100, Arend van Spriel wrote:

> +static u8 nl80211_band_to_fwil(enum nl80211_band band)
> +{
> + switch (band) {
> + case NL80211_BAND_2GHZ:
> + return WLC_BAND_2G;
> + case NL80211_BAND_5GHZ:
> + return WLC_BAND_5G;
> + default:
> + WARN_ON(1);
>
This, btw, is trivially triggerable - you need to check in nl80211 that
the specified band is actually supported by the driver, otherwise even
non-sense like "17" can be specified.

johannes