From: Ben Greear <[email protected]>
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.
Ben Greear (5):
cfg80211: Add framework to support ethtool stats.
mac80211: Support getting sta_info stats via ethtool.
mac80211: Framework to get wifi-driver stats via ethtool.
mac80211: Add more ethtools stats: survey, rates, etc
mac80211: Add sta_state to ethtool stats.
include/net/cfg80211.h | 17 +++++
include/net/mac80211.h | 17 +++++
net/mac80211/cfg.c | 159 +++++++++++++++++++++++++++++++++++++++++++
net/mac80211/driver-ops.h | 37 ++++++++++
net/mac80211/driver-trace.h | 15 ++++
net/wireless/ethtool.c | 29 ++++++++
6 files changed, 274 insertions(+), 0 deletions(-)
--
1.7.3.4
On 04/12/2012 09:42 AM, Florian Fainelli wrote:
> Hi,
>
> Le 04/12/12 18:32, [email protected] a écrit :
>> From: Ben Greear<[email protected]>
>>
>> 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.
>
> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
> understand it. Does not ethtool know about 32-bits signed integers?
Ethtool stats only supports u64. I think it's easy enough for
humans or programs to add the negative sign. Can signal or noise
ever be > 0? If so, that could actually break something that depends
on flipping the value to negative....
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
From: Ben Greear <[email protected]>
Signed-off-by: Ben Greear <[email protected]>
---
:100644 100644 a587867... a3a635c... 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 a587867..a3a635c 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);
@@ -1711,6 +1721,13 @@ struct cfg80211_ops {
u16 noack_map);
struct ieee80211_channel *(*get_channel)(struct wiphy *wiphy);
+
+ 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
Hi,
Le 04/12/12 18:32, [email protected] a écrit :
> From: Ben Greear<[email protected]>
>
> 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.
Uh, that's misleading, the signal and noise values are typically
negative, so one needs to think about mentally adding a minus sign if
he/she wants to understand it. Does not ethtool know about 32-bits
signed integers?
>
> Signed-off-by: Ben Greear<[email protected]>
> ---
> :100644 100644 b37fb0d... 99e3597... M net/mac80211/cfg.c
> net/mac80211/cfg.c | 155 ++++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 114 insertions(+), 41 deletions(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index b37fb0d..99e3597 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,117 @@ 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] = abs(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 only */
> + survey.filled = 0;
> + if (drv_get_survey(local, 0,&survey) != 0) {
> + survey.filled = 0;
> + data[i++] = 0;
> + } else {
> + data[i++] = survey.channel->center_freq;
> + }
> +
> + if (survey.filled& SURVEY_INFO_NOISE_DBM)
> + data[i++] = abs(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)
On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
> > Hi,
> >
> > Le 04/12/12 18:32, [email protected] a écrit :
> >> From: Ben Greear<[email protected]>
> >>
> >> 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.
> >
> > Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
> > understand it. Does not ethtool know about 32-bits signed integers?
>
> Ethtool stats only supports u64. I think it's easy enough for
> humans or programs to add the negative sign. Can signal or noise
> ever be > 0? If so, that could actually break something that depends
> on flipping the value to negative....
So far as I can see, the ethtool stats were expected to be counters,
which obviously cannot become negative (or fractional). Maybe it's time
to define another command and string-set to cover other types of status
information. The ethtool utility could ask for those typed statistics
as well, so 'ethtool -S' would get all of them.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
On 2012-04-12 09:51 -0700, Ben Greear wrote Florian Fainelli:
BG>On 04/12/2012 09:42 AM, Florian Fainelli wrote:
BG>> Hi,
BG>>
BG>> Le 04/12/12 18:32, [email protected] a ?crit :
BG>> > From: Ben Greear<[email protected]>
BG>> >
BG>> > The signal and noise are forced to be positive since ethtool
BG>> > deals in unsigned 64-bit values and this number should be human
BG>> > readable. This gives easy access to some of the data formerly
BG>> > exposed in the deprecated /proc/net/wireless file.
BG>>
BG>> Uh, that's misleading, the signal and noise values are typically negative,
BG>> so one needs to think about mentally adding a minus sign if he/she wants to
BG>> understand it. Does not ethtool know about 32-bits signed integers?
BG>
BG>Ethtool stats only supports u64. I think it's easy enough for
BG>humans or programs to add the negative sign. Can signal or noise
BG>ever be > 0? If so, that could actually break something that depends
BG>on flipping the value to negative....
Don't know is this is a bug or it's reaaly can be positive, but:
iw dev mp0 station dump
Station 00:02:6f:b8:94:d3 (on mp0)
inactive time: 49 ms
rx bytes: 36318341
rx packets: 271741
tx bytes: 4180152
tx packets: 35445
tx retries: 7724
tx failed: 123
signal: 1 dBm
signal avg: -2 dBm
tx bitrate: 240.0 MBit/s MCS 13 40Mhz short GI
rx bitrate: 180.0 MBit/s MCS 12 40Mhz short GI
mesh llid: 8349
mesh plid: 49801
mesh plink: ESTAB
authorized: yes
authenticated: yes
preamble: long
WMM/WME: yes
FP: no
TDLS peer: no
C уважением With Best Regards
Георгиевский Юрий. Georgiewskiy Yuriy
+7 4872 711666 +7 4872 711666
факс +7 4872 711143 fax +7 4872 711143
Компания ООО "Ай Ти Сервис" IT Service Ltd
http://nkoort.ru http://nkoort.ru
JID: [email protected] JID: [email protected]
YG129-RIPE YG129-RIPE
Ben Greear wrote:
> Ethtool stats only supports u64. I think it's easy enough for
> humans or programs to add the negative sign. Can signal or noise
> ever be > 0? If so, that could actually break something that depends
> on flipping the value to negative....
The unit of these quantities is dBm, right? So if the {signal|noise} is
greater than 1mW the corresponding value in dBm would be positive.
From: Ben Greear <[email protected]>
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 <[email protected]>
---
:100644 100644 b37fb0d... 99e3597... M net/mac80211/cfg.c
net/mac80211/cfg.c | 155 ++++++++++++++++++++++++++++++++++++++--------------
1 files changed, 114 insertions(+), 41 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index b37fb0d..99e3597 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,117 @@ 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] = abs(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 only */
+ survey.filled = 0;
+ if (drv_get_survey(local, 0, &survey) != 0) {
+ survey.filled = 0;
+ data[i++] = 0;
+ } else {
+ data[i++] = survey.channel->center_freq;
+ }
+
+ if (survey.filled & SURVEY_INFO_NOISE_DBM)
+ data[i++] = abs(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
From: Ben Greear <[email protected]>
Helps to know how the station is doing in it's association
attempt.
Signed-off-by: Ben Greear <[email protected]>
---
:100644 100644 99e3597... 8375168... 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 99e3597..8375168 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
On Thu, 2012-04-12 at 13:46 -0700, Ben Greear wrote:
> On 04/12/2012 12:30 PM, Ben Hutchings wrote:
> > On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
> >> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
> >>> Hi,
> >>>
> >>> Le 04/12/12 18:32, [email protected] a écrit :
> >>>> From: Ben Greear<[email protected]>
> >>>>
> >>>> 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.
> >>>
> >>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
> >>> understand it. Does not ethtool know about 32-bits signed integers?
> >>
> >> Ethtool stats only supports u64. I think it's easy enough for
> >> humans or programs to add the negative sign. Can signal or noise
> >> ever be> 0? If so, that could actually break something that depends
> >> on flipping the value to negative....
> >
> > So far as I can see, the ethtool stats were expected to be counters,
> > which obviously cannot become negative (or fractional). Maybe it's time
> > to define another command and string-set to cover other types of status
> > information. The ethtool utility could ask for those typed statistics
> > as well, so 'ethtool -S' would get all of them.
>
> One nice thing about ethtool stats API is that it is backwards and forwards
> compatible for a long while.
Agreed.
> Also, adding a new API means a second call
> into the kernel (most likely) for the other set of strings, which effectively
> doubles the cost of getting stats (and allows the stats to be updated
> out-of-sync with each other more easily).
It's generally not possible to take an atomic snapshot of all statistics
for a NIC, so that shouldn't be a consideration.
> I wonder if instead we could add a convention where we add a short
> prefix (or suffix) to a string to denote it's type (and cast the value
> into u64). So, an old ethtool might see:
>
> foo:s32: 4294967295
Actually it would be:
foo:s32: 18446744073709551615
> while a new one understand that the s32: prefix is special, do
> some casting, and could show:
> foo: -1
>
> Both are still at least somewhat human readable,
I don't think many humans can mentally substract 2^64.
> and probably wouldn't confuse anyone that is parsing the output of
> existing ethtool output.
I think you have this backwards: printing numbers in two different ways
(old and new versions of ethtool) is likely to confuse scripts that are
parsing and doing calculations with these numbers. While I try to avoid
gratuitous changes in output formatting, scripts should use the ethtool
API if they really want interface stability. It's not difficult (there
are at least Python and Perl bindings available) and it's a lot more
efficient.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
On 04/12/2012 12:53 PM, Georgiewskiy Yuriy wrote:
> On 2012-04-12 09:51 -0700, Ben Greear wrote Florian Fainelli:
>
> BG>On 04/12/2012 09:42 AM, Florian Fainelli wrote:
> BG>> Hi,
> BG>>
> BG>> Le 04/12/12 18:32, [email protected] a ?crit :
> BG>> > From: Ben Greear<[email protected]>
> BG>> >
> BG>> > The signal and noise are forced to be positive since ethtool
> BG>> > deals in unsigned 64-bit values and this number should be human
> BG>> > readable. This gives easy access to some of the data formerly
> BG>> > exposed in the deprecated /proc/net/wireless file.
> BG>>
> BG>> Uh, that's misleading, the signal and noise values are typically negative,
> BG>> so one needs to think about mentally adding a minus sign if he/she wants to
> BG>> understand it. Does not ethtool know about 32-bits signed integers?
> BG>
> BG>Ethtool stats only supports u64. I think it's easy enough for
> BG>humans or programs to add the negative sign. Can signal or noise
> BG>ever be> 0? If so, that could actually break something that depends
> BG>on flipping the value to negative....
>
> Don't know is this is a bug or it's reaaly can be positive, but:
>
> iw dev mp0 station dump
> Station 00:02:6f:b8:94:d3 (on mp0)
> inactive time: 49 ms
> rx bytes: 36318341
> rx packets: 271741
> tx bytes: 4180152
> tx packets: 35445
> tx retries: 7724
> tx failed: 123
> signal: 1 dBm
> signal avg: -2 dBm
So, how about I just cast it to u8 and pass that to user-space.
Let user-space apps that care just understand the data is really just
a signed 8-bit number for now.
For the future, we can figure out a way to make ethtool
API deal with different data types better.
if (survey.filled & SURVEY_INFO_NOISE_DBM)
data[i++] = (u8)(survey.noise & 0xff);
else
...
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 04/12/2012 12:30 PM, Ben Hutchings wrote:
> On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
>> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
>>> Hi,
>>>
>>> Le 04/12/12 18:32, [email protected] a écrit :
>>>> From: Ben Greear<[email protected]>
>>>>
>>>> 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.
>>>
>>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
>>> understand it. Does not ethtool know about 32-bits signed integers?
>>
>> Ethtool stats only supports u64. I think it's easy enough for
>> humans or programs to add the negative sign. Can signal or noise
>> ever be> 0? If so, that could actually break something that depends
>> on flipping the value to negative....
>
> So far as I can see, the ethtool stats were expected to be counters,
> which obviously cannot become negative (or fractional). Maybe it's time
> to define another command and string-set to cover other types of status
> information. The ethtool utility could ask for those typed statistics
> as well, so 'ethtool -S' would get all of them.
One nice thing about ethtool stats API is that it is backwards and forwards
compatible for a long while. Also, adding a new API means a second call
into the kernel (most likely) for the other set of strings, which effectively
doubles the cost of getting stats (and allows the stats to be updated
out-of-sync with each other more easily).
I wonder if instead we could add a convention where we add a short
prefix (or suffix) to a string to denote it's type (and cast the value
into u64). So, an old ethtool might see:
foo:s32: 4294967295
while a new one understand that the s32: prefix is special, do
some casting, and could show:
foo: -1
Both are still at least somewhat human readable, and probably wouldn't
confuse anyone that is parsing the output of existing ethtool output.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
From: Ben Greear <[email protected]>
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 <[email protected]>
---
:100644 100644 3557354... 420b6eb... 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 3557354..420b6eb 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)
@@ -2773,4 +2841,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
On 04/12/2012 02:05 PM, Ben Hutchings wrote:
> On Thu, 2012-04-12 at 13:46 -0700, Ben Greear wrote:
>> On 04/12/2012 12:30 PM, Ben Hutchings wrote:
>>> On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
>>>> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
>>>>> Hi,
>>>>>
>>>>> Le 04/12/12 18:32, [email protected] a écrit :
>>>>>> From: Ben Greear<[email protected]>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
>>>>> understand it. Does not ethtool know about 32-bits signed integers?
>>>>
>>>> Ethtool stats only supports u64. I think it's easy enough for
>>>> humans or programs to add the negative sign. Can signal or noise
>>>> ever be> 0? If so, that could actually break something that depends
>>>> on flipping the value to negative....
>>>
>>> So far as I can see, the ethtool stats were expected to be counters,
>>> which obviously cannot become negative (or fractional). Maybe it's time
>>> to define another command and string-set to cover other types of status
>>> information. The ethtool utility could ask for those typed statistics
>>> as well, so 'ethtool -S' would get all of them.
>>
>> One nice thing about ethtool stats API is that it is backwards and forwards
>> compatible for a long while.
>
> Agreed.
> Actually it would be:
>
> foo:s32: 18446744073709551615
>
>> while a new one understand that the s32: prefix is special, do
>> some casting, and could show:
>> foo: -1
>>
>> Both are still at least somewhat human readable,
>
> I don't think many humans can mentally substract 2^64.
Well, if we add a new API, then anyone on older ethtool
won't see it at all, which is even more useless than a
large ugly number.
Those of us using ethtool API directly would have to add new
ioctl calls (and the performance is important to me, even
if atomicity isn't so important). That is more work than
adding some logic to parse suffixes on the strings I think.
>> and probably wouldn't confuse anyone that is parsing the output of
>> existing ethtool output.
>
> I think you have this backwards: printing numbers in two different ways
> (old and new versions of ethtool) is likely to confuse scripts that are
> parsing and doing calculations with these numbers. While I try to avoid
> gratuitous changes in output formatting, scripts should use the ethtool
> API if they really want interface stability. It's not difficult (there
> are at least Python and Perl bindings available) and it's a lot more
> efficient.
If the new ethtool -S is going to nicely present things (ie, show "foo: -1"),
then the negative numbers are there anyway, so maybe the compatibility issue
for anyone parsing the output of 'ethtool -S' is moot. Anyone parsing the
binary API sees no changes, but *could* update code to look at the suffix
if they cared.
That said, I *would* like a new 'ethool get-stats' API that took a 'verbose'
argument so that we could return more or less verbose results (dependent on the
driver to determine what that means). That way, we could probe easy-to-obtain
information quickly and often, and if there is something more expensive to obtain,
that could be probed less often. If this idea is worth pursuing, then perhaps
it could also include a new binary API that includes a type identifier.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
From: Ben Greear <[email protected]>
This adds hooks to call into the driver to get additional
stats for the ethtool API.
Signed-off-by: Ben Greear <[email protected]>
---
:100644 100644 32cd517... af61f3a... M include/net/mac80211.h
:100644 100644 420b6eb... b37fb0d... 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 32cd517..af61f3a 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 420b6eb..b37fb0d 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