[PATCH v8] P2P find phase offload

[PATCH v8] P2P find phase offload

[Vladimir Kondratiev]

- Fix kerneldoc for start_p2p_find/stop_p2p_find (forgotten '@')
- rebase

Vladimir Kondratiev (1):
  cfg80211: P2P find phase offload

 include/net/cfg80211.h       |  65 ++++++++++++++++
 include/uapi/linux/nl80211.h |  13 ++++
 net/wireless/nl80211.c       | 175 +++++++++++++++++++++++++++++++++++++++++++
 net/wireless/rdev-ops.h      |  19 +++++
 net/wireless/trace.h         |  42 +++++++++++
 5 files changed, 314 insertions(+)

-- 
1.8.1.2

[PATCH v8] cfg80211: P2P find phase offload

[Vladimir Kondratiev]

Allow to implement P2P find phase in the driver/firmware.

Offload scheme designed as follows:

- Driver provide methods start_p2p_find and stop_p2p_find in the cfg80211_ops;
- Driver indicate firmware or driver responds to the probe requests by setting
  feature NL80211_FEATURE_P2P_PROBE_RESP_OFFLOAD
- wpa_supplicant analyses methods supported to discover p2p offload support;
- wpa_supplicant analyses feature flags to discover p2p probe response
  offload support;
to perform p2p scan, wpa_supplicant:
- perform legacy scan, through driver's cfg80211_ops 'scan' method
- configure rx management filter to get probe-request and probe-response frames
- start p2p find via driver's cfg80211_ops start_p2p_find method
- driver start p2p find with hardware and notify wpa_supplicant with
  cfg80211_p2p_find_notify_start()
- driver/firmware toggle search/listen states. Received probe-request and
  probe-response frames passed to the wpa_supplicant via cfg80211_rx_mgmt
- when wpa_supplicant wants to stop p2p find, it calls driver's
  cfg80211_ops stop_p2p_find method. Alternatively, driver/firmware may decide
  to stop p2p find. In all cases, driver notifies wpa_supplicant using
  cfg80211_p2p_find_notify_end()

All driver to user space communication done through nl80211 layer.

Offloaded P2P find does not support variations like progressive scan.

Signed-off-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
---
 include/net/cfg80211.h       |  65 ++++++++++++++++
 include/uapi/linux/nl80211.h |  13 ++++
 net/wireless/nl80211.c       | 175 +++++++++++++++++++++++++++++++++++++++++++
 net/wireless/rdev-ops.h      |  19 +++++
 net/wireless/trace.h         |  42 +++++++++++
 5 files changed, 314 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 6dd1959..ae6f433 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1799,6 +1799,34 @@ struct cfg80211_update_ft_ies_params {
 };
 
 /**
+ * struct cfg80211_p2p_find_params - parameters for P2P find
+ * @probe_ie: extra IE's for probe frames
+ * @probe_ie_len: length, bytes, of @probe_ie
+ * @probe_resp_ie: extra IE's for probe response frames
+ * @probe_resp_ie_len: length, bytes, of @probe_resp_ie
+ *	Driver/firmware may add additional IE's as well as modify
+ *	provided ones; typical IE's to be added are
+ *	WLAN_EID_EXT_SUPP_RATES, WLAN_EID_DS_PARAMS,
+ *	WLAN_EID_HT_CAPABILITY.
+ * @min_discoverable_interval: and
+ * @max_discoverable_interval: min/max for random multiplier of 100TU's
+ *	for the listen state duration
+ * @n_channels: number of channels to operate on
+ * @channels: channels to operate on
+ */
+struct cfg80211_p2p_find_params {
+	const u8 *probe_ie;
+	size_t probe_ie_len;
+	const u8 *probe_resp_ie;
+	size_t probe_resp_ie_len;
+	u32 min_discoverable_interval;
+	u32 max_discoverable_interval;
+
+	int n_channels;
+	struct ieee80211_channel **channels;
+};
+
+/**
  * struct cfg80211_ops - backend description for wireless configuration
  *
  * This struct is registered by fullmac card drivers and/or wireless stacks
@@ -2037,6 +2065,17 @@ struct cfg80211_update_ft_ies_params {
  *	driver can take the most appropriate actions.
  * @crit_proto_stop: Indicates critical protocol no longer needs increased link
  *	reliability. This operation can not fail.
+ *
+ * @start_p2p_find: start P2P find phase
+ *	Parameters include IEs for probe/probe-resp frames;
+ *	and channels to operate on.
+ *	Parameters are not retained after call, driver need to copy data if
+ *	it need it later.
+ *	P2P find can't run concurrently with ROC or scan, and driver should
+ *	check this.
+ * @stop_p2p_find: stop P2P find phase
+ *	After stopping p2p find, driver should call
+ *	cfg80211_p2p_find_notify_end() to inform upper layers
  */
 struct cfg80211_ops {
 	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -2272,6 +2311,12 @@ struct cfg80211_ops {
 				    u16 duration);
 	void	(*crit_proto_stop)(struct wiphy *wiphy,
 				   struct wireless_dev *wdev);
+
+	int	(*start_p2p_find)(struct wiphy *wiphy,
+				  struct wireless_dev *wdev,
+				  struct cfg80211_p2p_find_params *params);
+	void	(*stop_p2p_find)(struct wiphy *wiphy,
+				 struct wireless_dev *wdev);
 };
 
 /*
@@ -4185,6 +4230,26 @@ void cfg80211_report_wowlan_wakeup(struct wireless_dev *wdev,
  */
 void cfg80211_crit_proto_stopped(struct wireless_dev *wdev, gfp_t gfp);
 
