[PATCH 0/2] Commands for FILS discovery and unsolicited probe response

[PATCH 0/2] Commands for FILS discovery and unsolicited probe response

[Aloka Dixit]

This patchset removes the dependency of FILS discovery and unsolicited
probe response configurations on beacon setup command and instead adds new
netlink commands which allow setting parameters dynamically.

Aloka Dixit (2):
  nl80211: Commands for FILS discovery and unsolicited broadcast probe
    response
  mac80211: Commands for FILS discovery and unsolicited broadcast probe
    response

 include/net/cfg80211.h       |  13 +++--
 include/uapi/linux/nl80211.h |  12 +++++
 net/mac80211/cfg.c           |  33 +++++-------
 net/wireless/nl80211.c       | 102 ++++++++++++++++++++---------------
 net/wireless/rdev-ops.h      |  24 +++++++++
 net/wireless/trace.h         |  43 +++++++++++++++
 6 files changed, 159 insertions(+), 68 deletions(-)


base-commit: 0ae5b43d6dde6003070106e97cd0d41bace2eeb2
-- 
2.25.0

[PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

[Aloka Dixit]

This change moves the FILS discovery and unsolicited probe response
configurations to new netlink commands instead of during beacon set up.
It allows modification of transmission parameters such as time interval
dynamically.

Signed-off-by: Aloka Dixit <alokad@codeaurora.org>
---
 include/net/cfg80211.h       |  13 +++--
 include/uapi/linux/nl80211.h |  12 +++++
 net/wireless/nl80211.c       | 102 ++++++++++++++++++++---------------
 net/wireless/rdev-ops.h      |  24 +++++++++
 net/wireless/trace.h         |  43 +++++++++++++++
 5 files changed, 147 insertions(+), 47 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9a4bbccddc7f..b88d7e69a1e4 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1179,8 +1179,6 @@ 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
- * @unsol_bcast_probe_resp: Unsolicited broadcast probe response parameters
  */
 struct cfg80211_ap_settings {
 	struct cfg80211_chan_def chandef;
@@ -1211,8 +1209,6 @@ 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;
-	struct cfg80211_unsol_bcast_probe_resp unsol_bcast_probe_resp;
 };
 
 /**
@@ -3972,6 +3968,9 @@ struct mgmt_frame_regs {
  *	This callback may sleep.
  * @reset_tid_config: Reset TID specific configuration for the peer, for the
  *	given TIDs. This callback may sleep.
+ * @set_fils_discovery: Set FILS discovery transmission parameters.
+ * @set_unsol_bcast_probe_resp: Set unsolicited broadcast probe response
+ *	transmission parameters.
  */
 struct cfg80211_ops {
 	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -4299,6 +4298,12 @@ struct cfg80211_ops {
 				    const u8 *peer, u8 tids);
 	int	(*set_sar_specs)(struct wiphy *wiphy,
 				 struct cfg80211_sar_specs *sar);
+	int     (*set_fils_discovery)(struct wiphy *wiphy,
+				      struct net_device *dev,
+				      struct cfg80211_fils_discovery *params);
+	int	(*set_unsol_bcast_probe_resp)(struct wiphy *wiphy,
+					      struct net_device *dev,
+					      struct cfg80211_unsol_bcast_probe_resp *params);
 };
 
 /*
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 40832d13c2f1..f78ba0e7e9a1 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1182,6 +1182,14 @@
  *	passed using %NL80211_ATTR_SAR_SPEC. %NL80211_ATTR_WIPHY is used to
  *	specify the wiphy index to be applied to.
  *
+ * @NL80211_CMD_SET_FILS_DISCOVERY: Set FILS discovery transmission parameters.
+ *	Inputs to the command are provided using the attribute
+ *	%NL80211_ATTR_FILS_DISCOVERY.
+ *
+ * @NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP: Set unsolicited broadcast
+ *	probe response transmission parameters. Inputs to the command are
+ *	provided using the attribute %NL80211_ATTR_UNSOL_BCAST_PROBE_RESP.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -1414,6 +1422,10 @@ enum nl80211_commands {
 
 	NL80211_CMD_SET_SAR_SPECS,
 
+	NL80211_CMD_SET_FILS_DISCOVERY,
+
+	NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 775d0c4d86c3..c2689642e9be 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -387,9 +387,10 @@ static const struct nla_policy
 nl80211_fils_discovery_policy[NL80211_FILS_DISCOVERY_ATTR_MAX + 1] = {
 	[NL80211_FILS_DISCOVERY_ATTR_INT_MIN] = NLA_POLICY_MAX(NLA_U32, 10000),
 	[NL80211_FILS_DISCOVERY_ATTR_INT_MAX] = NLA_POLICY_MAX(NLA_U32, 10000),
-	NLA_POLICY_RANGE(NLA_BINARY,
-			 NL80211_FILS_DISCOVERY_TMPL_MIN_LEN,
-			 IEEE80211_MAX_DATA_LEN),
+	[NL80211_FILS_DISCOVERY_ATTR_TMPL] =
+			NLA_POLICY_RANGE(NLA_BINARY,
+					 NL80211_FILS_DISCOVERY_TMPL_MIN_LEN,
+					 IEEE80211_MAX_DATA_LEN),
 };
 
 static const struct nla_policy
@@ -5015,19 +5016,24 @@ static int nl80211_parse_he_bss_color(struct nlattr *attrs,
 	return 0;
 }
 
-static int nl80211_parse_fils_discovery(struct cfg80211_registered_device *rdev,
-					struct nlattr *attrs,
-					struct cfg80211_ap_settings *params)
+static int nl80211_set_fils_discovery(struct sk_buff *skb,
+				      struct genl_info *info)
 {
 	struct nlattr *tb[NL80211_FILS_DISCOVERY_ATTR_MAX + 1];
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct net_device *dev = info->user_ptr[1];
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	struct cfg80211_fils_discovery fd = { 0 };
 	int ret;
-	struct cfg80211_fils_discovery *fd = &params->fils_discovery;
 
-	if (!wiphy_ext_feature_isset(&rdev->wiphy,
-				     NL80211_EXT_FEATURE_FILS_DISCOVERY))
-		return -EINVAL;
+	if (!rdev->ops->set_fils_discovery ||
+	    !wiphy_ext_feature_isset(&rdev->wiphy,
+				     NL80211_EXT_FEATURE_FILS_DISCOVERY)) {
+		return -EOPNOTSUPP;
+	}
 
-	ret = nla_parse_nested(tb, NL80211_FILS_DISCOVERY_ATTR_MAX, attrs,
+	ret = nla_parse_nested(tb, NL80211_FILS_DISCOVERY_ATTR_MAX,
+			       info->attrs[NL80211_ATTR_FILS_DISCOVERY],
 			       NULL, NULL);
 	if (ret)
 		return ret;
@@ -5037,30 +5043,35 @@ static int nl80211_parse_fils_discovery(struct cfg80211_registered_device *rdev,
 	    !tb[NL80211_FILS_DISCOVERY_ATTR_TMPL])
 		return -EINVAL;
 
-	fd->tmpl_len = nla_len(tb[NL80211_FILS_DISCOVERY_ATTR_TMPL]);
-	fd->tmpl = nla_data(tb[NL80211_FILS_DISCOVERY_ATTR_TMPL]);
-	fd->min_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_ATTR_INT_MIN]);
-	fd->max_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_ATTR_INT_MAX]);
+	fd.tmpl_len = nla_len(tb[NL80211_FILS_DISCOVERY_ATTR_TMPL]);
+	fd.tmpl = nla_data(tb[NL80211_FILS_DISCOVERY_ATTR_TMPL]);
+	fd.min_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_ATTR_INT_MIN]);
+	fd.max_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_ATTR_INT_MAX]);
 
-	return 0;
+	wdev_lock(wdev);
+	ret = rdev_set_fils_discovery(rdev, dev, &fd);
+	wdev_unlock(wdev);
+	return ret;
 }
 
 static int
-nl80211_parse_unsol_bcast_probe_resp(struct cfg80211_registered_device *rdev,
-				     struct nlattr *attrs,
-				     struct cfg80211_ap_settings *params)
+nl80211_set_unsol_bcast_probe_resp(struct sk_buff *skb, struct genl_info *info)
 {
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct net_device *dev = info->user_ptr[1];
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	struct cfg80211_unsol_bcast_probe_resp params = { 0 };
 	struct nlattr *tb[NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_MAX + 1];
 	int ret;
-	struct cfg80211_unsol_bcast_probe_resp *presp =
-					&params->unsol_bcast_probe_resp;
 
-	if (!wiphy_ext_feature_isset(&rdev->wiphy,
+	if (!rdev->ops->set_unsol_bcast_probe_resp ||
+	    !wiphy_ext_feature_isset(&rdev->wiphy,
 				     NL80211_EXT_FEATURE_UNSOL_BCAST_PROBE_RESP))
-		return -EINVAL;
+		return -EOPNOTSUPP;
 
 	ret = nla_parse_nested(tb, NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_MAX,
-			       attrs, NULL, NULL);
+			       info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP],
+			       NULL, NULL);
 	if (ret)
 		return ret;
 
@@ -5068,9 +5079,14 @@ nl80211_parse_unsol_bcast_probe_resp(struct cfg80211_registered_device *rdev,
 	    !tb[NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_TMPL])
 		return -EINVAL;
 
-	presp->tmpl = nla_data(tb[NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_TMPL]);
-	presp->tmpl_len = nla_len(tb[NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_TMPL]);
-	presp->interval = nla_get_u32(tb[NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_INT]);
+	params.tmpl = nla_data(tb[NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_TMPL]);
+	params.tmpl_len = nla_len(tb[NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_TMPL]);
+	params.interval = nla_get_u32(tb[NL80211_UNSOL_BCAST_PROBE_RESP_ATTR_INT]);
+
+	wdev_lock(wdev);
+	ret = rdev_set_unsol_bcast_probe_resp(rdev, dev, &params);
+	wdev_unlock(wdev);
+
 	return 0;
 }
 
@@ -5384,22 +5400,6 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 			goto out;
 	}
 
-	if (info->attrs[NL80211_ATTR_FILS_DISCOVERY]) {
-		err = nl80211_parse_fils_discovery(rdev,
-						   info->attrs[NL80211_ATTR_FILS_DISCOVERY],
-						   &params);
-		if (err)
-			goto out;
-	}
-
-	if (info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP]) {
-		err = nl80211_parse_unsol_bcast_probe_resp(
-			rdev, info->attrs[NL80211_ATTR_UNSOL_BCAST_PROBE_RESP],
-			&params);
-		if (err)
-			return err;
-	}
-
 	nl80211_calculate_ap_params(&params);
 
 	if (info->attrs[NL80211_ATTR_EXTERNAL_AUTH_SUPPORT])
@@ -15704,6 +15704,22 @@ static const struct genl_small_ops nl80211_small_ops[] = {
 		.internal_flags = NL80211_FLAG_NEED_WIPHY |
 				  NL80211_FLAG_NEED_RTNL,
 	},
+	{
+		.cmd = NL80211_CMD_SET_FILS_DISCOVERY,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = nl80211_set_fils_discovery,
+		.flags = GENL_UNS_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_NEED_RTNL,
+	},
+	{
+		.cmd = NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = nl80211_set_unsol_bcast_probe_resp,
+		.flags = GENL_UNS_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_NEED_RTNL,
+	},
 };
 
 static struct genl_family nl80211_fam __ro_after_init = {
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 8b1358d04ca2..ed9bfbebe695 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1358,4 +1358,28 @@ static inline int rdev_set_sar_specs(struct cfg80211_registered_device *rdev,
 	return ret;
 }
 
+static inline int rdev_set_fils_discovery(struct cfg80211_registered_device *rdev,
+					  struct net_device *dev,
+					  struct cfg80211_fils_discovery *params)
+{
+	int ret;
+
+	trace_rdev_set_fils_discovery(&rdev->wiphy, dev, params);
+	ret = rdev->ops->set_fils_discovery(&rdev->wiphy, dev, params);
+	trace_rdev_return_int(&rdev->wiphy, ret);
+	return ret;
+}
+
+static inline int rdev_set_unsol_bcast_probe_resp(struct cfg80211_registered_device *rdev,
+						  struct net_device *dev,
+						  struct cfg80211_unsol_bcast_probe_resp *params)
+{
+	int ret;
+
+	trace_rdev_set_unsol_bcast_probe_resp(&rdev->wiphy, dev, params);
+	ret = rdev->ops->set_unsol_bcast_probe_resp(&rdev->wiphy, dev, params);
+	trace_rdev_return_int(&rdev->wiphy, ret);
+	return ret;
+}
+
 #endif /* __CFG80211_RDEV_OPS */
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 76b777d5903f..9640e0d3f81e 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -3565,6 +3565,49 @@ TRACE_EVENT(rdev_set_sar_specs,
 		  WIPHY_PR_ARG, __entry->type, __entry->num)
 );
 
