[PATCH v2 0/3] ethtool: add ETHTOOL_RESET support via --reset command

[PATCH v2 0/3] ethtool: add ETHTOOL_RESET support via --reset command

[Scott Branden]

Patch series to add ETHTOOL_RESET support to ethtool userspace tool.
Include:
- revert custom change to ethtool-copy.h that is not in linux kernel
- sync ethtool-copy.h with ethtool.h in linux kernel net-next
- add ETHTOOL_RESET support with up to date ethtool.h reset defines

Scott Branden (3):
  Revert "ethtool: Add DMA Coalescing support"
  ethtool-copy.h: sync with net-next
  ethtool: Add ETHTOOL_RESET support via --reset command

 ethtool-copy.h | 68 ++++++++++++++++++++++++++++++++++++++++++-----
 ethtool.8.in   | 59 +++++++++++++++++++++++++++++++++++++++--
 ethtool.c      | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 194 insertions(+), 16 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] Revert "ethtool: Add DMA Coalescing support"

[Scott Branden]

This reverts commit 5dd7bfbc5079cb375876e4e76191263fc28ae1a6.

As Stephen Hemminger mentioned
there is an ABI compatibility issue with this patch:

https://patchwork.ozlabs.org/patch/806049/#1757846
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 ethtool-copy.h | 2 --
 ethtool.8.in   | 1 -
 ethtool.c      | 8 +-------
 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 4bb91eb..06fc04c 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -400,7 +400,6 @@ struct ethtool_modinfo {
  *	a TX interrupt, when the packet rate is above @pkt_rate_high.
  * @rate_sample_interval: How often to do adaptive coalescing packet rate
  *	sampling, measured in seconds.  Must not be zero.
- * @dmac: How many usecs to store packets before moving to host memory.
  *
  * Each pair of (usecs, max_frames) fields specifies that interrupts
  * should be coalesced until
@@ -451,7 +450,6 @@ struct ethtool_coalesce {
 	__u32	tx_coalesce_usecs_high;
 	__u32	tx_max_coalesced_frames_high;
 	__u32	rate_sample_interval;
-	__u32	dmac;
 };
 
 /**
diff --git a/ethtool.8.in b/ethtool.8.in
index 6ad3065..90ead41 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -165,7 +165,6 @@ ethtool \- query or control network driver and hardware settings
 .BN tx\-usecs\-high
 .BN tx\-frames\-high
 .BN sample\-interval
-.BN dmac
 .HP
 .B ethtool \-g|\-\-show\-ring
 .I devname
diff --git a/ethtool.c b/ethtool.c
index 1a2b7cc..c89b660 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1337,7 +1337,6 @@ static int dump_coalesce(const struct ethtool_coalesce *ecoal)
 		"sample-interval: %u\n"
 		"pkt-rate-low: %u\n"
 		"pkt-rate-high: %u\n"
-		"dmac: %u\n"
 		"\n"
 		"rx-usecs: %u\n"
 		"rx-frames: %u\n"
@@ -1363,7 +1362,6 @@ static int dump_coalesce(const struct ethtool_coalesce *ecoal)
 		ecoal->rate_sample_interval,
 		ecoal->pkt_rate_low,
 		ecoal->pkt_rate_high,
-		ecoal->dmac,
 
 		ecoal->rx_coalesce_usecs,
 		ecoal->rx_max_coalesced_frames,
@@ -2071,7 +2069,6 @@ static int do_scoalesce(struct cmd_context *ctx)
 	int coal_adaptive_rx_wanted = -1;
 	int coal_adaptive_tx_wanted = -1;
 	s32 coal_sample_rate_wanted = -1;
-	s32 coal_dmac_wanted = -1;
 	s32 coal_pkt_rate_low_wanted = -1;
 	s32 coal_pkt_rate_high_wanted = -1;
 	s32 coal_rx_usec_wanted = -1;
@@ -2097,8 +2094,6 @@ static int do_scoalesce(struct cmd_context *ctx)
 		  &ecoal.use_adaptive_tx_coalesce },
 		{ "sample-interval", CMDL_S32, &coal_sample_rate_wanted,
 		  &ecoal.rate_sample_interval },
-		{ "dmac", CMDL_S32, &coal_dmac_wanted,
-		  &ecoal.dmac },
 		{ "stats-block-usecs", CMDL_S32, &coal_stats_wanted,
 		  &ecoal.stats_block_coalesce_usecs },
 		{ "pkt-rate-low", CMDL_S32, &coal_pkt_rate_low_wanted,
@@ -4794,8 +4789,7 @@ static const struct option {
 	  "		[rx-frames-high N]\n"
 	  "		[tx-usecs-high N]\n"
 	  "		[tx-frames-high N]\n"
-	  "		[sample-interval N]\n"
-	  "		[dmac N]\n" },
+	  "		[sample-interval N]\n" },
 	{ "-g|--show-ring", 1, do_gring, "Query RX/TX ring parameters" },
 	{ "-G|--set-ring", 1, do_sring, "Set RX/TX ring parameters",
 	  "		[ rx N ]\n"
-- 
2.5.0

[PATCH v2 2/3] ethtool-copy.h: sync with net-next

[Scott Branden]

This covers kernel changes up to:

commit 40e44a1e669d078946f46853808a60d29e6f0885
Author: Scott Branden <scott.branden@broadcom.com>
Date:   Thu Nov 30 11:35:59 2017 -0800

    net: ethtool: add support for reset of AP inside NIC interface.

    Add ETH_RESET_AP to reset the application processor(s) inside the NIC
    interface.

    Current ETH_RESET_MGMT supports a management processor inside this NIC.
    This is typically used for remote NIC management purposes.

    Application processors exist inside some SmartNICs to run various
    applications inside the NIC processor - be it a simple algorithm without
    an OS to as complex as hosting multiple VMs.

    Signed-off-by: Scott Branden <scott.branden@broadcom.com>
    Reviewed-by: Andrew Lunn <andrew@lunn.ch>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 ethtool-copy.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 5 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 06fc04c..f4e7bb2 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
 /*
  * ethtool.h: Defines for Linux ethtool.
  *
@@ -1236,6 +1237,47 @@ struct ethtool_per_queue_op {
 	char	data[];
 };
 
+/**
+ * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
+ * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
+ * @active_fec: FEC mode which is active on porte
+ * @fec: Bitmask of supported/configured FEC modes
+ * @rsvd: Reserved for future extensions. i.e FEC bypass feature.
+ *
+ * Drivers should reject a non-zero setting of @autoneg when
+ * autoneogotiation is disabled (or not supported) for the link.
+ *
+ */
+struct ethtool_fecparam {
+	__u32   cmd;
+	/* bitmask of FEC modes */
+	__u32   active_fec;
+	__u32   fec;
+	__u32   reserved;
+};
+
+/**
+ * enum ethtool_fec_config_bits - flags definition of ethtool_fec_configuration
+ * @ETHTOOL_FEC_NONE: FEC mode configuration is not supported
+ * @ETHTOOL_FEC_AUTO: Default/Best FEC mode provided by driver
+ * @ETHTOOL_FEC_OFF: No FEC Mode
+ * @ETHTOOL_FEC_RS: Reed-Solomon Forward Error Detection mode
+ * @ETHTOOL_FEC_BASER: Base-R/Reed-Solomon Forward Error Detection mode
+ */
+enum ethtool_fec_config_bits {
+	ETHTOOL_FEC_NONE_BIT,
+	ETHTOOL_FEC_AUTO_BIT,
+	ETHTOOL_FEC_OFF_BIT,
+	ETHTOOL_FEC_RS_BIT,
+	ETHTOOL_FEC_BASER_BIT,
+};
+
+#define ETHTOOL_FEC_NONE		(1 << ETHTOOL_FEC_NONE_BIT)
+#define ETHTOOL_FEC_AUTO		(1 << ETHTOOL_FEC_AUTO_BIT)
+#define ETHTOOL_FEC_OFF			(1 << ETHTOOL_FEC_OFF_BIT)
+#define ETHTOOL_FEC_RS			(1 << ETHTOOL_FEC_RS_BIT)
+#define ETHTOOL_FEC_BASER		(1 << ETHTOOL_FEC_BASER_BIT)
+
 /* CMDs currently supported */
 #define ETHTOOL_GSET		0x00000001 /* DEPRECATED, Get settings.
 					    * Please use ETHTOOL_GLINKSETTINGS
@@ -1328,6 +1370,8 @@ struct ethtool_per_queue_op {
 #define ETHTOOL_SLINKSETTINGS	0x0000004d /* Set ethtool_link_settings */
 #define ETHTOOL_PHY_GTUNABLE	0x0000004e /* Get PHY tunable configuration */
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
+#define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
+#define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
@@ -1382,9 +1426,12 @@ enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_10000baseLR_Full_BIT	= 44,
 	ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT	= 45,
 	ETHTOOL_LINK_MODE_10000baseER_Full_BIT	= 46,
-	ETHTOOL_LINK_MODE_2500baseT_Full_BIT = 47,
-	ETHTOOL_LINK_MODE_5000baseT_Full_BIT = 48,
+	ETHTOOL_LINK_MODE_2500baseT_Full_BIT	= 47,
+	ETHTOOL_LINK_MODE_5000baseT_Full_BIT	= 48,
 
+	ETHTOOL_LINK_MODE_FEC_NONE_BIT	= 49,
+	ETHTOOL_LINK_MODE_FEC_RS_BIT	= 50,
+	ETHTOOL_LINK_MODE_FEC_BASER_BIT	= 51,
 
 	/* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
 	 * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
@@ -1393,7 +1440,7 @@ enum ethtool_link_mode_bit_indices {
 	 */
 
 	__ETHTOOL_LINK_MODE_LAST
-	  = ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+	  = ETHTOOL_LINK_MODE_FEC_BASER_BIT,
 };
 
 #define __ETHTOOL_LINK_MODE_LEGACY_MASK(base_name)	\
@@ -1484,13 +1531,17 @@ enum ethtool_link_mode_bit_indices {
  * it was forced up into this mode or autonegotiated.
  */
 
-/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. */
+/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.
+ * Update drivers/net/phy/phy.c:phy_speed_to_str() and
+ * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values.
+ */
 #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_14000		14000
 #define SPEED_20000		20000
 #define SPEED_25000		25000
 #define SPEED_40000		40000
@@ -1633,6 +1684,7 @@ enum ethtool_reset_flags {
 	ETH_RESET_PHY		= 1 << 6,	/* Transceiver/PHY */
 	ETH_RESET_RAM		= 1 << 7,	/* RAM shared between
 						 * multiple components */
+	ETH_RESET_AP		= 1 << 8,	/* Application processor */
 
 	ETH_RESET_DEDICATED	= 0x0000ffff,	/* All components dedicated to
 						 * this interface */
@@ -1701,6 +1753,8 @@ enum ethtool_reset_flags {
  *	%ethtool_link_mode_bit_indices for the link modes, and other
  *	link features that the link partner advertised through
  *	autonegotiation; 0 if unknown or not applicable.  Read-only.
+ * @transceiver: Used to distinguish different possible PHY types,
+ *	reported consistently by PHYLIB.  Read-only.
  *
  * If autonegotiation is disabled, the speed and @duplex represent the
  * fixed link mode and are writable if the driver supports multiple
@@ -1752,7 +1806,9 @@ struct ethtool_link_settings {
 	__u8	eth_tp_mdix;
 	__u8	eth_tp_mdix_ctrl;
 	__s8	link_mode_masks_nwords;
-	__u32	reserved[8];
+	__u8	transceiver;
+	__u8	reserved1[3];
+	__u32	reserved[7];
 	__u32	link_mode_masks[0];
 	/* layout of link_mode_masks fields:
 	 * __u32 map_supported[link_mode_masks_nwords];
-- 
2.5.0

[PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command

[Scott Branden]

Add ETHTOOL_RESET support via --reset command.

ie.  ethtool --reset DEVNAME <flagname(s)>

flagnames currently match the ETH_RESET_xxx names:
mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all

Alternatively, you can specific component bitfield directly using
ethtool --reset DEVNAME flags %x

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 ethtool.8.in | 58 +++++++++++++++++++++++++++++++++++++++++++++-
 ethtool.c    | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 1 deletion(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index 90ead41..e77ecd3 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -354,6 +354,21 @@ ethtool \- query or control network driver and hardware settings
 .B ethtool \-\-get\-phy\-tunable
 .I devname
 .RB [ downshift ]
+.HP
+.B ethtool \-\-reset
+.I devname
+.BN flags
+.RB [ mgmt ]
+.RB [ irq ]
+.RB [ dma ]
+.RB [ filter ]
+.RB [ offload ]
+.RB [ mac ]
+.RB [ phy ]
+.RB [ ram ]
+.RB [ ap ]
+.RB [ dedicated ]
+.RB [ all ]
 .
 .\" Adjust lines (i.e. full justification) and hyphenate.
 .ad
@@ -1006,6 +1021,46 @@ Downshift is useful where cable does not have the 4 pairs instance.
 
 Gets the PHY downshift count/status.
 .RE
+.TP
+.B \-\-reset
+Reset hardware components specified by flags and components listed below
+.RS 4
+.TP
+.BI flags \ N
+Resets the components based on direct flags mask
+.TP
+.B mgmt
+Management processor
+.TP
+.B irq
+Interrupt requester
+.TP
+.B dma
+DMA engine
+.TP
+.B filter
+Filtering/flow direction
+.TP
+.B offload
+Protocol offload
+.TP
+.B mac
+Media access controller
+.TP
+.B phy
+Transceiver/PHY
+.TP
+.B ram
+RAM shared between multiple components
+.B ap
+Application Processor
+.TP
+.B dedicated
+All components dedicated to this interface
+.TP
+.B all
+All components used by this interface, even if shared
+.RE
 .SH BUGS
 Not supported (in part or whole) on all network drivers.
 .SH AUTHOR
@@ -1023,7 +1078,8 @@ Andi Kleen,
 Alexander Duyck,
 Sucheta Chakraborty,
 Jesse Brandeburg,
-Ben Hutchings.
+Ben Hutchings,
+Scott Branden.
 .SH AVAILABILITY
 .B ethtool
 is available from
diff --git a/ethtool.c b/ethtool.c
index c89b660..5bcf5d2 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4636,6 +4636,68 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
 	return err;
 }
 
+static int do_reset(struct cmd_context *ctx)
+{
+	struct ethtool_value resetinfo;
+	int argc = ctx->argc;
+	char **argp = ctx->argp;
+	int i;
+
+	if (argc == 0)
+		exit_bad_args();
+
+	resetinfo.data = 0;
+
+	for (i = 0; i < argc; i++) {
+		if (!strcmp(argp[i], "flags")) {
+			__u32 flags;
+
+			i++;
+			if (i >= argc)
+				exit_bad_args();
+			flags = strtoul(argp[i], NULL, 0);
+			if (flags == 0)
+				exit_bad_args();
+			else
+				resetinfo.data |= flags;
+		} else if (!strcmp(argp[i], "mgmt")) {
+			resetinfo.data |= ETH_RESET_MGMT;
+		} else if (!strcmp(argp[i], "irq")) {
+			resetinfo.data |= ETH_RESET_IRQ;
+		} else if (!strcmp(argp[i], "dma")) {
+			resetinfo.data |= ETH_RESET_DMA;
+		} else if (!strcmp(argp[i], "filter")) {
+			resetinfo.data |= ETH_RESET_FILTER;
+		} else if (!strcmp(argp[i], "offload")) {
+			resetinfo.data |= ETH_RESET_OFFLOAD;
+		} else if (!strcmp(argp[i], "mac")) {
+			resetinfo.data |= ETH_RESET_MAC;
+		} else if (!strcmp(argp[i], "phy")) {
+			resetinfo.data |= ETH_RESET_PHY;
+		} else if (!strcmp(argp[i], "ram")) {
+			resetinfo.data |= ETH_RESET_RAM;
+		} else if (!strcmp(argp[i], "ap")) {
+			resetinfo.data |= ETH_RESET_AP;
+		} else if (!strcmp(argp[i], "dedicated")) {
+			resetinfo.data |= ETH_RESET_DEDICATED;
+		} else if (!strcmp(argp[i], "all")) {
+			resetinfo.data |= ETH_RESET_ALL;
+		} else {
+			exit_bad_args();
+		}
+	}
+
+	resetinfo.cmd = ETHTOOL_RESET;
+
+	if (send_ioctl(ctx, &resetinfo)) {
+		perror("Cannot issue RESET");
+		return 1;
+	}
+	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);
+
+	return 0;
+}
+
 static int parse_named_bool(struct cmd_context *ctx, const char *name, u8 *on)
 {
 	if (ctx->argc < 2)
@@ -4898,6 +4960,19 @@ static const struct option {
 	  "		[ downshift on|off [count N] ]\n"},
 	{ "--get-phy-tunable", 1, do_get_phy_tunable, "Get PHY tunable",
 	  "		[ downshift ]\n"},
+	{ "--reset", 1, do_reset, "Reset components",
+	  "		[ flags %x ]\n"
+	  "		[ mgmt ]\n"
+	  "		[ irq ]\n"
+	  "		[ dma ]\n"
+	  "		[ filter ]\n"
+	  "		[ offload ]\n"
+	  "		[ mac ]\n"
+	  "		[ phy ]\n"
+	  "		[ ram ]\n"
+	  "		[ ap ]\n"
+	  "		[ dedicated ]\n"
+	  "		[ all ]\n"},
 	{ "-h|--help", 0, show_usage, "Show this help" },
 	{ "--version", 0, do_version, "Show version number" },
 	{}
-- 
2.5.0

Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command

[Michal Kubecek]

On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
> Add ETHTOOL_RESET support via --reset command.
> 
> ie.  ethtool --reset DEVNAME <flagname(s)>
> 
> flagnames currently match the ETH_RESET_xxx names:
> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
> 
> Alternatively, you can specific component bitfield directly using
> ethtool --reset DEVNAME flags %x

IMHO it would be more consistent with e.g. msglvl without the keyword
"flags". It would be also nice to provide a symbolic way to specify the
shared flags.

> +static int do_reset(struct cmd_context *ctx)
> +{
> +	struct ethtool_value resetinfo;
> +	int argc = ctx->argc;
> +	char **argp = ctx->argp;
> +	int i;
> +
> +	if (argc == 0)
> +		exit_bad_args();
> +
> +	resetinfo.data = 0;
> +
> +	for (i = 0; i < argc; i++) {
> +		if (!strcmp(argp[i], "flags")) {
> +			__u32 flags;
> +
> +			i++;
> +			if (i >= argc)
> +				exit_bad_args();
> +			flags = strtoul(argp[i], NULL, 0);
> +			if (flags == 0)
> +				exit_bad_args();
> +			else
> +				resetinfo.data |= flags;
> +		} else if (!strcmp(argp[i], "mgmt")) {
> +			resetinfo.data |= ETH_RESET_MGMT;
> +		} else if (!strcmp(argp[i], "irq")) {
> +			resetinfo.data |= ETH_RESET_IRQ;
> +		} else if (!strcmp(argp[i], "dma")) {
> +			resetinfo.data |= ETH_RESET_DMA;
> +		} else if (!strcmp(argp[i], "filter")) {
> +			resetinfo.data |= ETH_RESET_FILTER;
> +		} else if (!strcmp(argp[i], "offload")) {
> +			resetinfo.data |= ETH_RESET_OFFLOAD;
> +		} else if (!strcmp(argp[i], "mac")) {
> +			resetinfo.data |= ETH_RESET_MAC;
> +		} else if (!strcmp(argp[i], "phy")) {
> +			resetinfo.data |= ETH_RESET_PHY;
> +		} else if (!strcmp(argp[i], "ram")) {
> +			resetinfo.data |= ETH_RESET_RAM;
> +		} else if (!strcmp(argp[i], "ap")) {
> +			resetinfo.data |= ETH_RESET_AP;
> +		} else if (!strcmp(argp[i], "dedicated")) {
> +			resetinfo.data |= ETH_RESET_DEDICATED;
> +		} else if (!strcmp(argp[i], "all")) {
> +			resetinfo.data |= ETH_RESET_ALL;
> +		} else {
> +			exit_bad_args();
> +		}
> +	}
> +
> +	resetinfo.cmd = ETHTOOL_RESET;
> +
> +	if (send_ioctl(ctx, &resetinfo)) {
> +		perror("Cannot issue RESET");
> +		return 1;
> +	}
> +	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);

According to documentation, driver is supposed to clear the flags
corresponding to components which were reset so that what is left are
those which were _not_ reset.

Michal Kubecek

Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command

[Scott Branden]

Hi Michal,

thanks for review - see comments

On 17-12-05 01:30 PM, Michal Kubecek wrote:
> On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
>> Add ETHTOOL_RESET support via --reset command.
>>
>> ie.  ethtool --reset DEVNAME <flagname(s)>
>>
>> flagnames currently match the ETH_RESET_xxx names:
>> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
>>
>> Alternatively, you can specific component bitfield directly using
>> ethtool --reset DEVNAME flags %x
> IMHO it would be more consistent with e.g. msglvl without the keyword
> "flags".
I don't see the consistency in ethtool of specifying a number without a 
keyword in front of it.
I can only find --set-dump specify a number?
Others have keyword and number.  msglvl is the keyword after specifying 
-s - same as flags is the keyword I use after specifying --reset.
Whichever convention is decided for the --reset command I will change 
the patch to though.

>   It would be also nice to provide a symbolic way to specify the
> shared flags.
I'll change to allow -shared to be added to the end of each component 
specified to use the shared bit.
  IE. mgmt-shared, irq-shared, dma-shared ?
>
>> +static int do_reset(struct cmd_context *ctx)
>> +{
>> +	struct ethtool_value resetinfo;
>> +	int argc = ctx->argc;
>> +	char **argp = ctx->argp;
>> +	int i;
>> +
>> +	if (argc == 0)
>> +		exit_bad_args();
>> +
>> +	resetinfo.data = 0;
>> +
>> +	for (i = 0; i < argc; i++) {
>> +		if (!strcmp(argp[i], "flags")) {
>> +			__u32 flags;
>> +
>> +			i++;
>> +			if (i >= argc)
>> +				exit_bad_args();
>> +			flags = strtoul(argp[i], NULL, 0);
>> +			if (flags == 0)
>> +				exit_bad_args();
>> +			else
>> +				resetinfo.data |= flags;
>> +		} else if (!strcmp(argp[i], "mgmt")) {
>> +			resetinfo.data |= ETH_RESET_MGMT;
>> +		} else if (!strcmp(argp[i], "irq")) {
>> +			resetinfo.data |= ETH_RESET_IRQ;
>> +		} else if (!strcmp(argp[i], "dma")) {
>> +			resetinfo.data |= ETH_RESET_DMA;
>> +		} else if (!strcmp(argp[i], "filter")) {
>> +			resetinfo.data |= ETH_RESET_FILTER;
>> +		} else if (!strcmp(argp[i], "offload")) {
>> +			resetinfo.data |= ETH_RESET_OFFLOAD;
>> +		} else if (!strcmp(argp[i], "mac")) {
>> +			resetinfo.data |= ETH_RESET_MAC;
>> +		} else if (!strcmp(argp[i], "phy")) {
>> +			resetinfo.data |= ETH_RESET_PHY;
>> +		} else if (!strcmp(argp[i], "ram")) {
>> +			resetinfo.data |= ETH_RESET_RAM;
>> +		} else if (!strcmp(argp[i], "ap")) {
>> +			resetinfo.data |= ETH_RESET_AP;
>> +		} else if (!strcmp(argp[i], "dedicated")) {
>> +			resetinfo.data |= ETH_RESET_DEDICATED;
>> +		} else if (!strcmp(argp[i], "all")) {
>> +			resetinfo.data |= ETH_RESET_ALL;
>> +		} else {
>> +			exit_bad_args();
>> +		}
>> +	}
>> +
>> +	resetinfo.cmd = ETHTOOL_RESET;
>> +
>> +	if (send_ioctl(ctx, &resetinfo)) {
>> +		perror("Cannot issue RESET");
>> +		return 1;
>> +	}
>> +	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);
> According to documentation, driver is supposed to clear the flags
> corresponding to components which were reset so that what is left are
> those which were _not_ reset.
I'll move the print above the send_ioctl.
> Michal Kubecek

Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command

[Andrew Lunn]

On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
> Add ETHTOOL_RESET support via --reset command.
> 
> ie.  ethtool --reset DEVNAME <flagname(s)>
> 
> flagnames currently match the ETH_RESET_xxx names:
> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all

[Snip]

> +.B ethtool \-\-reset
> +.I devname
> +.BN flags
> +.RB [ mgmt ]
> +.RB [ irq ]
> +.RB [ dma ]
> +.RB [ filter ]
> +.RB [ offload ]
> +.RB [ mac ]
> +.RB [ phy ]
> +.RB [ ram ]
> +.RB [ ap ]
> +.RB [ dedicated ]
> +.RB [ all ]

Hi Scott

Just a nick pick. You don't list ap above, which is kind of why you
are doing this, if i remember correctly.

    Andrew

Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command

[Michal Kubecek]

On Tue, Dec 05, 2017 at 02:06:09PM -0800, Scott Branden wrote:
> On 17-12-05 01:30 PM, Michal Kubecek wrote:
> > On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
> > > Add ETHTOOL_RESET support via --reset command.
> > > 
> > > ie.  ethtool --reset DEVNAME <flagname(s)>
> > > 
> > > flagnames currently match the ETH_RESET_xxx names:
> > > mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
> > > 
> > > Alternatively, you can specific component bitfield directly using
> > > ethtool --reset DEVNAME flags %x
> > IMHO it would be more consistent with e.g. msglvl without the keyword
> > "flags".
> I don't see the consistency in ethtool of specifying a number without a
> keyword in front of it.
> I can only find --set-dump specify a number?
> Others have keyword and number.  msglvl is the keyword after specifying -s -
> same as flags is the keyword I use after specifying --reset.

What I meant is that you can write

    ethtool -s eth0 msglvl drv on probe off
    ethtool -s eth0 msglvl 0x7

i.e. either number or names (with on/off in this case) while your patch
has

    ethtool --reset eth0 mgmg,irq
    ethtool --reset eth0 flags 0x3

i.e. an extra keyword if a number is used.

But it's not really important, it doesn't seem I would be able to share
a parser for this with any other subcommand or parameter anyway.

> >   It would be also nice to provide a symbolic way to specify the
> > shared flags.
> 
> I'll change to allow -shared to be added to the end of each component
> specified to use the shared bit.
>  IE. mgmt-shared, irq-shared, dma-shared ?

Sounds good to me.

> > > +	resetinfo.cmd = ETHTOOL_RESET;
> > > +
> > > +	if (send_ioctl(ctx, &resetinfo)) {
> > > +		perror("Cannot issue RESET");
> > > +		return 1;
> > > +	}
> > > +	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);
> > 
> > According to documentation, driver is supposed to clear the flags
> > corresponding to components which were reset so that what is left are
> > those which were _not_ reset.
> 
> I'll move the print above the send_ioctl.

It might be even more useful if ethtool informed user what actually
happened, i.e. either change the message to saying these are bits for
components not reset (if resetinfo.data is not zero) or save the
original value of resetinfo.data and show  saved_data & ~resetinfo.data

Michal Kubecek

Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command

[Scott Branden]

Hi Andrew,


On 17-12-05 02:26 PM, Andrew Lunn wrote:
> On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
>> Add ETHTOOL_RESET support via --reset command.
>>
>> ie.  ethtool --reset DEVNAME <flagname(s)>
>>
>> flagnames currently match the ETH_RESET_xxx names:
>> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
Yes, I missed adding ap to the commit message here.
> [Snip]
>
>> +.B ethtool \-\-reset
>> +.I devname
>> +.BN flags
>> +.RB [ mgmt ]
>> +.RB [ irq ]
>> +.RB [ dma ]
>> +.RB [ filter ]
>> +.RB [ offload ]
>> +.RB [ mac ]
>> +.RB [ phy ]
>> +.RB [ ram ]
>> +.RB [ ap ]
>> +.RB [ dedicated ]
>> +.RB [ all ]
> Hi Scott
>
> Just a nick pick. You don't list ap above, which is kind of why you
> are doing this, if i remember correctly.
Yes - I added ap to v2 of this patch but didn't add it to the commit 
message. Will update in v3.
>
>      Andrew

Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command

[Scott Branden]

Hi Michal,

Thanks - one question below hopefully someone can help with.


On 17-12-05 02:29 PM, Michal Kubecek wrote:
> On Tue, Dec 05, 2017 at 02:06:09PM -0800, Scott Branden wrote:
>> On 17-12-05 01:30 PM, Michal Kubecek wrote:
>>> On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
>>>> Add ETHTOOL_RESET support via --reset command.
>>>>
>>>> ie.  ethtool --reset DEVNAME <flagname(s)>
>>>>
>>>> flagnames currently match the ETH_RESET_xxx names:
>>>> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
>>>>
>>>> Alternatively, you can specific component bitfield directly using
>>>> ethtool --reset DEVNAME flags %x
>>> IMHO it would be more consistent with e.g. msglvl without the keyword
>>> "flags".
>> I don't see the consistency in ethtool of specifying a number without a
>> keyword in front of it.
>> I can only find --set-dump specify a number?
>> Others have keyword and number.  msglvl is the keyword after specifying -s -
>> same as flags is the keyword I use after specifying --reset.
> What I meant is that you can write
>
>      ethtool -s eth0 msglvl drv on probe off
>      ethtool -s eth0 msglvl 0x7
>
> i.e. either number or names (with on/off in this case) while your patch
> has
>
>      ethtool --reset eth0 mgmg,irq
>      ethtool --reset eth0 flags 0x3
>
> i.e. an extra keyword if a number is used.
>
> But it's not really important, it doesn't seem I would be able to share
> a parser for this with any other subcommand or parameter anyway.
>
>>>    It would be also nice to provide a symbolic way to specify the
>>> shared flags.
>> I'll change to allow -shared to be added to the end of each component
>> specified to use the shared bit.
>>   IE. mgmt-shared, irq-shared, dma-shared ?
> Sounds good to me.
>
>>>> +	resetinfo.cmd = ETHTOOL_RESET;
>>>> +
>>>> +	if (send_ioctl(ctx, &resetinfo)) {
>>>> +		perror("Cannot issue RESET");
>>>> +		return 1;
>>>> +	}
>>>> +	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);
>>> According to documentation, driver is supposed to clear the flags
>>> corresponding to components which were reset so that what is left are
>>> those which were _not_ reset.
>> I'll move the print above the send_ioctl.
> It might be even more useful if ethtool informed user what actually
> happened, i.e. either change the message to saying these are bits for
> components not reset (if resetinfo.data is not zero) or save the
> original value of resetinfo.data and show  saved_data & ~resetinfo.data
In making the improvement I found a bug in the bnxt_en kernel driver.
The bnxt_en driver currently doesn't clear any of the component flags on 
success so I need to send in a fix for that.

Although in one case (RESET_ALL) in the driver it doesn't actually 
execute the reset until all necessary drivers are unloaded to prevent 
the PCIe bus from hanging.
So question: should the flags be cleared if the reset is "pending" but 
hasn't actually happened yet, but will reset once all the drivers are 
all properly unloaded?
>
> Michal Kubecek
>