+/**
+ * cfg80211_p2p_find_notify_start - report p2p find phase started
+ * @wdev: the wireless device reporting the event
+ * @gfp: allocation flags
+ */
+void cfg80211_p2p_find_notify_start(struct wireless_dev *wdev, gfp_t gfp);
+
+/**
+ * cfg80211_p2p_find_notify_end - report p2p find phase ended
+ * @wdev: the wireless device reporting the event
+ * @gfp: allocation flags
+ *
+ * p2p find phase may be ended either unsolicited or in response to
+ * ops->stop_p2p_find
+ *
+ * In any case, if @start_p2p_find from driver's struct cfg80211_ops called,
+ * @cfg80211_p2p_find_notify_end should be eventually called
+ */
+void cfg80211_p2p_find_notify_end(struct wireless_dev *wdev, gfp_t gfp);
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* wiphy_printk helpers, similar to dev_printk */
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 5920715..57a22a2 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -810,6 +810,9 @@ enum nl80211_commands {
 	NL80211_CMD_CRIT_PROTOCOL_START,
 	NL80211_CMD_CRIT_PROTOCOL_STOP,
 
+	NL80211_CMD_START_P2P_FIND,
+	NL80211_CMD_STOP_P2P_FIND,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -1436,6 +1439,10 @@ enum nl80211_commands {
  *	allowed to be used with the first @NL80211_CMD_SET_STATION command to
  *	update a TDLS peer STA entry.
  *
+ * @NL80211_ATTR_MIN_DISCOVERABLE_INTERVAL:
+ * @NL80211_ATTR_MAX_DISCOVERABLE_INTERVAL: min/max discoverable interval
+ *	for the p2p find, multiple of 100 TUs, represented as u32
+ *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
  */
@@ -1736,6 +1743,9 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_PEER_AID,
 
+	NL80211_ATTR_MIN_DISCOVERABLE_INTERVAL,
+	NL80211_ATTR_MAX_DISCOVERABLE_INTERVAL,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -3579,6 +3589,8 @@ enum nl80211_ap_sme_features {
  *	Peering Management entity which may be implemented by registering for
  *	beacons or NL80211_CMD_NEW_PEER_CANDIDATE events. The mesh beacon is
  *	still generated by the driver.
+ * @NL80211_FEATURE_P2P_PROBE_RESP_OFFLOAD: When in P2P find phase,
+ *	the device responds to probe-requests in hardware.
  */
 enum nl80211_feature_flags {
 	NL80211_FEATURE_SK_TX_STATUS			= 1 << 0,
@@ -3599,6 +3611,7 @@ enum nl80211_feature_flags {
 	NL80211_FEATURE_FULL_AP_CLIENT_STATE		= 1 << 15,
 	NL80211_FEATURE_USERSPACE_MPM			= 1 << 16,
 	NL80211_FEATURE_ACTIVE_MONITOR			= 1 << 17,
+	NL80211_FEATURE_P2P_PROBE_RESP_OFFLOAD		= 1 << 18,
 };
 
 /**
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 31d265f..d68dfdd 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -349,6 +349,8 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
 	[NL80211_ATTR_IE_RIC] = { .type = NLA_BINARY,
 				  .len = IEEE80211_MAX_DATA_LEN },
 	[NL80211_ATTR_PEER_AID] = { .type = NLA_U16 },
+	[NL80211_ATTR_MIN_DISCOVERABLE_INTERVAL] = { .type = NLA_U32 },
+	[NL80211_ATTR_MAX_DISCOVERABLE_INTERVAL] = { .type = NLA_U32 },
 };
 
 /* policy for the key attributes */
@@ -1390,6 +1392,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *dev,
 		if (split) {
 			CMD(crit_proto_start, CRIT_PROTOCOL_START);
 			CMD(crit_proto_stop, CRIT_PROTOCOL_STOP);
+			CMD(start_p2p_find, START_P2P_FIND);
+			CMD(stop_p2p_find, STOP_P2P_FIND);
 		}
 
 #ifdef CONFIG_NL80211_TESTMODE
@@ -8265,6 +8269,121 @@ static int nl80211_crit_protocol_stop(struct sk_buff *skb,
 	return 0;
 }
 
+static int nl80211_start_p2p_find(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct wireless_dev *wdev = info->user_ptr[1];
+	struct wiphy *wiphy = &rdev->wiphy;
+	/*
+	 * Defaults, as defined in the spec
+	 * Ref: Wi-Fi Peer-to-Peer (P2P) Technical Specification v1.1
+	 * Clause: 3.1.2.1.3 Find Phase
+	 */
+	struct cfg80211_p2p_find_params params = {
+		.min_discoverable_interval = 1,
+		.max_discoverable_interval = 3,
+	};
+	struct nlattr *attr_freq = info->attrs[NL80211_ATTR_SCAN_FREQUENCIES];
+	struct nlattr *attr;
+	int err, tmp, n_channels, i = 0;
+	struct ieee80211_channel **channels = NULL;
+
+	if (wdev->iftype != NL80211_IFTYPE_P2P_DEVICE)
+		return -EOPNOTSUPP;
+
+	if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_IE]))
+		return -EINVAL;
+
+	if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_IE_PROBE_RESP]))
+		return -EINVAL;
+
+	if (!rdev->ops->start_p2p_find || !rdev->ops->stop_p2p_find)
+		return -EOPNOTSUPP;
+
+	if (rdev->scan_req)
+		return -EBUSY;
+
+	if (attr_freq) {
+		n_channels = validate_scan_freqs(attr_freq);
+		if (!n_channels)
+			return -EINVAL;
+
+		channels = kzalloc(n_channels * sizeof(*channels), GFP_KERNEL);
+		if (!channels)
+			return -ENOMEM;
+
+		/* user specified, bail out if channel not found */
+		nla_for_each_nested(attr, attr_freq, tmp) {
+			struct ieee80211_channel *chan;
+
+			chan = ieee80211_get_channel(wiphy, nla_get_u32(attr));
+
+			if (!chan) {
+				err = -EINVAL;
+				goto out_free;
+			}
+
+			/* ignore disabled channels */
+			if (chan->flags & IEEE80211_CHAN_DISABLED)
+				continue;
+
+			params.channels[i] = chan;
+			i++;
+		}
+		if (!i) {
+			err = -EINVAL;
+			goto out_free;
+		}
+
+		params.n_channels = i;
+		params.channels = channels;
+	}
+
+
+	attr = info->attrs[NL80211_ATTR_IE];
+	if (attr) {
+		params.probe_ie_len = nla_len(attr);
+		params.probe_ie = nla_data(attr);
+	}
+
+	attr = info->attrs[NL80211_ATTR_IE_PROBE_RESP];
+	if (attr) {
+		params.probe_resp_ie_len = nla_len(attr);
+		params.probe_resp_ie = nla_data(attr);
+	}
+
+	attr = info->attrs[NL80211_ATTR_MIN_DISCOVERABLE_INTERVAL];
+	if (attr)
+		params.min_discoverable_interval = nla_get_u32(attr);
+
+	attr = info->attrs[NL80211_ATTR_MAX_DISCOVERABLE_INTERVAL];
+	if (attr)
+		params.max_discoverable_interval = nla_get_u32(attr);
+
+	err = rdev_start_p2p_find(rdev, wdev, &params);
+
+out_free:
+	kfree(channels);
+
+	return err;
+}
+
+static int nl80211_stop_p2p_find(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct wireless_dev *wdev = info->user_ptr[1];
+
+	if (wdev->iftype != NL80211_IFTYPE_P2P_DEVICE)
+		return -EOPNOTSUPP;
+
+	if (!rdev->ops->start_p2p_find || !rdev->ops->stop_p2p_find)
+		return -EOPNOTSUPP;
+
+	rdev_stop_p2p_find(rdev, wdev);
+
+	return 0;
+}
+
 #define NL80211_FLAG_NEED_WIPHY		0x01
 #define NL80211_FLAG_NEED_NETDEV	0x02
 #define NL80211_FLAG_NEED_RTNL		0x04
