[PATCH ethtool 0/5] Fix and enhance command-line parsing

[PATCH ethtool 0/5] Fix and enhance command-line parsing

[Ben Hutchings]

This patch series fixes the initialisation of RX n-tuple specs, adds
general support for named flags, and then uses that for message types
and extended offload flags.

Ben.

Ben Hutchings (5):
  ethtool: Parse integers into variables of different sizes and byte
    orders
  ethtool: Mark show_usage() as noreturn
  ethtool: Add support for named flags
  ethtool: Implement named message type flags
  ethtool: Use named flag support to update extended offload flags

 ethtool-util.h |   29 ++++-
 ethtool.8      |   66 ++++++++-
 ethtool.c      |  440 +++++++++++++++++++++++++++++++++++--------------------
 3 files changed, 368 insertions(+), 167 deletions(-)

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH ethtool 1/5] ethtool: Parse integers into variables of different sizes and byte orders

[Ben Hutchings]

The arguments for RX n-tuple traffic direction are filled into
structure fields of varying size, some of which are in big-endian
rather than native byte order.  Currently parse_generic_cmdline()
only supports 32-bit integers in native byte order, so this does
not work correctly.

Replace CMDL_INT and CMDL_UINT with the more explicit CMDL_S32 and
CMDL_U32.  Add CMDL_U16 and CMDL_U64 for narrower and wider integers,
and CMDL_BE16 and CMDL_BE32 for big-endian unsigned integers.  Use
them for RX n-tuple argument parsing.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
I've removed the 'historical' comment from the kernel-style named types
because I see no sign of a transition away from them.

Ben.

 ethtool-util.h |   29 ++++++-
 ethtool.c      |  226 +++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 160 insertions(+), 95 deletions(-)

diff --git a/ethtool-util.h b/ethtool-util.h
index e9a998a..01b1d03 100644
--- a/ethtool-util.h
+++ b/ethtool-util.h
@@ -4,14 +4,35 @@
 #define ETHTOOL_UTIL_H__
 
 #include <sys/types.h>
+#include <endian.h>
 
 #include "ethtool-copy.h"
 
-/* historical: we used to use kernel-like types; remove these once cleaned */
 typedef unsigned long long u64;
-typedef __uint32_t u32;         /* ditto */
-typedef __uint16_t u16;         /* ditto */
-typedef __uint8_t u8;           /* ditto */
+typedef __uint32_t u32;
+typedef __uint16_t u16;
+typedef __uint8_t u8;
+typedef __int32_t s32;
+
+#if __BYTE_ORDER == __BIG_ENDIAN
+static inline u16 cpu_to_be16(u16 value)
+{
+    return value;
+}
+static inline u32 cpu_to_be32(u32 value)
+{
+    return value;
+}
+#else
+static inline u16 cpu_to_be16(u16 value)
+{
+	return (value >> 8) | (value << 8);
+}
+static inline u32 cpu_to_be32(u32 value)
+{
+	return cpu_to_be16(value >> 16) | (cpu_to_be16(value) << 16);
+}
+#endif
 
 /* National Semiconductor DP83815, DP83816 */
 int natsemi_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
diff --git a/ethtool.c b/ethtool.c
index 5d61439..8969390 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -268,35 +268,35 @@ static int pause_tx_wanted = -1;
 
 static struct ethtool_ringparam ering;
 static int gring_changed = 0;
-static int ring_rx_wanted = -1;
-static int ring_rx_mini_wanted = -1;
-static int ring_rx_jumbo_wanted = -1;
-static int ring_tx_wanted = -1;
+static s32 ring_rx_wanted = -1;
+static s32 ring_rx_mini_wanted = -1;
+static s32 ring_rx_jumbo_wanted = -1;
+static s32 ring_tx_wanted = -1;
 
 static struct ethtool_coalesce ecoal;
 static int gcoalesce_changed = 0;
-static int coal_stats_wanted = -1;
+static s32 coal_stats_wanted = -1;
 static int coal_adaptive_rx_wanted = -1;
 static int coal_adaptive_tx_wanted = -1;