+TRACE_EVENT(rdev_set_fils_discovery,
+	    TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+		     struct cfg80211_fils_discovery *params),
+	    TP_ARGS(wiphy, netdev, params),
+	    TP_STRUCT__entry(WIPHY_ENTRY
+			     NETDEV_ENTRY
+			     __field(u32, min_interval)
+			     __field(u32, max_interval)
+			     __field(size_t, tmpl_len)
+	    ),
+	    TP_fast_assign(WIPHY_ASSIGN;
+			   NETDEV_ASSIGN;
+			   __entry->min_interval = params->min_interval;
+			   __entry->max_interval = params->max_interval;
+			   __entry->tmpl_len = params->tmpl_len;
+	    ),
+	    TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT
+		      ", minimum interval: %u, maximum interval: %u, template length: %zu",
+		      WIPHY_PR_ARG, NETDEV_PR_ARG,
+		      __entry->min_interval, __entry->max_interval,
+		      __entry->tmpl_len)
+);
+
+TRACE_EVENT(rdev_set_unsol_bcast_probe_resp,
+	    TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+		     struct cfg80211_unsol_bcast_probe_resp *params),
+	    TP_ARGS(wiphy, netdev, params),
+	    TP_STRUCT__entry(WIPHY_ENTRY
+			     NETDEV_ENTRY
+			     __field(u32, interval)
+			     __field(size_t, tmpl_len)
+	    ),
+	    TP_fast_assign(WIPHY_ASSIGN;
+			   NETDEV_ASSIGN;
+			   __entry->interval = params->interval;
+			   __entry->tmpl_len = params->tmpl_len;
+	    ),
+	    TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT
+		      ", interval: %u, template length: %zu",
+		      WIPHY_PR_ARG, NETDEV_PR_ARG,
+		      __entry->interval, __entry->tmpl_len)
+);
+
 #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