@@ -8937,6 +9056,22 @@ static struct genl_ops nl80211_ops[] = {
 				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
+		.cmd = NL80211_CMD_START_P2P_FIND,
+		.doit = nl80211_start_p2p_find,
+		.policy = nl80211_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_NEED_RTNL,
+	},
+	{
+		.cmd = NL80211_CMD_STOP_P2P_FIND,
+		.doit = nl80211_stop_p2p_find,
+		.policy = nl80211_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_NEED_RTNL,
+	},
+	{
 		.cmd = NL80211_CMD_GET_PROTOCOL_FEATURES,
 		.doit = nl80211_get_protocol_features,
 		.policy = nl80211_policy,
@@ -10627,6 +10762,46 @@ void cfg80211_tdls_oper_request(struct net_device *dev, const u8 *peer,
 }
 EXPORT_SYMBOL(cfg80211_tdls_oper_request);
 
+static
+void cfg80211_p2p_find_notify(struct wireless_dev *wdev, int cmd, gfp_t gfp)
+{
+	struct wiphy *wiphy = wdev->wiphy;
+	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
+	struct sk_buff *msg;
+	void *hdr;
+
+	trace_cfg80211_p2p_find_notify(wdev, cmd);
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return;
+
+	hdr = nl80211hdr_put(msg, 0, 0, 0, cmd);
+	if (!hdr) {
+		nlmsg_free(msg);
+		return;
+	}
+
+	if (genlmsg_end(msg, hdr) < 0) {
+		nlmsg_free(msg);
+		return;
+	}
+
+	genlmsg_multicast_netns(wiphy_net(&rdev->wiphy), msg, 0,
+				nl80211_scan_mcgrp.id, GFP_KERNEL);
+}
+
+void cfg80211_p2p_find_notify_start(struct wireless_dev *wdev, gfp_t gfp)
+{
+	cfg80211_p2p_find_notify(wdev, NL80211_CMD_START_P2P_FIND, gfp);
+}
+EXPORT_SYMBOL(cfg80211_p2p_find_notify_start);
+
+void cfg80211_p2p_find_notify_end(struct wireless_dev *wdev, gfp_t gfp)
+{
+	cfg80211_p2p_find_notify(wdev, NL80211_CMD_STOP_P2P_FIND, gfp);
+}
+EXPORT_SYMBOL(cfg80211_p2p_find_notify_end);
+
 static int nl80211_netlink_notify(struct notifier_block * nb,
 				  unsigned long state,
 				  void *_notify)
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 9f15f0a..94ff98a 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -923,4 +923,23 @@ static inline void rdev_crit_proto_stop(struct cfg80211_registered_device *rdev,
 	trace_rdev_return_void(&rdev->wiphy);
 }
 
+static inline int rdev_start_p2p_find(struct cfg80211_registered_device *rdev,
+				      struct wireless_dev *wdev,
+				      struct cfg80211_p2p_find_params *params)
+{
+	int ret;
+	trace_rdev_start_p2p_find(&rdev->wiphy, wdev, params);
+	ret = rdev->ops->start_p2p_find(&rdev->wiphy, wdev, params);
+	trace_rdev_return_int(&rdev->wiphy, ret);
+	return ret;
+}
+
+static inline void rdev_stop_p2p_find(struct cfg80211_registered_device *rdev,
+				      struct wireless_dev *wdev)
+{
+	trace_rdev_stop_p2p_find(&rdev->wiphy, wdev);
+	rdev->ops->stop_p2p_find(&rdev->wiphy, wdev);
+	trace_rdev_return_void(&rdev->wiphy);
+}
+
 #endif /* __CFG80211_RDEV_OPS */
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 23fafea..9eb3dd7 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -1841,6 +1841,34 @@ TRACE_EVENT(rdev_crit_proto_stop,
 		  WIPHY_PR_ARG, WDEV_PR_ARG)
 );
 
+TRACE_EVENT(rdev_start_p2p_find,
+	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev,
+		 struct cfg80211_p2p_find_params *params),
+	TP_ARGS(wiphy, wdev, params),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		WDEV_ENTRY
+		__field(u32, min_di)
+		__field(u32, max_di)
+		__field(int, n_channels)
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		WDEV_ASSIGN;
+		__entry->min_di = params->min_discoverable_interval;
+		__entry->max_di = params->max_discoverable_interval;
+		__entry->n_channels = params->n_channels;
+	),
+	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT ", disc. int. [%d..%d], n_channels %d",
+		  WIPHY_PR_ARG, WDEV_PR_ARG, __entry->min_di, __entry->max_di,
+		  __entry->n_channels)
+);
+
+DEFINE_EVENT(wiphy_wdev_evt, rdev_stop_p2p_find,
+	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev),
+	TP_ARGS(wiphy, wdev)
+);
+
 /*************************************************************
  *	     cfg80211 exported functions traces		     *
  *************************************************************/
@@ -2498,6 +2526,20 @@ TRACE_EVENT(cfg80211_ft_event,
 		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(target_ap))
 );
 
+TRACE_EVENT(cfg80211_p2p_find_notify,
+	TP_PROTO(struct wireless_dev *wdev, int cmd),
+	TP_ARGS(wdev, cmd),
+	TP_STRUCT__entry(
+		WDEV_ENTRY
+		__field(int, cmd)
+	),
+	TP_fast_assign(
+		WDEV_ASSIGN;
+		__entry->cmd = cmd;
+	),
+	TP_printk(WDEV_PR_FMT ", cmd: %d", WDEV_PR_ARG, __entry->cmd)
+);
+
 #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
-- 
1.8.1.2

RE: [PATCH v8] cfg80211: P2P find phase offload

[Peer, Ilan]

Hi Vladimir,

>  /**
> + * struct cfg80211_p2p_find_params - parameters for P2P find
> + * @probe_ie: extra IE's for probe frames
> + * @probe_ie_len: length, bytes, of @probe_ie
> + * @probe_resp_ie: extra IE's for probe response frames
> + * @probe_resp_ie_len: length, bytes, of @probe_resp_ie
> + *	Driver/firmware may add additional IE's as well as modify
> + *	provided ones; typical IE's to be added are
> + *	WLAN_EID_EXT_SUPP_RATES, WLAN_EID_DS_PARAMS,
> + *	WLAN_EID_HT_CAPABILITY.
> + * @min_discoverable_interval: and
> + * @max_discoverable_interval: min/max for random multiplier of 100TU's
> + *	for the listen state duration
> + * @n_channels: number of channels to operate on
> + * @channels: channels to operate on
> + */
> +struct cfg80211_p2p_find_params {

The parameters are missing the listen channel (which is needed to the listen phase).

> +	const u8 *probe_ie;
> +	size_t probe_ie_len;
> +	const u8 *probe_resp_ie;
> +	size_t probe_resp_ie_len;
> +	u32 min_discoverable_interval;
> +	u32 max_discoverable_interval;

I do not see a real value for the max/min_discoverable_interval. Is there a use case of specifying these limits and not using the default ones?

> +
> +	int n_channels;
> +	struct ieee80211_channel **channels;
> +};
> +
> +/**
>   * struct cfg80211_ops - backend description for wireless configuration
>   *
>   * This struct is registered by fullmac card drivers and/or wireless stacks @@ -
> 2037,6 +2065,17 @@ struct cfg80211_update_ft_ies_params {
>   *	driver can take the most appropriate actions.
>   * @crit_proto_stop: Indicates critical protocol no longer needs increased link
>   *	reliability. This operation can not fail.
> + *
> + * @start_p2p_find: start P2P find phase
> + *	Parameters include IEs for probe/probe-resp frames;
> + *	and channels to operate on.
> + *	Parameters are not retained after call, driver need to copy data if
> + *	it need it later.
> + *	P2P find can't run concurrently with ROC or scan, and driver should
> + *	check this.

It might be good to define the exact semantics of p2p_find with regards to ROC/Scan, i.e., when p2p_find is requested during ROC/scan, should the driver save the request and handle it when the other operation is done, or should it return EBUSY or something similar (and also what are the semantics when requesting ROC/scan while p2p_find is in progress).

> 
> +static int nl80211_start_p2p_find(struct sk_buff *skb, struct genl_info
> +*info) {
> +	struct cfg80211_registered_device *rdev = info->user_ptr[0];
> +	struct wireless_dev *wdev = info->user_ptr[1];
> +	struct wiphy *wiphy = &rdev->wiphy;
> +	/*
> +	 * Defaults, as defined in the spec
> +	 * Ref: Wi-Fi Peer-to-Peer (P2P) Technical Specification v1.1
> +	 * Clause: 3.1.2.1.3 Find Phase
> +	 */
> +	struct cfg80211_p2p_find_params params = {
> +		.min_discoverable_interval = 1,
> +		.max_discoverable_interval = 3,
> +	};
> +	struct nlattr *attr_freq = info-
> >attrs[NL80211_ATTR_SCAN_FREQUENCIES];
> +	struct nlattr *attr;
> +	int err, tmp, n_channels, i = 0;
> +	struct ieee80211_channel **channels = NULL;
> +
> +	if (wdev->iftype != NL80211_IFTYPE_P2P_DEVICE)
> +		return -EOPNOTSUPP;
> +
> +	if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_IE]))
> +		return -EINVAL;
> +
> +	if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_IE_PROBE_RESP]))
> +		return -EINVAL;
> +
> +	if (!rdev->ops->start_p2p_find || !rdev->ops->stop_p2p_find)
> +		return -EOPNOTSUPP;
> +
> +	if (rdev->scan_req)
> +		return -EBUSY;
> +
> +	if (attr_freq) {
> +		n_channels = validate_scan_freqs(attr_freq);
> +		if (!n_channels)
> +			return -EINVAL;
> +
> +		channels = kzalloc(n_channels * sizeof(*channels),
> GFP_KERNEL);
> +		if (!channels)
> +			return -ENOMEM;
> +
> +		/* user specified, bail out if channel not found */
> +		nla_for_each_nested(attr, attr_freq, tmp) {
> +			struct ieee80211_channel *chan;
> +
> +			chan = ieee80211_get_channel(wiphy,
> nla_get_u32(attr));
> +
> +			if (!chan) {
> +				err = -EINVAL;
> +				goto out_free;
> +			}
> +
> +			/* ignore disabled channels */
> +			if (chan->flags & IEEE80211_CHAN_DISABLED)
> +				continue;
> +
> +			params.channels[i] = chan;
> +			i++;
> +		}
> +		if (!i) {
> +			err = -EINVAL;
> +			goto out_free;
> +		}
> +
> +		params.n_channels = i;
> +		params.channels = channels;
> +	}
> +