-static int coal_sample_rate_wanted = -1;
-static int coal_pkt_rate_low_wanted = -1;
-static int coal_pkt_rate_high_wanted = -1;
-static int coal_rx_usec_wanted = -1;
-static int coal_rx_frames_wanted = -1;
-static int coal_rx_usec_irq_wanted = -1;
-static int coal_rx_frames_irq_wanted = -1;
-static int coal_tx_usec_wanted = -1;
-static int coal_tx_frames_wanted = -1;
-static int coal_tx_usec_irq_wanted = -1;
-static int coal_tx_frames_irq_wanted = -1;
-static int coal_rx_usec_low_wanted = -1;
-static int coal_rx_frames_low_wanted = -1;
-static int coal_tx_usec_low_wanted = -1;
-static int coal_tx_frames_low_wanted = -1;
-static int coal_rx_usec_high_wanted = -1;
-static int coal_rx_frames_high_wanted = -1;
-static int coal_tx_usec_high_wanted = -1;
-static int coal_tx_frames_high_wanted = -1;
+static s32 coal_sample_rate_wanted = -1;
+static s32 coal_pkt_rate_low_wanted = -1;
+static s32 coal_pkt_rate_high_wanted = -1;
+static s32 coal_rx_usec_wanted = -1;
+static s32 coal_rx_frames_wanted = -1;
+static s32 coal_rx_usec_irq_wanted = -1;
+static s32 coal_rx_frames_irq_wanted = -1;
+static s32 coal_tx_usec_wanted = -1;
+static s32 coal_tx_frames_wanted = -1;
+static s32 coal_tx_usec_irq_wanted = -1;
+static s32 coal_tx_frames_irq_wanted = -1;
+static s32 coal_rx_usec_low_wanted = -1;
+static s32 coal_rx_frames_low_wanted = -1;
+static s32 coal_tx_usec_low_wanted = -1;
+static s32 coal_tx_frames_low_wanted = -1;
+static s32 coal_rx_usec_high_wanted = -1;
+static s32 coal_rx_frames_high_wanted = -1;
+static s32 coal_tx_usec_high_wanted = -1;
+static s32 coal_tx_frames_high_wanted = -1;
 
 static int speed_wanted = -1;
 static int duplex_wanted = -1;
@@ -319,13 +319,13 @@ static int gregs_dump_hex = 0;
 static char *gregs_dump_file = NULL;
 static int geeprom_changed = 0;
 static int geeprom_dump_raw = 0;
-static int geeprom_offset = 0;
-static int geeprom_length = -1;
+static s32 geeprom_offset = 0;
+static s32 geeprom_length = -1;
 static int seeprom_changed = 0;
-static int seeprom_magic = 0;
-static int seeprom_length = -1;
-static int seeprom_offset = 0;
-static int seeprom_value = EOF;
+static s32 seeprom_magic = 0;
+static s32 seeprom_length = -1;
+static s32 seeprom_offset = 0;
+static s32 seeprom_value = EOF;
 static int rx_fhash_get = 0;
 static int rx_fhash_set = 0;
 static u32 rx_fhash_val = 0;
