[PATCH v2 0/6] Add ethtool stats support for Wireless Devices

[PATCH v2 0/6] Add ethtool stats support for Wireless Devices

[greearb]

From: Ben Greear <greearb@candelatech.com>


This enables ethtool stats for mac80211 devices.  It also
adds hooks to call down into mac80211 drivers for additional
stats.  Patches to enable this hook in ath9k will be posted
in a different series.

There was a review question about how to make the ethtool
strings line up with the data in a less error prone manner.
This patch series does NOT address that.  I think it may
be more work than it's worth to try to do this, but we
can always retro-fit such behaviour later if desired.
Many drivers have this issue, so perhaps some support
code in the ethtool core is the way to go.

V2:  Fix getting survey stats:  Need to specify the
  current channel index, not just use index zero.  The
  fix for this involved adding a new helper method which
  adds a patch to the series.


Ben Greear (6):
  cfg80211: Add framework to support ethtool stats.
  mac80211: Support getting sta_info stats via ethtool.
  mac80211: Framework to get wifi-driver stats via ethtool.
  wireless: Add util method to get channel index from frequency.
  mac80211: Add more ethtools stats: survey, rates, etc
  mac80211: Add sta_state to ethtool stats.

 include/net/cfg80211.h      |   24 ++++++
 include/net/mac80211.h      |   17 ++++
 net/mac80211/cfg.c          |  176 +++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/driver-ops.h   |   37 +++++++++
 net/mac80211/driver-trace.h |   15 ++++
 net/wireless/ethtool.c      |   29 +++++++
 net/wireless/util.c         |   24 ++++++
 7 files changed, 322 insertions(+), 0 deletions(-)

-- 
1.7.3.4

[PATCH v2 1/6] cfg80211: Add framework to support ethtool stats.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 815dc3f... 27f9561... M	include/net/cfg80211.h
:100644 100644 9bde4d1... 7eecdf4... M	net/wireless/ethtool.c
 include/net/cfg80211.h |   17 +++++++++++++++++
 net/wireless/ethtool.c |   29 +++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 815dc3f..27f9561 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1514,6 +1514,16 @@ struct cfg80211_gtk_rekey_data {
  *	later passes to cfg80211_probe_status().
  *
  * @set_noack_map: Set the NoAck Map for the TIDs.
+ *
+ * @get_et_sset_count:  Ethtool API to get string-set count.
+ *	See @ethtool_ops.get_sset_count
+ *
+ * @get_et_stats:  Ethtool API to get a set of u64 stats.
+ *	See @ethtool_ops.get_ethtool_stats
+ *
+ * @get_et_strings:  Ethtool API to get a set of strings to describe stats
+ *	and perhaps other supported types of ethtool data-sets.
+ *	See @ethtool_ops.get_strings
  */
 struct cfg80211_ops {
 	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -1712,6 +1722,13 @@ struct cfg80211_ops {
 
 	struct ieee80211_channel *(*get_channel)(struct wiphy *wiphy,
 					       enum nl80211_channel_type *type);
+
+	int	(*get_et_sset_count)(struct wiphy *wiphy,
+				     struct net_device *dev, int sset);
+	void	(*get_et_stats)(struct wiphy *wiphy, struct net_device *dev,
+				struct ethtool_stats *stats, u64 *data);
+	void	(*get_et_strings)(struct wiphy *wiphy, struct net_device *dev,
+				  u32 sset, u8 *data);
 };
 
 /*
diff --git a/net/wireless/ethtool.c b/net/wireless/ethtool.c
index 9bde4d1..7eecdf4 100644
--- a/net/wireless/ethtool.c
+++ b/net/wireless/ethtool.c
@@ -68,6 +68,32 @@ static int cfg80211_set_ringparam(struct net_device *dev,
 	return -ENOTSUPP;
 }
 
+static int cfg80211_get_sset_count(struct net_device *dev, int sset)
+{
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+	if (rdev->ops->get_et_sset_count)
+		return rdev->ops->get_et_sset_count(wdev->wiphy, dev, sset);
+	return -EOPNOTSUPP;
+}
+
+static void cfg80211_get_stats(struct net_device *dev,
+			       struct ethtool_stats *stats, u64 *data)
+{
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+	if (rdev->ops->get_et_stats)
+		rdev->ops->get_et_stats(wdev->wiphy, dev, stats, data);
+}
+
+static void cfg80211_get_strings(struct net_device *dev, u32 sset, u8 *data)
+{
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+	if (rdev->ops->get_et_strings)
+		rdev->ops->get_et_strings(wdev->wiphy, dev, sset, data);
+}
+
 const struct ethtool_ops cfg80211_ethtool_ops = {
 	.get_drvinfo = cfg80211_get_drvinfo,
 	.get_regs_len = cfg80211_get_regs_len,
@@ -75,4 +101,7 @@ const struct ethtool_ops cfg80211_ethtool_ops = {
 	.get_link = ethtool_op_get_link,
 	.get_ringparam = cfg80211_get_ringparam,
 	.set_ringparam = cfg80211_set_ringparam,
+	.get_strings = cfg80211_get_strings,
+	.get_ethtool_stats = cfg80211_get_stats,
+	.get_sset_count = cfg80211_get_sset_count,
 };
-- 
1.7.3.4

[PATCH v2 2/6] mac80211: Support getting sta_info stats via ethtool.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This lets ethtool print out stats related to stations
connected to the interface.  Does not yet get stats
from the underlying driver.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 510a745... 1cb0529... M	net/mac80211/cfg.c
 net/mac80211/cfg.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 510a745..1cb0529 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -112,6 +112,74 @@ static int ieee80211_set_noack_map(struct wiphy *wiphy,
 	return 0;
 }
 
+static const char ieee80211_gstrings_sta_stats[][ETH_GSTRING_LEN] = {
+	"rx_packets", "rx_bytes", "wep_weak_iv_count",
+	"rx_duplicates", "rx_fragments", "rx_dropped",
+	"tx_packets", "tx_bytes", "tx_fragments",
+	"tx_filtered", "tx_retry_failed", "tx_retries",
+	"beacon_loss"
+};
+#define STA_STATS_LEN	ARRAY_SIZE(ieee80211_gstrings_sta_stats)
+
+static int ieee80211_get_et_sset_count(struct wiphy *wiphy,
+				       struct net_device *dev,
+				       int sset)
+{
+	if (sset == ETH_SS_STATS)
+		return STA_STATS_LEN;
+
+	return -EOPNOTSUPP;
+}
+
+static void ieee80211_get_et_stats(struct wiphy *wiphy,
+				   struct net_device *dev,
+				   struct ethtool_stats *stats,
+				   u64 *data)
+{
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct sta_info *sta;
+	struct ieee80211_local *local = sdata->local;
+
+	memset(data, 0, sizeof(u64) * STA_STATS_LEN);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sta, &local->sta_list, list) {
+		int i = 0;
+
+		/* Make sure this station belongs to the proper dev */
+		if (sta->sdata->dev != dev)
+			continue;
+
+		data[i++] += sta->rx_packets;
+		data[i++] += sta->rx_bytes;
+		data[i++] += sta->wep_weak_iv_count;
+		data[i++] += sta->num_duplicates;
+		data[i++] += sta->rx_fragments;
+		data[i++] += sta->rx_dropped;
+
+		data[i++] += sta->tx_packets;
+		data[i++] += sta->tx_bytes;
+		data[i++] += sta->tx_fragments;
+		data[i++] += sta->tx_filtered_count;
+		data[i++] += sta->tx_retry_failed;
+		data[i++] += sta->tx_retry_count;
+		data[i++] += sta->beacon_loss_count;
+		BUG_ON(i != STA_STATS_LEN);
+	}
+	rcu_read_unlock();
+}
+
+static void ieee80211_get_et_strings(struct wiphy *wiphy,
+				     struct net_device *dev,
+				     u32 sset, u8 *data)
+{
+	if (sset == ETH_SS_STATS) {
+		int sz_sta_stats = sizeof(ieee80211_gstrings_sta_stats);
+		memcpy(data, *ieee80211_gstrings_sta_stats, sz_sta_stats);
+	}
+}
+
+
 static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
 			     u8 key_idx, bool pairwise, const u8 *mac_addr,
 			     struct key_params *params)