Do we need to set the default channels if the attribute is not set (or require that this attribute will be part of the command).

> +	attr = info->attrs[NL80211_ATTR_IE];
> +	if (attr) {
> +		params.probe_ie_len = nla_len(attr);
> +		params.probe_ie = nla_data(attr);
> +	}
> +
> +	attr = info->attrs[NL80211_ATTR_IE_PROBE_RESP];
> +	if (attr) {
> +		params.probe_resp_ie_len = nla_len(attr);
> +		params.probe_resp_ie = nla_data(attr);
> +	}

Is it valid to get Probe response IEs even if the driver did not report support for it?

Regards,

Ilan.

Re: [PATCH v8] cfg80211: P2P find phase offload

[Vladimir Kondratiev]

On Tuesday, June 04, 2013 10:07:31 AM Peer, Ilan wrote:
> Hi Vladimir,
> 
> > + * @min_discoverable_interval: and
> > + * @max_discoverable_interval: min/max for random multiplier of 100TU's
> > + *	for the listen state duration
> > + * @n_channels: number of channels to operate on
> > + * @channels: channels to operate on
> > + */
> > +struct cfg80211_p2p_find_params {
> 
> The parameters are missing the listen channel (which is needed to the listen phase).

There are n_channels and channels that define set of social channels
to operate on. It applies to both listen and search.

> > +	u32 min_discoverable_interval;
> > +	u32 max_discoverable_interval;
> 
> I do not see a real value for the max/min_discoverable_interval. Is there a use case of specifying these limits and not using the default ones?

Spec says these parameters may be changed.
Practical use, suggested by spec:

- search-only P2P device may set
min_discoverable_interval = max_discoverable_interval = 0
- listen-most P2P device may set min_discoverable_interval >> 0 and
max_discoverable_interval >= min_discoverable_interval

> > + *
> > + * @start_p2p_find: start P2P find phase
> > + *	Parameters include IEs for probe/probe-resp frames;
> > + *	and channels to operate on.
> > + *	Parameters are not retained after call, driver need to copy data if
> > + *	it need it later.
> > + *	P2P find can't run concurrently with ROC or scan, and driver should
> > + *	check this.
> 
> It might be good to define the exact semantics of p2p_find with regards to ROC/Scan, i.e., when p2p_find is requested during ROC/scan, should the driver save the request and handle it when the other operation is done, or should it return EBUSY or something similar (and also what are the semantics when requesting ROC/scan while p2p_find is in progress).

My opinion is ROC/scan collision with p2p_find is indication of error
in the supplicant; and proper response would be -EBUSY. Actually, code
in nl80211 check for scan (see below), but ROC is harder to detect.
I'll add explanation to the comment - anyway Johannes wants me to fix
for genlmsg_end() never returning negative value.

> > +
> > +	if (rdev->scan_req)
> > +		return -EBUSY;

> > +
> > +	if (attr_freq) {
> > +		n_channels = validate_scan_freqs(attr_freq);
> > +		if (!n_channels)
> > +			return -EINVAL;
> > +
> > +		channels = kzalloc(n_channels * sizeof(*channels),
> > GFP_KERNEL);
> > +		if (!channels)
> > +			return -ENOMEM;
> > +
> > +		/* user specified, bail out if channel not found */
> > +		nla_for_each_nested(attr, attr_freq, tmp) {
> > +			struct ieee80211_channel *chan;
> > +
> > +			chan = ieee80211_get_channel(wiphy,
> > nla_get_u32(attr));
> > +
> > +			if (!chan) {
> > +				err = -EINVAL;
> > +				goto out_free;
> > +			}
> > +
> > +			/* ignore disabled channels */
> > +			if (chan->flags & IEEE80211_CHAN_DISABLED)
> > +				continue;
> > +
> > +			params.channels[i] = chan;
> > +			i++;
> > +		}
> > +		if (!i) {
> > +			err = -EINVAL;
> > +			goto out_free;
> > +		}
> > +
> > +		params.n_channels = i;
> > +		params.channels = channels;
> > +	}
> > +
> 
> Do we need to set the default channels if the attribute is not set (or require that this attribute will be part of the command).

Logic is the following:
- supplicant may omit channel list at all, this mean
"use all social channels", kind of default behavior
- supplicant may specify channel subset, in this case it should be not empty.

I'll comment it too.

> > +
> > +	attr = info->attrs[NL80211_ATTR_IE_PROBE_RESP];
> > +	if (attr) {
> > +		params.probe_resp_ie_len = nla_len(attr);
> > +		params.probe_resp_ie = nla_data(attr);
> > +	}
> 
> Is it valid to get Probe response IEs even if the driver did not report support for it?

One can't do p2p_find if it does not reply to P2P discovery probes.
It is unrelated to answering probes in AP mode - driver may leave this
to supplicant.

Thanks, Vladimir

Re: [PATCH v8] cfg80211: P2P find phase offload

[Johannes Berg]

> > > +struct cfg80211_p2p_find_params {
> > 
> > The parameters are missing the listen channel (which is needed to the listen phase).
> 
> There are n_channels and channels that define set of social channels
> to operate on. It applies to both listen and search.

I don't think that's true -- you only respond on the listen channel but
send probe requests on all the channels?

> > > +	attr = info->attrs[NL80211_ATTR_IE_PROBE_RESP];
> > > +	if (attr) {
> > > +		params.probe_resp_ie_len = nla_len(attr);
> > > +		params.probe_resp_ie = nla_data(attr);
> > > +	}
> > 
> > Is it valid to get Probe response IEs even if the driver did not report support for it?
> 
> One can't do p2p_find if it does not reply to P2P discovery probes.
> It is unrelated to answering probes in AP mode - driver may leave this
> to supplicant.

I think what Ilan is asking is whether you should reject the probe_resp
attribute if NL80211_FEATURE_P2P_PROBE_RESP_OFFLOAD isn't set. I'm fine
with just ignoring it (as is done now) though, might make wpa-s easier.

johannes

Re: [PATCH v8] cfg80211: P2P find phase offload

[Vladimir Kondratiev]

On Tuesday, June 04, 2013 02:07:36 PM Johannes Berg wrote:
> 
> > > > +struct cfg80211_p2p_find_params {
> > > 
> > > The parameters are missing the listen channel (which is needed to the listen phase).
> > 
> > There are n_channels and channels that define set of social channels
> > to operate on. It applies to both listen and search.
> 
> I don't think that's true -- you only respond on the listen channel but
> send probe requests on all the channels?

Yes, you are right - it was my mis-interpretation.
I'll add listen channel as mandatory attribute.
Indeed, spec says:

The Listen Channel shall be chosen at the beginning of the
Device Discovery and shall remain the same until P2P Discovery completes.

> 
> > > > +	attr = info->attrs[NL80211_ATTR_IE_PROBE_RESP];
> > > > +	if (attr) {
> > > > +		params.probe_resp_ie_len = nla_len(attr);
> > > > +		params.probe_resp_ie = nla_data(attr);
> > > > +	}
> > > 
> > > Is it valid to get Probe response IEs even if the driver did not report support for it?
> > 
> > One can't do p2p_find if it does not reply to P2P discovery probes.
> > It is unrelated to answering probes in AP mode - driver may leave this
> > to supplicant.
> 
> I think what Ilan is asking is whether you should reject the probe_resp
> attribute if NL80211_FEATURE_P2P_PROBE_RESP_OFFLOAD isn't set. I'm fine
> with just ignoring it (as is done now) though, might make wpa-s easier.

On Tuesday, June 04, 2013 12:10:52 PM Peer, Ilan wrote:
> But you got NL80211_FEATURE_P2P_PROBE_RESP_OFFLOAD for driver that support probe response offloading. In case that the driver does not report this flag, wpa_supplicant should understand this and not configure the driver with the probe response IEs.
> 

> BTW, in case that the probe response offload is enabled, do you think that the corresponding probe requests should be reported to the host? If this is not the case, the wpa_supplicant will not have the full list of p2p_peers.
> 

Agree with Johannes, it is easier for wpa_s if we will not be too strict here.
And, probes must be reported to the host in all cases, for wpa_s to have peer list.
Need to add comment describing this.

Thanks, Vladimir

Re: [PATCH v8] cfg80211: P2P find phase offload

[Johannes Berg]

On Tue, 2013-06-04 at 17:03 +0300, Vladimir Kondratiev wrote:

> And, probes must be reported to the host in all cases, for wpa_s to have peer list.
> Need to add comment describing this.

I don't think that's necessarily true. wpa_s will pre-build a list, but
note that it doesn't make those peers as discovered and you can't really
do anything with them. As such, I would argue that reporting probe
requests ("probes") would be harmful and counter to one potential goal
of this patch (powersaving.)

johannes

Re: [PATCH v8] cfg80211: P2P find phase offload

[Malinen, Jouni]

On 6/4/13 5:29 PM, "Johannes Berg" <johannes@sipsolutions.net> wrote:

>On Tue, 2013-06-04 at 17:03 +0300, Vladimir Kondratiev wrote:
>
>> And, probes must be reported to the host in all cases, for wpa_s to
>>have peer list.
>> Need to add comment describing this.
>
>I don't think that's necessarily true. wpa_s will pre-build a list, but
>note that it doesn't make those peers as discovered and you can't really
>do anything with them. As such, I would argue that reporting probe
>requests ("probes") would be harmful and counter to one potential goal
>of this patch (powersaving.)

In general, I'd agree. However, I would like to get information of any
peer device being in active PBC mode. This would require either getting
those Probe Request frames or alternatively that being tracked in
kernel/driver/firmware with some access for wpa_supplicant to fetch
information of all STAs (list of MAC Address + UUID) that have indicated
active PBC mode within last 120 seconds.

- Jouni

Re: [PATCH v8] cfg80211: P2P find phase offload

[Vladimir Kondratiev]

On Tuesday, June 04, 2013 05:35:30 PM Malinen, Jouni wrote:
> 
> On 6/4/13 5:29 PM, "Johannes Berg" <johannes@sipsolutions.net> wrote:
> 
> >On Tue, 2013-06-04 at 17:03 +0300, Vladimir Kondratiev wrote:
> >
> >> And, probes must be reported to the host in all cases, for wpa_s to
> >>have peer list.
> >> Need to add comment describing this.
> >
> >I don't think that's necessarily true. wpa_s will pre-build a list, but
> >note that it doesn't make those peers as discovered and you can't really
> >do anything with them. As such, I would argue that reporting probe
> >requests ("probes") would be harmful and counter to one potential goal
> >of this patch (powersaving.)
> 
> In general, I'd agree. However, I would like to get information of any
> peer device being in active PBC mode. This would require either getting
> those Probe Request frames or alternatively that being tracked in
> kernel/driver/firmware with some access for wpa_supplicant to fetch
> information of all STAs (list of MAC Address + UUID) that have indicated
> active PBC mode within last 120 seconds.
> 
> - Jouni

I am a bit confused. I supposed that if devices A and B (not in a group yet)
try to discover each other, it works like the following

A ---- probe-req  ---> B (now B knows A)
A <--- probe-resp ---- B (now A knows B)

But if device is discovered by probe-resp only, it mean this should be

A ---- probe-req  ---> B
A <--- probe-resp ---- B (now A knows B)
A <--- probe-req  ---- B
A ---- probe-resp ---> B (now B knows A)


What is the case?

Thanks, Vladimir

Re: [PATCH v8] cfg80211: P2P find phase offload

[Jouni Malinen]

On Tue, Jun 04, 2013 at 07:47:20PM +0300, Vladimir Kondratiev wrote:
> I am a bit confused. I supposed that if devices A and B (not in a group yet)
> try to discover each other, it works like the following
> 
> A ---- probe-req  ---> B (now B knows A)
> A <--- probe-resp ---- B (now A knows B)

This depends on what exactly you mean with "discover". Sure, it is known
that there is a P2P peer in the environment if a Probe Request frame
with P2P IE is received from it. However, that does not necessarily mean
we know enough of that peer to initiate new P2P operations.

> But if device is discovered by probe-resp only, it mean this should be
> 
> A ---- probe-req  ---> B
> A <--- probe-resp ---- B (now A knows B)
> A <--- probe-req  ---- B
> A ---- probe-resp ---> B (now B knows A)

Probe Response frames include more information about the peer and that
allows wpa_supplicant to complete the peer entry. This is the point when
the peer is fully known and indicated to upper layers. The Probe Request
frame RX case is just used internally within wpa_supplicant. (For
completeness, number of other frames, i.e., P2P Action frames, can also
complete the P2P peer entry, so this does not need to be Probe
Response).

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH v8] cfg80211: P2P find phase offload

[Vladimir Kondratiev]

On Tuesday, June 04, 2013 09:30:23 PM Jouni Malinen wrote:
> On Tue, Jun 04, 2013 at 07:47:20PM +0300, Vladimir Kondratiev wrote:
> > I am a bit confused. I supposed that if devices A and B (not in a group yet)
> > try to discover each other, it works like the following
> > 
> > A ---- probe-req  ---> B (now B knows A)
> > A <--- probe-resp ---- B (now A knows B)
> 
> This depends on what exactly you mean with "discover". Sure, it is known
> that there is a P2P peer in the environment if a Probe Request frame
> with P2P IE is received from it. However, that does not necessarily mean
> we know enough of that peer to initiate new P2P operations.
> 
> > But if device is discovered by probe-resp only, it mean this should be
> > 
> > A ---- probe-req  ---> B
> > A <--- probe-resp ---- B (now A knows B)
> > A <--- probe-req  ---- B
> > A ---- probe-resp ---> B (now B knows A)
> 
> Probe Response frames include more information about the peer and that
> allows wpa_supplicant to complete the peer entry. This is the point when
> the peer is fully known and indicated to upper layers. The Probe Request
> frame RX case is just used internally within wpa_supplicant. (For
> completeness, number of other frames, i.e., P2P Action frames, can also
> complete the P2P peer entry, so this does not need to be Probe
> Response).

Thanks for clarification. So, I see that wpa_s wants all probe responses and
may want either all or selected probes, depending on the driver's capability
(NL80211_FEATURE_P2P_PROBE_RESP_OFFLOAD) and its own reasons.

Then, I think it would be appropriate to say following in the comment for
start_p2p_find (no code changes, it is only expectations for the friver behavior):

---cut---
While performing P2P discovery, driver should report all received
probe-request and probe-response frames via cfg80211_rx_mgmt,
accordingly to the rx mgmt filter, as set by mgmt_frame_register().
When reporting probes, driver/firmware may do its best to filter out
probes that would not be replied with probe-resp accordingly to the
P2P rules for active device configuration.
---cut---

This makes possible for wpa_s to choose exact configuration, and good
power saving is possible provided firmware answer probes.

Comments?

Thanks, Vladimir

Re: [PATCH v8] cfg80211: P2P find phase offload

[Johannes Berg]

On Wed, 2013-06-05 at 10:10 +0300, Vladimir Kondratiev wrote:

> Then, I think it would be appropriate to say following in the comment for
> start_p2p_find (no code changes, it is only expectations for the friver behavior):
> 

> While performing P2P discovery, driver should report all received
> probe-request and probe-response frames via cfg80211_rx_mgmt,
> accordingly to the rx mgmt filter, as set by mgmt_frame_register().

Well, realistically there will be a frame registration for probe
requests, so stating that is kinda pointless, but OK.

> When reporting probes, driver/firmware may do its best to filter out
> probes that would not be replied with probe-resp accordingly to the
> P2P rules for active device configuration.

This I think should be more specific. "[W]ould not be replied" is
clearly one step, but in an environment where you actually want to
offload the P2P probe responses that is pretty much useless. I'd rather
say something like:

---
When probe response offload it supported, the device should not report
probe requests to the host that it already responded to. It must report
(and therefore not respond to) probe requests that indicate the sending
device is in active PBC mode (specifically, <...add more details...>).
It may also drop invalid or malformed probe requests or ones that would
not be replied to for other reasons.
---

I think this would be a reasonable tradeoff. It means that if a probe
request is actually reported, wpa_supplicant must reply to it, and we
don't have to get into the business of having to decide whether or not
it needs to respond.

Alternatively, we could specify that the device _must_ respond if
offload is supported, and then report it.

However, we need to clearly specify this so that we don't get two
responses, one from the device and one from wpa_s. If there's no way to
specify this, we need to introduce a "reply already sent" flag into the
frame reporting.

johannes

Re: [PATCH v8] cfg80211: P2P find phase offload

[Vladimir Kondratiev]

On Wednesday, June 05, 2013 09:53:00 AM Johannes Berg wrote:
> > When reporting probes, driver/firmware may do its best to filter out
> > probes that would not be replied with probe-resp accordingly to the
> > P2P rules for active device configuration.
> 
> This I think should be more specific. "[W]ould not be replied" is
> clearly one step, but in an environment where you actually want to
> offload the P2P probe responses that is pretty much useless. I'd rather
> say something like:
> 
> ---
> When probe response offload it supported, the device should not report
> probe requests to the host that it already responded to. It must report
> (and therefore not respond to) probe requests that indicate the sending
> device is in active PBC mode (specifically, <...add more details...>).
> It may also drop invalid or malformed probe requests or ones that would
> not be replied to for other reasons.
> ---

> 
> I think this would be a reasonable tradeoff. It means that if a probe
> request is actually reported, wpa_supplicant must reply to it, and we
> don't have to get into the business of having to decide whether or not
> it needs to respond.
> 
> Alternatively, we could specify that the device _must_ respond if
> offload is supported, and then report it.

I like this alternative. Except, reporting is accordingly to the rx filter.
wpa_s may be not interesting in probes at all, or want only ones with PBC.
If device answer probes in firmware, this would reduce CPU wake-ups.

> 
> However, we need to clearly specify this so that we don't get two
> responses, one from the device and one from wpa_s. If there's no way to
> specify this, we need to introduce a "reply already sent" flag into the
> frame reporting.

Said above translates to all-or-nothing approach w.r.t. responses:

- If device indicate probe-resp offload, it must reply all probes, supplicant
must not send probe-resp.
- If device does not indicate probe-resp offload, it should never send
probe-resp by itself; it should report all matching probes
and supplicant will generate probe-resp.

Regarding what probes to report, I'd specify relaxed requirements
for the driver:

- in non-offload case, driver must report all matching probes
(but may just report all, and wpa_s will filter)
- in offload case, must report matching probes if rx filter says so. It is not
neccessary to report all probes that device replied to.

I don't want to force firmware or driver to parse probes to detect PBC; if we
got frame to the host, from power perspective there is no much difference who
will filter it - driver or wpa_s; and wpa_s already do this parsing.

For PBC - can one specify "probes with PBC" using existing rx filter mechanism?

Also, I feel this explanation get large, it deserves separate comment block,
it is overkill for start_p2p_find comment. Maybe do it in another patch?

Thanks, Vladimir

Re: [PATCH v8] cfg80211: P2P find phase offload

[Johannes Berg]

> > ---
> > When probe response offload it supported, the device should not report
> > probe requests to the host that it already responded to. It must report
> > (and therefore not respond to) probe requests that indicate the sending
> > device is in active PBC mode (specifically, <...add more details...>).
> > It may also drop invalid or malformed probe requests or ones that would
> > not be replied to for other reasons.
> > ---
> 
> > 
> > I think this would be a reasonable tradeoff. It means that if a probe
> > request is actually reported, wpa_supplicant must reply to it, and we
> > don't have to get into the business of having to decide whether or not
> > it needs to respond.
> > 
> > Alternatively, we could specify that the device _must_ respond if
> > offload is supported, and then report it.
> 
> I like this alternative. Except, reporting is accordingly to the rx filter.
> wpa_s may be not interesting in probes at all, or want only ones with PBC.
> If device answer probes in firmware, this would reduce CPU wake-ups.

Well, wpa_s is always going to be interested, and it can't really
specify in the subscription filter that it only wants PBC ones. The
filters aren't expressive enough for that.

This isn't really my favourite alternative because it means the device
can't just "chicken out" and treat this as a best effort thing, it means
that it always has to reply. I fear that for some devices this might
mean having to reply in software in certain circumstances, which seems
like a bad idea.

> > However, we need to clearly specify this so that we don't get two
> > responses, one from the device and one from wpa_s. If there's no way to
> > specify this, we need to introduce a "reply already sent" flag into the
> > frame reporting.
> 
> Said above translates to all-or-nothing approach w.r.t. responses:
> 
> - If device indicate probe-resp offload, it must reply all probes, supplicant
> must not send probe-resp.
> - If device does not indicate probe-resp offload, it should never send
> probe-resp by itself; it should report all matching probes
> and supplicant will generate probe-resp.

Right. I guess I can live with this, though see above.

> Regarding what probes to report, I'd specify relaxed requirements
> for the driver:
> 
> - in non-offload case, driver must report all matching probes
> (but may just report all, and wpa_s will filter)
> - in offload case, must report matching probes if rx filter says so. It is not
> neccessary to report all probes that device replied to.

see above -- the filters aren't expressive enough to allow specifying "I
want PBC". For probe requests, the filter is going to be all or nothing,
so this won't really do anything useful.

> I don't want to force firmware or driver to parse probes to detect PBC; if we
> got frame to the host, from power perspective there is no much difference who
> will filter it - driver or wpa_s; and wpa_s already do this parsing.

Firmware should parse it, obviously.

> For PBC - can one specify "probes with PBC" using existing rx filter mechanism?

No.

> Also, I feel this explanation get large, it deserves separate comment block,
> it is overkill for start_p2p_find comment. Maybe do it in another patch?

I think we need to nail this down before we merge the API. I have a
feeling we may end up having to implement the third alternative (report
whether a response was sent or not)

johannes

Re: [PATCH v8] cfg80211: P2P find phase offload

[Johannes Berg]

On Tue, 2013-06-04 at 14:35 +0000, Malinen, Jouni wrote:

> In general, I'd agree. However, I would like to get information of any
> peer device being in active PBC mode. This would require either getting
> those Probe Request frames

I think I like that alternative better :-)

I know I wouldn't find it quickly ... can you be more specific about
what "being in active PBC mode" means? Is that easily detectable?

I'm thinking we should just document this (and be as specific about it
as we can) in the documentation for the offload feature.