-- 
2.25.0

[PATCH 2/2] mac80211: Commands for FILS discovery and unsolicited broadcast probe response

[Aloka Dixit]

This patch registers ieee80211_set_fils_discovery() and
ieee80211_set_unsol_bcast_probe_resp() as callback functions with
mac80211_config_ops() to be used for configuring FILS discovery
transmission.

Changes also include new calls to ieee80211_bss_info_change_notify()
which were not needed earlier as configurations were completed through
ieee80211_start_ap().

Signed-off-by: Aloka Dixit <alokad@codeaurora.org>
---
 net/mac80211/cfg.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index c4c70e30ad7f..27c97fd88e76 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -839,11 +839,13 @@ 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,
+static int ieee80211_set_fils_discovery(struct wiphy *wiphy,
+					struct net_device *dev,
 					struct cfg80211_fils_discovery *params)
 {
 	struct fils_discovery_data *new, *old = NULL;
 	struct ieee80211_fils_discovery *fd;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
 	if (!params->tmpl || !params->tmpl_len)
 		return -EINVAL;
@@ -863,18 +865,23 @@ static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata,
 	if (old)
 		kfree_rcu(old, rcu_head);
 
+	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_FILS_DISCOVERY);
 	return 0;
 }
 
 static int
-ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata,
+ieee80211_set_unsol_bcast_probe_resp(struct wiphy *wiphy,
+				     struct net_device *dev,
 				     struct cfg80211_unsol_bcast_probe_resp *params)
 {
 	struct unsol_bcast_probe_resp_data *new, *old = NULL;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
 	if (!params->tmpl || !params->tmpl_len)
 		return -EINVAL;
 
+	sdata->vif.bss_conf.unsol_bcast_probe_resp_interval = params->interval;
+
 	old = sdata_dereference(sdata->u.ap.unsol_bcast_probe_resp, sdata);
 	new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
 	if (!new)
@@ -886,9 +893,7 @@ ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata,
 	if (old)
 		kfree_rcu(old, rcu_head);
 
-	sdata->vif.bss_conf.unsol_bcast_probe_resp_interval =
-							params->interval;
-
+	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_UNSOL_BCAST_PROBE_RESP);
 	return 0;
 }
 