@@ -2775,4 +2843,7 @@ struct cfg80211_ops mac80211_config_ops = {
 #ifdef CONFIG_PM
 	.set_wakeup = ieee80211_set_wakeup,
 #endif
+	.get_et_sset_count = ieee80211_get_et_sset_count,
+	.get_et_stats = ieee80211_get_et_stats,
+	.get_et_strings = ieee80211_get_et_strings,
 };
-- 
1.7.3.4

[PATCH v2 3/6] mac80211: Framework to get wifi-driver stats via ethtool.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This adds hooks to call into the driver to get additional
stats for the ethtool API.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 bebd89f... 95a3ef2... M	include/net/mac80211.h
:100644 100644 1cb0529... e90accc... M	net/mac80211/cfg.c
:100644 100644 4a0e559... 6d33a0c... M	net/mac80211/driver-ops.h
:100644 100644 7c0754b... 6de00b2... M	net/mac80211/driver-trace.h
 include/net/mac80211.h      |   17 +++++++++++++++++
 net/mac80211/cfg.c          |   19 ++++++++++++++++---
 net/mac80211/driver-ops.h   |   37 +++++++++++++++++++++++++++++++++++++
 net/mac80211/driver-trace.h |   15 +++++++++++++++
 4 files changed, 85 insertions(+), 3 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index bebd89f..95a3ef2 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2223,6 +2223,14 @@ enum ieee80211_rate_control_changed {
  *	The @tids parameter is a bitmap and tells the driver which TIDs the
  *	frames will be on; it will at most have two bits set.
  *	This callback must be atomic.
+ *
+ * @get_et_sset_count:  Ethtool API to get string-set count.
+ *
+ * @get_et_stats:  Ethtool API to get a set of u64 stats.
+ *
+ * @get_et_strings:  Ethtool API to get a set of strings to describe stats
+ *	and perhaps other supported types of ethtool data-sets.
+ *
  */
 struct ieee80211_ops {
 	void (*tx)(struct ieee80211_hw *hw, struct sk_buff *skb);
@@ -2353,6 +2361,15 @@ struct ieee80211_ops {
 					u16 tids, int num_frames,
 					enum ieee80211_frame_release_type reason,
 					bool more_data);
+
+	int	(*get_et_sset_count)(struct ieee80211_hw *hw,
+				     struct ieee80211_vif *vif, int sset);
+	void	(*get_et_stats)(struct ieee80211_hw *hw,
+				struct ieee80211_vif *vif,
+				struct ethtool_stats *stats, u64 *data);
+	void	(*get_et_strings)(struct ieee80211_hw *hw,
+				  struct ieee80211_vif *vif,
+				  u32 sset, u8 *data);
 };
 
 /**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 1cb0529..e90accc 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -125,10 +125,17 @@ static int ieee80211_get_et_sset_count(struct wiphy *wiphy,
 				       struct net_device *dev,
 				       int sset)
 {
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	int rv = 0;
+
 	if (sset == ETH_SS_STATS)
-		return STA_STATS_LEN;
+		rv += STA_STATS_LEN;
 
-	return -EOPNOTSUPP;
+	rv += drv_get_et_sset_count(sdata, sset);
+
+	if (rv == 0)
+		return -EOPNOTSUPP;
+	return rv;
 }
 
 static void ieee80211_get_et_stats(struct wiphy *wiphy,
@@ -167,16 +174,22 @@ static void ieee80211_get_et_stats(struct wiphy *wiphy,
 		BUG_ON(i != STA_STATS_LEN);
 	}
 	rcu_read_unlock();
+
+	drv_get_et_stats(sdata, stats, &(data[STA_STATS_LEN]));
 }
 
 static void ieee80211_get_et_strings(struct wiphy *wiphy,
 				     struct net_device *dev,
 				     u32 sset, u8 *data)
 {
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	int sz_sta_stats = 0;
+
 	if (sset == ETH_SS_STATS) {
-		int sz_sta_stats = sizeof(ieee80211_gstrings_sta_stats);
+		sz_sta_stats = sizeof(ieee80211_gstrings_sta_stats);
 		memcpy(data, *ieee80211_gstrings_sta_stats, sz_sta_stats);
 	}
+	drv_get_et_strings(sdata, sset, &(data[sz_sta_stats]));
 }
 
 
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 4a0e559..6d33a0c 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -35,6 +35,43 @@ static inline void drv_tx_frags(struct ieee80211_local *local,
 	local->ops->tx_frags(&local->hw, vif, sta, skbs);
 }
 
+static inline void drv_get_et_strings(struct ieee80211_sub_if_data *sdata,
+				      u32 sset, u8 *data)
+{
+	struct ieee80211_local *local = sdata->local;
+	if (local->ops->get_et_strings) {
+		trace_drv_get_et_strings(local, sset);
+		local->ops->get_et_strings(&local->hw, &sdata->vif, sset, data);
+		trace_drv_return_void(local);
+	}
+}
+
+static inline void drv_get_et_stats(struct ieee80211_sub_if_data *sdata,
+				    struct ethtool_stats *stats,
+				    u64 *data)
+{
+	struct ieee80211_local *local = sdata->local;
+	if (local->ops->get_et_stats) {
+		trace_drv_get_et_stats(local);
+		local->ops->get_et_stats(&local->hw, &sdata->vif, stats, data);
+		trace_drv_return_void(local);
+	}
+}
+
+static inline int drv_get_et_sset_count(struct ieee80211_sub_if_data *sdata,
+					int sset)
+{
+	struct ieee80211_local *local = sdata->local;
+	int rv = 0;
+	if (local->ops->get_et_sset_count) {
+		trace_drv_get_et_sset_count(local, sset);
+		rv = local->ops->get_et_sset_count(&local->hw, &sdata->vif,
+						   sset);
+		trace_drv_return_int(local, rv);
+	}
+	return rv;
+}
+
 static inline int drv_start(struct ieee80211_local *local)
 {
 	int ret;
diff --git a/net/mac80211/driver-trace.h b/net/mac80211/driver-trace.h
index 7c0754b..6de00b2 100644
--- a/net/mac80211/driver-trace.h
+++ b/net/mac80211/driver-trace.h
@@ -161,6 +161,21 @@ DEFINE_EVENT(local_only_evt, drv_start,
 	TP_ARGS(local)
 );
 
+DEFINE_EVENT(local_u32_evt, drv_get_et_strings,
+	     TP_PROTO(struct ieee80211_local *local, u32 sset),
+	     TP_ARGS(local, sset)
+);
+
+DEFINE_EVENT(local_u32_evt, drv_get_et_sset_count,
+	     TP_PROTO(struct ieee80211_local *local, u32 sset),
+	     TP_ARGS(local, sset)
+);
+
+DEFINE_EVENT(local_only_evt, drv_get_et_stats,
+	     TP_PROTO(struct ieee80211_local *local),
+	     TP_ARGS(local)
+);
+
 DEFINE_EVENT(local_only_evt, drv_suspend,
 	TP_PROTO(struct ieee80211_local *local),
 	TP_ARGS(local)
-- 
1.7.3.4

[PATCH v2 4/6] wireless: Add util method to get channel index from frequency.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 27f9561... be6fb62... M	include/net/cfg80211.h
:100644 100644 6cba001... 2fd0e97... M	net/wireless/util.c
 include/net/cfg80211.h |    7 +++++++
 net/wireless/util.c    |   24 ++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 27f9561..be6fb62 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2388,6 +2388,13 @@ ieee80211_get_channel(struct wiphy *wiphy, int freq)
 }
 
 /**
+ * ieee80211_get_channel_idx - get channel index from wiphy for specified freq
+ * @wiphy: the struct wiphy to get the channel for
+ * @freq: the center frequency of the channel
+ */
+extern int ieee80211_get_channel_idx(struct wiphy *wiphy, int freq);
+
+/**
  * ieee80211_get_response_rate - get basic rate for a given rate
  *
  * @sband: the band to look for rates in
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 6cba001..2fd0e97 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -88,6 +88,30 @@ struct ieee80211_channel *__ieee80211_get_channel(struct wiphy *wiphy,
 }
 EXPORT_SYMBOL(__ieee80211_get_channel);
 
+int ieee80211_get_channel_idx(struct wiphy *wiphy, int freq)
+{
+	enum ieee80211_band band;
+	struct ieee80211_supported_band *sband;
+	int i;
+	int rv = 0;
+
+	for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
+		sband = wiphy->bands[band];
+
+		if (!sband)
+			continue;
+
+		for (i = 0; i < sband->n_channels; i++) {
+			if (sband->channels[i].center_freq == freq)
+				return rv;
+			rv++;
+		}
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(ieee80211_get_channel_idx);
+
 static void set_mandatory_flags_band(struct ieee80211_supported_band *sband,
 				     enum ieee80211_band band)
 {
-- 
1.7.3.4

[PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[greearb]

From: Ben Greear <greearb@candelatech.com>

The signal and noise are forced to be positive since ethtool
deals in unsigned 64-bit values and this number should be human
readable.  This gives easy access to some of the data formerly
exposed in the deprecated /proc/net/wireless file.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

v2:  Properly get survey results.

:100644 100644 e90accc... 94a340b... M	net/mac80211/cfg.c
 net/mac80211/cfg.c |  172 +++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 131 insertions(+), 41 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index e90accc..94a340b 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -117,7 +117,9 @@ static const char ieee80211_gstrings_sta_stats[][ETH_GSTRING_LEN] = {
 	"rx_duplicates", "rx_fragments", "rx_dropped",
 	"tx_packets", "tx_bytes", "tx_fragments",
 	"tx_filtered", "tx_retry_failed", "tx_retries",
-	"beacon_loss"
+	"beacon_loss", "txrate", "rxrate", "signal",
+	"channel", "noise", "ch_time", "ch_time_busy",
+	"ch_time_ext_busy", "ch_time_rx", "ch_time_tx"
 };
 #define STA_STATS_LEN	ARRAY_SIZE(ieee80211_gstrings_sta_stats)
 
@@ -138,46 +140,6 @@ static int ieee80211_get_et_sset_count(struct wiphy *wiphy,
 	return rv;
 }
 
-static void ieee80211_get_et_stats(struct wiphy *wiphy,
-				   struct net_device *dev,
-				   struct ethtool_stats *stats,
-				   u64 *data)
-{
-	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
-	struct sta_info *sta;
-	struct ieee80211_local *local = sdata->local;
-
-	memset(data, 0, sizeof(u64) * STA_STATS_LEN);
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(sta, &local->sta_list, list) {
-		int i = 0;
-
-		/* Make sure this station belongs to the proper dev */
-		if (sta->sdata->dev != dev)
-			continue;
-
-		data[i++] += sta->rx_packets;
-		data[i++] += sta->rx_bytes;
-		data[i++] += sta->wep_weak_iv_count;
-		data[i++] += sta->num_duplicates;
-		data[i++] += sta->rx_fragments;
-		data[i++] += sta->rx_dropped;
-
-		data[i++] += sta->tx_packets;
-		data[i++] += sta->tx_bytes;
-		data[i++] += sta->tx_fragments;
-		data[i++] += sta->tx_filtered_count;
-		data[i++] += sta->tx_retry_failed;
-		data[i++] += sta->tx_retry_count;
-		data[i++] += sta->beacon_loss_count;
-		BUG_ON(i != STA_STATS_LEN);
-	}
-	rcu_read_unlock();
-
-	drv_get_et_stats(sdata, stats, &(data[STA_STATS_LEN]));
-}
-
 static void ieee80211_get_et_strings(struct wiphy *wiphy,
 				     struct net_device *dev,
 				     u32 sset, u8 *data)