johannes

Re: [PATCH v8] cfg80211: P2P find phase offload

[Arend van Spriel]

On 06/05/2013 09:46 AM, Johannes Berg wrote:
> On Tue, 2013-06-04 at 14:35 +0000, Malinen, Jouni wrote:
>
>> In general, I'd agree. However, I would like to get information of any
>> peer device being in active PBC mode. This would require either getting
>> those Probe Request frames
>
> I think I like that alternative better :-)
>
> I know I wouldn't find it quickly ... can you be more specific about
> what "being in active PBC mode" means? Is that easily detectable?
>
> I'm thinking we should just document this (and be as specific about it
> as we can) in the documentation for the offload feature.

Remark from the sideline. One of the motivations for the P2P find 
offload (when done on the device) is that the host can sleep, right? So 
how/when does wpa_s want these Probe Request frames. After the 
driver/device completed the find?

Regards,
Arend

Re: [PATCH v8] cfg80211: P2P find phase offload

[Johannes Berg]

On Wed, 2013-06-05 at 10:10 +0200, Arend van Spriel wrote:
> On 06/05/2013 09:46 AM, Johannes Berg wrote:
> > On Tue, 2013-06-04 at 14:35 +0000, Malinen, Jouni wrote:
> >
> >> In general, I'd agree. However, I would like to get information of any
> >> peer device being in active PBC mode. This would require either getting
> >> those Probe Request frames
> >
> > I think I like that alternative better :-)
> >
> > I know I wouldn't find it quickly ... can you be more specific about
> > what "being in active PBC mode" means? Is that easily detectable?
> >
> > I'm thinking we should just document this (and be as specific about it
> > as we can) in the documentation for the offload feature.
> 
> Remark from the sideline. One of the motivations for the P2P find 
> offload (when done on the device) is that the host can sleep, right? So 
> how/when does wpa_s want these Probe Request frames. After the 
> driver/device completed the find?