@@ -343,14 +343,19 @@ static enum {
 typedef enum {
 	CMDL_NONE,
 	CMDL_BOOL,
-	CMDL_INT,
-	CMDL_UINT,
+	CMDL_S32,
+	CMDL_U16,
+	CMDL_U32,
+	CMDL_U64,
+	CMDL_BE16,
+	CMDL_BE32,
 	CMDL_STR,
 } cmdline_type_t;
 
 struct cmdline_info {
 	const char *name;
 	cmdline_type_t type;
+	/* Points to int (BOOL), s32, u16, u32, u64 or char * (STR) */
 	void *wanted_val;
 	void *ioctl_val;
 };
@@ -362,16 +367,16 @@ static struct cmdline_info cmdline_gregs[] = {
 };
 
 static struct cmdline_info cmdline_geeprom[] = {
-	{ "offset", CMDL_INT, &geeprom_offset, NULL },
-	{ "length", CMDL_INT, &geeprom_length, NULL },
+	{ "offset", CMDL_S32, &geeprom_offset, NULL },
+	{ "length", CMDL_S32, &geeprom_length, NULL },
 	{ "raw", CMDL_BOOL, &geeprom_dump_raw, NULL },
 };
 
 static struct cmdline_info cmdline_seeprom[] = {
-	{ "magic", CMDL_INT, &seeprom_magic, NULL },
-	{ "offset", CMDL_INT, &seeprom_offset, NULL },
-	{ "length", CMDL_INT, &seeprom_length, NULL },
-	{ "value", CMDL_INT, &seeprom_value, NULL },
+	{ "magic", CMDL_S32, &seeprom_magic, NULL },
+	{ "offset", CMDL_S32, &seeprom_offset, NULL },
+	{ "length", CMDL_S32, &seeprom_length, NULL },
+	{ "value", CMDL_S32, &seeprom_value, NULL },
 };
 
 static struct cmdline_info cmdline_offload[] = {
@@ -394,87 +399,94 @@ static struct cmdline_info cmdline_pause[] = {
 };
 
 static struct cmdline_info cmdline_ring[] = {
-	{ "rx", CMDL_INT, &ring_rx_wanted, &ering.rx_pending },
-	{ "rx-mini", CMDL_INT, &ring_rx_mini_wanted, &ering.rx_mini_pending },
-	{ "rx-jumbo", CMDL_INT, &ring_rx_jumbo_wanted, &ering.rx_jumbo_pending },
-	{ "tx", CMDL_INT, &ring_tx_wanted, &ering.tx_pending },
+	{ "rx", CMDL_S32, &ring_rx_wanted, &ering.rx_pending },
+	{ "rx-mini", CMDL_S32, &ring_rx_mini_wanted, &ering.rx_mini_pending },
+	{ "rx-jumbo", CMDL_S32, &ring_rx_jumbo_wanted, &ering.rx_jumbo_pending },
+	{ "tx", CMDL_S32, &ring_tx_wanted, &ering.tx_pending },
 };
 
 static struct cmdline_info cmdline_coalesce[] = {
 	{ "adaptive-rx", CMDL_BOOL, &coal_adaptive_rx_wanted, &ecoal.use_adaptive_rx_coalesce },
 	{ "adaptive-tx", CMDL_BOOL, &coal_adaptive_tx_wanted, &ecoal.use_adaptive_tx_coalesce },
-	{ "sample-interval", CMDL_INT, &coal_sample_rate_wanted, &ecoal.rate_sample_interval },
-	{ "stats-block-usecs", CMDL_INT, &coal_stats_wanted, &ecoal.stats_block_coalesce_usecs },
-	{ "pkt-rate-low", CMDL_INT, &coal_pkt_rate_low_wanted, &ecoal.pkt_rate_low },
-	{ "pkt-rate-high", CMDL_INT, &coal_pkt_rate_high_wanted, &ecoal.pkt_rate_high },
-	{ "rx-usecs", CMDL_INT, &coal_rx_usec_wanted, &ecoal.rx_coalesce_usecs },
-	{ "rx-frames", CMDL_INT, &coal_rx_frames_wanted, &ecoal.rx_max_coalesced_frames },
-	{ "rx-usecs-irq", CMDL_INT, &coal_rx_usec_irq_wanted, &ecoal.rx_coalesce_usecs_irq },
-	{ "rx-frames-irq", CMDL_INT, &coal_rx_frames_irq_wanted, &ecoal.rx_max_coalesced_frames_irq },
-	{ "tx-usecs", CMDL_INT, &coal_tx_usec_wanted, &ecoal.tx_coalesce_usecs },
-	{ "tx-frames", CMDL_INT, &coal_tx_frames_wanted, &ecoal.tx_max_coalesced_frames },
-	{ "tx-usecs-irq", CMDL_INT, &coal_tx_usec_irq_wanted, &ecoal.tx_coalesce_usecs_irq },
-	{ "tx-frames-irq", CMDL_INT, &coal_tx_frames_irq_wanted, &ecoal.tx_max_coalesced_frames_irq },
-	{ "rx-usecs-low", CMDL_INT, &coal_rx_usec_low_wanted, &ecoal.rx_coalesce_usecs_low },
-	{ "rx-frames-low", CMDL_INT, &coal_rx_frames_low_wanted, &ecoal.rx_max_coalesced_frames_low },
-	{ "tx-usecs-low", CMDL_INT, &coal_tx_usec_low_wanted, &ecoal.tx_coalesce_usecs_low },
-	{ "tx-frames-low", CMDL_INT, &coal_tx_frames_low_wanted, &ecoal.tx_max_coalesced_frames_low },
-	{ "rx-usecs-high", CMDL_INT, &coal_rx_usec_high_wanted, &ecoal.rx_coalesce_usecs_high },
-	{ "rx-frames-high", CMDL_INT, &coal_rx_frames_high_wanted, &ecoal.rx_max_coalesced_frames_high },
-	{ "tx-usecs-high", CMDL_INT, &coal_tx_usec_high_wanted, &ecoal.tx_coalesce_usecs_high },
-	{ "tx-frames-high", CMDL_INT, &coal_tx_frames_high_wanted, &ecoal.tx_max_coalesced_frames_high },
+	{ "sample-interval", CMDL_S32, &coal_sample_rate_wanted, &ecoal.rate_sample_interval },
+	{ "stats-block-usecs", CMDL_S32, &coal_stats_wanted, &ecoal.stats_block_coalesce_usecs },
+	{ "pkt-rate-low", CMDL_S32, &coal_pkt_rate_low_wanted, &ecoal.pkt_rate_low },
+	{ "pkt-rate-high", CMDL_S32, &coal_pkt_rate_high_wanted, &ecoal.pkt_rate_high },
+	{ "rx-usecs", CMDL_S32, &coal_rx_usec_wanted, &ecoal.rx_coalesce_usecs },
+	{ "rx-frames", CMDL_S32, &coal_rx_frames_wanted, &ecoal.rx_max_coalesced_frames },
+	{ "rx-usecs-irq", CMDL_S32, &coal_rx_usec_irq_wanted, &ecoal.rx_coalesce_usecs_irq },
+	{ "rx-frames-irq", CMDL_S32, &coal_rx_frames_irq_wanted, &ecoal.rx_max_coalesced_frames_irq },
+	{ "tx-usecs", CMDL_S32, &coal_tx_usec_wanted, &ecoal.tx_coalesce_usecs },
+	{ "tx-frames", CMDL_S32, &coal_tx_frames_wanted, &ecoal.tx_max_coalesced_frames },
+	{ "tx-usecs-irq", CMDL_S32, &coal_tx_usec_irq_wanted, &ecoal.tx_coalesce_usecs_irq },
+	{ "tx-frames-irq", CMDL_S32, &coal_tx_frames_irq_wanted, &ecoal.tx_max_coalesced_frames_irq },
+	{ "rx-usecs-low", CMDL_S32, &coal_rx_usec_low_wanted, &ecoal.rx_coalesce_usecs_low },
+	{ "rx-frames-low", CMDL_S32, &coal_rx_frames_low_wanted, &ecoal.rx_max_coalesced_frames_low },
+	{ "tx-usecs-low", CMDL_S32, &coal_tx_usec_low_wanted, &ecoal.tx_coalesce_usecs_low },
+	{ "tx-frames-low", CMDL_S32, &coal_tx_frames_low_wanted, &ecoal.tx_max_coalesced_frames_low },
+	{ "rx-usecs-high", CMDL_S32, &coal_rx_usec_high_wanted, &ecoal.rx_coalesce_usecs_high },
+	{ "rx-frames-high", CMDL_S32, &coal_rx_frames_high_wanted, &ecoal.rx_max_coalesced_frames_high },
+	{ "tx-usecs-high", CMDL_S32, &coal_tx_usec_high_wanted, &ecoal.tx_coalesce_usecs_high },
+	{ "tx-frames-high", CMDL_S32, &coal_tx_frames_high_wanted, &ecoal.tx_max_coalesced_frames_high },
 };
 
 static struct cmdline_info cmdline_ntuple[] = {
-	{ "src-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL },
-	{ "src-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL },
-	{ "dst-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL },
-	{ "dst-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL },
-	{ "src-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL },
-	{ "src-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL },
-	{ "dst-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL },
-	{ "dst-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL },
-	{ "vlan", CMDL_INT, &ntuple_fs.vlan_tag, NULL },
-	{ "vlan-mask", CMDL_UINT, &ntuple_fs.vlan_tag_mask, NULL },
-	{ "user-def", CMDL_INT, &ntuple_fs.data, NULL },
-	{ "user-def-mask", CMDL_UINT, &ntuple_fs.data_mask, NULL },
-	{ "action", CMDL_INT, &ntuple_fs.action, NULL },
+	{ "src-ip", CMDL_BE32, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL },
+	{ "src-ip-mask", CMDL_BE32, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL },
+	{ "dst-ip", CMDL_BE32, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL },
+	{ "dst-ip-mask", CMDL_BE32, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL },
+	{ "src-port", CMDL_BE16, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL },
+	{ "src-port-mask", CMDL_BE16, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL },
+	{ "dst-port", CMDL_BE16, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL },
+	{ "dst-port-mask", CMDL_BE16, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL },
+	{ "vlan", CMDL_U16, &ntuple_fs.vlan_tag, NULL },
+	{ "vlan-mask", CMDL_U16, &ntuple_fs.vlan_tag_mask, NULL },
+	{ "user-def", CMDL_U64, &ntuple_fs.data, NULL },
+	{ "user-def-mask", CMDL_U64, &ntuple_fs.data_mask, NULL },
+	{ "action", CMDL_S32, &ntuple_fs.action, NULL },
 };
 
-static int get_int(char *str, int base)
+static long long
+get_int_range(char *str, int base, long long min, long long max)
 {
-	long v;
+	long long v;
 	char *endp;
 
 	if (!str)
 		show_usage(1);
 	errno = 0;
-	v = strtol(str, &endp, base);
-	if ( errno || *endp || v > INT_MAX)
+	v = strtoll(str, &endp, base);
+	if (errno || *endp || v < min || v > max)
 		show_usage(1);
-	return (int)v;
+	return v;
 }
 
-static int get_uint(char *str, int base)
+static unsigned long long
+get_uint_range(char *str, int base, unsigned long long max)
 {
-	unsigned long v;
+	unsigned long long v;
 	char *endp;
 
 	if (!str)
 		show_usage(1);
 	errno = 0;
-	v = strtoul(str, &endp, base);
-	if ( errno || *endp || v > UINT_MAX)
+	v = strtoull(str, &endp, base);
+	if ( errno || *endp || v > max)
 		show_usage(1);
 	return v;
 }
 
+static int get_int(char *str, int base)
+{
+	return get_int_range(str, base, INT_MIN, INT_MAX);
+}
+
 static void parse_generic_cmdline(int argc, char **argp,
 				  int first_arg, int *changed,
 				  struct cmdline_info *info,
 				  unsigned int n_info)
 {
-	int i, idx, *p;
+	int i, idx;
 	int found;
 
 	for (i = first_arg; i < argc; i++) {
@@ -486,9 +498,9 @@ static void parse_generic_cmdline(int argc, char **argp,
 				i += 1;
 				if (i >= argc)
 					show_usage(1);
-				p = info[idx].wanted_val;
 				switch (info[idx].type) {
-				case CMDL_BOOL:
+				case CMDL_BOOL: {
+					int *p = info[idx].wanted_val;
 					if (!strcmp(argp[i], "on"))
 						*p = 1;
 					else if (!strcmp(argp[i], "off"))
@@ -496,12 +508,44 @@ static void parse_generic_cmdline(int argc, char **argp,
 					else
 						show_usage(1);
 					break;
-				case CMDL_INT: {
-					*p = get_int(argp[i],0);
+				}
+				case CMDL_S32: {
+					s32 *p = info[idx].wanted_val;
+					*p = get_int_range(argp[i], 0,
+							   -0x80000000LL,
+							   0x7fffffff);
+					break;
+				}
+				case CMDL_U16: {
+					u16 *p = info[idx].wanted_val;
+					*p = get_uint_range(argp[i], 0, 0xffff);
+					break;
+				}
+				case CMDL_U32: {
+					u32 *p = info[idx].wanted_val;
+					*p = get_uint_range(argp[i], 0,
+							    0xffffffff);
+					break;
+				}
+				case CMDL_U64: {
+					u64 *p = info[idx].wanted_val;
+					*p = get_uint_range(
+						argp[i], 0,
+						0xffffffffffffffffLL);
+					break;
+				}
+				case CMDL_BE16: {
+					u16 *p = info[idx].wanted_val;
+					*p = cpu_to_be16(
+						get_uint_range(argp[i], 0,
+							       0xffff));
 					break;
 				}
-				case CMDL_UINT: {
-					*p = get_uint(argp[i],0);
+				case CMDL_BE32: {
+					u32 *p = info[idx].wanted_val;
+					*p = cpu_to_be32(
+						get_uint_range(argp[i], 0,
+							       0xffffffff));
 					break;
 				}
 				case CMDL_STR: {
-- 
1.6.2.5


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH ethtool 2/5] ethtool: Mark show_usage() as noreturn

[Ben Hutchings]

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
This prevents the next patch from provoking an incorrect warning about
an uninitialised variable.

Ben.

 ethtool.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 8969390..d4bf5a8 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -220,6 +220,8 @@ static struct option {
 };
 

+static void show_usage(int badarg) __attribute__((noreturn));
+
 static void show_usage(int badarg)
 {
 	int i;
-- 
1.6.2.5


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH ethtool 3/5] ethtool: Add support for named flags

[Ben Hutchings]

Define an argument type CMDL_FLAG, which can be used to set and
clear flags.  For each setting that can be modified in this way,
the flags to be set and cleared are accumulated in two variables.

Add support for CMDL_FLAGS parse_generic_cmdline().

Add utility function print_flags().

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
print_flags() is unused here, but will be used in the next patch.

Ben.

 ethtool.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index d4bf5a8..4adab4b 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -352,14 +352,21 @@ typedef enum {
 	CMDL_BE16,
 	CMDL_BE32,
 	CMDL_STR,
+	CMDL_FLAG,
 } cmdline_type_t;
 
 struct cmdline_info {
 	const char *name;
 	cmdline_type_t type;
-	/* Points to int (BOOL), s32, u16, u32, u64 or char * (STR) */
+	/* Points to int (BOOL), s32, u16, u32 (U32/FLAG), u64 or
+	 * char * (STR).  For FLAG, the value accumulates all flags
+	 * to be set. */
 	void *wanted_val;
 	void *ioctl_val;
+	/* For FLAG, the flag value to be set/cleared */
+	u32 flag_val;
+	/* For FLAG, accumulates all flags to be cleared */
+	u32 *unwanted_val;
 };
 
 static struct cmdline_info cmdline_gregs[] = {
@@ -550,6 +557,17 @@ static void parse_generic_cmdline(int argc, char **argp,
 							       0xffffffff));
 					break;
 				}
+				case CMDL_FLAG: {
+					u32 *p;
+					if (!strcmp(argp[i], "on"))
+						p = info[idx].wanted_val;
+					else if (!strcmp(argp[i], "off"))
+						p = info[idx].unwanted_val;
+					else
+						show_usage(1);
+					*p |= info[idx].flag_val;
+					break;
+				}
 				case CMDL_STR: {
 					char **s = info[idx].wanted_val;
 					*s = strdup(argp[i]);
@@ -566,6 +584,26 @@ static void parse_generic_cmdline(int argc, char **argp,
 	}
 }
 
+static void
+print_flags(const struct cmdline_info *info, unsigned int n_info, u32 value)
+{
+	const char *sep = "";
+
+	while (n_info) {
+		if (info->type == CMDL_FLAG && value & info->flag_val) {
+			printf("%s%s", sep, info->name);
+			sep = " ";
+			value &= ~info->flag_val;
+		}
+		++info;
+		--n_info;
+	}
+
+	/* Print any unrecognised flags in hex */
+	if (value)
+		printf("%s%#x", sep, value);
+}
+
 static int rxflow_str_to_type(const char *str)
 {
 	int flow_type = 0;
-- 
1.6.2.5


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH ethtool 4/5] ethtool: Implement named message type flags

[Ben Hutchings]

Allow message type flags to be turned on and off by name.
Print the names of the currently set flags below the numeric value.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 ethtool.8 |   66 +++++++++++++++++++++++++++++++++++++--
 ethtool.c |  104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 156 insertions(+), 14 deletions(-)

diff --git a/ethtool.8 b/ethtool.8
index a7b43d5..5983d0e 100644
--- a/ethtool.8
+++ b/ethtool.8
@@ -200,7 +200,10 @@ ethtool \- Display or change ethernet card settings
 .RB [ wol \ \*(WO]
 .RB [ sopass \ \*(MA]
 .RB [ msglvl
-.IR N ]
+.IR N \ |
+.BI msglvl \ type
+.A1 on off
+.RB ...]
 
 .B ethtool \-n
 .I ethX
@@ -482,9 +485,66 @@ Disable (wake on nothing).  This option clears all previous options.
 .B sopass \*(MA\c
 Sets the SecureOn(tm) password.  The argument to this option must be 6
 bytes in ethernet MAC hex format (\*(MA).
-.TP
+.PP
 .BI msglvl \ N
-Sets the driver message level. Meanings differ per driver.
+.br
+.BI msglvl \ type
+.A1 on off
+.RB ...
+.RS
+Sets the driver message type flags by name or number. \fItype\fR
+names the type of message to enable or disable; \fIN\fR specifies the
+new flags numerically. The defined type names and numbers are:
+.PD 0
+.TP 12
+.B drv
+0x0001  General driver status
+.TP 12
+.B probe
+0x0002  Hardware probing
+.TP 12
+.B link
+0x0004  Link state
+.TP 12
+.B timer
+0x0008  Periodic status check
+.TP 12
+.B ifdown
+0x0010  Interface being brought down
+.TP 12
+.B ifup
+0x0020  Interface being brought up
+.TP 12
+.B rx_err
+0x0040  Receive error
+.TP 12
+.B tx_err
+0x0080  Transmit error
+.TP 12
+.B tx_queued
+0x0100  Transmit queueing
+.TP 12
+.B intr
+0x0200  Interrupt handling
+.TP 12
+.B tx_done
+0x0400  Transmit completion
+.TP 12
+.B rx_status
+0x0800  Receive completion
+.TP 12
+.B pktdata
+0x1000  Packet contents
+.TP 12
+.B hw
+0x2000  Hardware status
+.TP 12
+.B wol
+0x4000  Wake-on-LAN status
+.PP
+The precise meanings of these type flags differ between drivers.
+.PD
+.RE
 .TP
 .B \-n \-\-show-nfc
 Retrieves the receive network flow classification configurations.
diff --git a/ethtool.c b/ethtool.c
index 4adab4b..a70cd03 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -20,7 +20,6 @@
  *   * better man page (steal from mii-tool?)
  *   * fall back on SIOCMII* ioctl()s and possibly SIOCDEVPRIVATE*
  *   * abstract ioctls to allow for fallback modes of data gathering
- *   * symbolic names for msglvl bitmask
  */
 
 #ifdef HAVE_CONFIG_H
@@ -39,6 +38,7 @@
 #include <net/if.h>
 #include <sys/utsname.h>
 #include <limits.h>
+#include <ctype.h>
 
 #include <linux/sockios.h>
 #include "ethtool-util.h"
@@ -51,6 +51,26 @@
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 #endif
 
+#ifndef HAVE_NETIF_MSG
+enum {
+	NETIF_MSG_DRV		= 0x0001,
+	NETIF_MSG_PROBE		= 0x0002,
+	NETIF_MSG_LINK		= 0x0004,
+	NETIF_MSG_TIMER		= 0x0008,
+	NETIF_MSG_IFDOWN	= 0x0010,
+	NETIF_MSG_IFUP		= 0x0020,
+	NETIF_MSG_RX_ERR	= 0x0040,
+	NETIF_MSG_TX_ERR	= 0x0080,
+	NETIF_MSG_TX_QUEUED	= 0x0100,
+	NETIF_MSG_INTR		= 0x0200,
+	NETIF_MSG_TX_DONE	= 0x0400,
+	NETIF_MSG_RX_STATUS	= 0x0800,
+	NETIF_MSG_PKTDATA	= 0x1000,
+	NETIF_MSG_HW		= 0x2000,
+	NETIF_MSG_WOL		= 0x4000,
+};
+#endif
+
 static int parse_wolopts(char *optstr, u32 *data);
 static char *unparse_wolopts(int wolopts);
 static int parse_sopass(char *src, unsigned char *dest);
@@ -126,7 +146,7 @@ static struct option {
 		"		[ xcvr internal|external ]\n"
 		"		[ wol p|u|m|b|a|g|s|d... ]\n"
 		"		[ sopass %x:%x:%x:%x:%x:%x ]\n"
-		"		[ msglvl %d ] \n" },
+		"		[ msglvl %d | msglvl type on|off ... ]\n" },
     { "-a", "--show-pause", MODE_GPAUSE, "Show pause options" },
     { "-A", "--pause", MODE_SPAUSE, "Set pause options",
       "		[ autoneg on|off ]\n"
@@ -313,7 +333,6 @@ static int wol_change = 0;
 static u8 sopass_wanted[SOPASS_MAX];
 static int sopass_change = 0;
 static int gwol_changed = 0; /* did anything in GWOL change? */
-static int msglvl_wanted = -1;
 static int phys_id_time = 0;
 static int gregs_changed = 0;
 static int gregs_dump_raw = 0;
@@ -337,6 +356,11 @@ static struct ethtool_rx_ntuple_flow_spec ntuple_fs;
 static char *flash_file = NULL;
 static int flash = -1;
 static int flash_region = -1;
+
+static int msglvl_changed;
+static u32 msglvl_wanted = 0;
+static u32 msglvl_unwanted =0;
+
 static enum {
 	ONLINE=0,
 	OFFLINE,
@@ -455,6 +479,39 @@ static struct cmdline_info cmdline_ntuple[] = {
 	{ "action", CMDL_S32, &ntuple_fs.action, NULL },
 };
 
+static struct cmdline_info cmdline_msglvl[] = {
+	{ "drv", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_DRV, &msglvl_unwanted },
+	{ "probe", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_PROBE, &msglvl_unwanted },
+	{ "link", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_LINK, &msglvl_unwanted },
+	{ "timer", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_TIMER, &msglvl_unwanted },
+	{ "ifdown", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_IFDOWN, &msglvl_unwanted },
+	{ "ifup", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_IFUP, &msglvl_unwanted },
+	{ "rx_err", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_RX_ERR, &msglvl_unwanted },
+	{ "tx_err", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_TX_ERR, &msglvl_unwanted },
+	{ "tx_queued", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_TX_QUEUED, &msglvl_unwanted },
+	{ "intr", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_INTR, &msglvl_unwanted },
+	{ "tx_done", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_TX_DONE, &msglvl_unwanted },
+	{ "rx_status", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_RX_STATUS, &msglvl_unwanted },
+	{ "pktdata", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_PKTDATA, &msglvl_unwanted },
+	{ "hw", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_HW, &msglvl_unwanted },
+	{ "wol", CMDL_FLAG, &msglvl_wanted, NULL,
+	  NETIF_MSG_WOL, &msglvl_unwanted },
+};
+
 static long long
 get_int_range(char *str, int base, long long min, long long max)
 {
@@ -920,7 +977,20 @@ static void parse_cmdline(int argc, char **argp)
 				i++;
 				if (i >= argc)
 					show_usage(1);
-				msglvl_wanted = get_int(argp[i], 0);
+				if (isdigit((unsigned char)argp[i][0])) {
+					msglvl_changed = 1;
+					msglvl_unwanted = ~0;
+					msglvl_wanted =
+						get_uint_range(argp[i], 0,
+							       0xffffffff);
+				} else {
+					parse_generic_cmdline(
+						argc, argp, i,
+						&msglvl_changed,
+						cmdline_msglvl,
+						ARRAY_SIZE(cmdline_msglvl));
+					i = argc;
+				}
 				break;
 			}
 			show_usage(1);
@@ -2247,8 +2317,12 @@ static int do_gset(int fd, struct ifreq *ifr)
 	ifr->ifr_data = (caddr_t)&edata;
 	err = send_ioctl(fd, ifr);
 	if (err == 0) {
-		fprintf(stdout, "	Current message level: 0x%08x (%d)\n",
+		fprintf(stdout, "	Current message level: 0x%08x (%d)\n"
+			"			       ",
 			edata.data, edata.data);
+		print_flags(cmdline_msglvl, ARRAY_SIZE(cmdline_msglvl),
+			    edata.data);
+		fprintf(stdout, "\n");
 		allfail = 0;
 	} else if (errno != EOPNOTSUPP) {
 		perror("Cannot get message level");
@@ -2371,15 +2445,23 @@ static int do_sset(int fd, struct ifreq *ifr)
 		}
 	}
 
-	if (msglvl_wanted != -1) {
+	if (msglvl_changed) {
 		struct ethtool_value edata;
 
-		edata.cmd = ETHTOOL_SMSGLVL;
-		edata.data = msglvl_wanted;
-		ifr->ifr_data = (caddr_t)&edata;;
+		edata.cmd = ETHTOOL_GMSGLVL;
+		ifr->ifr_data = (caddr_t)&edata;
 		err = send_ioctl(fd, ifr);
-		if (err < 0)
-			perror("Cannot set new msglvl");
+		if (err < 0) {
+			perror("Cannot get msglvl");
+		} else {
+			edata.cmd = ETHTOOL_SMSGLVL;
+			edata.data = ((edata.data & ~msglvl_unwanted) |
+				      msglvl_wanted);
+			ifr->ifr_data = (caddr_t)&edata;
+			err = send_ioctl(fd, ifr);
+			if (err < 0)
+				perror("Cannot set new msglvl");
+		}
 	}
 
 	return 0;
-- 
1.6.2.5


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH ethtool 5/5] ethtool: Use named flag support to update extended offload flags

[Ben Hutchings]

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 ethtool.c |   70 ++++++++++--------------------------------------------------
 1 files changed, 12 insertions(+), 58 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index a70cd03..4073745 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -277,10 +277,9 @@ static int off_sg_wanted = -1;
 static int off_tso_wanted = -1;
 static int off_ufo_wanted = -1;
 static int off_gso_wanted = -1;
-static int off_lro_wanted = -1;
+static u32 off_flags_wanted = 0;
+static u32 off_flags_unwanted = 0;
 static int off_gro_wanted = -1;
-static int off_ntuple_wanted = -1;
-static int off_rxhash_wanted = -1;
 
 static struct ethtool_pauseparam epause;
 static int gpause_changed = 0;
@@ -419,10 +418,13 @@ static struct cmdline_info cmdline_offload[] = {
 	{ "tso", CMDL_BOOL, &off_tso_wanted, NULL },
 	{ "ufo", CMDL_BOOL, &off_ufo_wanted, NULL },
 	{ "gso", CMDL_BOOL, &off_gso_wanted, NULL },
-	{ "lro", CMDL_BOOL, &off_lro_wanted, NULL },
+	{ "lro", CMDL_FLAG, &off_flags_wanted, NULL,
+	  ETH_FLAG_LRO, &off_flags_unwanted },
 	{ "gro", CMDL_BOOL, &off_gro_wanted, NULL },
-	{ "ntuple", CMDL_BOOL, &off_ntuple_wanted, NULL },
-	{ "rxhash", CMDL_BOOL, &off_rxhash_wanted, NULL },
+	{ "ntuple", CMDL_FLAG, &off_flags_wanted, NULL,
+	  ETH_FLAG_NTUPLE, &off_flags_unwanted },
+	{ "rxhash", CMDL_FLAG, &off_flags_wanted, NULL,
+	  ETH_FLAG_RXHASH, &off_flags_unwanted },
 };
 
 static struct cmdline_info cmdline_pause[] = {
@@ -2191,7 +2193,7 @@ static int do_soffload(int fd, struct ifreq *ifr)
 			return 90;
 		}
 	}
-	if (off_lro_wanted >= 0) {
+	if (off_flags_wanted || off_flags_unwanted) {
 		changed = 1;
 		eval.cmd = ETHTOOL_GFLAGS;
 		eval.data = 0;
@@ -2203,14 +2205,12 @@ static int do_soffload(int fd, struct ifreq *ifr)
 		}
 
 		eval.cmd = ETHTOOL_SFLAGS;
-		if (off_lro_wanted == 1)
-			eval.data |= ETH_FLAG_LRO;
-		else
-			eval.data &= ~ETH_FLAG_LRO;
+		eval.data = ((eval.data & ~off_flags_unwanted) |
+			     off_flags_wanted);
 
 		err = ioctl(fd, SIOCETHTOOL, ifr);
 		if (err) {
-			perror("Cannot set large receive offload settings");
+			perror("Cannot set device flag settings");
 			return 92;
 		}
 	}
@@ -2225,52 +2225,6 @@ static int do_soffload(int fd, struct ifreq *ifr)
 			return 93;
 		}
 	}
-	if (off_ntuple_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_GFLAGS;
-		eval.data = 0;
-		ifr->ifr_data = (caddr_t)&eval;
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot get device flag settings");
-			return 91;
-		}
-
-		eval.cmd = ETHTOOL_SFLAGS;
-		if (off_ntuple_wanted == 1)
-			eval.data |= ETH_FLAG_NTUPLE;
-		else
-			eval.data &= ~ETH_FLAG_NTUPLE;
-
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot set n-tuple filter settings");
-			return 93;
-		}
-	}
-	if (off_rxhash_wanted >= 0) {
-		changed = 1;
-		eval.cmd = ETHTOOL_GFLAGS;
-		eval.data = 0;
-		ifr->ifr_data = (caddr_t)&eval;
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot get device flag settings");
-			return 91;
-		}
-
-		eval.cmd = ETHTOOL_SFLAGS;
-		if (off_rxhash_wanted)
-			eval.data |= ETH_FLAG_RXHASH;
-		else
-			eval.data &= ~ETH_FLAG_RXHASH;
-
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err) {
-			perror("Cannot set receive hash settings");
-			return 93;
-		}
-	}
 
 	if (!changed) {
 		fprintf(stdout, "no offload settings changed\n");
-- 
1.6.2.5

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH ethtool 4/5] ethtool: Implement named message type flags

[Jeff Garzik]

On 06/25/2010 10:49 AM, Ben Hutchings wrote:
> +static struct cmdline_info cmdline_msglvl[] = {
> +	{ "drv", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_DRV,&msglvl_unwanted },
> +	{ "probe", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_PROBE,&msglvl_unwanted },
> +	{ "link", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_LINK,&msglvl_unwanted },
> +	{ "timer", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_TIMER,&msglvl_unwanted },
> +	{ "ifdown", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_IFDOWN,&msglvl_unwanted },
> +	{ "ifup", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_IFUP,&msglvl_unwanted },
> +	{ "rx_err", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_RX_ERR,&msglvl_unwanted },
> +	{ "tx_err", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_TX_ERR,&msglvl_unwanted },
> +	{ "tx_queued", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_TX_QUEUED,&msglvl_unwanted },
> +	{ "intr", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_INTR,&msglvl_unwanted },
> +	{ "tx_done", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_TX_DONE,&msglvl_unwanted },
> +	{ "rx_status", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_RX_STATUS,&msglvl_unwanted },
> +	{ "pktdata", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_PKTDATA,&msglvl_unwanted },
> +	{ "hw", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_HW,&msglvl_unwanted },
> +	{ "wol", CMDL_FLAG,&msglvl_wanted, NULL,
> +	  NETIF_MSG_WOL,&msglvl_unwanted },


A nice improvement over one-variable-per-bit, I think.

applied patches 1-5, thanks!

	Jeff



P.S.  If you could use my open source email address (jgarzik@pobox.com 
or jeff@garzik.org), that would be greatly appreciated.  @redhat.com is 
only used for sign-offs, as it's less portable than !@redhat.com addresses.