| Message ID | 1304378957-24123-1-git-send-email-anirban.chakraborty@qlogic.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote: > From: Anirban Chakraborty <anirban.chakraborty@qlogic.com> > > Added support to take FW dump via ethtool. [...] > --- a/ethtool.c > +++ b/ethtool.c [...] > @@ -263,6 +270,12 @@ static struct option { > "Get Rx ntuple filters and actions\n" }, > { "-P", "--show-permaddr", MODE_PERMADDR, > "Show permanent hardware address" }, > + { "-W", "--get-dump", MODE_GET_DUMP, > + "Get dump level\n" }, > + { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA, > + "Get dump data", "FILENAME " "Name of the dump file\n" }, The short options should only include one letter. Also the general pattern is that 'get' options use lower-case letters and 'set' options use upper-case letters. No, I'm not sure how best to handle a set of 3 options. Maybe you can combine --get-dump and --get-dump-data, making the filename optional? > + { "-w", "--set-dump", MODE_SET_DUMP, > + "Set dump level", "DUMPLEVEL " "Dump level for the device\n" }, The field this sets is described as 'flags' so does it consist of flags or is it a level? [...] > @@ -3241,6 +3270,86 @@ static int do_grxntuple(int fd, struct ifreq *ifr) > return 0; > } > > +static void do_writedump(struct ethtool_dump *dump) > +{ > + FILE *f = fopen(dump_file, "wb+"); > + size_t bytes; > + > + if (!f ) { > + fprintf(stderr, "Can't open file %s: %s\n", > + dump_file, strerror(errno)); > + return; On error, we must exit with code 1. > + } > + > + bytes = fwrite(dump->data, 1, dump->len, f); > + fclose(f); [...] These functions can also fail and need to be checked. (Yes, fclose() can fail, since it may have to flush buffered data.) Ben.
On May 4, 2011, at 10:40 AM, Ben Hutchings wrote: > On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote: >> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com> >> >> Added support to take FW dump via ethtool. > [...] >> --- a/ethtool.c >> +++ b/ethtool.c > [...] >> @@ -263,6 +270,12 @@ static struct option { >> "Get Rx ntuple filters and actions\n" }, >> { "-P", "--show-permaddr", MODE_PERMADDR, >> "Show permanent hardware address" }, >> + { "-W", "--get-dump", MODE_GET_DUMP, >> + "Get dump level\n" }, >> + { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA, >> + "Get dump data", "FILENAME " "Name of the dump file\n" }, > > The short options should only include one letter. Also the general > pattern is that 'get' options use lower-case letters and 'set' options > use upper-case letters. No, I'm not sure how best to handle a set of 3 > options. Maybe you can combine --get-dump and --get-dump-data, making > the filename optional? Thanks for the info, I was not aware of it. Will address it. > >> + { "-w", "--set-dump", MODE_SET_DUMP, >> + "Set dump level", "DUMPLEVEL " "Dump level for the device\n" }, > > The field this sets is described as 'flags' so does it consist of flags > or is it a level? The idea is to have an opaque flag for the driver that it uses to set the dump level (and hence the size) of it. I will use flag instead of level here so that there is no ambiguity. > > [...] >> @@ -3241,6 +3270,86 @@ static int do_grxntuple(int fd, struct ifreq *ifr) >> return 0; >> } >> >> +static void do_writedump(struct ethtool_dump *dump) >> +{ >> + FILE *f = fopen(dump_file, "wb+"); >> + size_t bytes; >> + >> + if (!f ) { >> + fprintf(stderr, "Can't open file %s: %s\n", >> + dump_file, strerror(errno)); >> + return; > > On error, we must exit with code 1. Will do. > >> + } >> + >> + bytes = fwrite(dump->data, 1, dump->len, f); >> + fclose(f); > [...] > > These functions can also fail and need to be checked. (Yes, fclose() > can fail, since it may have to flush buffered data.) I agree. Will fix it. Thanks for the feedback. -Anirban -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
One more thing: please can you update the manual page as well as the built-in usage information. Ben.
On Wed, 2011-05-04 at 11:15 -0700, Ajit.Khaparde@Emulex.Com wrote: > ________________________________________ > From: netdev-owner@vger.kernel.org [netdev-owner@vger.kernel.org] On Behalf Of Ben Hutchings [bhutchings@solarflare.com] > Sent: Wednesday, May 04, 2011 12:40 PM > To: anirban.chakraborty@qlogic.com > Cc: netdev@vger.kernel.org; --no-chain-reply-to@mv.qlogic.com; davem@davemloft.net > Subject: Re: [ethtool PATCH] FW dump support > > > On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote: > >> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com> > >> > >> Added support to take FW dump via ethtool. > > [...] > >> --- a/ethtool.c > >> +++ b/ethtool.c > > [...] > >> @@ -263,6 +270,12 @@ static struct option { > >> "Get Rx ntuple filters and actions\n" }, > >> { "-P", "--show-permaddr", MODE_PERMADDR, > >> "Show permanent hardware address" }, > >> + { "-W", "--get-dump", MODE_GET_DUMP, > >> + "Get dump level\n" }, > >> + { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA, > >> + "Get dump data", "FILENAME " "Name of the dump file\n" }, > > > > The short options should only include one letter. Also the general > > pattern is that 'get' options use lower-case letters and 'set' options > > use upper-case letters. No, I'm not sure how best to handle a set of 3 > > options. Maybe you can combine --get-dump and --get-dump-data, making > > the filename optional? > > ethtool already has "-f" option to flash/write the FW image. > Can you use "-F" to get the FW dump data out? > And then may be, these options can be extended to get the get/set dump levels? Although '-F' would be nicely mnemonic, firmware update and dump are different enough that I don't think they make sense as a pair. If a driver implements both then there is the risk of a typo causing the firmware image file to be overwritten with a firmware dump. I also don't think it's possible to extend the '-f' option without breaking compatibility with scripts that already use it. Ben.
diff --git a/ethtool-copy.h b/ethtool-copy.h index 5b45442..0c0e847 100644 --- a/ethtool-copy.h +++ b/ethtool-copy.h @@ -76,6 +76,19 @@ struct ethtool_drvinfo { __u32 regdump_len; /* Size of data from ETHTOOL_GREGS (bytes) */ }; +/* dump struct + * cmd: commnad to identify type of operation + * flag: specify dump related options + * len: length of dumped data + * data: dump data + */ +struct ethtool_dump { + __u32 cmd; + __u32 flag; + __u32 len; + __u8 data[0]; +}; + #define SOPASS_MAX 6 /* wake-on-lan settings */ struct ethtool_wolinfo { @@ -538,7 +551,6 @@ struct ethtool_flash { char data[ETHTOOL_FLASH_MAX_FILENAME]; }; - /* CMDs currently supported */ #define ETHTOOL_GSET 0x00000001 /* Get settings. */ #define ETHTOOL_SSET 0x00000002 /* Set settings. */ @@ -599,6 +611,9 @@ struct ethtool_flash { #define ETHTOOL_GSSET_INFO 0x00000037 /* Get string set info */ #define ETHTOOL_GRXFHINDIR 0x00000038 /* Get RX flow hash indir'n table */ #define ETHTOOL_SRXFHINDIR 0x00000039 /* Set RX flow hash indir'n table */ +#define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */ +#define ETHTOOL_GET_DUMP 0x0000003f /* Get dump settings */ +#define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ /* compatibility with older code */ #define SPARC_ETH_GSET ETHTOOL_GSET diff --git a/ethtool.c b/ethtool.c index cfdac65..1a826ae 100644 --- a/ethtool.c +++ b/ethtool.c @@ -28,6 +28,7 @@ #include <sys/types.h> #include <string.h> #include <stdlib.h> +#include <stddef.h> #include <sys/types.h> #include <sys/ioctl.h> #include <sys/stat.h> @@ -110,6 +111,9 @@ static int do_srxntuple(int fd, struct ifreq *ifr); static int do_grxntuple(int fd, struct ifreq *ifr); static int do_flash(int fd, struct ifreq *ifr); static int do_permaddr(int fd, struct ifreq *ifr); +static int do_getdump(int fd, struct ifreq *ifr); +static int do_getdumpdata(int fd, struct ifreq *ifr); +static int do_setdump(int fd, struct ifreq *ifr); static int send_ioctl(int fd, struct ifreq *ifr); @@ -142,6 +146,9 @@ static enum { MODE_GNTUPLE, MODE_FLASHDEV, MODE_PERMADDR, + MODE_SET_DUMP, + MODE_GET_DUMP, + MODE_GET_DUMP_DATA, } mode = MODE_GSET; static struct option { @@ -263,6 +270,12 @@ static struct option { "Get Rx ntuple filters and actions\n" }, { "-P", "--show-permaddr", MODE_PERMADDR, "Show permanent hardware address" }, + { "-W", "--get-dump", MODE_GET_DUMP, + "Get dump level\n" }, + { "-Wd", "--get-dump-data", MODE_GET_DUMP_DATA, + "Get dump data", "FILENAME " "Name of the dump file\n" }, + { "-w", "--set-dump", MODE_SET_DUMP, + "Set dump level", "DUMPLEVEL " "Dump level for the device\n" }, { "-h", "--help", MODE_HELP, "Show this help" }, { NULL, "--version", MODE_VERSION, "Show version number" }, {} @@ -413,6 +426,8 @@ static int flash_region = -1; static int msglvl_changed; static u32 msglvl_wanted = 0; static u32 msglvl_mask = 0; +static u32 dump_flag; +static char *dump_file = NULL; static enum { ONLINE=0, @@ -852,7 +867,10 @@ static void parse_cmdline(int argc, char **argp) (mode == MODE_GNTUPLE) || (mode == MODE_PHYS_ID) || (mode == MODE_FLASHDEV) || - (mode == MODE_PERMADDR)) { + (mode == MODE_PERMADDR) || + (mode == MODE_SET_DUMP) || + (mode == MODE_GET_DUMP_DATA) || + (mode == MODE_GET_DUMP)) { devname = argp[i]; break; } @@ -874,6 +892,12 @@ static void parse_cmdline(int argc, char **argp) flash_file = argp[i]; flash = 1; break; + } else if (mode == MODE_SET_DUMP) { + dump_flag = get_u32(argp[i], 0); + break; + } else if (mode == MODE_GET_DUMP_DATA) { + dump_file = argp[i]; + break; } /* fallthrough */ default: @@ -2042,6 +2066,12 @@ static int doit(void) return do_flash(fd, &ifr); } else if (mode == MODE_PERMADDR) { return do_permaddr(fd, &ifr); + } else if (mode == MODE_GET_DUMP_DATA) { + return do_getdumpdata(fd, &ifr); + } else if (mode == MODE_GET_DUMP) { + return do_getdump(fd, &ifr); + } else if (mode == MODE_SET_DUMP) { + return do_setdump(fd, &ifr); } return 69; @@ -2679,7 +2709,6 @@ static int do_gregs(int fd, struct ifreq *ifr) perror("Cannot get driver information"); return 72; } - regs = calloc(1, sizeof(*regs)+drvinfo.regdump_len); if (!regs) { perror("Cannot allocate memory for register dump"); @@ -3241,6 +3270,86 @@ static int do_grxntuple(int fd, struct ifreq *ifr) return 0; } +static void do_writedump(struct ethtool_dump *dump) +{ + FILE *f = fopen(dump_file, "wb+"); + size_t bytes; + + if (!f ) { + fprintf(stderr, "Can't open file %s: %s\n", + dump_file, strerror(errno)); + return; + } + + bytes = fwrite(dump->data, 1, dump->len, f); + fclose(f); +} + +static int do_getdump(int fd, struct ifreq *ifr) +{ + struct ethtool_dump ddata; + int err; + + ddata.cmd = ETHTOOL_GET_DUMP; + ifr->ifr_data = (caddr_t) &ddata; + err = send_ioctl(fd, ifr); + if (err < 0) { + perror("Can not get dump flag"); + return 74; + } + fprintf(stdout, "Dump flag: 0x%x\n", ddata.flag); + return 0; +} + +static int do_getdumpdata(int fd, struct ifreq *ifr) +{ + int err; + struct ethtool_dump edata; + struct ethtool_dump *data; + + edata.cmd = ETHTOOL_GET_DUMP; + + ifr->ifr_data = (caddr_t) &edata; + err = send_ioctl(fd, ifr); + if (err < 0) { + perror("Can not get dump level"); + return 74; + } + data = calloc(1, offsetof(struct ethtool_dump, data)+edata.len); + if (!data) { + perror("Can not allocate enough memory"); + return 0; + } + data->cmd = ETHTOOL_GET_DUMP_DATA; + data->len = edata.len; + ifr->ifr_data = (caddr_t) data; + err = send_ioctl(fd, ifr); + if (err < 0) { + perror("Can not get dump data\n"); + goto free; + } + do_writedump(data); +free: + free(data); + return 0; +} + +static int do_setdump(int fd, struct ifreq *ifr) +{ + int err; + struct ethtool_dump dump; + + dump.cmd = ETHTOOL_SET_DUMP; + dump.flag = dump_flag; + ifr->ifr_data = (caddr_t)&dump; + err = send_ioctl(fd, ifr); + if (err < 0) { + perror("Can not set dump level"); + return 74; + } + return 0; +} + static int send_ioctl(int fd, struct ifreq *ifr) { return ioctl(fd, SIOCETHTOOL, ifr);