I don't think the device can really _sleep_ during this time, but it
allows reducing CPU wakeups. If you really wanted to sleep then you'd
have to buffer at least some information and have some sort of wakeup
trigger. That'll probably be done eventually, but it's not the rationale
here I think.

The way I see it, this patch allows being discoverable and discovering
other peers with much less impact on CPU usage.

The next step would be P2P-listen offload, which is more interesting for
being discoverable, and should also have probe request offload similar
to what we're discussing here.

johannes

Re: [PATCH v8] cfg80211: P2P find phase offload

[Arend van Spriel]

On 06/05/2013 10:18 AM, Johannes Berg wrote:
> On Wed, 2013-06-05 at 10:10 +0200, Arend van Spriel wrote:
>> On 06/05/2013 09:46 AM, Johannes Berg wrote:
>>> On Tue, 2013-06-04 at 14:35 +0000, Malinen, Jouni wrote:
>>>
>>>> In general, I'd agree. However, I would like to get information of any
>>>> peer device being in active PBC mode. This would require either getting
>>>> those Probe Request frames
>>>
>>> I think I like that alternative better :-)
>>>
>>> I know I wouldn't find it quickly ... can you be more specific about
>>> what "being in active PBC mode" means? Is that easily detectable?
>>>
>>> I'm thinking we should just document this (and be as specific about it
>>> as we can) in the documentation for the offload feature.
>>
>> Remark from the sideline. One of the motivations for the P2P find
>> offload (when done on the device) is that the host can sleep, right? So
>> how/when does wpa_s want these Probe Request frames. After the
>> driver/device completed the find?
>
> I don't think the device can really _sleep_ during this time, but it
> allows reducing CPU wakeups. If you really wanted to sleep then you'd
> have to buffer at least some information and have some sort of wakeup
> trigger. That'll probably be done eventually, but it's not the rationale
> here I think.

