[PATCH v4 0/2] Add FILS discovery support

[PATCH v4 0/2] Add FILS discovery support

[Aloka Dixit]

This patchset adds support for FILS discovery transmission as per
IEEE Std 802.11ai-2016.

v4: Removed separate flag 'enabled' and data related to beacon
response window and omit replicated probe response features.

Aloka Dixit (2):
  nl80211: Add FILS discovery support
  mac80211: Add FILS discovery support

 include/net/cfg80211.h       | 19 +++++++++++++++
 include/net/mac80211.h       | 27 +++++++++++++++++++++
 include/uapi/linux/nl80211.h | 36 ++++++++++++++++++++++++++++
 net/mac80211/cfg.c           | 41 ++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h   |  7 ++++++
 net/mac80211/tx.c            | 25 ++++++++++++++++++++
 net/wireless/nl80211.c       | 46 ++++++++++++++++++++++++++++++++++++
 7 files changed, 201 insertions(+)

-- 
2.25.0

[PATCH v4 1/2] nl80211: Add FILS discovery support

[Aloka Dixit]

FILS discovery attribute, NL80211_ATTR_FILS_DISCOVERY, is nested which
supports following parameters (IEEE Std 802.11ai-2016, Annex C.3 MIB
detail):
(1) NL80211_FILS_DISCOVERY_INT_MIN - Minimum packet interval
(2) NL80211_FILS_DISCOVERY_INT_MAX - Maximum packet interval
(3) NL0211_FILS_DISCOVERY_TMPL - Template data

Signed-off-by: Aloka Dixit <alokad@codeaurora.org>
---
v4: Removed attributes unrelated to FILS discovery -
    NL80211_FILS_DISCOVERY_BCN_RESP_WIN, 
    NL80211_FILS_DISCOVERY_OMIT_REPLICATE_PROBE_RESP and
    member named enabled in struct cfg80211_fils_discovery.

 include/net/cfg80211.h       | 19 +++++++++++++++
 include/uapi/linux/nl80211.h | 36 ++++++++++++++++++++++++++++
 net/wireless/nl80211.c       | 46 ++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index fc7e8807838d..64d47463155a 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1047,6 +1047,23 @@ struct cfg80211_acl_data {
 	struct mac_address mac_addrs[];
 };
 