@@ -531,6 +493,134 @@ static void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo)
 		sinfo->sta_flags.set |= BIT(NL80211_STA_FLAG_TDLS_PEER);
 }
 
+static void ieee80211_get_et_stats(struct wiphy *wiphy,
+				   struct net_device *dev,
+				   struct ethtool_stats *stats,
+				   u64 *data)
+{
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct sta_info *sta;
+	struct ieee80211_local *local = sdata->local;
+	struct station_info sinfo;
+	struct survey_info survey;
+	bool do_once = true;
+	int i;
+#define STA_STATS_SURVEY_LEN 7
+
+	memset(data, 0, sizeof(u64) * STA_STATS_LEN);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sta, &local->sta_list, list) {
+		i = 0;
+
+		/* Make sure this station belongs to the proper dev */
+		if (sta->sdata->dev != dev)
+			continue;
+
+		data[i++] += sta->rx_packets;
+		data[i++] += sta->rx_bytes;
+		data[i++] += sta->wep_weak_iv_count;
+		data[i++] += sta->num_duplicates;
+		data[i++] += sta->rx_fragments;
+		data[i++] += sta->rx_dropped;
+
+		data[i++] += sta->tx_packets;
+		data[i++] += sta->tx_bytes;
+		data[i++] += sta->tx_fragments;
+		data[i++] += sta->tx_filtered_count;
+		data[i++] += sta->tx_retry_failed;
+		data[i++] += sta->tx_retry_count;
+		data[i++] += sta->beacon_loss_count;
+
+		if (!do_once) {
+			i += 3;
+			goto after_once;
+		}
+
+		do_once = false;
+		sinfo.filled = 0;
+		sta_set_sinfo(sta, &sinfo);
+
+		if (sinfo.filled | STATION_INFO_TX_BITRATE)
+			data[i] = 100000 *
+				cfg80211_calculate_bitrate(&sinfo.txrate);
+		i++;
+		if (sinfo.filled | STATION_INFO_RX_BITRATE)
+			data[i] = 100000 *
+				cfg80211_calculate_bitrate(&sinfo.rxrate);
+		i++;
+
+		if (sinfo.filled | STATION_INFO_SIGNAL_AVG)
+			data[i] = (u8)sinfo.signal_avg;
+		i++;
+
+after_once:
+		if (WARN_ON(i != (STA_STATS_LEN - STA_STATS_SURVEY_LEN))) {
+			rcu_read_unlock();
+			return;
+		}
+	}
+
+	i = STA_STATS_LEN - STA_STATS_SURVEY_LEN;
+	/* Get survey stats for current channel */
+	survey.filled = 0;
+	if (drv_get_survey(local, 0, &survey) != 0) {
+		survey.filled = 0;
+		data[i++] = 0;
+	} else {
+		/* ath9k (and maybe others??) only updates internal stats
+		 * when you get channel index 0, so if
+		 * we are NOT on channel zero, get the real stats
+		 * now.
+		 */
+		int ch_idx = ieee80211_get_channel_idx(local->hw.wiphy,
+			local->oper_channel->center_freq);
+		if (ch_idx == 0) {
+			data[i++] = survey.channel->center_freq;
+		} else {
+			survey.filled = 0;
+			if (drv_get_survey(local, ch_idx, &survey) != 0) {
+				survey.filled = 0;
+				data[i++] = 0;
+			} else {
+				data[i++] = survey.channel->center_freq;
+			}
+		}
+	}
+
+	if (survey.filled & SURVEY_INFO_NOISE_DBM)
+		data[i++] = (u8)survey.noise;
+	else
+		data[i++] = -1LL;
+	if (survey.filled & SURVEY_INFO_CHANNEL_TIME)
+		data[i++] = survey.channel_time;
+	else
+		data[i++] = -1LL;
+	if (survey.filled & SURVEY_INFO_CHANNEL_TIME_BUSY)
+		data[i++] = survey.channel_time_busy;
+	else
+		data[i++] = -1LL;
+	if (survey.filled & SURVEY_INFO_CHANNEL_TIME_EXT_BUSY)
+		data[i++] = survey.channel_time_ext_busy;
+	else
+		data[i++] = -1LL;
+	if (survey.filled & SURVEY_INFO_CHANNEL_TIME_RX)
+		data[i++] = survey.channel_time_rx;
+	else
+		data[i++] = -1LL;
+	if (survey.filled & SURVEY_INFO_CHANNEL_TIME_TX)
+		data[i++] = survey.channel_time_tx;
+	else
+		data[i++] = -1LL;
+
+	rcu_read_unlock();
+
+	if (WARN_ON(i != STA_STATS_LEN))
+		return;
+
+	drv_get_et_stats(sdata, stats, &(data[STA_STATS_LEN]));
+}
+
 
 static int ieee80211_dump_station(struct wiphy *wiphy, struct net_device *dev,
 				 int idx, u8 *mac, struct station_info *sinfo)
-- 
1.7.3.4

[PATCH v2 6/6] mac80211: Add sta_state to ethtool stats.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Helps to know how the station is doing in it's association
attempt.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 94a340b... a7c62c4... M	net/mac80211/cfg.c
 net/mac80211/cfg.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 94a340b..a7c62c4 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -117,7 +117,7 @@ static const char ieee80211_gstrings_sta_stats[][ETH_GSTRING_LEN] = {
 	"rx_duplicates", "rx_fragments", "rx_dropped",
 	"tx_packets", "tx_bytes", "tx_fragments",
 	"tx_filtered", "tx_retry_failed", "tx_retries",
-	"beacon_loss", "txrate", "rxrate", "signal",
+	"beacon_loss", "sta_state", "txrate", "rxrate", "signal",
 	"channel", "noise", "ch_time", "ch_time_busy",
 	"ch_time_ext_busy", "ch_time_rx", "ch_time_tx"
 };
@@ -533,10 +533,12 @@ static void ieee80211_get_et_stats(struct wiphy *wiphy,
 		data[i++] += sta->beacon_loss_count;
 
 		if (!do_once) {
-			i += 3;
+			i += 4;
 			goto after_once;
 		}
 
+		data[i++] = sta->sta_state;
+
 		do_once = false;
 		sinfo.filled = 0;
 		sta_set_sinfo(sta, &sinfo);
-- 
1.7.3.4

Re: [PATCH v2 2/6] mac80211: Support getting sta_info stats via ethtool.

[Johannes Berg]

On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:

> +static void ieee80211_get_et_stats(struct wiphy *wiphy,
> +				   struct net_device *dev,
> +				   struct ethtool_stats *stats,
> +				   u64 *data)
> +{
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +	struct sta_info *sta;
> +	struct ieee80211_local *local = sdata->local;
> +
> +	memset(data, 0, sizeof(u64) * STA_STATS_LEN);
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(sta, &local->sta_list, list) {

This doesn't seem right -- shouldn't it look up the BSSID or something
and only work on managed interfaces? What if there really are two
stations on this interface -- then it'll just overwrite it and return a
random station's data? That's useless.

johannes

Re: [PATCH v2 4/6] wireless: Add util method to get channel index from frequency.

[Johannes Berg]

On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> :100644 100644 27f9561... be6fb62... M	include/net/cfg80211.h
> :100644 100644 6cba001... 2fd0e97... M	net/wireless/util.c
>  include/net/cfg80211.h |    7 +++++++
>  net/wireless/util.c    |   24 ++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 27f9561..be6fb62 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2388,6 +2388,13 @@ ieee80211_get_channel(struct wiphy *wiphy, int freq)
>  }
>  
>  /**
> + * ieee80211_get_channel_idx - get channel index from wiphy for specified freq
> + * @wiphy: the struct wiphy to get the channel for
> + * @freq: the center frequency of the channel
> + */
> +extern int ieee80211_get_channel_idx(struct wiphy *wiphy, int freq);

I prefer you drop the extern, but ...


> +int ieee80211_get_channel_idx(struct wiphy *wiphy, int freq)
> +{
> +       enum ieee80211_band band;
> +       struct ieee80211_supported_band *sband;
> +       int i;
> +       int rv = 0;
> +
> +       for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
> +               sband = wiphy->bands[band];
> +
> +               if (!sband)
> +                       continue;
> +
> +               for (i = 0; i < sband->n_channels; i++) {
> +                       if (sband->channels[i].center_freq == freq)
> +                               return rv;
> +                       rv++;
> +               }
> +       }
> +
> +       return NULL;
> +}


"return NULL"? Really?

Also, what use is the index? It's some kind of global channel index, but
that's almost completely useless. I think you need a very very very good
reason to have this function and you're not even stating a single one.

johannes

Re: [PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[Johannes Berg]

On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
> +	/* Get survey stats for current channel */
> +	survey.filled = 0;
> +	if (drv_get_survey(local, 0, &survey) != 0) {
> +		survey.filled = 0;
> +		data[i++] = 0;
> +	} else {
> +		/* ath9k (and maybe others??) only updates internal stats
> +		 * when you get channel index 0, so if
> +		 * we are NOT on channel zero, get the real stats
> +		 * now.
> +		 */
> +		int ch_idx = ieee80211_get_channel_idx(local->hw.wiphy,
> +			local->oper_channel->center_freq);
> +		if (ch_idx == 0) {
> +			data[i++] = survey.channel->center_freq;
> +		} else {
> +			survey.filled = 0;
> +			if (drv_get_survey(local, ch_idx, &survey) != 0) {
> +				survey.filled = 0;
> +				data[i++] = 0;
> +			} else {
> +				data[i++] = survey.channel->center_freq;
> +			}
> +		}
> +	}

This is completely incomprehensible to me. I don't think the channel
index is what you think it is?

johannes

Re: [PATCH v2 6/6] mac80211: Add sta_state to ethtool stats.

[Johannes Berg]

On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Helps to know how the station is doing in it's association
> attempt.

Since you keep resending, why not roll this into another patch?

Also you could put the code into the right place so patch 5 doesn't move
it all.

johannes

Re: [PATCH v2 0/6] Add ethtool stats support for Wireless Devices

[Johannes Berg]

On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> 
> This enables ethtool stats for mac80211 devices.  It also
> adds hooks to call down into mac80211 drivers for additional
> stats.  Patches to enable this hook in ath9k will be posted
> in a different series.
> 
> There was a review question about how to make the ethtool
> strings line up with the data in a less error prone manner.
> This patch series does NOT address that.  I think it may
> be more work than it's worth to try to do this, but we
> can always retro-fit such behaviour later if desired.
> Many drivers have this issue, so perhaps some support
> code in the ethtool core is the way to go.
> 
> V2:  Fix getting survey stats:  Need to specify the
>   current channel index, not just use index zero.  The
>   fix for this involved adding a new helper method which
>   adds a patch to the series.

I'm in training all day tomorrow, and then I'll make my way home only to
go on vacation this weekend, for two weeks.

Would you mind holding all of this until I return from vacation? There
are some issues with this where at least one of us has a total
misunderstanding about how some things work. TBH, it could be me because
I'm not very familiar with the survey code.

johannes

Re: [PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[Ben Greear]

On 04/17/2012 06:41 PM, Johannes Berg wrote:
> On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
>> +	/* Get survey stats for current channel */
>> +	survey.filled = 0;
>> +	if (drv_get_survey(local, 0,&survey) != 0) {
>> +		survey.filled = 0;
>> +		data[i++] = 0;
>> +	} else {
>> +		/* ath9k (and maybe others??) only updates internal stats
>> +		 * when you get channel index 0, so if
>> +		 * we are NOT on channel zero, get the real stats
>> +		 * now.
>> +		 */
>> +		int ch_idx = ieee80211_get_channel_idx(local->hw.wiphy,
>> +			local->oper_channel->center_freq);
>> +		if (ch_idx == 0) {
>> +			data[i++] = survey.channel->center_freq;
>> +		} else {
>> +			survey.filled = 0;
>> +			if (drv_get_survey(local, ch_idx,&survey) != 0) {
>> +				survey.filled = 0;
>> +				data[i++] = 0;
>> +			} else {
>> +				data[i++] = survey.channel->center_freq;
>> +			}
>> +		}
>> +	}
>
> This is completely incomprehensible to me. I don't think the channel
> index is what you think it is?

It is ugly, but as far as I can tell it works.  The survey method
wants an index, not channel number or frequency, from what I can tell
(I just looked at and tested ath9k).

Based on the freq returned as part of the survey results, the logic
above appears to function as I would hope.

That said, a new get-survey() method that took a channel object instead of an
index would make this code cleaner.  Or, we could store the channel
index in the channel object for fast lookup, but I was afraid storing the
index would be too risky in case channels are ever moved around for some reason
or another.

And, I could be missing something yet....

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 4/6] wireless: Add util method to get channel index from frequency.

[Ben Greear]

On 04/17/2012 06:40 PM, Johannes Berg wrote:
> On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>> ---
>> :100644 100644 27f9561... be6fb62... M	include/net/cfg80211.h
>> :100644 100644 6cba001... 2fd0e97... M	net/wireless/util.c
>>   include/net/cfg80211.h |    7 +++++++
>>   net/wireless/util.c    |   24 ++++++++++++++++++++++++
>>   2 files changed, 31 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 27f9561..be6fb62 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -2388,6 +2388,13 @@ ieee80211_get_channel(struct wiphy *wiphy, int freq)
>>   }
>>
>>   /**
>> + * ieee80211_get_channel_idx - get channel index from wiphy for specified freq
>> + * @wiphy: the struct wiphy to get the channel for
>> + * @freq: the center frequency of the channel
>> + */
>> +extern int ieee80211_get_channel_idx(struct wiphy *wiphy, int freq);
>
> I prefer you drop the extern, but ...
>
>
>> +int ieee80211_get_channel_idx(struct wiphy *wiphy, int freq)
>> +{
>> +       enum ieee80211_band band;
>> +       struct ieee80211_supported_band *sband;
>> +       int i;
>> +       int rv = 0;
>> +
>> +       for (band = 0; band<  IEEE80211_NUM_BANDS; band++) {
>> +               sband = wiphy->bands[band];
>> +
>> +               if (!sband)
>> +                       continue;
>> +
>> +               for (i = 0; i<  sband->n_channels; i++) {
>> +                       if (sband->channels[i].center_freq == freq)
>> +                               return rv;
>> +                       rv++;
>> +               }
>> +       }
>> +
>> +       return NULL;
>> +}
>
>
> "return NULL"? Really?

Gah, that is wrong.

> Also, what use is the index? It's some kind of global channel index, but
> that's almost completely useless. I think you need a very very very good
> reason to have this function and you're not even stating a single one.

Well, it's used in the next patch that gets the survey info by index.

Wouldn't hurt my feelings to re-write how the get-survey() method
is called, but that would touch a lot of drivers, break out-of-tree drivers,
etc.

When you get a chance, please take a look at how the survey code
appears to work and let me know if you want it changed to take
channel objects instead of channel indexes.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 2/6] mac80211: Support getting sta_info stats via ethtool.