@@ -1161,22 +1166,6 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 		goto error;
 	changed |= err;
 
-	if (params->fils_discovery.max_interval) {
-		err = ieee80211_set_fils_discovery(sdata,
-						   &params->fils_discovery);
-		if (err < 0)
-			goto error;
-		changed |= BSS_CHANGED_FILS_DISCOVERY;
-	}
-
-	if (params->unsol_bcast_probe_resp.interval) {
-		err = ieee80211_set_unsol_bcast_probe_resp(sdata,
-							   &params->unsol_bcast_probe_resp);
-		if (err < 0)
-			goto error;
-		changed |= BSS_CHANGED_UNSOL_BCAST_PROBE_RESP;
-	}
-
 	err = drv_start_ap(sdata->local, sdata);
 	if (err) {
 		old = sdata_dereference(sdata->u.ap.beacon, sdata);
@@ -4187,4 +4176,6 @@ const struct cfg80211_ops mac80211_config_ops = {
 	.set_tid_config = ieee80211_set_tid_config,
 	.reset_tid_config = ieee80211_reset_tid_config,
 	.set_sar_specs = ieee80211_set_sar_specs,
+	.set_fils_discovery = ieee80211_set_fils_discovery,
+	.set_unsol_bcast_probe_resp = ieee80211_set_unsol_bcast_probe_resp,
 };
-- 
2.25.0

Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On 20-01-2021 01:52, Aloka Dixit wrote:
> This change moves the FILS discovery and unsolicited probe response
> configurations to new netlink commands instead of during beacon set up.
> It allows modification of transmission parameters such as time interval
> dynamically.

Why not have both, ie. support initial setup through START_AP and allow 
modifications with new command or an existing one. Maybe we should 
consolidate to what has been discussed in thread about AP power save [1].

Regards,
Arend

[1] 
https://patchwork.kernel.org/project/linux-wireless/patch/1598257589-19091-1-git-send-email-vnaralas@codeaurora.org/

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4176 bytes --]

Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

