[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1337005139.2550.6.camel@bwh-desktop.uk.solarflarecom.com>
Date: Mon, 14 May 2012 15:18:59 +0100
From: Ben Hutchings <bhutchings@...arflare.com>
To: Yaniv Rosner <yanivr@...adcom.com>
CC: David Miller <davem@...emloft.net>, <netdev@...r.kernel.org>,
Eilon Greenstein <eilong@...adcom.com>,
Stuart Hodgson <smhodgson@...arflare.com>
Subject: Re: [PATCH ethtool] Add command to dump module EEPROM
On Mon, 2012-05-14 at 18:13 +0300, Yaniv Rosner wrote:
> Hi Ben,
> This patch adds a new option to dump (SFP+, XFP, ...) module EEPROM following
> recent support to kernel side. Below some examples:
>
> bash-3.00# ethtool -m eth1 offset 0x14 length 32 raw on
> JDSU PLRXPLSCS432
>
> bash-3.00# ethtool -m eth1 offset 0x14 length 32
> Offset Values
> ------ ------
> 0x0014 4a 44 53 55 20 20 20 20 20 20 20 20 20 20 20 20
> 0x0024 00 00 01 9c 50 4c 52 58 50 4c 53 43 53 34 33 32
>
> Please consider applying to ethtool.
I agree there should be ASCII-hex and binary dump modes, but we should
also support decoding of recognised EEPROM types (as Stuart proposed
earlier).
> Thanks,
> Yaniv
>
> Signed-off-by: Yaniv Rosner <yanivr@...adcom.com>
> ---
> ethtool-copy.h | 312 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> ethtool.8.in | 23 ++++-
> ethtool.c | 63 +++++++++++
> 3 files changed, 393 insertions(+), 5 deletions(-)
>
> diff --git a/ethtool-copy.h b/ethtool-copy.h
> index d904c1a..604dbef 100644
> --- a/ethtool-copy.h
> +++ b/ethtool-copy.h
> @@ -13,6 +13,9 @@
> #ifndef _LINUX_ETHTOOL_H
> #define _LINUX_ETHTOOL_H
>
> +#ifdef __KERNEL__
> +#include <linux/compat.h>
> +#endif
> #include <linux/types.h>
> #include <linux/if_ether.h>
>
[...]
You've updated this wrongly; run 'make headers_install' in the kernel
tree and then copy from usr/include/ethtool.h.
[...]
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 63d5d48..470fd8d 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -325,6 +325,13 @@ ethtool \- query or control network driver and hardware settings
> .I devname flag
> .A1 on off
> .RB ...
> +.HP
> +.B ethtool \-m|\-\-mod\-eeprom\-dump
> +.I devname
> +.B2 raw on off
> +.BN offset
> +.BN length
> +.HP
> .
> .\" Adjust lines (i.e. full justification) and hyphenate.
> .ad
> @@ -800,6 +807,19 @@ Sets the device's private flags as specified.
> .I flag
> .A1 on off
> Sets the state of the named private flag.
> +.TP
> +.B \-m \-\-mod\-eeprom\-dump
> +Retrieves and prints module (SFP+, XFP, ...) EEPROMs dump for the specified network device.
> +Default is to dump the entire EEPROM.
> +.TP
> +.BI raw \ on|off
> +Dumps the raw EEPROM data to stdout.
Only true for 'raw on', not 'raw off'!
> +.TP
> +.BI offset \ N
> +Start address of module EEPROM dump.
> +.TP
> +.BI length \ N
> +Length of module EEPROM dump.
> .SH BUGS
> Not supported (in part or whole) on all network drivers.
> .SH AUTHOR
> @@ -815,7 +835,8 @@ Eli Kupermann,
> Scott Feldman,
> Andi Kleen,
> Alexander Duyck,
> -Sucheta Chakraborty.
> +Sucheta Chakraborty,
> +Yaniv Rosner.
> .SH AVAILABILITY
> .B ethtool
> is available from
> diff --git a/ethtool.c b/ethtool.c
> index e80b38b..6d022c3 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -2214,6 +2214,64 @@ static int do_nway_rst(struct cmd_context *ctx)
> return err;
> }
>
> +static int do_gmoduleeeprom(struct cmd_context *ctx)
> +{
> + int geeprom_changed = 0;
> + int geeprom_dump_raw = 0;
> + u32 geeprom_offset = 0;
> + u32 geeprom_length = -1;
> + struct cmdline_info cmdline_geeprom[] = {
> + { "offset", CMDL_U32, &geeprom_offset, NULL },
> + { "length", CMDL_U32, &geeprom_length, NULL },
> + { "raw", CMDL_BOOL, &geeprom_dump_raw, NULL },
> + };
> + int err;
> + struct ethtool_modinfo modinfo;
> + struct ethtool_eeprom *eeprom;
> + struct ethtool_drvinfo drvinfo;
> +
> + parse_generic_cmdline(ctx, &geeprom_changed,
> + cmdline_geeprom, ARRAY_SIZE(cmdline_geeprom));
> +
> + drvinfo.cmd = ETHTOOL_GDRVINFO;
> + err = send_ioctl(ctx, &drvinfo);
> + if (err < 0) {
> + perror("Cannot get driver information");
> + return 74;
> + }
What is the point of running ETHTOOL_GDRVINFO?
> + modinfo.cmd = ETHTOOL_GMODULEINFO;
> + err = send_ioctl(ctx, &modinfo);
> + if (err < 0) {
> + perror("Cannot get driver information");
> + return 74;
No more magic return codes, please. Just return 1 on error. Also the
error message here seems wrong.
> + }
> +
> + if (geeprom_length == -1)
> + geeprom_length = modinfo.eeprom_len;
> +
> + if (modinfo.eeprom_len < geeprom_offset + geeprom_length)
> + geeprom_length = modinfo.eeprom_len - geeprom_offset;
> + eeprom = calloc(1, sizeof(*eeprom)+geeprom_length);
> + if (!eeprom) {
> + perror("Cannot allocate memory for EEPROM data");
> + return 75;
> + }
> + eeprom->cmd = ETHTOOL_GMODULEEEPROM;
> + eeprom->len = geeprom_length;
> + eeprom->offset = geeprom_offset;
> + err = send_ioctl(ctx, eeprom);
> + if (err < 0) {
> + perror("Cannot get EEPROM data");
> + free(eeprom);
> + return 74;
> + }
> + err = dump_eeprom(geeprom_dump_raw, &drvinfo, eeprom);
> + free(eeprom);
> +
> + return err;
> +}
> +
> static int do_geeprom(struct cmd_context *ctx)
> {
> int geeprom_changed = 0;
> @@ -3231,6 +3289,11 @@ static const struct option {
> { "--show-priv-flags" , 1, do_gprivflags, "Query private flags" },
> { "--set-priv-flags", 1, do_sprivflags, "Set private flags",
> " FLAG on|off ...\n" },
> + { "-m|--mod-eeprom-dump", 1, do_gmoduleeeprom,
> + "Dumps SFP+ module EEPROM",
As you correctly noted in the manual page, this isn't limited to SFP+.
Ben.
> + " [ raw on|off ]\n"
> + " [ offset N ]\n"
> + " [ length N ]\n" },
> { "-h|--help", 0, show_usage, "Show this help" },
> { "--version", 0, do_version, "Show version number" },
> {}
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists