[PATCH 0/2] beacon measurement (beacon filtering disable)

[PATCH 0/2] beacon measurement (beacon filtering disable)

[Johannes Berg]

We have beacon filtering (to reduce host wakeups) in our device,
but for some measurement/debug purposes we need to turn it off.

This adds API for it -- comments welcome.

johannes

[PATCH 1/2] cfg80211: add beacon measurements command

[Johannes Berg]

From: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

This new nl80211 command allow a user space application to
instruct the WiFi stack to let all the beacons through so
that measurements can be conducted on these beacons (i.e.
RSSI etc...)

Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/cfg80211.h       | 12 ++++++++++++
 include/uapi/linux/nl80211.h | 10 ++++++++++
 net/wireless/nl80211.c       | 32 +++++++++++++++++++++++++++++++-
 net/wireless/rdev-ops.h      | 13 +++++++++++++
 net/wireless/trace.h         | 17 +++++++++++++++++
 5 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index aeaf6df..019e256 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2139,6 +2139,10 @@ struct cfg80211_update_ft_ies_params {
  * @crit_proto_stop: Indicates critical protocol no longer needs increased link
  *	reliability. This operation can not fail.
  * @set_coalesce: Set coalesce parameters.
+ *
+ * @beacon_measurement: Start / stops the beacon measurement. When enabled,
+ *	beacons should be let through cfg80211 to proceed to measurements (e.g.
+ *	RSSI analysis).
  */
 struct cfg80211_ops {
 	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -2376,6 +2380,10 @@ struct cfg80211_ops {
 				   struct wireless_dev *wdev);
 	int	(*set_coalesce)(struct wiphy *wiphy,
 				struct cfg80211_coalesce *coalesce);
+
+	int	(*beacon_measurement)(struct wiphy *wiphy,
+				      struct wireless_dev *wdev,
+				      bool state);
 };
 
 /*
@@ -2441,6 +2449,9 @@ struct cfg80211_ops {
  * @WIPHY_FLAG_OFFCHAN_TX: Device supports direct off-channel TX.
  * @WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL: Device supports remain-on-channel call.
  * @WIPHY_FLAG_SUPPORTS_5_10_MHZ: Device supports 5 MHz and 10 MHz channels.
+ * @WIPHY_FLAG_SUPPORTS_BEACON_MEAS: Device supports beacon measurements. Should
+ *	be set when the device can let all the beacon through up to cfg80211
+ *	to proceed to measurements (i.e. RSSI measurements).
  */
 enum wiphy_flags {
 	WIPHY_FLAG_CUSTOM_REGULATORY		= BIT(0),
@@ -2465,6 +2476,7 @@ enum wiphy_flags {
 	WIPHY_FLAG_OFFCHAN_TX			= BIT(20),
 	WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL	= BIT(21),
 	WIPHY_FLAG_SUPPORTS_5_10_MHZ		= BIT(22),
+	WIPHY_FLAG_SUPPORTS_BEACON_MEAS		= BIT(23),
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index eb68735..1a5edad 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -676,6 +676,9 @@
  * @NL80211_CMD_GET_COALESCE: Get currently supported coalesce rules.
  * @NL80211_CMD_SET_COALESCE: Configure coalesce rules or clear existing rules.
  *
+ * @NL80211_CMD_BEACON_MEASUREMENT: Indicates the beacon measurement is
+ *	started / stopped. Disassociation should stop the measurements.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -841,6 +844,8 @@ enum nl80211_commands {
 	NL80211_CMD_GET_COALESCE,
 	NL80211_CMD_SET_COALESCE,
 
+	NL80211_CMD_BEACON_MEASUREMENT,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -1469,6 +1474,9 @@ enum nl80211_commands {
  *
  * @NL80211_ATTR_COALESCE_RULE: Coalesce rule information.
  *
+ * @NL80211_ATTR_BCON_MEAS_STATE: The state of the beacon measurements. This is
+ *	a flag attribute.
+ *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
  */
@@ -1771,6 +1779,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_COALESCE_RULE,
 
+	NL80211_ATTR_BCON_MEAS_STATE,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 03d4ef9..c2b43b6 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -349,6 +349,7 @@ 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_BCON_MEAS_STATE] = { .type = NLA_FLAG, },
 };
 
 /* policy for the key attributes */
@@ -8646,6 +8647,27 @@ static int nl80211_crit_protocol_stop(struct sk_buff *skb,
 	return 0;
 }
 
+static int nl80211_beacon_measurement(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];
+	bool state;
+
+
+	if (!rdev->ops->beacon_measurement ||
+	    !(rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_BEACON_MEAS))
+		return -EOPNOTSUPP;
+
+	/* Relevant for associated mgd interfaces only */
+	if (wdev->iftype != NL80211_IFTYPE_STATION || !wdev->current_bss)
+		return -EOPNOTSUPP;
+
+	state = info->attrs[NL80211_ATTR_BCON_MEAS_STATE];
+
+	return rdev_beacon_measurement(rdev, wdev, state);
+}
+
 #define NL80211_FLAG_NEED_WIPHY		0x01
 #define NL80211_FLAG_NEED_NETDEV	0x02
 #define NL80211_FLAG_NEED_RTNL		0x04
@@ -9361,7 +9383,15 @@ static struct genl_ops nl80211_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WIPHY |
 				  NL80211_FLAG_NEED_RTNL,
-	}
+	},
+	{
+		.cmd = NL80211_CMD_BEACON_MEASUREMENT,
+		.doit = nl80211_beacon_measurement,
+		.policy = nl80211_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_NEED_RTNL,
+	},
 };
 
 static struct genl_multicast_group nl80211_mlme_mcgrp = {
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 9f15f0a..e5b0efb 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -923,4 +923,17 @@ static inline void rdev_crit_proto_stop(struct cfg80211_registered_device *rdev,
 	trace_rdev_return_void(&rdev->wiphy);
 }
 
+static inline int
+rdev_beacon_measurement(struct cfg80211_registered_device *rdev,
+			struct wireless_dev *wdev, bool state)
+{
+	int ret;
+
+	trace_rdev_beacon_measurement(&rdev->wiphy, wdev, state);
+	ret = rdev->ops->beacon_measurement(&rdev->wiphy, wdev, state);
+	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 09af6eb..5f6e7f0 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -1841,6 +1841,23 @@ TRACE_EVENT(rdev_crit_proto_stop,
 		  WIPHY_PR_ARG, WDEV_PR_ARG)
 );
 
+TRACE_EVENT(rdev_beacon_measurement,
+	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev, bool state),
+	TP_ARGS(wiphy, wdev, state),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		WDEV_ENTRY
+		__field(bool, state)
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		WDEV_ASSIGN;
+		__entry->state = state;
+	),
+	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT " %s",
+		  WIPHY_PR_ARG, WDEV_PR_ARG, BOOL_TO_STR(__entry->state))
+);
+
 /*************************************************************
  *	     cfg80211 exported functions traces		     *
  *************************************************************/