[Ben Greear]

On 04/17/2012 06:37 PM, Johannes Berg wrote:
> On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
>
>> +static void ieee80211_get_et_stats(struct wiphy *wiphy,
>> +				   struct net_device *dev,
>> +				   struct ethtool_stats *stats,
>> +				   u64 *data)
>> +{
>> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>> +	struct sta_info *sta;
>> +	struct ieee80211_local *local = sdata->local;
>> +
>> +	memset(data, 0, sizeof(u64) * STA_STATS_LEN);
>> +
>> +	rcu_read_lock();
>> +	list_for_each_entry_rcu(sta,&local->sta_list, list) {
>
> This doesn't seem right -- shouldn't it look up the BSSID or something
> and only work on managed interfaces? What if there really are two
> stations on this interface -- then it'll just overwrite it and return a
> random station's data? That's useless.

Well, its weird at least.

But, if there are multiple stations, like for APs??, then it will
add the station's stats together.  Perhaps not horribly useful, but better
than nothing.

For managed interface, I *think* they don't have more than one station, right?

And, as for the underlying driver stats and survey stats (in later patches),
that is only probed once.  I guess if you somehow had two
stations on different channels on the same network device,
the survey stats would be a bit dodgy, but it does return
the freq for the stats in question, so at least you know
what you are getting.

Thanks,
Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 0/6] Add ethtool stats support for Wireless Devices

[Ben Greear]

On 04/17/2012 06:44 PM, Johannes Berg wrote:
> On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear<greearb@candelatech.com>
>>
>>
>> This enables ethtool stats for mac80211 devices.  It also
>> adds hooks to call down into mac80211 drivers for additional
>> stats.  Patches to enable this hook in ath9k will be posted
>> in a different series.
>>
>> There was a review question about how to make the ethtool
>> strings line up with the data in a less error prone manner.
>> This patch series does NOT address that.  I think it may
>> be more work than it's worth to try to do this, but we
>> can always retro-fit such behaviour later if desired.
>> Many drivers have this issue, so perhaps some support
>> code in the ethtool core is the way to go.
>>
>> V2:  Fix getting survey stats:  Need to specify the
>>    current channel index, not just use index zero.  The
>>    fix for this involved adding a new helper method which
>>    adds a patch to the series.
>
> I'm in training all day tomorrow, and then I'll make my way home only to
> go on vacation this weekend, for two weeks.
>
> Would you mind holding all of this until I return from vacation? There
> are some issues with this where at least one of us has a total
> misunderstanding about how some things work. TBH, it could be me because
> I'm not very familiar with the survey code.

Fair enough.  If the basic infrastructure patches (1-3) seems good
enough, maybe those could go in?  That would let us start tying
the drivers in, and we can work out the survey logic later....

But, if patch 2 and how it adds a netdev's station stats together doesn't
seem right, then we can just hold everything until you are back.

Thanks,
Ben


>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 2/6] mac80211: Support getting sta_info stats via ethtool.

