* [PATCH 0/2] ethtool(1) support for reading Phy stats
@ 2015-12-23 11:58 Andrew Lunn
2015-12-23 11:58 ` [PATCH 1/2] ethtool-copy.h: sync with net Andrew Lunn
2015-12-23 11:58 ` [PATCH 2/2] ethtool: Add PHY statistics support Andrew Lunn
0 siblings, 2 replies; 9+ messages in thread
From: Andrew Lunn @ 2015-12-23 11:58 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Florian Fainelli, netdev, Andrew Lunn
Add support to ethertool(1) for reading phy statistics. The mechanism
is the same for reading MAC stats.
Andrew Lunn (2):
ethtool-copy.h: sync with net
ethtool: Add PHY statistics support
ethtool-copy.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++------
ethtool.8.in | 6 ++++++
ethtool.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 115 insertions(+), 6 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] ethtool-copy.h: sync with net
2015-12-23 11:58 [PATCH 0/2] ethtool(1) support for reading Phy stats Andrew Lunn
@ 2015-12-23 11:58 ` Andrew Lunn
2015-12-23 11:58 ` [PATCH 2/2] ethtool: Add PHY statistics support Andrew Lunn
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Lunn @ 2015-12-23 11:58 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Florian Fainelli, netdev, Andrew Lunn
This covers kernel changes up to:
commit eff3cddc222c88943ff515ae9335687c9e2cbaf6
Author: Jacob Keller <jacob.e.keller@intel.com>
Date: Wed Apr 22 14:40:30 2015 -0700
clarify implementation of ethtool's get_ts_info op
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
ethtool-copy.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 49 insertions(+), 6 deletions(-)
diff --git a/ethtool-copy.h b/ethtool-copy.h
index d23ffc4..57fa390 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -10,8 +10,8 @@
* Portions Copyright (C) Sun Microsystems 2008
*/
-#ifndef _LINUX_ETHTOOL_H
-#define _LINUX_ETHTOOL_H
+#ifndef _UAPI_LINUX_ETHTOOL_H
+#define _UAPI_LINUX_ETHTOOL_H
#include <linux/types.h>
#include <linux/if_ether.h>
@@ -110,7 +110,7 @@ struct ethtool_cmd {
__u32 reserved[2];
};
-static __inline__ void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
+static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
__u32 speed)
{
@@ -118,7 +118,7 @@ static __inline__ void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
ep->speed_hi = (__u16)(speed >> 16);
}
-static __inline__ __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
+static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
{
return (ep->speed_hi << 16) | ep->speed;
}
@@ -215,6 +215,11 @@ enum tunable_id {
ETHTOOL_ID_UNSPEC,
ETHTOOL_RX_COPYBREAK,
ETHTOOL_TX_COPYBREAK,
+ /*
+ * Add your fresh new tubale attribute above and remember to update
+ * tunable_strings[] in net/core/ethtool.c
+ */
+ __ETHTOOL_TUNABLE_COUNT,
};
enum tunable_type_id {
@@ -537,6 +542,7 @@ struct ethtool_pauseparam {
* now deprecated
* @ETH_SS_FEATURES: Device feature names
* @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
+ * @ETH_SS_PHY_STATS: Statistic names, for use with %ETHTOOL_GPHYSTATS
*/
enum ethtool_stringset {
ETH_SS_TEST = 0,
@@ -545,6 +551,8 @@ enum ethtool_stringset {
ETH_SS_NTUPLE_FILTERS,
ETH_SS_FEATURES,
ETH_SS_RSS_HASH_FUNCS,
+ ETH_SS_TUNABLES,
+ ETH_SS_PHY_STATS,
};
/**
@@ -796,6 +804,31 @@ struct ethtool_rx_flow_spec {
__u32 location;
};
+/* How rings are layed out when accessing virtual functions or
+ * offloaded queues is device specific. To allow users to do flow
+ * steering and specify these queues the ring cookie is partitioned
+ * into a 32bit queue index with an 8 bit virtual function id.
+ * This also leaves the 3bytes for further specifiers. It is possible
+ * future devices may support more than 256 virtual functions if
+ * devices start supporting PCIe w/ARI. However at the moment I
+ * do not know of any devices that support this so I do not reserve
+ * space for this at this time. If a future patch consumes the next
+ * byte it should be aware of this possiblity.
+ */
+#define ETHTOOL_RX_FLOW_SPEC_RING 0x00000000FFFFFFFFLL
+#define ETHTOOL_RX_FLOW_SPEC_RING_VF 0x000000FF00000000LL
+#define ETHTOOL_RX_FLOW_SPEC_RING_VF_OFF 32
+static inline __u64 ethtool_get_flow_spec_ring(__u64 ring_cookie)
+{
+ return ETHTOOL_RX_FLOW_SPEC_RING & ring_cookie;
+};
+
+static inline __u64 ethtool_get_flow_spec_ring_vf(__u64 ring_cookie)
+{
+ return (ETHTOOL_RX_FLOW_SPEC_RING_VF & ring_cookie) >>
+ ETHTOOL_RX_FLOW_SPEC_RING_VF_OFF;
+};
+
/**
* struct ethtool_rxnfc - command to get or set RX flow classification rules
* @cmd: Specific command number - %ETHTOOL_GRXFH, %ETHTOOL_SRXFH,
@@ -1062,6 +1095,11 @@ struct ethtool_sfeatures {
* the 'hwtstamp_tx_types' and 'hwtstamp_rx_filters' enumeration values,
* respectively. For example, if the device supports HWTSTAMP_TX_ON,
* then (1 << HWTSTAMP_TX_ON) in 'tx_types' will be set.
+ *
+ * Drivers should only report the filters they actually support without
+ * upscaling in the SIOCSHWTSTAMP ioctl. If the SIOCSHWSTAMP request for
+ * HWTSTAMP_FILTER_V1_SYNC is supported by HWTSTAMP_FILTER_V1_EVENT, then the
+ * driver should only report HWTSTAMP_FILTER_V1_EVENT in this op.
*/
struct ethtool_ts_info {
__u32 cmd;
@@ -1189,6 +1227,7 @@ enum ethtool_sfeatures_retval_bits {
#define ETHTOOL_SRSSH 0x00000047 /* Set RX flow hash configuration */
#define ETHTOOL_GTUNABLE 0x00000048 /* Get tunable configuration */
#define ETHTOOL_STUNABLE 0x00000049 /* Set tunable configuration */
+#define ETHTOOL_GPHYSTATS 0x0000004a /* get PHY-specific statistics */
/* compatibility with older code */
#define SPARC_ETH_GSET ETHTOOL_GSET
@@ -1264,15 +1303,19 @@ enum ethtool_sfeatures_retval_bits {
* it was forced up into this mode or autonegotiated.
*/
-/* The forced speed, 10Mb, 100Mb, gigabit, [2.5|10|20|40|56]GbE. */
+/* The forced speed, 10Mb, 100Mb, gigabit, [2.5|5|10|20|25|40|50|56|100]GbE. */
#define SPEED_10 10
#define SPEED_100 100
#define SPEED_1000 1000
#define SPEED_2500 2500
+#define SPEED_5000 5000
#define SPEED_10000 10000
#define SPEED_20000 20000
+#define SPEED_25000 25000
#define SPEED_40000 40000
+#define SPEED_50000 50000
#define SPEED_56000 56000
+#define SPEED_100000 100000
#define SPEED_UNKNOWN -1
@@ -1398,4 +1441,4 @@ enum ethtool_reset_flags {
};
#define ETH_RESET_SHARED_SHIFT 16
-#endif /* _LINUX_ETHTOOL_H */
+#endif /* _UAPI_LINUX_ETHTOOL_H */
--
2.1.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] ethtool: Add PHY statistics support
2015-12-23 11:58 [PATCH 0/2] ethtool(1) support for reading Phy stats Andrew Lunn
2015-12-23 11:58 ` [PATCH 1/2] ethtool-copy.h: sync with net Andrew Lunn
@ 2015-12-23 11:58 ` Andrew Lunn
2016-03-13 15:50 ` Ben Hutchings
` (2 more replies)
1 sibling, 3 replies; 9+ messages in thread
From: Andrew Lunn @ 2015-12-23 11:58 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Florian Fainelli, netdev, Andrew Lunn
This adds support for printing statistics from the network devices PHY.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
ethtool.8.in | 6 ++++++
ethtool.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
diff --git a/ethtool.8.in b/ethtool.8.in
index eeffa70..2316556 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -220,6 +220,9 @@ ethtool \- query or control network driver and hardware settings
.B ethtool \-S|\-\-statistics
.I devname
.HP
+.B ethtool \-I|\-\-phy-statistics
+.I devname
+.HP
.B ethtool \-t|\-\-test
.I devname
.RI [\*(SD]
@@ -492,6 +495,9 @@ auto-negotiation is enabled.
Queries the specified network device for NIC- and driver-specific
statistics.
.TP
+.B \-I \-\-phy\-statistics
+Queries the specified network device for PHY specific statistics.
+.TP
.B \-t \-\-test
Executes adapter selftest on the specified network device. Possible test modes are:
.TP
diff --git a/ethtool.c b/ethtool.c
index 92c40b8..480c14c 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2995,6 +2995,64 @@ static int do_gstats(struct cmd_context *ctx)
return 0;
}
+static int do_gphystats(struct cmd_context *ctx)
+{
+ struct ethtool_gstrings *strings;
+ struct ethtool_stats *stats;
+ unsigned int n_stats, sz_stats, i;
+ int err;
+
+ if (ctx->argc != 0)
+ exit_bad_args();
+
+ strings = get_stringset(ctx, ETH_SS_PHY_STATS,
+ offsetof(struct ethtool_drvinfo, n_stats),
+ 0);
+ if (!strings) {
+ perror("Cannot get stats strings information");
+ return 96;
+ }
+
+ n_stats = strings->len;
+ if (n_stats < 1) {
+ fprintf(stderr, "no stats available\n");
+ free(strings);
+ return 94;
+ }
+
+ sz_stats = n_stats * sizeof(u64);
+
+ stats = calloc(1, sz_stats + sizeof(struct ethtool_stats));
+ if (!stats) {
+ fprintf(stderr, "no memory available\n");
+ free(strings);
+ return 95;
+ }
+
+ stats->cmd = ETHTOOL_GPHYSTATS;
+ stats->n_stats = n_stats;
+ err = send_ioctl(ctx, stats);
+ if (err < 0) {
+ perror("Cannot get stats information");
+ free(strings);
+ free(stats);
+ return 97;
+ }
+
+ /* todo - pretty-print the strings per-driver */
+ fprintf(stdout, "PHY statistics:\n");
+ for (i = 0; i < n_stats; i++) {
+ fprintf(stdout, " %.*s: %llu\n",
+ ETH_GSTRING_LEN,
+ &strings->data[i * ETH_GSTRING_LEN],
+ stats->data[i]);
+ }
+ free(strings);
+ free(stats);
+
+ return 0;
+}
+
static int do_srxntuple(struct cmd_context *ctx,
struct ethtool_rx_flow_spec *rx_rule_fs);
@@ -4078,6 +4136,8 @@ static const struct option {
{ "-t|--test", 1, do_test, "Execute adapter self test",
" [ online | offline | external_lb ]\n" },
{ "-S|--statistics", 1, do_gstats, "Show adapter statistics" },
+ { "-I|--phy-statistics", 1, do_gphystats,
+ "Show phy statistics" },
{ "-n|-u|--show-nfc|--show-ntuple", 1, do_grxclass,
"Show Rx network flow classification options or rules",
" [ rx-flow-hash tcp4|udp4|ah4|esp4|sctp4|"
--
2.1.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ethtool: Add PHY statistics support
2015-12-23 11:58 ` [PATCH 2/2] ethtool: Add PHY statistics support Andrew Lunn
@ 2016-03-13 15:50 ` Ben Hutchings
2016-03-13 16:08 ` Andrew Lunn
2016-03-13 16:01 ` [PATCH ethtool 1/2] Remove short option -I for PHY statistics Ben Hutchings
2016-03-13 16:01 ` [PATCH ethtool 2/2] Refactor do_gstats() and do_gphystats() to avoid code duplication Ben Hutchings
2 siblings, 1 reply; 9+ messages in thread
From: Ben Hutchings @ 2016-03-13 15:50 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev
[-- Attachment #1: Type: text/plain, Size: 3777 bytes --]
On Wed, 2015-12-23 at 12:58 +0100, Andrew Lunn wrote:
> This adds support for printing statistics from the network devices PHY.
Sorry it's taken me so long to respond to this. There are a few issues
with it but I'll apply it anyway then fix them up.
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> ethtool.8.in | 6 ++++++
> ethtool.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 66 insertions(+)
>
> diff --git a/ethtool.8.in b/ethtool.8.in
> index eeffa70..2316556 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -220,6 +220,9 @@ ethtool \- query or control network driver and hardware settings
> .B ethtool \-S|\-\-statistics
> .I devname
> .HP
> +.B ethtool \-I|\-\-phy-statistics
> +.I devname
> +.HP
-I isn't a useful mnemonic, so I will drop the short option altogether.
> .B ethtool \-t|\-\-test
> .I devname
> .RI [\*(SD]
> @@ -492,6 +495,9 @@ auto-negotiation is enabled.
> Queries the specified network device for NIC- and driver-specific
> statistics.
> .TP
> +.B \-I \-\-phy\-statistics
> +Queries the specified network device for PHY specific statistics.
> +.TP
> .B \-t \-\-test
> Executes adapter selftest on the specified network device. Possible test modes are:
> .TP
> diff --git a/ethtool.c b/ethtool.c
> index 92c40b8..480c14c 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -2995,6 +2995,64 @@ static int do_gstats(struct cmd_context *ctx)
> return 0;
> }
>
> +static int do_gphystats(struct cmd_context *ctx)
> +{
> + struct ethtool_gstrings *strings;
> + struct ethtool_stats *stats;
> + unsigned int n_stats, sz_stats, i;
> + int err;
> +
> + if (ctx->argc != 0)
> + exit_bad_args();
> +
> + strings = get_stringset(ctx, ETH_SS_PHY_STATS,
> + offsetof(struct ethtool_drvinfo, n_stats),
> + 0);
> + if (!strings) {
> + perror("Cannot get stats strings information");
> + return 96;
> + }
> +
> + n_stats = strings->len;
> + if (n_stats < 1) {
> + fprintf(stderr, "no stats available\n");
> + free(strings);
> + return 94;
> + }
> +
> + sz_stats = n_stats * sizeof(u64);
> +
> + stats = calloc(1, sz_stats + sizeof(struct ethtool_stats));
> + if (!stats) {
> + fprintf(stderr, "no memory available\n");
> + free(strings);
> + return 95;
> + }
> +
> + stats->cmd = ETHTOOL_GPHYSTATS;
> + stats->n_stats = n_stats;
> + err = send_ioctl(ctx, stats);
> + if (err < 0) {
> + perror("Cannot get stats information");
> + free(strings);
> + free(stats);
> + return 97;
> + }
> +
> + /* todo - pretty-print the strings per-driver */
> + fprintf(stdout, "PHY statistics:\n");
> + for (i = 0; i < n_stats; i++) {
> + fprintf(stdout, " %.*s: %llu\n",
> + ETH_GSTRING_LEN,
> + &strings->data[i * ETH_GSTRING_LEN],
> + stats->data[i]);
> + }
> + free(strings);
> + free(stats);
> +
> + return 0;
> +}
This is basically a copy-paste of do_gstats() so they should be merged
into one function.
Ben.
> static int do_srxntuple(struct cmd_context *ctx,
> struct ethtool_rx_flow_spec *rx_rule_fs);
>
> @@ -4078,6 +4136,8 @@ static const struct option {
> { "-t|--test", 1, do_test, "Execute adapter self test",
> " [ online | offline | external_lb ]\n" },
> { "-S|--statistics", 1, do_gstats, "Show adapter statistics" },
> + { "-I|--phy-statistics", 1, do_gphystats,
> + "Show phy statistics" },
> { "-n|-u|--show-nfc|--show-ntuple", 1, do_grxclass,
> "Show Rx network flow classification options or rules",
> " [ rx-flow-hash tcp4|udp4|ah4|esp4|sctp4|"
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH ethtool 1/2] Remove short option -I for PHY statistics
2015-12-23 11:58 ` [PATCH 2/2] ethtool: Add PHY statistics support Andrew Lunn
2016-03-13 15:50 ` Ben Hutchings
@ 2016-03-13 16:01 ` Ben Hutchings
2016-03-13 16:09 ` Andrew Lunn
2016-03-13 16:01 ` [PATCH ethtool 2/2] Refactor do_gstats() and do_gphystats() to avoid code duplication Ben Hutchings
2 siblings, 1 reply; 9+ messages in thread
From: Ben Hutchings @ 2016-03-13 16:01 UTC (permalink / raw)
To: netdev; +Cc: Andrew Lunn
It's not mnemonic and there's no requirement to have short options
for every command.
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
ethtool.8.in | 4 ++--
ethtool.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/ethtool.8.in b/ethtool.8.in
index 2316556d65de..e44db99dc5d6 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -220,7 +220,7 @@ ethtool \- query or control network driver and hardware settings
.B ethtool \-S|\-\-statistics
.I devname
.HP
-.B ethtool \-I|\-\-phy-statistics
+.B \-\-phy\-statistics
.I devname
.HP
.B ethtool \-t|\-\-test
@@ -495,7 +495,7 @@ auto-negotiation is enabled.
Queries the specified network device for NIC- and driver-specific
statistics.
.TP
-.B \-I \-\-phy\-statistics
+.B \-\-phy\-statistics
Queries the specified network device for PHY specific statistics.
.TP
.B \-t \-\-test
diff --git a/ethtool.c b/ethtool.c
index 480c14c8d30c..1c988f7d8a9d 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4136,7 +4136,7 @@ static const struct option {
{ "-t|--test", 1, do_test, "Execute adapter self test",
" [ online | offline | external_lb ]\n" },
{ "-S|--statistics", 1, do_gstats, "Show adapter statistics" },
- { "-I|--phy-statistics", 1, do_gphystats,
+ { "--phy-statistics", 1, do_gphystats,
"Show phy statistics" },
{ "-n|-u|--show-nfc|--show-ntuple", 1, do_grxclass,
"Show Rx network flow classification options or rules",
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH ethtool 2/2] Refactor do_gstats() and do_gphystats() to avoid code duplication
2015-12-23 11:58 ` [PATCH 2/2] ethtool: Add PHY statistics support Andrew Lunn
2016-03-13 15:50 ` Ben Hutchings
2016-03-13 16:01 ` [PATCH ethtool 1/2] Remove short option -I for PHY statistics Ben Hutchings
@ 2016-03-13 16:01 ` Ben Hutchings
2016-03-13 16:12 ` Andrew Lunn
2 siblings, 1 reply; 9+ messages in thread
From: Ben Hutchings @ 2016-03-13 16:01 UTC (permalink / raw)
To: netdev; +Cc: Andrew Lunn
The new do_gphystats() function is almost exactly the same as
do_gstats(), which is silly.
* Add parameters to do_gstats() for the command number, string set
number and heading
* Introduce do_gnicstats() as a wrapper for do_gstats() that does
what do_gstats() used to
* Change do_gphystats() into a wrapper for do_gstats()
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
ethtool.c | 71 +++++++++++----------------------------------------------------
1 file changed, 12 insertions(+), 59 deletions(-)
diff --git a/ethtool.c b/ethtool.c
index 1c988f7d8a9d..4f69a825849a 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2937,7 +2937,8 @@ static int do_phys_id(struct cmd_context *ctx)
return err;
}
-static int do_gstats(struct cmd_context *ctx)
+static int do_gstats(struct cmd_context *ctx, int cmd, int stringset,
+ const char *name)
{
struct ethtool_gstrings *strings;
struct ethtool_stats *stats;
@@ -2947,7 +2948,7 @@ static int do_gstats(struct cmd_context *ctx)
if (ctx->argc != 0)
exit_bad_args();
- strings = get_stringset(ctx, ETH_SS_STATS,
+ strings = get_stringset(ctx, stringset,
offsetof(struct ethtool_drvinfo, n_stats),
0);
if (!strings) {
@@ -2971,7 +2972,7 @@ static int do_gstats(struct cmd_context *ctx)
return 95;
}
- stats->cmd = ETHTOOL_GSTATS;
+ stats->cmd = cmd;
stats->n_stats = n_stats;
err = send_ioctl(ctx, stats);
if (err < 0) {
@@ -2982,7 +2983,7 @@ static int do_gstats(struct cmd_context *ctx)
}
/* todo - pretty-print the strings per-driver */
- fprintf(stdout, "NIC statistics:\n");
+ fprintf(stdout, "%s statistics:\n", name);
for (i = 0; i < n_stats; i++) {
fprintf(stdout, " %.*s: %llu\n",
ETH_GSTRING_LEN,
@@ -2995,62 +2996,14 @@ static int do_gstats(struct cmd_context *ctx)
return 0;
}
-static int do_gphystats(struct cmd_context *ctx)
+static int do_gnicstats(struct cmd_context *ctx)
{
- struct ethtool_gstrings *strings;
- struct ethtool_stats *stats;
- unsigned int n_stats, sz_stats, i;
- int err;
-
- if (ctx->argc != 0)
- exit_bad_args();
-
- strings = get_stringset(ctx, ETH_SS_PHY_STATS,
- offsetof(struct ethtool_drvinfo, n_stats),
- 0);
- if (!strings) {
- perror("Cannot get stats strings information");
- return 96;
- }
-
- n_stats = strings->len;
- if (n_stats < 1) {
- fprintf(stderr, "no stats available\n");
- free(strings);
- return 94;
- }
-
- sz_stats = n_stats * sizeof(u64);
-
- stats = calloc(1, sz_stats + sizeof(struct ethtool_stats));
- if (!stats) {
- fprintf(stderr, "no memory available\n");
- free(strings);
- return 95;
- }
-
- stats->cmd = ETHTOOL_GPHYSTATS;
- stats->n_stats = n_stats;
- err = send_ioctl(ctx, stats);
- if (err < 0) {
- perror("Cannot get stats information");
- free(strings);
- free(stats);
- return 97;
- }
-
- /* todo - pretty-print the strings per-driver */
- fprintf(stdout, "PHY statistics:\n");
- for (i = 0; i < n_stats; i++) {
- fprintf(stdout, " %.*s: %llu\n",
- ETH_GSTRING_LEN,
- &strings->data[i * ETH_GSTRING_LEN],
- stats->data[i]);
- }
- free(strings);
- free(stats);
+ return do_gstats(ctx, ETHTOOL_GSTATS, ETH_SS_STATS, "NIC");
+}
- return 0;
+static int do_gphystats(struct cmd_context *ctx)
+{
+ return do_gstats(ctx, ETHTOOL_GPHYSTATS, ETH_SS_PHY_STATS, "PHY");
}
static int do_srxntuple(struct cmd_context *ctx,
@@ -4135,7 +4088,7 @@ static const struct option {
" [ TIME-IN-SECONDS ]\n" },
{ "-t|--test", 1, do_test, "Execute adapter self test",
" [ online | offline | external_lb ]\n" },
- { "-S|--statistics", 1, do_gstats, "Show adapter statistics" },
+ { "-S|--statistics", 1, do_gnicstats, "Show adapter statistics" },
{ "--phy-statistics", 1, do_gphystats,
"Show phy statistics" },
{ "-n|-u|--show-nfc|--show-ntuple", 1, do_grxclass,
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ethtool: Add PHY statistics support
2016-03-13 15:50 ` Ben Hutchings
@ 2016-03-13 16:08 ` Andrew Lunn
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Lunn @ 2016-03-13 16:08 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Florian Fainelli, netdev
> > +.B ethtool \-I|\-\-phy-statistics
> > +.I devname
> > +.HP
>
> -I isn't a useful mnemonic, so I will drop the short option altogether.
It was the best i could come up with. -P would of been nice, since we
have -S for MAC statistics, but that is taken. PHY sounds a bit like
I, so why not. But i'm O.K. for -I to be dropped.
> > + /* todo - pretty-print the strings per-driver */
> > + fprintf(stdout, "PHY statistics:\n");
> > + for (i = 0; i < n_stats; i++) {
> > + fprintf(stdout, " %.*s: %llu\n",
> > + ETH_GSTRING_LEN,
> > + &strings->data[i * ETH_GSTRING_LEN],
> > + stats->data[i]);
> > + }
> > + free(strings);
> > + free(stats);
> > +
> > + return 0;
> > +}
>
> This is basically a copy-paste of do_gstats() so they should be merged
> into one function.
O.K, i can do that.
Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH ethtool 1/2] Remove short option -I for PHY statistics
2016-03-13 16:01 ` [PATCH ethtool 1/2] Remove short option -I for PHY statistics Ben Hutchings
@ 2016-03-13 16:09 ` Andrew Lunn
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Lunn @ 2016-03-13 16:09 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev
On Sun, Mar 13, 2016 at 04:01:20PM +0000, Ben Hutchings wrote:
> It's not mnemonic and there's no requirement to have short options
> for every command.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Thanks
Andrew
> ---
> ethtool.8.in | 4 ++--
> ethtool.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 2316556d65de..e44db99dc5d6 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -220,7 +220,7 @@ ethtool \- query or control network driver and hardware settings
> .B ethtool \-S|\-\-statistics
> .I devname
> .HP
> -.B ethtool \-I|\-\-phy-statistics
> +.B \-\-phy\-statistics
> .I devname
> .HP
> .B ethtool \-t|\-\-test
> @@ -495,7 +495,7 @@ auto-negotiation is enabled.
> Queries the specified network device for NIC- and driver-specific
> statistics.
> .TP
> -.B \-I \-\-phy\-statistics
> +.B \-\-phy\-statistics
> Queries the specified network device for PHY specific statistics.
> .TP
> .B \-t \-\-test
> diff --git a/ethtool.c b/ethtool.c
> index 480c14c8d30c..1c988f7d8a9d 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -4136,7 +4136,7 @@ static const struct option {
> { "-t|--test", 1, do_test, "Execute adapter self test",
> " [ online | offline | external_lb ]\n" },
> { "-S|--statistics", 1, do_gstats, "Show adapter statistics" },
> - { "-I|--phy-statistics", 1, do_gphystats,
> + { "--phy-statistics", 1, do_gphystats,
> "Show phy statistics" },
> { "-n|-u|--show-nfc|--show-ntuple", 1, do_grxclass,
> "Show Rx network flow classification options or rules",
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH ethtool 2/2] Refactor do_gstats() and do_gphystats() to avoid code duplication
2016-03-13 16:01 ` [PATCH ethtool 2/2] Refactor do_gstats() and do_gphystats() to avoid code duplication Ben Hutchings
@ 2016-03-13 16:12 ` Andrew Lunn
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Lunn @ 2016-03-13 16:12 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev
On Sun, Mar 13, 2016 at 04:01:49PM +0000, Ben Hutchings wrote:
> The new do_gphystats() function is almost exactly the same as
> do_gstats(), which is silly.
>
> * Add parameters to do_gstats() for the command number, string set
> number and heading
> * Introduce do_gnicstats() as a wrapper for do_gstats() that does
> what do_gstats() used to
> * Change do_gphystats() into a wrapper for do_gstats()
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Thanks
Andrew
> ---
> ethtool.c | 71 +++++++++++----------------------------------------------------
> 1 file changed, 12 insertions(+), 59 deletions(-)
>
> diff --git a/ethtool.c b/ethtool.c
> index 1c988f7d8a9d..4f69a825849a 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -2937,7 +2937,8 @@ static int do_phys_id(struct cmd_context *ctx)
> return err;
> }
>
> -static int do_gstats(struct cmd_context *ctx)
> +static int do_gstats(struct cmd_context *ctx, int cmd, int stringset,
> + const char *name)
> {
> struct ethtool_gstrings *strings;
> struct ethtool_stats *stats;
> @@ -2947,7 +2948,7 @@ static int do_gstats(struct cmd_context *ctx)
> if (ctx->argc != 0)
> exit_bad_args();
>
> - strings = get_stringset(ctx, ETH_SS_STATS,
> + strings = get_stringset(ctx, stringset,
> offsetof(struct ethtool_drvinfo, n_stats),
> 0);
> if (!strings) {
> @@ -2971,7 +2972,7 @@ static int do_gstats(struct cmd_context *ctx)
> return 95;
> }
>
> - stats->cmd = ETHTOOL_GSTATS;
> + stats->cmd = cmd;
> stats->n_stats = n_stats;
> err = send_ioctl(ctx, stats);
> if (err < 0) {
> @@ -2982,7 +2983,7 @@ static int do_gstats(struct cmd_context *ctx)
> }
>
> /* todo - pretty-print the strings per-driver */
> - fprintf(stdout, "NIC statistics:\n");
> + fprintf(stdout, "%s statistics:\n", name);
> for (i = 0; i < n_stats; i++) {
> fprintf(stdout, " %.*s: %llu\n",
> ETH_GSTRING_LEN,
> @@ -2995,62 +2996,14 @@ static int do_gstats(struct cmd_context *ctx)
> return 0;
> }
>
> -static int do_gphystats(struct cmd_context *ctx)
> +static int do_gnicstats(struct cmd_context *ctx)
> {
> - struct ethtool_gstrings *strings;
> - struct ethtool_stats *stats;
> - unsigned int n_stats, sz_stats, i;
> - int err;
> -
> - if (ctx->argc != 0)
> - exit_bad_args();
> -
> - strings = get_stringset(ctx, ETH_SS_PHY_STATS,
> - offsetof(struct ethtool_drvinfo, n_stats),
> - 0);
> - if (!strings) {
> - perror("Cannot get stats strings information");
> - return 96;
> - }
> -
> - n_stats = strings->len;
> - if (n_stats < 1) {
> - fprintf(stderr, "no stats available\n");
> - free(strings);
> - return 94;
> - }
> -
> - sz_stats = n_stats * sizeof(u64);
> -
> - stats = calloc(1, sz_stats + sizeof(struct ethtool_stats));
> - if (!stats) {
> - fprintf(stderr, "no memory available\n");
> - free(strings);
> - return 95;
> - }
> -
> - stats->cmd = ETHTOOL_GPHYSTATS;
> - stats->n_stats = n_stats;
> - err = send_ioctl(ctx, stats);
> - if (err < 0) {
> - perror("Cannot get stats information");
> - free(strings);
> - free(stats);
> - return 97;
> - }
> -
> - /* todo - pretty-print the strings per-driver */
> - fprintf(stdout, "PHY statistics:\n");
> - for (i = 0; i < n_stats; i++) {
> - fprintf(stdout, " %.*s: %llu\n",
> - ETH_GSTRING_LEN,
> - &strings->data[i * ETH_GSTRING_LEN],
> - stats->data[i]);
> - }
> - free(strings);
> - free(stats);
> + return do_gstats(ctx, ETHTOOL_GSTATS, ETH_SS_STATS, "NIC");
> +}
>
> - return 0;
> +static int do_gphystats(struct cmd_context *ctx)
> +{
> + return do_gstats(ctx, ETHTOOL_GPHYSTATS, ETH_SS_PHY_STATS, "PHY");
> }
>
> static int do_srxntuple(struct cmd_context *ctx,
> @@ -4135,7 +4088,7 @@ static const struct option {
> " [ TIME-IN-SECONDS ]\n" },
> { "-t|--test", 1, do_test, "Execute adapter self test",
> " [ online | offline | external_lb ]\n" },
> - { "-S|--statistics", 1, do_gstats, "Show adapter statistics" },
> + { "-S|--statistics", 1, do_gnicstats, "Show adapter statistics" },
> { "--phy-statistics", 1, do_gphystats,
> "Show phy statistics" },
> { "-n|-u|--show-nfc|--show-ntuple", 1, do_grxclass,
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-03-13 16:12 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-23 11:58 [PATCH 0/2] ethtool(1) support for reading Phy stats Andrew Lunn
2015-12-23 11:58 ` [PATCH 1/2] ethtool-copy.h: sync with net Andrew Lunn
2015-12-23 11:58 ` [PATCH 2/2] ethtool: Add PHY statistics support Andrew Lunn
2016-03-13 15:50 ` Ben Hutchings
2016-03-13 16:08 ` Andrew Lunn
2016-03-13 16:01 ` [PATCH ethtool 1/2] Remove short option -I for PHY statistics Ben Hutchings
2016-03-13 16:09 ` Andrew Lunn
2016-03-13 16:01 ` [PATCH ethtool 2/2] Refactor do_gstats() and do_gphystats() to avoid code duplication Ben Hutchings
2016-03-13 16:12 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).