[Aloka Dixit]

On 2021-01-20 02:10, Arend van Spriel wrote:
> On 20-01-2021 01:52, Aloka Dixit wrote:
>> This change moves the FILS discovery and unsolicited probe response
>> configurations to new netlink commands instead of during beacon set 
>> up.
>> It allows modification of transmission parameters such as time 
>> interval
>> dynamically.
> 
> Why not have both, ie. support initial setup through START_AP and
> allow modifications with new command or an existing one. Maybe we
> should consolidate to what has been discussed in thread about AP power
> save [1].
> 
> Regards,
> Arend
> 
> [1]
> https://patchwork.kernel.org/project/linux-wireless/patch/1598257589-19091-1-git-send-email-vnaralas@codeaurora.org/

Hi Arend,
FILS discovery and especially unsolicited probe response templates are 
big. Sometimes send_and_recv() returns error due to memory 
unavailability during wpa_driver_nl80211_set_ap() depending on how many 
interfaces, which elements are added. Moving these to separate commands 
resolves this issue along with more control over the time interval 
during run-time.
Thanks.

Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

[Johannes Berg]

On Mon, 2021-01-25 at 13:52 -0800, Aloka Dixit wrote:
> 
> FILS discovery and especially unsolicited probe response templates are 
> big. Sometimes send_and_recv() returns error due to memory 
> unavailability during wpa_driver_nl80211_set_ap() depending on how many 
> interfaces, which elements are added. 