-- 
1.8.0

[PATCH 2/2] mac80211: implement the beacon measurement ops

[Johannes Berg]

From: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

This tells the low-level driver to let all the beacons
through and update the data needed to conduct the
measurements.

Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h    |  7 +++++++
 net/mac80211/cfg.c        | 14 ++++++++++++++
 net/mac80211/driver-ops.h | 20 ++++++++++++++++++++
 net/mac80211/trace.h      | 23 +++++++++++++++++++++++
 4 files changed, 64 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 3124036..fb8f6c6 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2634,6 +2634,10 @@ enum ieee80211_roc_type {
  *	driver's resume function returned 1, as this is just like an "inline"
  *	hardware restart. This callback may sleep.
  *
+ * @beacon_measurement: Start or stop beacon measurement. When started, all the
+ *	beacons must be let through and data for measurement should be set in
+ *	&ieee80211_rx_status (rssi etc...).
+ *
  * @ipv6_addr_change: IPv6 address assignment on the given interface changed.
  *	Currently, this is only called for managed or P2P client interfaces.
  *	This callback is optional; it must not sleep.
@@ -2819,6 +2823,9 @@ struct ieee80211_ops {
 
 	void (*restart_complete)(struct ieee80211_hw *hw);
 
+	int (*beacon_measurement)(struct ieee80211_hw *hw,
+				  struct ieee80211_vif *vif, bool state);
+
 #if IS_ENABLED(CONFIG_IPV6)
 	void (*ipv6_addr_change)(struct ieee80211_hw *hw,
 				 struct ieee80211_vif *vif,
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index b82fff6..34b57a9 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3407,6 +3407,19 @@ static int ieee80211_cfg_get_channel(struct wiphy *wiphy,
 	return ret;
 }
 
+static int ieee80211_beacon_measurement(struct wiphy *wiphy,
+					struct wireless_dev *wdev,
+					bool state)
+{
+	struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev);
+	struct ieee80211_local *local = wiphy_priv(wiphy);
+
+	if (WARN_ON_ONCE(sdata->vif.type != NL80211_IFTYPE_STATION))
+		return -EINVAL;
+
+	return drv_beacon_measurement(local, sdata, state);
+}
+
 #ifdef CONFIG_PM
 static void ieee80211_set_wakeup(struct wiphy *wiphy, bool enabled)
 {
@@ -3492,4 +3505,5 @@ struct cfg80211_ops mac80211_config_ops = {
 	.get_et_strings = ieee80211_get_et_strings,
 	.get_channel = ieee80211_cfg_get_channel,
 	.start_radar_detection = ieee80211_start_radar_detection,
+	.beacon_measurement = ieee80211_beacon_measurement,
 };
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index b931c96..7fbe57f 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1060,6 +1060,26 @@ drv_set_default_unicast_key(struct ieee80211_local *local,
 	trace_drv_return_void(local);
 }
 
+static inline int
+drv_beacon_measurement(struct ieee80211_local *local,
+		       struct ieee80211_sub_if_data *sdata, bool state)
+{
+	int ret = -EOPNOTSUPP;
+	check_sdata_in_driver(sdata);
+
+	trace_drv_beacon_measurement(local, sdata, state);
+
+	if (local->ops->beacon_measurement)
+		ret = local->ops->beacon_measurement(&local->hw, &sdata->vif,
+						     state);
+	else
+		/* Driver advertises caps but doesn't implement the callback? */
+		WARN_ON_ONCE(1);
+	trace_drv_return_int(local, ret);
+
+	return ret;
+}
+
 #if IS_ENABLED(CONFIG_IPV6)
 static inline void drv_ipv6_addr_change(struct ieee80211_local *local,
 					struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index c215fafd7..d41bbe2 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1887,6 +1887,29 @@ TRACE_EVENT(drv_set_default_unicast_key,
 		  LOCAL_PR_ARG, VIF_PR_ARG, __entry->key_idx)
 );
 
+TRACE_EVENT(drv_beacon_measurement,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_sub_if_data *sdata,
+		 bool state),
+
+	TP_ARGS(local, sdata, state),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		VIF_ENTRY
+		__field(bool, state)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		VIF_ASSIGN;
+		__entry->state = state;
+	),
+
+	TP_printk(LOCAL_PR_FMT VIF_PR_FMT " state:%d",
+		  LOCAL_PR_ARG, VIF_PR_ARG, __entry->state)
+);
+
 TRACE_EVENT(api_radar_detected,
 	TP_PROTO(struct ieee80211_local *local),
 
-- 
1.8.0

Re: [PATCH 0/2] beacon measurement (beacon filtering disable)

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> We have beacon filtering (to reduce host wakeups) in our device,
> but for some measurement/debug purposes we need to turn it off.

TBH I'm not really fond of this. I'm not really sure what's the use
case but first this sounded a like a factory test for me, not something
which a regular user would want to do.

Can't we connect this to power save? When disabling power save we could
also disable beacon filtering and would not need a separate command.

-- 
Kalle Valo

Re: [PATCH 0/2] beacon measurement (beacon filtering disable)

[Johannes Berg]

On Wed, 2013-07-17 at 07:10 +0300, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > We have beacon filtering (to reduce host wakeups) in our device,
> > but for some measurement/debug purposes we need to turn it off.
> 
> TBH I'm not really fond of this. I'm not really sure what's the use
> case but first this sounded a like a factory test for me, not something
> which a regular user would want to do.

Yeah, in a way that's true. FWIW, we could also connect it to testmode
and not worry about it for upstream, but it seemed that others might
want/need similar functionality.

> Can't we connect this to power save? When disabling power save we could
> also disable beacon filtering and would not need a separate command.

I'm not so sure that's a good idea. While superficially beacon filtering
is related to saving power, it's really a different thing - it's about
CPU/host power while powersave is about device power (RX chains etc.)
Connecting them, in particular where disabling beacon filtering isn't
even supported by all devices, doesn't really seem like a good idea,
particularly not for any tool that would require such functionality.

johannes

Re: [PATCH 0/2] beacon measurement (beacon filtering disable)

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2013-07-17 at 07:10 +0300, Kalle Valo wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>> 
>> > We have beacon filtering (to reduce host wakeups) in our device,
>> > but for some measurement/debug purposes we need to turn it off.
>> 
>> TBH I'm not really fond of this. I'm not really sure what's the use
>> case but first this sounded a like a factory test for me, not something
>> which a regular user would want to do.

Sure, that's good to do. I'm just worried that if we add a new command
to enable/disable each small feature we will have a lot of commands in
nl80211. But I guess that's not a problem as you are the maintainer
anyway :)

But I do see benefits from this, so I guess in the end this is good to
have. It would be nice if someone would add a similar command for BT
coexistance as well, that always seems to be a common source of
problems.

> Yeah, in a way that's true. FWIW, we could also connect it to testmode
> and not worry about it for upstream, but it seemed that others might
> want/need similar functionality.
>
>> Can't we connect this to power save? When disabling power save we could
>> also disable beacon filtering and would not need a separate command.
>
> I'm not so sure that's a good idea. While superficially beacon filtering
> is related to saving power, it's really a different thing - it's about
> CPU/host power while powersave is about device power (RX chains etc.)
> Connecting them, in particular where disabling beacon filtering isn't
> even supported by all devices, doesn't really seem like a good idea,
> particularly not for any tool that would require such functionality.

Makes sense.

-- 
Kalle Valo