+/**
+ * struct cfg80211_fils_discovery - FILS discovery parameters from
+ * IEEE Std 802.11ai-2016, Annex C.3 MIB detail.
+ *
+ * @min_interval: Minimum packet interval in TUs (0 - 10000)
+ * @max_interval: Maximum packet interval in TUs (0 - 10000)
+ * @tmpl_len: Template length
+ * @tmpl: Template data from IEEE Std 802.11ai-2016 9.6.8.36 FILS discovery
+ *	frame (Figure 9-687a).
+ */
+struct cfg80211_fils_discovery {
+	u32 min_interval;
+	u32 max_interval;
+	size_t tmpl_len;
+	const u8 *tmpl;
+};
+
 /**
  * enum cfg80211_ap_settings_flags - AP settings flags
  *
@@ -1094,6 +1111,7 @@ enum cfg80211_ap_settings_flags {
  * @he_obss_pd: OBSS Packet Detection settings
  * @he_bss_color: BSS Color settings
  * @he_oper: HE operation IE (or %NULL if HE isn't enabled)
+ * @fils_discovery: FILS discovery transmission parameters
  */
 struct cfg80211_ap_settings {
 	struct cfg80211_chan_def chandef;
@@ -1124,6 +1142,7 @@ struct cfg80211_ap_settings {
 	u32 flags;
 	struct ieee80211_he_obss_pd he_obss_pd;
 	struct cfg80211_he_bss_color he_bss_color;
+	struct cfg80211_fils_discovery fils_discovery;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 4e6339ab1fce..adab79c0f907 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2505,6 +2505,10 @@ enum nl80211_commands {
  * @NL80211_ATTR_HE_6GHZ_CAPABILITY: HE 6 GHz Band Capability element (from
  *	association request when used with NL80211_CMD_NEW_STATION).
  *
+ * @NL80211_ATTR_FILS_DISCOVERY: Optional parameter to configure FILS
+ *	discovery. It is a nested attribute, see
+ *	&enum nl80211_fils_discovery_attributes.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2987,6 +2991,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_HE_6GHZ_CAPABILITY,
 
+	NL80211_ATTR_FILS_DISCOVERY,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -6922,4 +6928,34 @@ enum nl80211_iftype_akm_attributes {
 	NL80211_IFTYPE_AKM_ATTR_MAX = __NL80211_IFTYPE_AKM_ATTR_LAST - 1,
 };
 
+/**
+ * enum nl80211_fils_discovery_attributes - FILS discovery configuration
+ * from IEEE Std 802.11ai-2016, Annex C.3 MIB detail.
+ *
+ * @__NL80211_FILS_DISCOVERY_INVALID: Invalid
+ *
+ * @NL80211_FILS_DISCOVERY_INT_MIN: Minimum packet interval (u32, TU).
+ *	Allowed range: 0..10000 (TU = Time Unit)
+ * @NL80211_FILS_DISCOVERY_INT_MAX: Maximum packet interval (u32, TU).
+ *	Allowed range: 0..10000 (TU = Time Unit)
+ * @NL80211_FILS_DISCOVERY_TMPL: Optional FILS discovery template.
+ *	It has contents of IEEE Std 802.11ai-2016 9.6.8.36 FILS discovery frame
+ *	(Figure 9-687a).
+ *	It may include 6GHz specific data specified in IEEE P802.11ax/D6.0,
+ *	9.6.7.36 FILS Discovery frame format.
+ *
+ * @__NL80211_FILS_DISCOVERY_LAST: Internal
+ * @NL80211_FILS_DISCOVERY_MAX: highest attribute
+ */
+enum nl80211_fils_discovery_attributes {
+	__NL80211_FILS_DISCOVERY_INVALID,
+
+	NL80211_FILS_DISCOVERY_INT_MIN,
+	NL80211_FILS_DISCOVERY_INT_MAX,
+	NL80211_FILS_DISCOVERY_TMPL,
+
+	/* keep last */
+	__NL80211_FILS_DISCOVERY_LAST,
+	NL80211_FILS_DISCOVERY_MAX = __NL80211_FILS_DISCOVERY_LAST - 1
+};
 #endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 263ae395ad44..89f787dc2421 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -360,6 +360,14 @@ nl80211_tid_config_attr_policy[NL80211_TID_CONFIG_ATTR_MAX + 1] = {
 			NLA_POLICY_NESTED(nl80211_txattr_policy),
 };
 
+static const struct nla_policy
+fils_discovery_policy[NL80211_FILS_DISCOVERY_MAX + 1] = {
+	[NL80211_FILS_DISCOVERY_INT_MIN] = NLA_POLICY_MAX(NLA_U32, 10000),
+	[NL80211_FILS_DISCOVERY_INT_MAX] = NLA_POLICY_MAX(NLA_U32, 10000),
+	[NL80211_FILS_DISCOVERY_TMPL] = { .type = NLA_BINARY,
+					  .len = IEEE80211_MAX_DATA_LEN }
+};
+
 static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[0] = { .strict_start_type = NL80211_ATTR_HE_OBSS_PD },
 	[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
@@ -658,6 +666,8 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 		.type = NLA_EXACT_LEN,
 		.len = sizeof(struct ieee80211_he_6ghz_capa),
 	},
+	[NL80211_ATTR_FILS_DISCOVERY] =
+		NLA_POLICY_NESTED(fils_discovery_policy),
 };
 
 /* policy for the key attributes */
@@ -4721,6 +4731,35 @@ static int nl80211_parse_he_bss_color(struct nlattr *attrs,
 	return 0;
 }
 
+static int nl80211_parse_fils_discovery(struct nlattr *attrs,
+					struct cfg80211_ap_settings *params)
+{
+	struct nlattr *tmpl;
+	struct nlattr *tb[NL80211_FILS_DISCOVERY_MAX + 1];
+	int ret;
+	struct cfg80211_fils_discovery *fd = &params->fils_discovery;
+
+	ret = nla_parse_nested(tb, NL80211_FILS_DISCOVERY_MAX, attrs,
+			       fils_discovery_policy, NULL);
+	if (ret)
+		return ret;
+
+	if (!tb[NL80211_FILS_DISCOVERY_INT_MIN] ||
+	    !tb[NL80211_FILS_DISCOVERY_INT_MAX])
+		return -EINVAL;
+
+	fd->min_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MIN]);
+	fd->max_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MAX]);
+
+	tmpl = tb[NL80211_FILS_DISCOVERY_TMPL];
+	if (tmpl) {
+		fd->tmpl = nla_data(tmpl);
+		fd->tmpl_len = nla_len(tmpl);
+	}
+
+	return 0;
+}
+
 static void nl80211_check_ap_rate_selectors(struct cfg80211_ap_settings *params,
 					    const u8 *rates)
 {
@@ -5027,6 +5066,13 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 			return err;
 	}
 
+	if (info->attrs[NL80211_ATTR_FILS_DISCOVERY]) {
+		err = nl80211_parse_fils_discovery(info->attrs[NL80211_ATTR_FILS_DISCOVERY],
+						   &params);
+		if (err)
+			return err;
+	}
+
 	nl80211_calculate_ap_params(&params);
 
 	if (info->attrs[NL80211_ATTR_EXTERNAL_AUTH_SUPPORT])
-- 
2.25.0

[PATCH v4 2/2] mac80211: Add FILS discovery support