What? Where do you get errors from? Netlink even supports vmalloc now, I
believe, so the kernel really shouldn't care?

> Moving these to separate commands 
> resolves this issue along with more control over the time interval 
> during run-time.

I tend to agree with Arend though, we have NL80211_CMD_SET_BEACON and
since NL80211_CMD_NEW_BEACON was renamed to NL80211_CMD_START_AP to more
accurately say what it does, I think generalizing "AP modifications" by
renaming NL80211_CMD_SET_BEACON to NL80211_CMD_UPDATE_AP (or such) would
make a lot of sense.

Let's not conflate the two issues here.

1) memory issues - need to understand better

2) update is needed - I'd say SET_BEACON/UPDATE_AP would be a better way
   than pulling everything into separate commands. Updates can be
   partial too, after all, if you include only the changed attributes,
   and that might even address case 1? I.e. why wouldn't userspace be
   able to do UPDATE_AP multiple times, just like with your patch it
   would do NL80211_CMD_SET_FILS_DISCOVERY
   and NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP?


Also, technically this constitutes an API break. One that perhaps nobody
cares about yet, but surely somebody already has hostapd versions that
use NL80211_ATTR_FILS_DISCOVERY or NL80211_ATTR_UNSOL_BCAST_PROBE_RESP?
After all, you don't want to tell me you never tested this code ;-)

johannes

Re: [PATCH 1/2] nl80211: Commands for FILS discovery and unsolicited broadcast probe response

[Aloka Dixit]

On 2021-02-12 01:33, Johannes Berg wrote:
> On Mon, 2021-01-25 at 13:52 -0800, Aloka Dixit wrote:
>> 
>> FILS discovery and especially unsolicited probe response templates are
>> big. Sometimes send_and_recv() returns error due to memory
>> unavailability during wpa_driver_nl80211_set_ap() depending on how 
>> many
>> interfaces, which elements are added.
> 
> What? Where do you get errors from? Netlink even supports vmalloc now, 
> I
> believe, so the kernel really shouldn't care?
> 
The error was shown on hostapd side during attribute building when
16 interfaces were added on one radio and unsolicited broadcast
probe response feature was enabled.
It was resolved with a separate command for this feature instead of
being a part of NL80211_CMD_START_AP.

>> Moving these to separate commands
>> resolves this issue along with more control over the time interval
>> during run-time.
> 
> I tend to agree with Arend though, we have NL80211_CMD_SET_BEACON and
> since NL80211_CMD_NEW_BEACON was renamed to NL80211_CMD_START_AP to 
> more
> accurately say what it does, I think generalizing "AP modifications" by
> renaming NL80211_CMD_SET_BEACON to NL80211_CMD_UPDATE_AP (or such) 
> would
> make a lot of sense.
> 
> Let's not conflate the two issues here.
> 
> 1) memory issues - need to understand better
> 
> 2) update is needed - I'd say SET_BEACON/UPDATE_AP would be a better 
> way
>    than pulling everything into separate commands. Updates can be
>    partial too, after all, if you include only the changed attributes,
>    and that might even address case 1? I.e. why wouldn't userspace be
>    able to do UPDATE_AP multiple times, just like with your patch it
>    would do NL80211_CMD_SET_FILS_DISCOVERY
>    and NL80211_CMD_SET_UNSOL_BCAST_PROBE_RESP?
> 
> 
> Also, technically this constitutes an API break. One that perhaps 
> nobody
> cares about yet, but surely somebody already has hostapd versions that
> use NL80211_ATTR_FILS_DISCOVERY or NL80211_ATTR_UNSOL_BCAST_PROBE_RESP?
> After all, you don't want to tell me you never tested this code ;-)
> 
> johannes

I myself added the hostapd changes corresponding to the original kernel 
code,
This patch-set doesn't remove these attributes so, if accepted,
current hostapd calls will become no-op but won't break any other
functionality until I send the next version for separate commands for 
hostapd.

On a different note, a line was missed in nl80211_fils_discovery_policy
definition from my side in the original change so included it in this
patch-set.
Should I send a separate patch for that while we discuss regarding the 
separate
command?
Thanks.