[Johannes Berg]

On Tue, 2012-04-17 at 20:46 -0700, Ben Greear wrote:

> >> +	rcu_read_lock();
> >> +	list_for_each_entry_rcu(sta,&local->sta_list, list) {
> >
> > This doesn't seem right -- shouldn't it look up the BSSID or something
> > and only work on managed interfaces? What if there really are two
> > stations on this interface -- then it'll just overwrite it and return a
> > random station's data? That's useless.
> 
> Well, its weird at least.
> 
> But, if there are multiple stations, like for APs??, then it will
> add the station's stats together.  Perhaps not horribly useful, but better
> than nothing.

Oh, right, it's adding, I missed that. But is that really useful?

> For managed interface, I *think* they don't have more than one station, right?

You can't rely on it. Typically they will, but with TDLS there might be
multiple.

> And, as for the underlying driver stats and survey stats (in later patches),
> that is only probed once.  I guess if you somehow had two
> stations on different channels on the same network device,
> the survey stats would be a bit dodgy, but it does return
> the freq for the stats in question, so at least you know
> what you are getting.

A single netdev is always going to be on a single channel. Actually, I
take that back, I think TDLS can work out of channel too, but we don't
support that right now.

johannes

Re: [PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[Johannes Berg]

On Tue, 2012-04-17 at 20:31 -0700, Ben Greear wrote:
> On 04/17/2012 06:41 PM, Johannes Berg wrote:
> > On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
> >> +	/* Get survey stats for current channel */
> >> +	survey.filled = 0;
> >> +	if (drv_get_survey(local, 0,&survey) != 0) {
> >> +		survey.filled = 0;
> >> +		data[i++] = 0;
> >> +	} else {
> >> +		/* ath9k (and maybe others??) only updates internal stats
> >> +		 * when you get channel index 0, so if
> >> +		 * we are NOT on channel zero, get the real stats
> >> +		 * now.
> >> +		 */
> >> +		int ch_idx = ieee80211_get_channel_idx(local->hw.wiphy,
> >> +			local->oper_channel->center_freq);
> >> +		if (ch_idx == 0) {
> >> +			data[i++] = survey.channel->center_freq;
> >> +		} else {
> >> +			survey.filled = 0;
> >> +			if (drv_get_survey(local, ch_idx,&survey) != 0) {
> >> +				survey.filled = 0;
> >> +				data[i++] = 0;
> >> +			} else {
> >> +				data[i++] = survey.channel->center_freq;
> >> +			}
> >> +		}
> >> +	}
> >
> > This is completely incomprehensible to me. I don't think the channel
> > index is what you think it is?
> 
> It is ugly, but as far as I can tell it works.  The survey method
> wants an index, not channel number or frequency, from what I can tell
> (I just looked at and tested ath9k).

But the get_survey method's index has no relation whatsoever to the
channel. It's just a cookie identifier.

> Based on the freq returned as part of the survey results, the logic
> above appears to function as I would hope.

Well then that's because ath9k happens to add the survey results in that
order, that is certainly not guaranteed. Some drivers only give you
survey results for index 0, and the channel contained in that index is
always just the channel that you're on right now.

> That said, a new get-survey() method that took a channel object instead of an
> index would make this code cleaner.  Or, we could store the channel
> index in the channel object for fast lookup, but I was afraid storing the
> index would be too risky in case channels are ever moved around for some reason
> or another.
> 
> And, I could be missing something yet....

Yeah I think you're missing how get_survey() works :-)

For every index in get_survey() it returns the channel & the information
for that channel, the index<->channel mapping is not fixed at all.

johannes

Re: [PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[Ben Greear]

On 04/17/2012 09:05 PM, Johannes Berg wrote:
> On Tue, 2012-04-17 at 20:31 -0700, Ben Greear wrote:
>> On 04/17/2012 06:41 PM, Johannes Berg wrote:
>>> On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
>>>> +	/* Get survey stats for current channel */
>>>> +	survey.filled = 0;
>>>> +	if (drv_get_survey(local, 0,&survey) != 0) {
>>>> +		survey.filled = 0;
>>>> +		data[i++] = 0;
>>>> +	} else {
>>>> +		/* ath9k (and maybe others??) only updates internal stats
>>>> +		 * when you get channel index 0, so if
>>>> +		 * we are NOT on channel zero, get the real stats
>>>> +		 * now.
>>>> +		 */
>>>> +		int ch_idx = ieee80211_get_channel_idx(local->hw.wiphy,
>>>> +			local->oper_channel->center_freq);
>>>> +		if (ch_idx == 0) {
>>>> +			data[i++] = survey.channel->center_freq;
>>>> +		} else {
>>>> +			survey.filled = 0;
>>>> +			if (drv_get_survey(local, ch_idx,&survey) != 0) {
>>>> +				survey.filled = 0;
>>>> +				data[i++] = 0;
>>>> +			} else {
>>>> +				data[i++] = survey.channel->center_freq;
>>>> +			}
>>>> +		}
>>>> +	}
>>>
>>> This is completely incomprehensible to me. I don't think the channel
>>> index is what you think it is?
>>
>> It is ugly, but as far as I can tell it works.  The survey method
>> wants an index, not channel number or frequency, from what I can tell
>> (I just looked at and tested ath9k).
>
> But the get_survey method's index has no relation whatsoever to the
> channel. It's just a cookie identifier.
>
>> Based on the freq returned as part of the survey results, the logic
>> above appears to function as I would hope.
>
> Well then that's because ath9k happens to add the survey results in that
> order, that is certainly not guaranteed. Some drivers only give you
> survey results for index 0, and the channel contained in that index is
> always just the channel that you're on right now.
>
>> That said, a new get-survey() method that took a channel object instead of an
>> index would make this code cleaner.  Or, we could store the channel
>> index in the channel object for fast lookup, but I was afraid storing the
>> index would be too risky in case channels are ever moved around for some reason
>> or another.
>>
>> And, I could be missing something yet....
>
> Yeah I think you're missing how get_survey() works :-)
>
> For every index in get_survey() it returns the channel&  the information
> for that channel, the index<->channel mapping is not fixed at all.

Well, that is a nasty API.

Can I add a new API that takes a channel object and returns the
survey results for that?  If channel is null, then the API should
return the survey for the current channel if possible.
I'll implement it on ath9k.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 2/6] mac80211: Support getting sta_info stats via ethtool.

[Ben Greear]

On 04/17/2012 09:00 PM, Johannes Berg wrote:
> On Tue, 2012-04-17 at 20:46 -0700, Ben Greear wrote:
>
>>>> +	rcu_read_lock();
>>>> +	list_for_each_entry_rcu(sta,&local->sta_list, list) {
>>>
>>> This doesn't seem right -- shouldn't it look up the BSSID or something
>>> and only work on managed interfaces? What if there really are two
>>> stations on this interface -- then it'll just overwrite it and return a
>>> random station's data? That's useless.
>>
>> Well, its weird at least.
>>
>> But, if there are multiple stations, like for APs??, then it will
>> add the station's stats together.  Perhaps not horribly useful, but better
>> than nothing.
>
> Oh, right, it's adding, I missed that. But is that really useful?

It provides a summary for AP, and precise stats for managed station
mode (excepting TDLS where it may return sums, it seems).

I could just return all zeros for non managed
station interfaces, but I *have* to return some value, so it seems
little loss to just add the station stats together.

>> For managed interface, I *think* they don't have more than one station, right?
>
> You can't rely on it. Typically they will, but with TDLS there might be
> multiple.
>
>> And, as for the underlying driver stats and survey stats (in later patches),
>> that is only probed once.  I guess if you somehow had two
>> stations on different channels on the same network device,
>> the survey stats would be a bit dodgy, but it does return
>> the freq for the stats in question, so at least you know
>> what you are getting.
>
> A single netdev is always going to be on a single channel. Actually, I
> take that back, I think TDLS can work out of channel too, but we don't
> support that right now.

So, based on your response, a sum still seems most useful to me.  The ethtool
API gives no way to request any subset of stats, so all I can key off of
is the netdev.

I have some plans percolating to add some new ethtool API to get subsets
of stats, but that is likely a ways off, and of course it may not be
accepted regardless.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 2/6] mac80211: Support getting sta_info stats via ethtool.

[Johannes Berg]

On 4/18/2012 9:27 AM, Ben Greear wrote:
>> Oh, right, it's adding, I missed that. But is that really useful?
>
> It provides a summary for AP, and precise stats for managed station
> mode (excepting TDLS where it may return sums, it seems).

I just have a feeling that people will rely on it working for managed 
mode, and will forget about TDLS since it's uncommon now ... so I really 
don't think this will work well with people's (even yours!) expectations.

johannes

Re: [PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[Johannes Berg]

On 4/18/2012 9:19 AM, Ben Greear wrote:
> Can I add a new API that takes a channel object and returns the
> survey results for that? If channel is null, then the API should
> return the survey for the current channel if possible.
> I'll implement it on ath9k.

I don't think you should. It's not nasty anyway since it's built to 
return all info.

johannes

Re: [PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[Ben Greear]

On 04/18/2012 03:40 PM, Johannes Berg wrote:
> On 4/18/2012 9:19 AM, Ben Greear wrote:
>> Can I add a new API that takes a channel object and returns the
>> survey results for that? If channel is null, then the API should
>> return the survey for the current channel if possible.
>> I'll implement it on ath9k.
>
> I don't think you should. It's not nasty anyway since it's built to return all info.

So, for ethtool stats, I can iterate through all available surveys,
and just report the value that matches the current channel (if any)?

Thanks,
Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 2/6] mac80211: Support getting sta_info stats via ethtool.

[Ben Greear]

On 04/18/2012 03:39 PM, Johannes Berg wrote:
> On 4/18/2012 9:27 AM, Ben Greear wrote:
>>> Oh, right, it's adding, I missed that. But is that really useful?
>>
>> It provides a summary for AP, and precise stats for managed station
>> mode (excepting TDLS where it may return sums, it seems).
>
> I just have a feeling that people will rely on it working for managed mode, and will forget about TDLS since it's uncommon now ... so I really don't think this
> will work well with people's (even yours!) expectations.

Ok, how about for managed mode, I will find the first non-TDLS station
matching the netdev, assign the values, and then quit looking for more
stations.

For all other modes, logic stays the same as it is now
(add all stations' stats together as long as the station belongs
to the netdev)?

Thanks,
Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[Johannes Berg]

On 4/18/2012 3:54 PM, Ben Greear wrote:
> On 04/18/2012 03:40 PM, Johannes Berg wrote:
>> On 4/18/2012 9:19 AM, Ben Greear wrote:
>>> Can I add a new API that takes a channel object and returns the
>>> survey results for that? If channel is null, then the API should
>>> return the survey for the current channel if possible.
>>> I'll implement it on ath9k.
>>
>> I don't think you should. It's not nasty anyway since it's built to
>> return all info.
>
> So, for ethtool stats, I can iterate through all available surveys,
> and just report the value that matches the current channel (if any)?

I think so, yes.

johannes

Re: [PATCH v2 2/6] mac80211: Support getting sta_info stats via ethtool.

[Johannes Berg]

On 4/18/2012 3:59 PM, Ben Greear wrote:
> On 04/18/2012 03:39 PM, Johannes Berg wrote:
>> On 4/18/2012 9:27 AM, Ben Greear wrote:
>>>> Oh, right, it's adding, I missed that. But is that really useful?
>>>
>>> It provides a summary for AP, and precise stats for managed station
>>> mode (excepting TDLS where it may return sums, it seems).
>>
>> I just have a feeling that people will rely on it working for managed
>> mode, and will forget about TDLS since it's uncommon now ... so I
>> really don't think this
>> will work well with people's (even yours!) expectations.
>
> Ok, how about for managed mode, I will find the first non-TDLS station
> matching the netdev, assign the values, and then quit looking for more
> stations.
>
> For all other modes, logic stays the same as it is now
> (add all stations' stats together as long as the station belongs
> to the netdev)?

Seems OK, but I'd implement it using the BSSID & getting the station 
rather than iterating? Though if you want the iteration for other 
interface types then I guess it'll be easier to just do it that way.

I just think it's bad to have relatively rare things like TDLS influence 
APIs that people might come to rely on.

johannes