[Aloka Dixit]

This patch adds mac80211 support to configure FILS discovery
transmission.
Changes include functions to store and retrieve FILS discovery
template, minimum and maximum packet intervals.

Signed-off-by: Aloka Dixit <alokad@codeaurora.org>
---
v4: Removed attributes unrelated to FILS discovery -
    bcn_resp_win, omit_replicate_probe_resp and
    member named enabled in struct ieee80211_fils_discovery.

 include/net/mac80211.h     | 27 +++++++++++++++++++++++++
 net/mac80211/cfg.c         | 41 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |  7 +++++++
 net/mac80211/tx.c          | 25 +++++++++++++++++++++++
 4 files changed, 100 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 11d5610d2ad5..87d9570497fe 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -317,6 +317,7 @@ struct ieee80211_vif_chanctx_switch {
  * @BSS_CHANGED_TWT: TWT status changed
  * @BSS_CHANGED_HE_OBSS_PD: OBSS Packet Detection status changed.
  * @BSS_CHANGED_HE_BSS_COLOR: BSS Color has changed
+ * @BSS_CHANGED_FILS_DISCOVERY: FILS discovery status changed.
  *
  */
 enum ieee80211_bss_change {
@@ -350,6 +351,7 @@ enum ieee80211_bss_change {
 	BSS_CHANGED_TWT			= 1<<27,
 	BSS_CHANGED_HE_OBSS_PD		= 1<<28,
 	BSS_CHANGED_HE_BSS_COLOR	= 1<<29,
+	BSS_CHANGED_FILS_DISCOVERY      = 1<<30,
 
 	/* when adding here, make sure to change ieee80211_reconfig */
 };
@@ -490,6 +492,18 @@ struct ieee80211_ftm_responder_params {
 	size_t civicloc_len;
 };
 
+/**
+ * struct ieee80211_fils_discovery - FILS discovery parameters from
+ * IEEE Std 802.11ai-2016, Annex C.3 MIB detail.
+ *
+ * @min_interval: Minimum packet interval in TUs (0 - 10000)
+ * @max_interval: Maximum packet interval in TUs (0 - 10000)
+ */
+struct ieee80211_fils_discovery {
+	u32 min_interval;
+	u32 max_interval;
+};
+
 /**
  * struct ieee80211_bss_conf - holds the BSS's changing parameters
  *
@@ -607,6 +621,7 @@ struct ieee80211_ftm_responder_params {
  * @he_oper: HE operation information of the AP we are connected to
  * @he_obss_pd: OBSS Packet Detection parameters.
  * @he_bss_color: BSS coloring settings, if BSS supports HE
+ * @fils_discovery: FILS discovery configuration
  */
 struct ieee80211_bss_conf {
 	const u8 *bssid;
@@ -674,6 +689,7 @@ struct ieee80211_bss_conf {
 	} he_oper;
 	struct ieee80211_he_obss_pd he_obss_pd;
 	struct cfg80211_he_bss_color he_bss_color;
+	struct ieee80211_fils_discovery fils_discovery;
 };
 
 /**
@@ -6558,4 +6574,15 @@ u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw,
  */
 bool ieee80211_set_hw_80211_encap(struct ieee80211_vif *vif, bool enable);
 
+/**
+ * ieee80211_get_fils_discovery_tmpl - Get FILS discovery template.
+ * @hw: pointer obtained from ieee80211_alloc_hw().
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ *
+ * The driver is responsible for freeing the returned skb.
+ *
+ * Return: FILS discovery template. %NULL on error.
+ */
+struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw *hw,
+						  struct ieee80211_vif *vif);
 #endif /* MAC80211_H */
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 9b360544ad6f..dae054b769b5 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -837,6 +837,33 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
+static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata,
+					struct cfg80211_fils_discovery *params)
+{
+	struct fils_discovery_data *new, *old = NULL;
+	struct ieee80211_fils_discovery *fd;
+
+	fd = &sdata->vif.bss_conf.fils_discovery;
+	fd->min_interval = params->min_interval;
+	fd->max_interval = params->max_interval;
+
+	if (!params->tmpl || !params->tmpl_len) /* Optional template */
+		return 0;
+
+	old = sdata_dereference(sdata->u.ap.fils_discovery, sdata);
+	new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+	new->len = params->tmpl_len;
+	memcpy(new->data, params->tmpl, params->tmpl_len);
+	rcu_assign_pointer(sdata->u.ap.fils_discovery, new);
+
+	if (old)
+		kfree_rcu(old, rcu_head);
+
+	return 0;
+}
+
 static int ieee80211_set_ftm_responder_params(
 				struct ieee80211_sub_if_data *sdata,
 				const u8 *lci, size_t lci_len,
@@ -1103,6 +1130,14 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 	}
 	changed |= err;
 
