| Message ID | 1512507203-17693-4-git-send-email-scott.branden@broadcom.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | John Linville |
| Headers | show |
| Series | ethtool: add ETHTOOL_RESET support via --reset command | expand |
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
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
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
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
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
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 >
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" }, {}
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(-)