[PATCH 0/3] cfg80211/mac80211: Add support to configure and monitor station's rssi threshold

[PATCH 0/3] cfg80211/mac80211: Add support to configure and monitor station's rssi threshold

[Tamizh chelvam]

This patchsets introduced new NL command and api to support
configuring rssi threshold for the connected stations and api to notify
userspace application upon crossing the configured threshold.
This will be useful for the application which requires
station's current signal strength change information.
Monitoring station's signal strength through station dump command
will unnecessarily increase the system overhead. This event based
mechanism will reduce the system overhead and helps application to
take a decision for the station for which event received.


Tamizh chelvam (3):
  cfg80211: Add support to configure station specific RSSI threshold
    for AP mode
  mac80211: Implement API to configure station specific rssi threshold
  mac80211: Implement functionality to monitor station's signal
    stregnth

 include/net/cfg80211.h       |   28 +++++++++
 include/net/mac80211.h       |    7 +++
 include/uapi/linux/nl80211.h |   18 ++++++
 net/mac80211/cfg.c           |   91 +++++++++++++++++++++++++++++
 net/mac80211/rx.c            |   51 +++++++++++++++-
 net/mac80211/sta_info.c      |    1 +
 net/mac80211/sta_info.h      |   24 ++++++++
 net/wireless/nl80211.c       |  131 +++++++++++++++++++++++++++++++++++++-----
 net/wireless/rdev-ops.h      |   18 ++++++
 9 files changed, 354 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] cfg80211: Add support to configure station specific RSSI threshold for AP mode

[Tamizh chelvam]

Add infrastructure to configure station specific RSSI threshold
configuration to monitor station's signal strength variation.
This configuration will be useful for the application like
steering which requires change in the station's current signal
strength.

New NL80211_CMD_SET_STA_MON introduced to configure the RSSI threshold
using NL80211_ATTR_CQM nested attributes. The MAC address of
a station is passed in NL80211_ATTR_MAC. And NL80211_ATTR_STA_MON_FIXED_THOLD
introduced to have this RSSI threshold as a fixed limit or moving range thresholds.
Depends on the application's use case user can have either fixed or
moving RSSI range thresholds.