Right. Indeed meant CPU wakeups.

> The way I see it, this patch allows being discoverable and discovering
> other peers with much less impact on CPU usage.
>
> The next step would be P2P-listen offload, which is more interesting for
> being discoverable, and should also have probe request offload similar
> to what we're discussing here.


Ah, I thought that was included. The WFA spec is a bit confusing using 
terms 'phases' and 'states'. There is a SCAN phase and a FIND phase. 
During the FIND phase a device can be in SEARCH or LISTEN state. I know 
it is all terminology, but I was confused. At least the commit message 
seems to imply both states are offloaded.

Regards,
Arend

Re: [PATCH v8] cfg80211: P2P find phase offload

[Vladimir Kondratiev]

On Wednesday, June 05, 2013 10:38:12 AM Arend van Spriel wrote:
> >
> > The next step would be P2P-listen offload, which is more interesting for
> > being discoverable, and should also have probe request offload similar
> > to what we're discussing here.
> 
> 
> Ah, I thought that was included. The WFA spec is a bit confusing using 
> terms 'phases' and 'states'. There is a SCAN phase and a FIND phase. 
> During the FIND phase a device can be in SEARCH or LISTEN state. I know 
> it is all terminology, but I was confused. At least the commit message 
> seems to imply both states are offloaded.
> 
Arend,