+	err = ieee80211_set_fils_discovery(sdata, &params->fils_discovery);
+	if (err < 0) {
+		ieee80211_vif_release_channel(sdata);
+		return err;
+	} else if (err == 0) {
+		changed |= BSS_CHANGED_FILS_DISCOVERY;
+	}
+
 	err = drv_start_ap(sdata->local, sdata);
 	if (err) {
 		old = sdata_dereference(sdata->u.ap.beacon, sdata);
@@ -1158,6 +1193,7 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 	struct ieee80211_local *local = sdata->local;
 	struct beacon_data *old_beacon;
 	struct probe_resp *old_probe_resp;
+	struct fils_discovery_data *old_fils_discovery;
 	struct cfg80211_chan_def chandef;
 
 	sdata_assert_lock(sdata);
@@ -1166,6 +1202,8 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 	if (!old_beacon)
 		return -ENOENT;
 	old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
+	old_fils_discovery = sdata_dereference(sdata->u.ap.fils_discovery,
+					       sdata);
 
 	/* abort any running channel switch */
 	mutex_lock(&local->mtx);
@@ -1189,9 +1227,12 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 	/* remove beacon and probe response */
 	RCU_INIT_POINTER(sdata->u.ap.beacon, NULL);
 	RCU_INIT_POINTER(sdata->u.ap.probe_resp, NULL);
+	RCU_INIT_POINTER(sdata->u.ap.fils_discovery, NULL);
 	kfree_rcu(old_beacon, rcu_head);
 	if (old_probe_resp)
 		kfree_rcu(old_probe_resp, rcu_head);
+	if (old_fils_discovery)
+		kfree_rcu(old_fils_discovery, rcu_head);
 
 	kfree(sdata->vif.bss_conf.ftmr_params);
 	sdata->vif.bss_conf.ftmr_params = NULL;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index ec1a71ac65f2..479c1691630b 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -272,6 +272,12 @@ struct probe_resp {
 	u8 data[];
 };
 
+struct fils_discovery_data {
+	struct rcu_head rcu_head;
+	int len;
+	u8 data[0];
+};
+
 struct ps_data {
 	/* yes, this looks ugly, but guarantees that we can later use
 	 * bitmap_empty :)
@@ -287,6 +293,7 @@ struct ps_data {
 struct ieee80211_if_ap {
 	struct beacon_data __rcu *beacon;
 	struct probe_resp __rcu *probe_resp;
+	struct fils_discovery_data __rcu *fils_discovery;
 
 	/* to be used after channel switch. */
 	struct cfg80211_beacon_data *next_beacon;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index e9ce658141f5..9c1044782561 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -4998,6 +4998,31 @@ struct sk_buff *ieee80211_proberesp_get(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(ieee80211_proberesp_get);
 
+struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw *hw,
+						  struct ieee80211_vif *vif)
+{
+	struct sk_buff *skb = NULL;
+	struct fils_discovery_data *tmpl = NULL;
+	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+
+	if (sdata->vif.type != NL80211_IFTYPE_AP)
+		return NULL;
+
+	rcu_read_lock();
+	tmpl = rcu_dereference(sdata->u.ap.fils_discovery);
+	if (!tmpl) {
+		rcu_read_unlock();
+		return NULL;
+	}
+
+	skb = dev_alloc_skb(tmpl->len);
+	if (skb)
+		skb_put_data(skb, tmpl->data, tmpl->len);
+	rcu_read_unlock();
+	return skb;
+}
+EXPORT_SYMBOL(ieee80211_get_fils_discovery_tmpl);
+
 struct sk_buff *ieee80211_pspoll_get(struct ieee80211_hw *hw,
 				     struct ieee80211_vif *vif)
 {
-- 
2.25.0

Re: [PATCH v4 1/2] nl80211: Add FILS discovery support

[Johannes Berg]

On Wed, 2020-06-17 at 22:04 -0700, Aloka Dixit wrote:
> + * @NL80211_FILS_DISCOVERY_TMPL: Optional FILS discovery template.
> + *	It has contents of IEEE Std 802.11ai-2016 9.6.8.36 FILS discovery frame
> + *	(Figure 9-687a).

Is that

"It has (contents of ... FILS discovery frame) ..."

or

"It has contents of (... FILS discovery frame) ..."?

I mean, is that with or without headers? The wording doesn't seem
entirely clear to me.

OTOH, if it's with headers, how could it be optional? In fact, either
way, how is it optional?

> +static int nl80211_parse_fils_discovery(struct nlattr *attrs,
> +					struct cfg80211_ap_settings *params)
> +{
> +	struct nlattr *tmpl;
> +	struct nlattr *tb[NL80211_FILS_DISCOVERY_MAX + 1];
> +	int ret;
> +	struct cfg80211_fils_discovery *fd = &params->fils_discovery;
> +
> +	ret = nla_parse_nested(tb, NL80211_FILS_DISCOVERY_MAX, attrs,
> +			       fils_discovery_policy, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (!tb[NL80211_FILS_DISCOVERY_INT_MIN] ||
> +	    !tb[NL80211_FILS_DISCOVERY_INT_MAX])
> +		return -EINVAL;
> +
> +	fd->min_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MIN]);
> +	fd->max_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MAX]);
> +
> +	tmpl = tb[NL80211_FILS_DISCOVERY_TMPL];
> +	if (tmpl) {
> +		fd->tmpl = nla_data(tmpl);
> +		fd->tmpl_len = nla_len(tmpl);

And if it's with headers, it should have some kind of minimum length
too? You've only put a maximum length into the policy.

(Which reminds me I wanted to have an NLA_POLICY_RANGE(NLA_BINARY, min,
max) but haven't done that yet ...)

johannes

Re: [PATCH v4 2/2] mac80211: Add FILS discovery support

[Johannes Berg]

On Wed, 2020-06-17 at 22:04 -0700, Aloka Dixit wrote:

> +++ b/net/mac80211/cfg.c
> @@ -837,6 +837,33 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata,
>  	return 0;
>  }
>  
> +static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata,
> +					struct cfg80211_fils_discovery *params)
> +{
> +	struct fils_discovery_data *new, *old = NULL;
> +	struct ieee80211_fils_discovery *fd;
> +
> +	fd = &sdata->vif.bss_conf.fils_discovery;
> +	fd->min_interval = params->min_interval;
> +	fd->max_interval = params->max_interval;
> +
> +	if (!params->tmpl || !params->tmpl_len) /* Optional template */
> +		return 0;

Now I'm even more confused. If the template is optional, then if it's
not given it doesn't mean *everything* should be ignored, does it?

What would be the point of that? OTOH, if the template isn't there, what
would you do?

But it still doesn't make sense - if no template means you shouldn't do
anything then that doesn't mean the template should be optional, that
just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY
attribute when it doesn't want anything to be done?

So it seems to me that something doesn't match. Either the template is
truly optional and then this shouldn't just return success, or the
template isn't actually optional?

> +	err = ieee80211_set_fils_discovery(sdata, &params->fils_discovery);
> +	if (err < 0) {
> +		ieee80211_vif_release_channel(sdata);
> +		return err;

Is there no goto label for this error case?

> +struct fils_discovery_data {
> +	struct rcu_head rcu_head;
> +	int len;
> +	u8 data[0];

please use [] not [0].

> +struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw *hw,
> +						  struct ieee80211_vif *vif)
> +{
> +	struct sk_buff *skb = NULL;
> +	struct fils_discovery_data *tmpl = NULL;
> +	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> +
> +	if (sdata->vif.type != NL80211_IFTYPE_AP)
> +		return NULL;
> +
> +	rcu_read_lock();
> +	tmpl = rcu_dereference(sdata->u.ap.fils_discovery);
> +	if (!tmpl) {
> +		rcu_read_unlock();
> +		return NULL;
> +	}
> +
> +	skb = dev_alloc_skb(tmpl->len);
> +	if (skb)
> +		skb_put_data(skb, tmpl->data, tmpl->len);

You should consider the headroom that the driver may have requested.

johannes

Re: [PATCH v4 2/2] mac80211: Add FILS discovery support

[Aloka Dixit]

On 2020-07-30 07:47, Johannes Berg wrote:
> On Wed, 2020-06-17 at 22:04 -0700, Aloka Dixit wrote:
> 
>> +++ b/net/mac80211/cfg.c
>> @@ -837,6 +837,33 @@ static int ieee80211_set_probe_resp(struct 
>> ieee80211_sub_if_data *sdata,
>>  	return 0;
>>  }
>> 
>> +static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data 
>> *sdata,
>> +					struct cfg80211_fils_discovery *params)
>> +{
>> +	struct fils_discovery_data *new, *old = NULL;
>> +	struct ieee80211_fils_discovery *fd;
>> +
>> +	fd = &sdata->vif.bss_conf.fils_discovery;
>> +	fd->min_interval = params->min_interval;
>> +	fd->max_interval = params->max_interval;
>> +
>> +	if (!params->tmpl || !params->tmpl_len) /* Optional template */
>> +		return 0;
> 
> Now I'm even more confused. If the template is optional, then if it's
> not given it doesn't mean *everything* should be ignored, does it?
> 
> What would be the point of that? OTOH, if the template isn't there, 
> what
> would you do?
> 
> But it still doesn't make sense - if no template means you shouldn't do
> anything then that doesn't mean the template should be optional, that
> just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY
> attribute when it doesn't want anything to be done?
> 
> So it seems to me that something doesn't match. Either the template is
> truly optional and then this shouldn't just return success, or the
> template isn't actually optional?
> 

Everything is not ignored, I set the minimum and maximum interval values 
before checking for the template so that those are accepted even if 
template isn't present.

For 6GHz, template is required, at least for ath11k driver.
But for 2.4GHz and 5GHz FILS discovery transmission is not offloaded to 
FW.

We can make the template mandatory instead and then the respective 
drivers will choose the handling.
Please suggest.

>> +	err = ieee80211_set_fils_discovery(sdata, &params->fils_discovery);
>> +	if (err < 0) {
>> +		ieee80211_vif_release_channel(sdata);
>> +		return err;
> 
> Is there no goto label for this error case?
> 

Existing function doesn't use goto labels for error cases, only return.

>> +struct fils_discovery_data {
>> +	struct rcu_head rcu_head;
>> +	int len;
>> +	u8 data[0];
> 
> please use [] not [0].
> 

Will fix in next version.

>> +struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw 
>> *hw,
>> +						  struct ieee80211_vif *vif)
>> +{
>> +	struct sk_buff *skb = NULL;
>> +	struct fils_discovery_data *tmpl = NULL;
>> +	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
>> +
>> +	if (sdata->vif.type != NL80211_IFTYPE_AP)
>> +		return NULL;
>> +
>> +	rcu_read_lock();
>> +	tmpl = rcu_dereference(sdata->u.ap.fils_discovery);
>> +	if (!tmpl) {
>> +		rcu_read_unlock();
>> +		return NULL;
>> +	}
>> +
>> +	skb = dev_alloc_skb(tmpl->len);
>> +	if (skb)
>> +		skb_put_data(skb, tmpl->data, tmpl->len);
> 
> You should consider the headroom that the driver may have requested.
> 

I didn't understand this point, what would the driver request headroom 
for?
Thanks

> johannes

Re: [PATCH v4 1/2] nl80211: Add FILS discovery support

[Aloka Dixit]

On 2020-07-30 07:43, Johannes Berg wrote:
> On Wed, 2020-06-17 at 22:04 -0700, Aloka Dixit wrote:
>> + * @NL80211_FILS_DISCOVERY_TMPL: Optional FILS discovery template.
>> + *	It has contents of IEEE Std 802.11ai-2016 9.6.8.36 FILS discovery 
>> frame
>> + *	(Figure 9-687a).
> 
> Is that
> 
> "It has (contents of ... FILS discovery frame) ..."
> 
> or
> 
> "It has contents of (... FILS discovery frame) ..."?
> 
> I mean, is that with or without headers? The wording doesn't seem
> entirely clear to me.
> 
> OTOH, if it's with headers, how could it be optional? In fact, either
> way, how is it optional?
> 

Template has management frame headers as well. Will change the wording 
accordingly.
I made the template optional because FILS discovery may or may not be 
offloaded to FW.
Another way would be to make it mandatory here.

>> +static int nl80211_parse_fils_discovery(struct nlattr *attrs,
>> +					struct cfg80211_ap_settings *params)
>> +{
>> +	struct nlattr *tmpl;
>> +	struct nlattr *tb[NL80211_FILS_DISCOVERY_MAX + 1];
>> +	int ret;
>> +	struct cfg80211_fils_discovery *fd = &params->fils_discovery;
>> +
>> +	ret = nla_parse_nested(tb, NL80211_FILS_DISCOVERY_MAX, attrs,
>> +			       fils_discovery_policy, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!tb[NL80211_FILS_DISCOVERY_INT_MIN] ||
>> +	    !tb[NL80211_FILS_DISCOVERY_INT_MAX])
>> +		return -EINVAL;
>> +
>> +	fd->min_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MIN]);
>> +	fd->max_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MAX]);
>> +
>> +	tmpl = tb[NL80211_FILS_DISCOVERY_TMPL];
>> +	if (tmpl) {
>> +		fd->tmpl = nla_data(tmpl);
>> +		fd->tmpl_len = nla_len(tmpl);
> 
> And if it's with headers, it should have some kind of minimum length
> too? You've only put a maximum length into the policy.
> 
> (Which reminds me I wanted to have an NLA_POLICY_RANGE(NLA_BINARY, min,
> max) but haven't done that yet ...)
> 

Yeah, I looked through existing examples for NLA_BINARY, those provide 
only the higher bound for length.
But I can modify it to range once that is added.

> johannes

Re: [PATCH v4 1/2] nl80211: Add FILS discovery support

[Johannes Berg]

On Thu, 2020-07-30 at 14:17 -0700, Aloka Dixit wrote:

> > OTOH, if it's with headers, how could it be optional? In fact, either
> > way, how is it optional?
> > 
> 
> Template has management frame headers as well. Will change the wording 
> accordingly.

OK.

> I made the template optional because FILS discovery may or may not be 
> offloaded to FW.

But how would anyone know? Try without it, and then try again if that
fails? Would it fail? I mean, you also said it was required at least for
6 GHz, so wouldn't userspace be better off always giving it - and then
we should probably make it mandatory so it doesn't fall into the trap?

However - and here that's my ignorance speaking - can it really be
offloaded? I mean, is everything in there completely determined by the
beacon already, and so you have no choice in how to build it? Or how
does that work?

> Yeah, I looked through existing examples for NLA_BINARY, those provide 
> only the higher bound for length.

Yeah, no way to do anything else right now. But you should have a lower
bound in the code, I think.

> But I can modify it to range once that is added.

Later maybe :)

johannes

Re: [PATCH v4 2/2] mac80211: Add FILS discovery support

[Johannes Berg]

On Thu, 2020-07-30 at 14:00 -0700, Aloka Dixit wrote:

> > > +	fd = &sdata->vif.bss_conf.fils_discovery;
> > > +	fd->min_interval = params->min_interval;
> > > +	fd->max_interval = params->max_interval;
> > > +
> > > +	if (!params->tmpl || !params->tmpl_len) /* Optional template */
> > > +		return 0;
> > 
> > Now I'm even more confused. If the template is optional, then if it's
> > not given it doesn't mean *everything* should be ignored, does it?
> > 
> > What would be the point of that? OTOH, if the template isn't there, 
> > what
> > would you do?
> > 
> > But it still doesn't make sense - if no template means you shouldn't do
> > anything then that doesn't mean the template should be optional, that
> > just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY
> > attribute when it doesn't want anything to be done?
> > 
> > So it seems to me that something doesn't match. Either the template is
> > truly optional and then this shouldn't just return success, or the
> > template isn't actually optional?
> > 
> 
> Everything is not ignored, I set the minimum and maximum interval values 
> before checking for the template so that those are accepted even if 
> template isn't present.

Right, oops, missed that.

> For 6GHz, template is required, at least for ath11k driver.
> But for 2.4GHz and 5GHz FILS discovery transmission is not offloaded to 
> FW.

But ... now I'm still confused.

If you *don't* offload it, how will it work? Will it all bubble up to
hostapd and that will send the response? Does that work without any
other changes?

But then what would you need the min/max for? I guess I still don't
understand it... I thought this was a periodic frame anyway like beacon,
so how could you _not_ offload it?

> We can make the template mandatory instead and then the respective 
> drivers will choose the handling.
> Please suggest.

I have no idea ... still trying to understand it.

> > > +	err = ieee80211_set_fils_discovery(sdata, &params->fils_discovery);
> > > +	if (err < 0) {
> > > +		ieee80211_vif_release_channel(sdata);
> > > +		return err;
> > 
> > Is there no goto label for this error case?
> > 
> 
> Existing function doesn't use goto labels for error cases, only return.

Maybe add one? Surely the release_channel() must alraedy exist there
somewhere.

> > > +	skb = dev_alloc_skb(tmpl->len);
> > > +	if (skb)
> > > +		skb_put_data(skb, tmpl->data, tmpl->len);
> > 
> > You should consider the headroom that the driver may have requested.
> > 
> 
> I didn't understand this point, what would the driver request headroom 
> for?

Whatever it wants for ... drivers are allowed request extra headroom
(hw->extra_tx_headroom) and we generally honour that for every skb we
build for the driver.

johannes

Re: [PATCH v4 1/2] nl80211: Add FILS discovery support

[Aloka Dixit]

On 2020-07-30 14:22, Johannes Berg wrote:
> On Thu, 2020-07-30 at 14:17 -0700, Aloka Dixit wrote:
> 
>> > OTOH, if it's with headers, how could it be optional? In fact, either
>> > way, how is it optional?
>> >
>> 
>> Template has management frame headers as well. Will change the wording
>> accordingly.
> 
> OK.
> 
>> I made the template optional because FILS discovery may or may not be
>> offloaded to FW.
> 
> But how would anyone know? Try without it, and then try again if that
> fails? Would it fail? I mean, you also said it was required at least 
> for
> 6 GHz, so wouldn't userspace be better off always giving it - and then
> we should probably make it mandatory so it doesn't fall into the trap?
> 

If the template is not provided, FW keeps sending event to get it.
But as my ath11k driver code is limited to 6GHz, it already throws error 
if template not provided.
Yes, in general it will be better to make it mandatory, I will do it in 
next version.

> However - and here that's my ignorance speaking - can it really be
> offloaded? I mean, is everything in there completely determined by the
> beacon already, and so you have no choice in how to build it? Or how
> does that work?
> 

Yes, the frame parameters are fixed except for the timestamp which FW is 
expected to fill.

>> Yeah, I looked through existing examples for NLA_BINARY, those provide
>> only the higher bound for length.
> 
> Yeah, no way to do anything else right now. But you should have a lower
> bound in the code, I think.
> 

Okay.

>> But I can modify it to range once that is added.
> 
> Later maybe :)
> 
> johannes

Re: [PATCH v4 2/2] mac80211: Add FILS discovery support

[Aloka Dixit]

On 2020-07-30 14:26, Johannes Berg wrote:
> On Thu, 2020-07-30 at 14:00 -0700, Aloka Dixit wrote:
> 
>> > > +	fd = &sdata->vif.bss_conf.fils_discovery;
>> > > +	fd->min_interval = params->min_interval;
>> > > +	fd->max_interval = params->max_interval;
>> > > +
>> > > +	if (!params->tmpl || !params->tmpl_len) /* Optional template */
>> > > +		return 0;
>> >
>> > Now I'm even more confused. If the template is optional, then if it's
>> > not given it doesn't mean *everything* should be ignored, does it?
>> >
>> > What would be the point of that? OTOH, if the template isn't there,
>> > what
>> > would you do?
>> >
>> > But it still doesn't make sense - if no template means you shouldn't do
>> > anything then that doesn't mean the template should be optional, that
>> > just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY
>> > attribute when it doesn't want anything to be done?
>> >
>> > So it seems to me that something doesn't match. Either the template is
>> > truly optional and then this shouldn't just return success, or the
>> > template isn't actually optional?
>> >
>> 
>> Everything is not ignored, I set the minimum and maximum interval 
>> values
>> before checking for the template so that those are accepted even if
>> template isn't present.
> 
> Right, oops, missed that.
> 
>> For 6GHz, template is required, at least for ath11k driver.
>> But for 2.4GHz and 5GHz FILS discovery transmission is not offloaded 
>> to
>> FW.
> 
> But ... now I'm still confused.
> 
> If you *don't* offload it, how will it work? Will it all bubble up to
> hostapd and that will send the response? Does that work without any
> other changes?
> 
> But then what would you need the min/max for? I guess I still don't
> understand it... I thought this was a periodic frame anyway like 
> beacon,
> so how could you _not_ offload it?
> 

Min and max intervals are used to decide if a FILS discovery frame 
should be sent at all when respective timers expires.
Depending on how close that time is to the next beacons, the device may 
just send the beacon instead.

In lower bands, for non-offloaded case, FW will send events asking for 
the frame until it gets one.
Whether that should go all the way to hostapd or should the driver 
itself handle it remains to be seen.
My current focus is only 6GHz, but didn't want to restrict kernel 
implementation so moved 6GHz related checks to the driver instead.

All in all, making the template mandatory will be safer so that the 
driver will always have one if required.

>> We can make the template mandatory instead and then the respective
>> drivers will choose the handling.
>> Please suggest.
> 
> I have no idea ... still trying to understand it.
> 
>> > > +	err = ieee80211_set_fils_discovery(sdata, &params->fils_discovery);
>> > > +	if (err < 0) {
>> > > +		ieee80211_vif_release_channel(sdata);
>> > > +		return err;
>> >
>> > Is there no goto label for this error case?
>> >
>> 
>> Existing function doesn't use goto labels for error cases, only 
>> return.
> 
> Maybe add one? Surely the release_channel() must alraedy exist there
> somewhere.
> 

Okay.

>> > > +	skb = dev_alloc_skb(tmpl->len);
>> > > +	if (skb)
>> > > +		skb_put_data(skb, tmpl->data, tmpl->len);
>> >
>> > You should consider the headroom that the driver may have requested.
>> >
>> 
>> I didn't understand this point, what would the driver request headroom
>> for?
> 
> Whatever it wants for ... drivers are allowed request extra headroom
> (hw->extra_tx_headroom) and we generally honour that for every skb we
> build for the driver.
> 

I will look into other instances to understand this requirement.

> johannes

Re: [PATCH v4 2/2] mac80211: Add FILS discovery support

[Johannes Berg]

On Thu, 2020-07-30 at 15:08 -0700, Aloka Dixit wrote:

> Min and max intervals are used to decide if a FILS discovery frame 
> should be sent at all when respective timers expires.
> Depending on how close that time is to the next beacons, the device may 
> just send the beacon instead.

Right, OK.

> In lower bands, for non-offloaded case, FW will send events asking for 
> the frame until it gets one.

Aha.

> Whether that should go all the way to hostapd or should the driver 
> itself handle it remains to be seen.

I don't see why it should go up - the driver can have the template and
answer that? I mean, we already push the template down.

> My current focus is only 6GHz, but didn't want to restrict kernel 
> implementation so moved 6GHz related checks to the driver instead.

It might still make sense to have a bitmap of where FILS discovery is
supported, if it's different for different bands?

Unless of course you think that a given device will always support FILS
discovery on all bands, you just haven't implemented it yet for all
bands - but will complete it before really enabling it in the driver?

> All in all, making the template mandatory will be safer so that the 
> driver will always have one if required.

Agree.

Thanks!

johannes