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

Message ID 1512507203-17693-4-git-send-email-scott.branden@broadcom.com
State Superseded
Delegated to: John Linville
Headers show
Series
  • ethtool: add ETHTOOL_RESET support via --reset command
Related show

Commit Message

Scott Branden Dec. 5, 2017, 8:53 p.m.
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(-)

Comments

Michal Kubecek Dec. 5, 2017, 9:30 p.m. | #1
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
Scott Branden Dec. 5, 2017, 10:06 p.m. | #2
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
Andrew Lunn Dec. 5, 2017, 10:26 p.m. | #3
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
Michal Kubecek Dec. 5, 2017, 10:29 p.m. | #4
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
Scott Branden Dec. 5, 2017, 10:31 p.m. | #5
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
Scott Branden Dec. 6, 2017, 12:45 a.m. | #6
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
>

Patch

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" },
 	{}