you are correct. Briefly, P2P flow consists of the following phases:

1) scan - just regular scan, to find infrastructure ESS as well
2) find - in this phase, device toggles between states:
2.1) search - similar to scan on social channels, but not exactly. On 60g, search is _NOT_ scan
2.2) listen - similar to ROC on listen channel, but again, on 60g, listen is _NOT_ ROC

Then, this follows by GO negotiation etc.

Thanks, Vladimir

Re: [PATCH v8] cfg80211: P2P find phase offload

[Jouni Malinen]

On Wed, Jun 05, 2013 at 09:46:18AM +0200, Johannes Berg wrote:
> I know I wouldn't find it quickly ... can you be more specific about
> what "being in active PBC mode" means? Is that easily detectable?

Having Device Password ID attribute in the WSC element set to 4
(PushButton) in Probe Request frames (or Probe Response and Beacon
frames in case of AP/GO).

-- 
Jouni Malinen                                            PGP id EFC895FA

RE: [PATCH v8] cfg80211: P2P find phase offload

[Peer, Ilan]

Hi Vladimir,

BTW, in case that the probe response offload is enabled, do you think that the corresponding probe requests should be reported to the host? If this is not the case, the wpa_supplicant will not have the full list of p2p_peers.

> > > + * @min_discoverable_interval: and
> > > + * @max_discoverable_interval: min/max for random multiplier of
> 100TU's
> > > + *	for the listen state duration
> > > + * @n_channels: number of channels to operate on
> > > + * @channels: channels to operate on  */ struct
> > > +cfg80211_p2p_find_params {
> >
> > The parameters are missing the listen channel (which is needed to the listen
> phase).
> 
> There are n_channels and channels that define set of social channels to operate
> on. It applies to both listen and search.
> 

The wpa_supplicant defines a specific listen channel in wpas_p2p_init() and uses this channel in all the following flows. I think that this setting should be communicated to the driver and it should be distinguished from the channels used in search. 

> > > +	u32 min_discoverable_interval;
> > > +	u32 max_discoverable_interval;
> >
> > I do not see a real value for the max/min_discoverable_interval. Is there a
> use case of specifying these limits and not using the default ones?
> 
> Spec says these parameters may be changed.
> Practical use, suggested by spec:
> 
> - search-only P2P device may set
> min_discoverable_interval = max_discoverable_interval = 0
> - listen-most P2P device may set min_discoverable_interval >> 0 and
> max_discoverable_interval >= min_discoverable_interval
> 

Ok.

> > > + *
> > > + * @start_p2p_find: start P2P find phase
> > > + *	Parameters include IEs for probe/probe-resp frames;
> > > + *	and channels to operate on.
> > > + *	Parameters are not retained after call, driver need to copy data if
> > > + *	it need it later.
> > > + *	P2P find can't run concurrently with ROC or scan, and driver should
> > > + *	check this.
> >
> > It might be good to define the exact semantics of p2p_find with regards to
> ROC/Scan, i.e., when p2p_find is requested during ROC/scan, should the driver
> save the request and handle it when the other operation is done, or should it
> return EBUSY or something similar (and also what are the semantics when
> requesting ROC/scan while p2p_find is in progress).
> 
> My opinion is ROC/scan collision with p2p_find is indication of error in the
> supplicant; and proper response would be -EBUSY. Actually, code in nl80211
> check for scan (see below), but ROC is harder to detect.
> I'll add explanation to the comment - anyway Johannes wants me to fix for
> genlmsg_end() never returning negative value.
> 

That sound reasonable.

> > > +
> > > +	if (rdev->scan_req)
> > > +		return -EBUSY;
> 
> > > +
> > > +	if (attr_freq) {
> > > +		n_channels = validate_scan_freqs(attr_freq);
> > > +		if (!n_channels)
> > > +			return -EINVAL;
> > > +
> > > +		channels = kzalloc(n_channels * sizeof(*channels),
> > > GFP_KERNEL);
> > > +		if (!channels)
> > > +			return -ENOMEM;
> > > +
> > > +		/* user specified, bail out if channel not found */
> > > +		nla_for_each_nested(attr, attr_freq, tmp) {
> > > +			struct ieee80211_channel *chan;
> > > +
> > > +			chan = ieee80211_get_channel(wiphy,
> > > nla_get_u32(attr));
> > > +
> > > +			if (!chan) {
> > > +				err = -EINVAL;
> > > +				goto out_free;
> > > +			}
> > > +
> > > +			/* ignore disabled channels */
> > > +			if (chan->flags & IEEE80211_CHAN_DISABLED)
> > > +				continue;
> > > +
> > > +			params.channels[i] = chan;
> > > +			i++;
> > > +		}
> > > +		if (!i) {
> > > +			err = -EINVAL;
> > > +			goto out_free;
> > > +		}
> > > +
> > > +		params.n_channels = i;
> > > +		params.channels = channels;
> > > +	}
> > > +
> >
> > Do we need to set the default channels if the attribute is not set (or require
> that this attribute will be part of the command).
> 
> Logic is the following:
> - supplicant may omit channel list at all, this mean "use all social channels",
> kind of default behavior
> - supplicant may specify channel subset, in this case it should be not 
empty.
> 
> I'll comment it too.
> 
> > > +
> > > +	attr = info->attrs[NL80211_ATTR_IE_PROBE_RESP];
> > > +	if (attr) {
> > > +		params.probe_resp_ie_len = nla_len(attr);
> > > +		params.probe_resp_ie = nla_data(attr);
> > > +	}
> >
> > Is it valid to get Probe response IEs even if the driver did not report support
> for it?
> 
> One can't do p2p_find if it does not reply to P2P discovery probes.
> It is unrelated to answering probes in AP mode - driver may leave this to
> supplicant.

But you got NL80211_FEATURE_P2P_PROBE_RESP_OFFLOAD for driver that support probe response offloading. In case that the driver does not report this flag, wpa_supplicant should understand this and not configure the driver with the probe response IEs.

Regards,

Ilan.

Re: [PATCH v8] cfg80211: P2P find phase offload

[Johannes Berg]

On Tue, 2013-06-04 at 09:44 +0300, Vladimir Kondratiev wrote:

> +	if (genlmsg_end(msg, hdr) < 0) {
> +		nlmsg_free(msg);
> +		return;
> +	}

I'm guessing you're going to (have to) respin for Ilan's comments --
please remove the bogus error checking here then, genlmsg_end() can only
ever return >= 0 since it returns msg->len.

johannes

Re: [PATCH v8] P2P find phase offload

[Johannes Berg]

On Tue, 2013-06-04 at 09:44 +0300, Vladimir Kondratiev wrote:
> - Fix kerneldoc for start_p2p_find/stop_p2p_find (forgotten '@')
> - rebase

So you just took it from my wip branch? ;-)

I also fixed another small issue -- genlmsg_end() can't return <0. I'm
waiting for a colleague to take a quick look but then I'll put it onto
the master branch.

johannes