cfg80211_sta_mon_rssi_notify introduced to notify change in the
station's signal stregnth cross event using NL80211_CMD_NOTIFY_STA_MON.
Driver supporting this feature should advertise
NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG feature flag.

Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
---
 include/net/cfg80211.h       |   28 +++++++++
 include/uapi/linux/nl80211.h |   18 ++++++
 net/wireless/nl80211.c       |  131 +++++++++++++++++++++++++++++++++++++-----
 net/wireless/rdev-ops.h      |   18 ++++++
 4 files changed, 181 insertions(+), 14 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 1fa41b7..8f6a906 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3183,6 +3183,12 @@ struct cfg80211_ftm_responder_stats {
  *
  * @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
  *	Statistics should be cumulative, currently no way to reset is provided.
+ *
+ * @set_sta_mon_rssi_config: Configure  RSSI threshold for a station.
+ *     After configuration, the driver should (soon) send an event indicating
+ *     the current level of a station is above/below the configured threshold;
+ *     this may need some care when the configuration is changed
+ *     (without first being disabled.)
  */
 struct cfg80211_ops {
 	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -3492,6 +3498,12 @@ struct cfg80211_ops {
 	int	(*get_ftm_responder_stats)(struct wiphy *wiphy,
 				struct net_device *dev,
 				struct cfg80211_ftm_responder_stats *ftm_stats);
+	int	(*set_sta_mon_rssi_config)(struct wiphy *wiphy,
+					  struct net_device *dev,
+					  const u8 *addr,
+					  const s32 *rssi_tholds,
+					  u32 rssi_hyst, int rssi_n_tholds,
+					  bool fixed_thold);
 };
 
 /*
@@ -6024,6 +6036,22 @@ bool cfg80211_rx_control_port(struct net_device *dev,
 			      struct sk_buff *skb, bool unencrypted);
 
 /**
+ * cfg80211_sta_mon_rssi_notify - Station's rssi out of range event
+ * @dev: network device
+ * @peer: Station's mac address
+ * @rssi_event: the triggered RSSI event
+ * @rssi_level: new RSSI level value or 0 if not available
+ * @gfp: context flags
+ *
+ * This function is called when a configured rssi threshold reached event
+ * occurs for a station.
+ */
+void
+cfg80211_sta_mon_rssi_notify(struct net_device *dev, const u8 *peer,
+			     enum nl80211_cqm_rssi_threshold_event rssi_event,
+			     s32 rssi_level, gfp_t gfp);
+
+/**
  * cfg80211_cqm_rssi_notify - connection quality monitoring rssi event
  * @dev: network device
  * @rssi_event: the triggered RSSI event
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 6d610ba..0f4cc09 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1036,6 +1036,11 @@
  * @NL80211_CMD_GET_FTM_RESPONDER_STATS: Retrieve FTM responder statistics, in
  *	the %NL80211_ATTR_FTM_RESPONDER_STATS attribute.
  *
+ * @NL80211_CMD_SET_STA_MON: This command is used to configure station's
+ *	connection monitoring notification trigger levels.
+ * @NL80211_CMD_NOTIFY_STA_MON: This is used as an event to notify
+ *	the user space that a trigger level was reached for a station.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -1250,6 +1255,9 @@ enum nl80211_commands {
 
 	NL80211_CMD_GET_FTM_RESPONDER_STATS,
 
+	NL80211_CMD_SET_STA_MON,
+	NL80211_CMD_NOTIFY_STA_MON,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -2254,6 +2262,10 @@ enum nl80211_commands {
  * @NL80211_ATTR_FTM_RESPONDER_STATS: Nested attribute with FTM responder
  *	statistics, see &enum nl80211_ftm_responder_stats.
  *
+ * @NL80211_ATTR_STA_MON_FIXED_THOLD: Flag attribute is used with
+ *	%NL80211_CMD_SET_STA_MON to indicate driver that the monitoring
+ *	configuration is fixed limit or a moving range threshold.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2699,6 +2711,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_FTM_RESPONDER_STATS,
 
+	NL80211_ATTR_STA_MON_FIXED_THOLD,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -5258,6 +5272,9 @@ enum nl80211_feature_flags {
  *      if this flag is not set. Ignoring this can leak clear text packets and/or
  *      freeze the connection.
  *
+ * @NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG: Driver supports monitoring
+ *	station's RSSI threshold value should adveritise this flag.
+ *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
@@ -5297,6 +5314,7 @@ enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
 	NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
 	NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER,
+	NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 744b585..3013c86 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -497,6 +497,7 @@ static int validate_ie_attr(const struct nlattr *attr,
 		.type = NLA_NESTED,
 		.validation_data = nl80211_ftm_responder_policy,
 	},
+	[NL80211_ATTR_STA_MON_FIXED_THOLD] = { .type = NLA_FLAG },
 };
 
 /* policy for the key attributes */
@@ -10208,23 +10209,33 @@ static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,
 	return rdev_set_cqm_rssi_range_config(rdev, dev, low, high);
 }
 
-static int nl80211_set_cqm_rssi(struct genl_info *info,
-				const s32 *thresholds, int n_thresholds,
-				u32 hysteresis)
+static int nl80211_validate_rssi_tholds(const s32 *thresholds, int n_tholds)
 {
-	struct cfg80211_registered_device *rdev = info->user_ptr[0];
-	struct net_device *dev = info->user_ptr[1];
-	struct wireless_dev *wdev = dev->ieee80211_ptr;
-	int i, err;
+	int i;
 	s32 prev = S32_MIN;
 
 	/* Check all values negative and sorted */
-	for (i = 0; i < n_thresholds; i++) {
+	for (i = 0; i < n_tholds; i++) {
 		if (thresholds[i] > 0 || thresholds[i] <= prev)
 			return -EINVAL;
 
 		prev = thresholds[i];
 	}
+	return 0;
+}
+
+static int nl80211_set_cqm_rssi(struct genl_info *info,
+				const s32 *thresholds, int n_thresholds,
+				u32 hysteresis)
+{
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct net_device *dev = info->user_ptr[1];
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	int err;
+
+	err = nl80211_validate_rssi_tholds(thresholds, n_thresholds);
+	if (err)
+		return err;
 
 	if (wdev->iftype != NL80211_IFTYPE_STATION &&
 	    wdev->iftype != NL80211_IFTYPE_P2P_CLIENT)
@@ -12980,6 +12991,55 @@ static int nl80211_get_ftm_responder_stats(struct sk_buff *skb,
 	return -ENOBUFS;
 }
 
+static int nl80211_set_sta_mon(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 nlattr *attrs[NL80211_ATTR_CQM_MAX + 1];
+	bool fixed_thold = false;
+	struct nlattr *sta_mon;
+	u8 *addr = NULL;
+	int err;
+
+	if (!wiphy_ext_feature_isset(&rdev->wiphy,
+				     NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG))
+		return -EOPNOTSUPP;
+
+	sta_mon = info->attrs[NL80211_ATTR_CQM];
+	if (!sta_mon || !(info->attrs[NL80211_ATTR_MAC]))
+		return -EINVAL;
+
+	err = nla_parse_nested(attrs, NL80211_ATTR_CQM_MAX, sta_mon,
+			       nl80211_attr_cqm_policy, info->extack);
+	if (err)
+		return err;
+
+	addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
+	fixed_thold =
+		nla_get_flag(info->attrs[NL80211_ATTR_STA_MON_FIXED_THOLD]);
+
+	if (attrs[NL80211_ATTR_CQM_RSSI_THOLD] &&
+	    attrs[NL80211_ATTR_CQM_RSSI_HYST]) {
+		const s32 *thresholds =
+			nla_data(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
+		int len = nla_len(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
+		u32 hysteresis = nla_get_u32(attrs[NL80211_ATTR_CQM_RSSI_HYST]);
+
+		if (len % 4)
+			return -EINVAL;
+
+		err = nl80211_validate_rssi_tholds(thresholds, len / 4);
+		if (err)
+			return err;
+
+		return rdev_set_sta_mon_rssi_config(rdev, dev, addr, thresholds,
+						    hysteresis, len / 4,
+						    fixed_thold);
+	}
+
+	return -EINVAL;
+}
+
 #define NL80211_FLAG_NEED_WIPHY		0x01
 #define NL80211_FLAG_NEED_NETDEV	0x02
 #define NL80211_FLAG_NEED_RTNL		0x04
@@ -13898,6 +13958,14 @@ static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
 		.internal_flags = NL80211_FLAG_NEED_NETDEV |
 				  NL80211_FLAG_NEED_RTNL,
 	},
+	{
+		.cmd = NL80211_CMD_SET_STA_MON,
+		.doit = nl80211_set_sta_mon,
+		.policy = nl80211_policy,
+		.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 = {
@@ -15093,7 +15161,8 @@ bool cfg80211_rx_control_port(struct net_device *dev,
 EXPORT_SYMBOL(cfg80211_rx_control_port);
 
 static struct sk_buff *cfg80211_prepare_cqm(struct net_device *dev,
-					    const char *mac, gfp_t gfp)
+					    const char *mac, gfp_t gfp,
+					    enum nl80211_commands cmd)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
@@ -15105,7 +15174,7 @@ static struct sk_buff *cfg80211_prepare_cqm(struct net_device *dev,
 
 	cb = (void **)msg->cb;
 
-	cb[0] = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_NOTIFY_CQM);
+	cb[0] = nl80211hdr_put(msg, 0, 0, 0, cmd);
 	if (!cb[0]) {
 		nlmsg_free(msg);
 		return NULL;
@@ -15167,7 +15236,7 @@ void cfg80211_cqm_rssi_notify(struct net_device *dev,
 			rssi_level = wdev->cqm_config->last_rssi_event_value;
 	}
 
-	msg = cfg80211_prepare_cqm(dev, NULL, gfp);
+	msg = cfg80211_prepare_cqm(dev, NULL, gfp, NL80211_CMD_NOTIFY_CQM);
 	if (!msg)
 		return;
 
@@ -15188,13 +15257,47 @@ void cfg80211_cqm_rssi_notify(struct net_device *dev,
 }
 EXPORT_SYMBOL(cfg80211_cqm_rssi_notify);
 
+void
+cfg80211_sta_mon_rssi_notify(struct net_device *dev, const u8 *peer,
+			     enum nl80211_cqm_rssi_threshold_event rssi_event,
+			     s32 rssi_level, gfp_t gfp)
+{
+	struct sk_buff *msg;
+
+	if (WARN_ON(!peer))
+		return;
+
+	if (WARN_ON(rssi_event != NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW &&
+		    rssi_event != NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH))
+		return;
+
+	msg = cfg80211_prepare_cqm(dev, peer, gfp, NL80211_CMD_NOTIFY_STA_MON);
+	if (!msg)
+		return;
+
+	if (nla_put_u32(msg, NL80211_ATTR_CQM_RSSI_THRESHOLD_EVENT,
+			rssi_event))
+		goto nla_put_failure;
+
+	if (rssi_level && nla_put_s32(msg, NL80211_ATTR_CQM_RSSI_LEVEL,
+				      rssi_level))
+		goto nla_put_failure;
+
+	cfg80211_send_cqm(msg, gfp);
+	return;
+
+ nla_put_failure:
+	nlmsg_free(msg);
+}
+EXPORT_SYMBOL(cfg80211_sta_mon_rssi_notify);
+
 void cfg80211_cqm_txe_notify(struct net_device *dev,
 			     const u8 *peer, u32 num_packets,
 			     u32 rate, u32 intvl, gfp_t gfp)
 {
 	struct sk_buff *msg;
 
-	msg = cfg80211_prepare_cqm(dev, peer, gfp);
+	msg = cfg80211_prepare_cqm(dev, peer, gfp, NL80211_CMD_NOTIFY_CQM);
 	if (!msg)
 		return;
 
@@ -15222,7 +15325,7 @@ void cfg80211_cqm_pktloss_notify(struct net_device *dev,
 
 	trace_cfg80211_cqm_pktloss_notify(dev, peer, num_packets);
 
-	msg = cfg80211_prepare_cqm(dev, peer, gfp);
+	msg = cfg80211_prepare_cqm(dev, peer, gfp, NL80211_CMD_NOTIFY_CQM);
 	if (!msg)
 		return;
 
@@ -15241,7 +15344,7 @@ void cfg80211_cqm_beacon_loss_notify(struct net_device *dev, gfp_t gfp)
 {
 	struct sk_buff *msg;
 
-	msg = cfg80211_prepare_cqm(dev, NULL, gfp);
+	msg = cfg80211_prepare_cqm(dev, NULL, gfp, NL80211_CMD_NOTIFY_CQM);
 	if (!msg)
 		return;
 
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 51380b5..b4ac0ad 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1247,4 +1247,22 @@ static inline int rdev_del_pmk(struct cfg80211_registered_device *rdev,
 	return ret;
 }
 
+static inline int
+rdev_set_sta_mon_rssi_config(struct cfg80211_registered_device *rdev,
+			     struct net_device *dev, const u8 *addr,
+			     const s32 *rssi_tholds, u32 rssi_hyst,
+			     int n_rssi_tholds, bool fixed_thold)
+{
+	int ret = -EOPNOTSUPP;
+
+	if (rdev->ops->set_sta_mon_rssi_config)
+		ret = rdev->ops->set_sta_mon_rssi_config(&rdev->wiphy, dev,
+							 addr, rssi_tholds,
+							 rssi_hyst,
+							 n_rssi_tholds,
+							 fixed_thold);
+	trace_rdev_return_int(&rdev->wiphy, ret);
+	return ret;
+}
+
 #endif /* __CFG80211_RDEV_OPS */
-- 
1.7.9.5

Re: [PATCH 1/3] cfg80211: Add support to configure station specific RSSI threshold for AP mode

[Sergey Matyukevich]

> Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
> ---
>  include/net/cfg80211.h       |   28 +++++++++
>  include/uapi/linux/nl80211.h |   18 ++++++
>  net/wireless/nl80211.c       |  131 +++++++++++++++++++++++++++++++++++++-----
>  net/wireless/rdev-ops.h      |   18 ++++++
>  4 files changed, 181 insertions(+), 14 deletions(-)

...

> +static int nl80211_set_sta_mon(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 nlattr *attrs[NL80211_ATTR_CQM_MAX + 1];
> +       bool fixed_thold = false;
> +       struct nlattr *sta_mon;
> +       u8 *addr = NULL;
> +       int err;
> +
> +       if (!wiphy_ext_feature_isset(&rdev->wiphy,
> +                                    NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG))
> +               return -EOPNOTSUPP;
> +
> +       sta_mon = info->attrs[NL80211_ATTR_CQM];
> +       if (!sta_mon || !(info->attrs[NL80211_ATTR_MAC]))
> +               return -EINVAL;
> +
> +       err = nla_parse_nested(attrs, NL80211_ATTR_CQM_MAX, sta_mon,
> +                              nl80211_attr_cqm_policy, info->extack);
> +       if (err)
> +               return err;
> +
> +       addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
> +       fixed_thold =
> +               nla_get_flag(info->attrs[NL80211_ATTR_STA_MON_FIXED_THOLD]);
> +
> +       if (attrs[NL80211_ATTR_CQM_RSSI_THOLD] &&
> +           attrs[NL80211_ATTR_CQM_RSSI_HYST]) {
> +               const s32 *thresholds =
> +                       nla_data(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
> +               int len = nla_len(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
> +               u32 hysteresis = nla_get_u32(attrs[NL80211_ATTR_CQM_RSSI_HYST]);
> +
> +               if (len % 4)
> +                       return -EINVAL;
> +
> +               err = nl80211_validate_rssi_tholds(thresholds, len / 4);
> +               if (err)
> +                       return err;
> +
> +               return rdev_set_sta_mon_rssi_config(rdev, dev, addr, thresholds,
> +                                                   hysteresis, len / 4,
> +                                                   fixed_thold);
> +       }
> +
> +       return -EINVAL;
> +}

IIUC there is a noticeable overlap between this new command and
existing nl80211_set_cqm_rssi command. Is there any reason why
nl80211_set_cqm_rss can not be adapted for AP case ?

If there is a reason to handle AP case separately, then it looks
like it makes sense to add explicit check for supported iftype
in this new command. Besides, it looks like there is no generic
way to handle disabling of RSSI monitoring in the new command.
As a result, we may end up in multiple driver
specific implementations.

Regards,
Sergey

RE: [EXTERNAL] Re: [PATCH 1/3] cfg80211: Add support to configure station specific RSSI threshold for AP mode

[Tamizh Chelvam Raja]

>> +static int nl80211_set_sta_mon(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 nlattr *attrs[NL80211_ATTR_CQM_MAX + 1];
>> +       bool fixed_thold = false;
>> +       struct nlattr *sta_mon;
>> +       u8 *addr = NULL;
>> +       int err;
>> +
>> +       if (!wiphy_ext_feature_isset(&rdev->wiphy,
>> +                                    NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG))
>> +               return -EOPNOTSUPP;
>> +
>> +       sta_mon = info->attrs[NL80211_ATTR_CQM];
>> +       if (!sta_mon || !(info->attrs[NL80211_ATTR_MAC]))
>> +               return -EINVAL;
>> +
>> +       err = nla_parse_nested(attrs, NL80211_ATTR_CQM_MAX, sta_mon,
>> +                              nl80211_attr_cqm_policy, info->extack);
>> +       if (err)
>> +               return err;
>> +
>> +       addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
>> +       fixed_thold =
>> +               
>> + nla_get_flag(info->attrs[NL80211_ATTR_STA_MON_FIXED_THOLD]);
>> +
>> +       if (attrs[NL80211_ATTR_CQM_RSSI_THOLD] &&
>> +           attrs[NL80211_ATTR_CQM_RSSI_HYST]) {
>> +               const s32 *thresholds =
>> +                       nla_data(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
>> +               int len = nla_len(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
>> +               u32 hysteresis = 
>> + nla_get_u32(attrs[NL80211_ATTR_CQM_RSSI_HYST]);
>> +
>> +               if (len % 4)
>> +                       return -EINVAL;
>> +
>> +               err = nl80211_validate_rssi_tholds(thresholds, len / 4);
>> +               if (err)
>> +                       return err;
>> +
>> +               return rdev_set_sta_mon_rssi_config(rdev, dev, addr, thresholds,
>> +                                                   hysteresis, len / 4,
>> +                                                   fixed_thold);
>> +       }
>> +
>> +       return -EINVAL;
>> +}

>IIUC there is a noticeable overlap between this new command and existing nl80211_set_cqm_rssi command. Is there any reason why >nl80211_set_cqm_rss can not be adapted for AP case ?

[Tamizh] This new command for AP mode introduced as per the previous discussion. 

>If there is a reason to handle AP case separately, then it looks like it makes sense to add explicit check for supported iftype in this new command. >Besides, it looks like there is no generic way to handle disabling of RSSI monitoring in the new command.
>As a result, we may end up in multiple driver specific implementations.

[Tamizh] thanks for pointing me out. I'll add the ifmode check in the next patchset version.

Thanks,
Tamizh.

Re: [PATCH 1/3] cfg80211: Add support to configure station specific RSSI threshold for AP mode

[Johannes Berg]

Hi,

Sorry for the delay in reviewing this.

> +	int	(*set_sta_mon_rssi_config)(struct wiphy *wiphy,
> +					  struct net_device *dev,
> +					  const u8 *addr,
> +					  const s32 *rssi_tholds,
> +					  u32 rssi_hyst, int rssi_n_tholds,
> +					  bool fixed_thold);
>  };

I think it might be better to pass all the last 4 arguments (rssi
related ones) as a struct? That's a pattern we typically have elsewhere
too, and it makes things easier to extend and also easier to pass
around.

> + * @NL80211_CMD_SET_STA_MON: This command is used to configure station's
> + *     connection monitoring notification trigger levels.
> + * @NL80211_CMD_NOTIFY_STA_MON: This is used as an event to notify
> + *     the user space that a trigger level was reached for a station.

Please describe the attributes to use with this.


> + * @NL80211_ATTR_STA_MON_FIXED_THOLD: Flag attribute is used with
> + *	%NL80211_CMD_SET_STA_MON to indicate driver that the monitoring
> + *	configuration is fixed limit or a moving range threshold.

This isn't really clear to me, what does it mean?

> +	if (!sta_mon || !(info->attrs[NL80211_ATTR_MAC]))

Don't really need the parentheses in !(info->...)

> +	err = nla_parse_nested(attrs, NL80211_ATTR_CQM_MAX, sta_mon,
> +			       nl80211_attr_cqm_policy, info->extack);

I *think* I made that a proper nested policy, check and then you can
remove passing it here.

> +void
> +cfg80211_sta_mon_rssi_notify(struct net_device *dev, const u8 *peer,
> +			     enum nl80211_cqm_rssi_threshold_event rssi_event,
> +			     s32 rssi_level, gfp_t gfp)
> +{
> +	struct sk_buff *msg;
> +
> +	if (WARN_ON(!peer))
> +		return;

Tracing for this might be nice too?

johannes

Re: [PATCH 1/3] cfg80211: Add support to configure station specific RSSI threshold for AP mode

[Tamizh chelvam]

>> +	int	(*set_sta_mon_rssi_config)(struct wiphy *wiphy,
>> +					  struct net_device *dev,
>> +					  const u8 *addr,
>> +					  const s32 *rssi_tholds,
>> +					  u32 rssi_hyst, int rssi_n_tholds,
>> +					  bool fixed_thold);
>>  };
> 
> I think it might be better to pass all the last 4 arguments (rssi
> related ones) as a struct? That's a pattern we typically have elsewhere
> too, and it makes things easier to extend and also easier to pass
> around.
> 
Sure.

>> + * @NL80211_CMD_SET_STA_MON: This command is used to configure 
>> station's
>> + *     connection monitoring notification trigger levels.
>> + * @NL80211_CMD_NOTIFY_STA_MON: This is used as an event to notify
>> + *     the user space that a trigger level was reached for a station.
> 
> Please describe the attributes to use with this.
> 
I'll add that in the next version of patchset
> 
>> + * @NL80211_ATTR_STA_MON_FIXED_THOLD: Flag attribute is used with
>> + *	%NL80211_CMD_SET_STA_MON to indicate driver that the monitoring
>> + *	configuration is fixed limit or a moving range threshold.
> 
> This isn't really clear to me, what does it mean?
> 
Sorry for not clear. This flag introduced to mention the driver that if 
this flag set then don't change the rssi_low and rssi_high limit upon
the station's rssi crossing the configured limit. Keep the rssi_low and 
rssi_high as a fixed limit.

>> +	if (!sta_mon || !(info->attrs[NL80211_ATTR_MAC]))
> 
> Don't really need the parentheses in !(info->...)
> 
Yes, I'll remove it.

>> +	err = nla_parse_nested(attrs, NL80211_ATTR_CQM_MAX, sta_mon,
>> +			       nl80211_attr_cqm_policy, info->extack);
> 
> I *think* I made that a proper nested policy, check and then you can
> remove passing it here.
> 
Sure. I'll modify it in the next patchset version.

>> +void
>> +cfg80211_sta_mon_rssi_notify(struct net_device *dev, const u8 *peer,
>> +			     enum nl80211_cqm_rssi_threshold_event rssi_event,
>> +			     s32 rssi_level, gfp_t gfp)
>> +{
>> +	struct sk_buff *msg;
>> +
>> +	if (WARN_ON(!peer))
>> +		return;
> 
> Tracing for this might be nice too?
> 
Sure.

Thanks,
Tamizh.

[PATCH 2/3] mac80211: Implement API to configure station specific rssi threshold

[Tamizh chelvam]

Implement set_sta_mon_rssi_config API to configure station
specific rssi threshold value to monitor change in connected
station's signal strength.

Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
---
 net/mac80211/cfg.c      |   91 +++++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/sta_info.c |    1 +
 net/mac80211/sta_info.h |   19 ++++++++++
 3 files changed, 111 insertions(+)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 5162233..72d2b07 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3849,6 +3849,96 @@ static int ieee80211_get_txq_stats(struct wiphy *wiphy,
 	return drv_get_ftm_responder_stats(local, sdata, ftm_stats);
 }
 
+void sta_mon_rssi_config_free(struct sta_info *sta)
+{
+	kfree(sta->rssi_tholds);
+	sta->rssi_tholds = NULL;
+	sta->n_rssi_tholds = 0;
+}
+
+static void ieee80211_update_rssi_config(struct sta_info *sta)
+{
+	s32 last;
+	u32 hyst;
+	int i, n;
+
+	if (!sta->n_rssi_tholds)
+		return;
+
+	if (!sta->last_rssi_event_value)
+		sta->last_rssi_event_value =
+			-ewma_signal_read(&sta->rx_stats_avg.signal);
+
+	last = sta->last_rssi_event_value;
+	hyst = sta->rssi_hyst;
+	n = sta->n_rssi_tholds;
+
+	for (i = 0; i < n; i++)
+		if (last < sta->rssi_tholds[i])
+			break;
+
+	sta->rssi_low = i > 0 ? (sta->rssi_tholds[i - 1] - hyst) : S32_MIN;
+	sta->rssi_high = i < n ? (sta->rssi_tholds[i] + hyst - 1) : S32_MAX;
+}
+
+static int ieee80211_set_sta_mon_rssi_config(struct wiphy *wiphy,
+					     struct net_device *dev,
+					     const u8 *mac_addr,
+					     const s32 *rssi_tholds,
+					     u32 rssi_hyst, int n_rssi_tholds,
+					     bool fixed_thold)
+{
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct ieee80211_local *local = sdata->local;
+	struct sta_info *sta;
+	int ret = 0;
+
+	mutex_lock(&local->sta_mtx);
+
+	if (mac_addr) {
+		sta = sta_info_get_bss(sdata, mac_addr);
+		if (!sta) {
+			ret = -ENOENT;
+			goto out;
+		}
+
+		sta_mon_rssi_config_free(sta);
+		sta->rssi_hyst = rssi_hyst;
+		if (fixed_thold) {
+			if (n_rssi_tholds > 2) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			if (n_rssi_tholds == 1) {
+				sta->rssi_low = rssi_tholds[0];
+				sta->rssi_high = rssi_tholds[0];
+			} else {
+				sta->rssi_low = rssi_tholds[0];
+				sta->rssi_high = rssi_tholds[1];
+			}
+		} else {
+			const s32 *rssi_tholds;
+
+			rssi_tholds = kmemdup(rssi_tholds,
+					      n_rssi_tholds * sizeof(s32),
+					      GFP_KERNEL);
+			if (!rssi_tholds) {
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			sta->rssi_tholds = rssi_tholds;
+			sta->n_rssi_tholds = n_rssi_tholds;
+			ieee80211_update_rssi_config(sta);
+		}
+	}
+
+out:
+	mutex_unlock(&local->sta_mtx);
+	return ret;
+}
+
 const struct cfg80211_ops mac80211_config_ops = {
 	.add_virtual_intf = ieee80211_add_iface,
 	.del_virtual_intf = ieee80211_del_iface,
@@ -3944,4 +4034,5 @@ static int ieee80211_get_txq_stats(struct wiphy *wiphy,
 	.tx_control_port = ieee80211_tx_control_port,
 	.get_txq_stats = ieee80211_get_txq_stats,
 	.get_ftm_responder_stats = ieee80211_get_ftm_responder_stats,
+	.set_sta_mon_rssi_config = ieee80211_set_sta_mon_rssi_config,
 };
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index fb8c225..28e9a6b 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1020,6 +1020,7 @@ static void __sta_info_destroy_part2(struct sta_info *sta)
 
 	rate_control_remove_sta_debugfs(sta);
 	ieee80211_sta_debugfs_remove(sta);
+	sta_mon_rssi_config_free(sta);
 
 	cleanup_single_sta(sta);
 }
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 9a04327..acbad98 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -411,6 +411,9 @@ struct ieee80211_sta_rx_stats {
 	u64 msdu[IEEE80211_NUM_TIDS + 1];
 };
 
+struct sta_mon_rssi_config {
+};
+
 /*
  * The bandwidth threshold below which the per-station CoDel parameters will be
  * scaled to be more lenient (to prevent starvation of slow stations). This
@@ -482,6 +485,14 @@ struct ieee80211_sta_rx_stats {
  * @pcpu_rx_stats: per-CPU RX statistics, assigned only if the driver needs
  *	this (by advertising the USES_RSS hw flag)
  * @status_stats: TX status statistics
+ * @n_rssi_tholds: Number of thresholds passed by user
+ * @rssi_tholds: RSSI threshold limit passed by the user
+ * @rssi_low: RSSI lower threshold for this station, a zero value implies
+ *	disabled
+ * @rssi_high: RSSI upper threshold for this station
+ * @rssi_hyst: RSSI hysteresis for this station
+ * @last_rssi_event_value: Last RSSI value for this station triggered the
+ *	RSSI cross event.
  */
 struct sta_info {
 	/* General information, mostly static */
@@ -583,6 +594,13 @@ struct sta_info {
 
 	struct cfg80211_chan_def tdls_chandef;
 
+	int n_rssi_tholds;
+	const s32 *rssi_tholds;
+	s32 rssi_low;
+	s32 rssi_high;
+	u32 rssi_hyst;
+	s32 last_rssi_event_value;
+
 	/* keep last! */
 	struct ieee80211_sta sta;
 };
@@ -720,6 +738,7 @@ int sta_info_destroy_addr(struct ieee80211_sub_if_data *sdata,
 			  const u8 *addr);
 int sta_info_destroy_addr_bss(struct ieee80211_sub_if_data *sdata,
 			      const u8 *addr);
+void sta_mon_rssi_config_free(struct sta_info *sta);
 
 void sta_info_recalc_tim(struct sta_info *sta);
 
-- 
1.7.9.5

Re: [PATCH 2/3] mac80211: Implement API to configure station specific rssi threshold

[Johannes Berg]

On Mon, 2018-10-15 at 23:27 +0530, Tamizh chelvam wrote:
> 
> +		sta_mon_rssi_config_free(sta);
> +		sta->rssi_hyst = rssi_hyst;
> +		if (fixed_thold) {
> +			if (n_rssi_tholds > 2) {
> +				ret = -EINVAL;
> +				goto out;
> +			}

This might be slightly wrong, you free and then can still return an
error.

> +			if (n_rssi_tholds == 1) {
> +				sta->rssi_low = rssi_tholds[0];
> +				sta->rssi_high = rssi_tholds[0];
> +			} else {
> +				sta->rssi_low = rssi_tholds[0];
> +				sta->rssi_high = rssi_tholds[1];
> +			}
> +		} else {
> +			const s32 *rssi_tholds;
> +
> +			rssi_tholds = kmemdup(rssi_tholds,
> +					      n_rssi_tholds * sizeof(s32),
> +					      GFP_KERNEL);
> +			if (!rssi_tholds) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}

Similarly here, I guess you should do the allocation (and other error
checking) before freeing.

> +			sta->rssi_tholds = rssi_tholds;
> +			sta->n_rssi_tholds = n_rssi_tholds;
> +			ieee80211_update_rssi_config(sta);



> +struct sta_mon_rssi_config {
> +};

Huh?


The commit log also makes it sounds like mac80211 actually *supports*
this, but clearly that's not the case. However you don't give any data
to the driver either, did you lose a patch along the way? Previously you
had patch 5 ("mac80211: Implement functionality to monitor station's
rssi cross event") and if I remember correctly I said you should squash
some, but now that's not here?

johannes

Re: [PATCH 2/3] mac80211: Implement API to configure station specific rssi threshold

[Tamizh chelvam]

On 2018-11-09 17:19, Johannes Berg wrote:
> On Mon, 2018-10-15 at 23:27 +0530, Tamizh chelvam wrote:
>> 
>> +		sta_mon_rssi_config_free(sta);
>> +		sta->rssi_hyst = rssi_hyst;
>> +		if (fixed_thold) {
>> +			if (n_rssi_tholds > 2) {
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
> 
> This might be slightly wrong, you free and then can still return an
> error.
> 
Sure. I'll move the free part after the validation check.

>> +			if (n_rssi_tholds == 1) {
>> +				sta->rssi_low = rssi_tholds[0];
>> +				sta->rssi_high = rssi_tholds[0];
>> +			} else {
>> +				sta->rssi_low = rssi_tholds[0];
>> +				sta->rssi_high = rssi_tholds[1];
>> +			}
>> +		} else {
>> +			const s32 *rssi_tholds;
>> +
>> +			rssi_tholds = kmemdup(rssi_tholds,
>> +					      n_rssi_tholds * sizeof(s32),
>> +					      GFP_KERNEL);
>> +			if (!rssi_tholds) {
>> +				ret = -ENOMEM;
>> +				goto out;
>> +			}
> 
> Similarly here, I guess you should do the allocation (and other error
> checking) before freeing.
> 
ditto, Sure. I'll move the free part after the validation check.

>> +			sta->rssi_tholds = rssi_tholds;
>> +			sta->n_rssi_tholds = n_rssi_tholds;
>> +			ieee80211_update_rssi_config(sta);
> 
> 
> 
>> +struct sta_mon_rssi_config {
>> +};
> 
> Huh?
> 
oops:( I have kept all configuring parameters in sta_info itself, 
mistakenly didn't remove this struct:(
> 
> The commit log also makes it sounds like mac80211 actually *supports*
> this, but clearly that's not the case. However you don't give any data
> to the driver either, did you lose a patch along the way? Previously 
> you
> had patch 5 ("mac80211: Implement functionality to monitor station's
> rssi cross event") and if I remember correctly I said you should squash
> some, but now that's not here?
> 

Thanks,
Tamizh.

[PATCH 3/3] mac80211: Implement functionality to monitor station's signal stregnth

[Tamizh chelvam]

Triggers cfg80211_sta_mon_rssi_notify with the corresponding event when
station signal goes out of configured threshold. It uses rx data signal
to check against rssi threshold configured by the user. And update
the lower and upper RSSI threshold for the station if it is not
configured as a fixed threshold.
This event will be useful for the application like steering to take
decision on any station depends on its current link quality.

Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
---
 include/net/mac80211.h  |    7 +++++++
 net/mac80211/cfg.c      |    2 +-
 net/mac80211/rx.c       |   51 ++++++++++++++++++++++++++++++++++++++++++++++-
 net/mac80211/sta_info.h |    5 +++++
 4 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 71985e9..355e522 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -849,6 +849,13 @@ enum mac80211_rate_control_flags {
 };
 
 
+/*
+ * How many frames need to have been used in average station's
+ * signal strength before checking against the threshold
+ */
+#define IEEE80211_STA_SIGNAL_AVE_MIN_COUNT	4
+
+
 /* there are 40 bytes if you don't need the rateset to be kept */
 #define IEEE80211_TX_INFO_DRIVER_DATA_SIZE 40
 
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 72d2b07..be386ff 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3856,7 +3856,7 @@ void sta_mon_rssi_config_free(struct sta_info *sta)
 	sta->n_rssi_tholds = 0;
 }
 
-static void ieee80211_update_rssi_config(struct sta_info *sta)
+void ieee80211_update_rssi_config(struct sta_info *sta)
 {
 	s32 last;
 	u32 hyst;
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 3bd3b57..1afef40 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1681,6 +1681,52 @@ void ieee80211_sta_uapsd_trigger(struct ieee80211_sta *pubsta, u8 tid)
 	return RX_CONTINUE;
 }
 
+static void ieee80211_sta_rx_signal_thold_check(struct ieee80211_rx_data *rx)
+{
+	struct sta_info *sta = rx->sta;
+	struct ieee80211_bss_conf *bss_conf = &rx->sdata->vif.bss_conf;
+	bool rssi_cross =  false;
+
+	if (!wiphy_ext_feature_isset(rx->local->hw.wiphy,
+				NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG))
+		return;
+
+	sta->count_rx_signal++;
+	if (sta->count_rx_signal < IEEE80211_STA_SIGNAL_AVE_MIN_COUNT)
+		return;
+
+	if (sta->rssi_low && bss_conf->enable_beacon) {
+		int last_event =
+			sta->last_rssi_event_value;
+		int sig = -ewma_signal_read(&sta->rx_stats_avg.signal);
+		int low = sta->rssi_low;
+		int high = sta->rssi_high;
+
+		if (sig < low &&
+		    (last_event == 0 || last_event >= low)) {
+			sta->last_rssi_event_value = sig;
+			cfg80211_sta_mon_rssi_notify(
+				rx->sdata->dev, sta->addr,
+				NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW,
+				sig, GFP_ATOMIC);
+			rssi_cross = true;
+		} else if (sig > high &&
+			   (last_event == 0 || last_event <= high)) {
+			sta->last_rssi_event_value = sig;
+			cfg80211_sta_mon_rssi_notify(
+				rx->sdata->dev, sta->addr,
+				NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH,
+				sig, GFP_ATOMIC);
+			rssi_cross = true;
+		}
+	}
+
+	if (rssi_cross) {
+		ieee80211_update_rssi_config(sta);
+		rssi_cross = false;
+	}
+}
+
 static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
 {
@@ -1736,6 +1782,7 @@ void ieee80211_sta_uapsd_trigger(struct ieee80211_sta *pubsta, u8 tid)
 	if (!(status->flag & RX_FLAG_NO_SIGNAL_VAL)) {
 		sta->rx_stats.last_signal = status->signal;
 		ewma_signal_add(&sta->rx_stats_avg.signal, -status->signal);
+		ieee80211_sta_rx_signal_thold_check(rx);
 	}
 
 	if (status->chains) {
@@ -4177,9 +4224,11 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
 	/* statistics part of ieee80211_rx_h_sta_process() */
 	if (!(status->flag & RX_FLAG_NO_SIGNAL_VAL)) {
 		stats->last_signal = status->signal;
-		if (!fast_rx->uses_rss)
+		if (!fast_rx->uses_rss) {
 			ewma_signal_add(&sta->rx_stats_avg.signal,
 					-status->signal);
+			ieee80211_sta_rx_signal_thold_check(rx);
+		}
 	}
 
 	if (status->chains) {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index acbad98..ebf5cb0 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -493,6 +493,9 @@ struct sta_mon_rssi_config {
  * @rssi_hyst: RSSI hysteresis for this station
  * @last_rssi_event_value: Last RSSI value for this station triggered the
  *	RSSI cross event.
+ * @count_rx_signal: Number of data frames used in averaging station signal.
+ *	This can be used to avoid generating less reliable station rssi cross
+ *	events that would be based only on couple of received frames.
  */
 struct sta_info {
 	/* General information, mostly static */
@@ -600,6 +603,7 @@ struct sta_info {
 	s32 rssi_high;
 	u32 rssi_hyst;
 	s32 last_rssi_event_value;
+	unsigned int count_rx_signal;
 
 	/* keep last! */
 	struct ieee80211_sta sta;
@@ -771,6 +775,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata,
 			  unsigned long exp_time);
 u8 sta_info_tx_streams(struct sta_info *sta);
+void ieee80211_update_rssi_config(struct sta_info *sta);
 
 void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta);
 void ieee80211_sta_ps_deliver_poll_response(struct sta_info *sta);
-- 
1.7.9.5

Re: [PATCH 3/3] mac80211: Implement functionality to monitor station's signal stregnth

[Sergey Matyukevich]

> +/*
> + * How many frames need to have been used in average station's
> + * signal strength before checking against the threshold
> + */
> +#define IEEE80211_STA_SIGNAL_AVE_MIN_COUNT     4
> +
> +
>  /* there are 40 bytes if you don't need the rateset to be kept */
>  #define IEEE80211_TX_INFO_DRIVER_DATA_SIZE 40

...

> +static void ieee80211_sta_rx_signal_thold_check(struct ieee80211_rx_data *rx)
> +{
> +       struct sta_info *sta = rx->sta;
> +       struct ieee80211_bss_conf *bss_conf = &rx->sdata->vif.bss_conf;
> +       bool rssi_cross =  false;
> +
> +       if (!wiphy_ext_feature_isset(rx->local->hw.wiphy,
> +                               NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG))
> +               return;
> +
> +       sta->count_rx_signal++;
> +       if (sta->count_rx_signal < IEEE80211_STA_SIGNAL_AVE_MIN_COUNT)
> +               return;

Could you please clarify count_rx_signal processing and averaging
approach ? I couldn't find where this counter is reset or
modified in any other way.

Regards,
Sergey

Re: [PATCH 3/3] mac80211: Implement functionality to monitor station's signal stregnth

[Johannes Berg]

Oh, umm, that patch is still here ...

I guess we can combine 2 and 3 too.


> +	if (sta->rssi_low && bss_conf->enable_beacon) {
> +		int last_event =
> +			sta->last_rssi_event_value;
> +		int sig = -ewma_signal_read(&sta->rx_stats_avg.signal);
> +		int low = sta->rssi_low;
> +		int high = sta->rssi_high;

This doesn't really support a list, like in patch 2 where you store
sta_info::rssi_tholds?

> +		if (sig < low &&
> +		    (last_event == 0 || last_event >= low)) {
> +			sta->last_rssi_event_value = sig;
> +			cfg80211_sta_mon_rssi_notify(
> +				rx->sdata->dev, sta->addr,
> +				NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW,
> +				sig, GFP_ATOMIC);
> +			rssi_cross = true;
> +		} else if (sig > high &&
> +			   (last_event == 0 || last_event <= high)) {
> +			sta->last_rssi_event_value = sig;
> +			cfg80211_sta_mon_rssi_notify(
> +				rx->sdata->dev, sta->addr,
> +				NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH,
> +				sig, GFP_ATOMIC);
> +			rssi_cross = true;
> +		}
> +	}
> +
> +	if (rssi_cross) {
> +		ieee80211_update_rssi_config(sta);
> +		rssi_cross = false;
> +	}
> +}

Ah, but it does, just hard to understand.

However, this does mean what I suspected on the other patch is true -
you're calling ieee80211_update_rssi_config() here, and that uses the
sta->rssi_tholds array without any protection, while a concurrent change
of configuration can free the data.

You need to sort that out. I would suggest to stick all the sta->rssi_*
fields you add into a new struct (you even had an empty one), and
protect that struct using RCU. That also saves the memory in case it's
not assigned at all. Something like

struct sta_mon_rssi_config {
	struct rcu_head rcu_head;
	int n_thresholds;
	s32 low, high;
	u32 hyst;
	s32 last_value;
	s32 thresholds[];
};

then you can kfree_rcu() it, and just do a single allocation using
struct_size() for however many entries you need.

> + * @count_rx_signal: Number of data frames used in averaging station signal.
> + *	This can be used to avoid generating less reliable station rssi cross
> + *	events that would be based only on couple of received frames.
>   */
>  struct sta_info {
>  	/* General information, mostly static */
> @@ -600,6 +603,7 @@ struct sta_info {
>  	s32 rssi_high;
>  	u32 rssi_hyst;
>  	s32 last_rssi_event_value;
> +	unsigned int count_rx_signal;

I guess that would also move into the new struct.

johannes

Re: [PATCH 3/3] mac80211: Implement functionality to monitor station's signal stregnth

[Tamizh chelvam]

On 2018-11-09 17:25, Johannes Berg wrote:
> Oh, umm, that patch is still here ...
> 
> I guess we can combine 2 and 3 too.
> 
> 
Sure.

>> +	if (sta->rssi_low && bss_conf->enable_beacon) {
>> +		int last_event =
>> +			sta->last_rssi_event_value;
>> +		int sig = -ewma_signal_read(&sta->rx_stats_avg.signal);
>> +		int low = sta->rssi_low;
>> +		int high = sta->rssi_high;
> 
> This doesn't really support a list, like in patch 2 where you store
> sta_info::rssi_tholds?
> 
rssi_low and rssi_high will have a configured value or zero know ? 
Configured value has been stored in the previous patch.

>> +		if (sig < low &&
>> +		    (last_event == 0 || last_event >= low)) {
>> +			sta->last_rssi_event_value = sig;
>> +			cfg80211_sta_mon_rssi_notify(
>> +				rx->sdata->dev, sta->addr,
>> +				NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW,
>> +				sig, GFP_ATOMIC);
>> +			rssi_cross = true;
>> +		} else if (sig > high &&
>> +			   (last_event == 0 || last_event <= high)) {
>> +			sta->last_rssi_event_value = sig;
>> +			cfg80211_sta_mon_rssi_notify(
>> +				rx->sdata->dev, sta->addr,
>> +				NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH,
>> +				sig, GFP_ATOMIC);
>> +			rssi_cross = true;
>> +		}
>> +	}
>> +
>> +	if (rssi_cross) {
>> +		ieee80211_update_rssi_config(sta);
>> +		rssi_cross = false;
>> +	}
>> +}
> 
> Ah, but it does, just hard to understand.
> 
> However, this does mean what I suspected on the other patch is true -
> you're calling ieee80211_update_rssi_config() here, and that uses the
> sta->rssi_tholds array without any protection, while a concurrent 
> change
> of configuration can free the data.
> 
yes, I'll add a protection mechanism.

> You need to sort that out. I would suggest to stick all the sta->rssi_*
> fields you add into a new struct (you even had an empty one), and
> protect that struct using RCU. That also saves the memory in case it's
> not assigned at all. Something like
> 
> struct sta_mon_rssi_config {
> 	struct rcu_head rcu_head;
> 	int n_thresholds;
> 	s32 low, high;
> 	u32 hyst;
> 	s32 last_value;
> 	s32 thresholds[];
> };
> 
> then you can kfree_rcu() it, and just do a single allocation using
> struct_size() for however many entries you need.
> 
Yes correct. I'll change it.

>> + * @count_rx_signal: Number of data frames used in averaging station 
>> signal.
>> + *	This can be used to avoid generating less reliable station rssi 
>> cross
>> + *	events that would be based only on couple of received frames.
>>   */
>>  struct sta_info {
>>  	/* General information, mostly static */
>> @@ -600,6 +603,7 @@ struct sta_info {
>>  	s32 rssi_high;
>>  	u32 rssi_hyst;
>>  	s32 last_rssi_event_value;
>> +	unsigned int count_rx_signal;
> 
> I guess that would also move into the new struct.
> 
Okay.

Thanks,
Tamizh.