[ethtool PATCH] FW dump support

[ethtool PATCH] FW dump support

[anirban.chakraborty]

From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>

Added support to take FW dump via ethtool.

Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
---
 ethtool-copy.h |   17 ++++++++-
 ethtool.c      |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 127 insertions(+), 3 deletions(-)

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);
-- 
1.7.4.1

[net-next-2.6 PATCH] ethtool: Support to take FW dump

[anirban.chakraborty]

From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>

Added code to take FW dump via ethtool. A pair of set and get functions are
added to configure dump level and fetch it from the driver respectively. A
third function is added to retrieve the dumped FW data from the driver.

Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
---
 include/linux/ethtool.h |   20 ++++++++++++
 net/core/ethtool.c      |   76 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9de3127..3dd91a5 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -595,6 +595,19 @@ struct ethtool_flash {
 	char	data[ETHTOOL_FLASH_MAX_FILENAME];
 };
 
+/**
+ * struct ethtool_dump - used for retrieving, setting device dump
+ * @flag: flag for dump setting
+ * @len: length of dump data
+ * @data: data collected for this command
+ */
+struct ethtool_dump {
+	__u32	cmd;
+	__u32	flag;
+	__u32	len;
+	u8	data[0];
+};
+
 /* for returning and changing feature sets */
 
 /**
@@ -926,6 +939,10 @@ struct ethtool_ops {
 				  const struct ethtool_rxfh_indir *);
 	void	(*get_channels)(struct net_device *, struct ethtool_channels *);
 	int	(*set_channels)(struct net_device *, struct ethtool_channels *);
+	int	(*set_dump)(struct net_device *, struct ethtool_dump *);
+	int	(*get_dump)(struct net_device *, struct ethtool_dump *);
+	int	(*get_dump_data)(struct net_device *,
+				struct ethtool_dump *, void *);
 
 };
 #endif /* __KERNEL__ */
@@ -997,6 +1014,9 @@ struct ethtool_ops {
 #define ETHTOOL_SFEATURES	0x0000003b /* Change device offload settings */
 #define ETHTOOL_GCHANNELS	0x0000003c /* Get no of channels */
 #define ETHTOOL_SCHANNELS	0x0000003d /* Set no of channels */
+#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/net/core/ethtool.c b/net/core/ethtool.c
index d8b1a8d..dce547c 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1827,6 +1827,73 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
 	return dev->ethtool_ops->flash_device(dev, &efl);
 }
 
+static int ethtool_set_dump(struct net_device *dev,
+			void __user *useraddr)
+{
+	struct ethtool_dump dump;
+
+	if (!dev->ethtool_ops->set_dump)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&dump, useraddr, sizeof(dump)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->set_dump(dev, &dump);
+}
+
+static int ethtool_get_dump(struct net_device *dev,
+			void __user *useraddr)
+{
+	struct ethtool_dump dump;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!dev->ethtool_ops->get_dump)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&dump, useraddr, sizeof(dump)))
+		return -EFAULT;
+
+	if (ops->get_dump(dev, &dump))
+		return -EFAULT;
+
+	if (copy_to_user(useraddr, &dump, sizeof(dump)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_get_dump_data(struct net_device *dev,
+				void __user *useraddr)
+{
+	int ret;
+	void *data;
+	struct ethtool_dump dump;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!dev->ethtool_ops->get_dump_data)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&dump, useraddr, sizeof(dump)))
+		return -EFAULT;
+	data = vzalloc(dump.len);
+	if (!data)
+		return -ENOMEM;
+	ret = ops->get_dump_data(dev, &dump, data);
+	if (ret) {
+		ret = -EFAULT;
+		goto out;
+	}
+	if (copy_to_user(useraddr, &dump, sizeof(dump))) {
+		ret = -EFAULT;
+		goto out;
+	}
+	useraddr += offsetof(struct ethtool_dump, data);
+	if (copy_to_user(useraddr, data, dump.len))
+		ret = -EFAULT;
+out:
+	vfree(data);
+	return ret;
+}
+
 /* The main entry point in this file.  Called from net/core/dev.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -2043,6 +2110,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SCHANNELS:
 		rc = ethtool_set_channels(dev, useraddr);
 		break;
+	case ETHTOOL_SET_DUMP:
+		rc = ethtool_set_dump(dev, useraddr);
+		break;
+	case ETHTOOL_GET_DUMP:
+		rc = ethtool_get_dump(dev, useraddr);
+		break;
+	case ETHTOOL_GET_DUMP_DATA:
+		rc = ethtool_get_dump_data(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
1.7.4.1

Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump

[Ben Hutchings]

On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
> 
> Added code to take FW dump via ethtool. A pair of set and get functions are
> added to configure dump level and fetch it from the driver respectively. A
> third function is added to retrieve the dumped FW data from the driver.
> 
> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
> ---
>  include/linux/ethtool.h |   20 ++++++++++++
>  net/core/ethtool.c      |   76 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 96 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 9de3127..3dd91a5 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -595,6 +595,19 @@ struct ethtool_flash {
>  	char	data[ETHTOOL_FLASH_MAX_FILENAME];
>  };
>  
> +/**
> + * struct ethtool_dump - used for retrieving, setting device dump
> + * @flag: flag for dump setting
> + * @len: length of dump data
> + * @data: data collected for this command
> + */
> +struct ethtool_dump {
> +	__u32	cmd;
> +	__u32	flag;
> +	__u32	len;
> +	u8	data[0];
> +};
> +

What are the legal values of flags?  Are they expected to be entirely
driver-dependent?

Shouldn't there be a version field, as for register dumps?

>  /* for returning and changing feature sets */
>  
>  /**
> @@ -926,6 +939,10 @@ struct ethtool_ops {
>  				  const struct ethtool_rxfh_indir *);
>  	void	(*get_channels)(struct net_device *, struct ethtool_channels *);
>  	int	(*set_channels)(struct net_device *, struct ethtool_channels *);
> +	int	(*set_dump)(struct net_device *, struct ethtool_dump *);
> +	int	(*get_dump)(struct net_device *, struct ethtool_dump *);
> +	int	(*get_dump_data)(struct net_device *,
> +				struct ethtool_dump *, void *);

These need to be properly documented in the header comment.

>  };
>  #endif /* __KERNEL__ */
> @@ -997,6 +1014,9 @@ struct ethtool_ops {
>  #define ETHTOOL_SFEATURES	0x0000003b /* Change device offload settings */
>  #define ETHTOOL_GCHANNELS	0x0000003c /* Get no of channels */
>  #define ETHTOOL_SCHANNELS	0x0000003d /* Set no of channels */
> +#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/net/core/ethtool.c b/net/core/ethtool.c
> index d8b1a8d..dce547c 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1827,6 +1827,73 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
>  	return dev->ethtool_ops->flash_device(dev, &efl);
>  }
>  
> +static int ethtool_set_dump(struct net_device *dev,
> +			void __user *useraddr)
> +{
> +	struct ethtool_dump dump;
> +
> +	if (!dev->ethtool_ops->set_dump)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&dump, useraddr, sizeof(dump)))
> +		return -EFAULT;
> +
> +	return dev->ethtool_ops->set_dump(dev, &dump);
> +}
> +
> +static int ethtool_get_dump(struct net_device *dev,
> +			void __user *useraddr)
> +{
> +	struct ethtool_dump dump;
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +
> +	if (!dev->ethtool_ops->get_dump)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&dump, useraddr, sizeof(dump)))
> +		return -EFAULT;
> +
> +	if (ops->get_dump(dev, &dump))
> +		return -EFAULT;

Why does this change the error code?

> +	if (copy_to_user(useraddr, &dump, sizeof(dump)))
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int ethtool_get_dump_data(struct net_device *dev,
> +				void __user *useraddr)
> +{
> +	int ret;
> +	void *data;
> +	struct ethtool_dump dump;
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +
> +	if (!dev->ethtool_ops->get_dump_data)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&dump, useraddr, sizeof(dump)))
> +		return -EFAULT;
> +	data = vzalloc(dump.len);

I think we ought to query the driver to find out the maximum dump
length, rather than using the length passed by the user (up to 4GB).  We
already check that the user has the CAP_NET_ADMIN capability but that
should not mean the ability to evade memory limits.

> +	if (!data)
> +		return -ENOMEM;
> +	ret = ops->get_dump_data(dev, &dump, data);
> +	if (ret) {
> +		ret = -EFAULT;

Why does this change the error code?

> +		goto out;
> +	}
> +	if (copy_to_user(useraddr, &dump, sizeof(dump))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +	useraddr += offsetof(struct ethtool_dump, data);
> +	if (copy_to_user(useraddr, data, dump.len))
> +		ret = -EFAULT;
> +out:
> +	vfree(data);
> +	return ret;
> +}
> +
>  /* The main entry point in this file.  Called from net/core/dev.c */
>  
>  int dev_ethtool(struct net *net, struct ifreq *ifr)
> @@ -2043,6 +2110,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>  	case ETHTOOL_SCHANNELS:
>  		rc = ethtool_set_channels(dev, useraddr);
>  		break;
> +	case ETHTOOL_SET_DUMP:
> +		rc = ethtool_set_dump(dev, useraddr);
> +		break;
> +	case ETHTOOL_GET_DUMP:
> +		rc = ethtool_get_dump(dev, useraddr);
> +		break;
> +	case ETHTOOL_GET_DUMP_DATA:
> +		rc = ethtool_get_dump_data(dev, useraddr);
> +		break;
>  	default:
>  		rc = -EOPNOTSUPP;
>  	}

-- 
Ben Hutchings, Senior Software 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.

Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump

[Anirban Chakraborty]

On May 3, 2011, at 4:53 PM, 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 code to take FW dump via ethtool. A pair of set and get functions are
>> added to configure dump level and fetch it from the driver respectively. A
>> third function is added to retrieve the dumped FW data from the driver.
>> 
>> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>> ---
>> include/linux/ethtool.h |   20 ++++++++++++
>> net/core/ethtool.c      |   76 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 96 insertions(+), 0 deletions(-)
>> 
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index 9de3127..3dd91a5 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -595,6 +595,19 @@ struct ethtool_flash {
>> 	char	data[ETHTOOL_FLASH_MAX_FILENAME];
>> };
>> 
>> +/**
>> + * struct ethtool_dump - used for retrieving, setting device dump
>> + * @flag: flag for dump setting
>> + * @len: length of dump data
>> + * @data: data collected for this command
>> + */
>> +struct ethtool_dump {
>> +	__u32	cmd;
>> +	__u32	flag;
>> +	__u32	len;
>> +	u8	data[0];
>> +};
>> +
> 
> What are the legal values of flags?  Are they expected to be entirely
> driver-dependent?
Yes, the flag could be completely driver dependent. For example, in the qlcnic
driver this would indicate the level of the dump that the driver should take. Also,
by specifying a magic key via the flag, the driver could be instructed to take a FW dump
forcibly.

> 
> Shouldn't there be a version field, as for register dumps?
I was thinking that the dump data itself could contain the version field specifying the FW version. However,
we can have an additional field if that turns out to be useful. 

> 
>> /* for returning and changing feature sets */
>> 
>> /**
>> @@ -926,6 +939,10 @@ struct ethtool_ops {
>> 				  const struct ethtool_rxfh_indir *);
>> 	void	(*get_channels)(struct net_device *, struct ethtool_channels *);
>> 	int	(*set_channels)(struct net_device *, struct ethtool_channels *);
>> +	int	(*set_dump)(struct net_device *, struct ethtool_dump *);
>> +	int	(*get_dump)(struct net_device *, struct ethtool_dump *);
>> +	int	(*get_dump_data)(struct net_device *,
>> +				struct ethtool_dump *, void *);
> 
> These need to be properly documented in the header comment.
WIll do.

> 
>> };
>> #endif /* __KERNEL__ */
>> @@ -997,6 +1014,9 @@ struct ethtool_ops {
>> #define ETHTOOL_SFEATURES	0x0000003b /* Change device offload settings */
>> #define ETHTOOL_GCHANNELS	0x0000003c /* Get no of channels */
>> #define ETHTOOL_SCHANNELS	0x0000003d /* Set no of channels */
>> +#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/net/core/ethtool.c b/net/core/ethtool.c
>> index d8b1a8d..dce547c 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -1827,6 +1827,73 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
>> 	return dev->ethtool_ops->flash_device(dev, &efl);
>> }
>> 
>> +static int ethtool_set_dump(struct net_device *dev,
>> +			void __user *useraddr)
>> +{
>> +	struct ethtool_dump dump;
>> +
>> +	if (!dev->ethtool_ops->set_dump)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (copy_from_user(&dump, useraddr, sizeof(dump)))
>> +		return -EFAULT;
>> +
>> +	return dev->ethtool_ops->set_dump(dev, &dump);
>> +}
>> +
>> +static int ethtool_get_dump(struct net_device *dev,
>> +			void __user *useraddr)
>> +{
>> +	struct ethtool_dump dump;
>> +	const struct ethtool_ops *ops = dev->ethtool_ops;
>> +
>> +	if (!dev->ethtool_ops->get_dump)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (copy_from_user(&dump, useraddr, sizeof(dump)))
>> +		return -EFAULT;
>> +
>> +	if (ops->get_dump(dev, &dump))
>> +		return -EFAULT;
> 
> Why does this change the error code?
It should not. Will fix.

> 
>> +	if (copy_to_user(useraddr, &dump, sizeof(dump)))
>> +		return -EFAULT;
>> +	return 0;
>> +}
>> +
>> +static int ethtool_get_dump_data(struct net_device *dev,
>> +				void __user *useraddr)
>> +{
>> +	int ret;
>> +	void *data;
>> +	struct ethtool_dump dump;
>> +	const struct ethtool_ops *ops = dev->ethtool_ops;
>> +
>> +	if (!dev->ethtool_ops->get_dump_data)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (copy_from_user(&dump, useraddr, sizeof(dump)))
>> +		return -EFAULT;
>> +	data = vzalloc(dump.len);
> 
> I think we ought to query the driver to find out the maximum dump
> length, rather than using the length passed by the user (up to 4GB).  We
> already check that the user has the CAP_NET_ADMIN capability but that
> should not mean the ability to evade memory limits.
That's right. The user makes a call to get_dump (ETHTOOL_GET_DUMP)
first to get the size of the collected dump data for the current
dump level (flag). Then it makes the call to get the dump data (ETHTOOL_GET_DUMP_DATA),
as in above where it specifies the dump size.
The patch ref-ed below demonstrated that.
http://patchwork.ozlabs.org/patch/93729/  

> 
>> +	if (!data)
>> +		return -ENOMEM;
>> +	ret = ops->get_dump_data(dev, &dump, data);
>> +	if (ret) {
>> +		ret = -EFAULT;
> 
> Why does this change the error code?
Will fix it.

> 
>> +		goto out;
>> +	}
>> +	if (copy_to_user(useraddr, &dump, sizeof(dump))) {
>> +		ret = -EFAULT;
>> +		goto out;
>> +	}
>> +	useraddr += offsetof(struct ethtool_dump, data);
>> +	if (copy_to_user(useraddr, data, dump.len))
>> +		ret = -EFAULT;
>> +out:
>> +	vfree(data);
>> +	return ret;
>> +}
>> +
>> /* The main entry point in this file.  Called from net/core/dev.c */
>> 
>> int dev_ethtool(struct net *net, struct ifreq *ifr)
>> @@ -2043,6 +2110,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>> 	case ETHTOOL_SCHANNELS:
>> 		rc = ethtool_set_channels(dev, useraddr);
>> 		break;
>> +	case ETHTOOL_SET_DUMP:
>> +		rc = ethtool_set_dump(dev, useraddr);
>> +		break;
>> +	case ETHTOOL_GET_DUMP:
>> +		rc = ethtool_get_dump(dev, useraddr);
>> +		break;
>> +	case ETHTOOL_GET_DUMP_DATA:
>> +		rc = ethtool_get_dump_data(dev, useraddr);
>> +		break;
>> 	default:
>> 		rc = -EOPNOTSUPP;
>> 	}
> 

Thanks for your feedback. I'll respin the patch incorporating your comments and submit it again.

-Anirban

Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump

[Ben Hutchings]

On Tue, 2011-05-03 at 17:29 -0700, Anirban Chakraborty wrote:
> On May 3, 2011, at 4:53 PM, Ben Hutchings wrote:
> 
> > On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
[...]
> >> +static int ethtool_get_dump_data(struct net_device *dev,
> >> +				void __user *useraddr)
> >> +{
> >> +	int ret;
> >> +	void *data;
> >> +	struct ethtool_dump dump;
> >> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> >> +
> >> +	if (!dev->ethtool_ops->get_dump_data)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	if (copy_from_user(&dump, useraddr, sizeof(dump)))
> >> +		return -EFAULT;
> >> +	data = vzalloc(dump.len);
> > 
> > I think we ought to query the driver to find out the maximum dump
> > length, rather than using the length passed by the user (up to 4GB).  We
> > already check that the user has the CAP_NET_ADMIN capability but that
> > should not mean the ability to evade memory limits.
> That's right. The user makes a call to get_dump (ETHTOOL_GET_DUMP)
> first to get the size of the collected dump data for the current
> dump level (flag). Then it makes the call to get the dump data (ETHTOOL_GET_DUMP_DATA),
> as in above where it specifies the dump size.
> The patch ref-ed below demonstrated that.
> http://patchwork.ozlabs.org/patch/93729/  
[...]

I understand that the userland application will need to get the size
that way.  I'm saying that this code in the kernel should also get the
size from the driver, so that a malicious application cannot make the
kernel allocate an excessively large buffer.

Also, you'll need to submit your implementation along with this, as
David won't accept an extension to the API without a driver that
implements it.

Ben.

-- 
Ben Hutchings, Senior Software 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.

Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump

[Anirban Chakraborty]

On May 3, 2011, at 6:06 PM, Ben Hutchings wrote:

> <snip>
> 
> I understand that the userland application will need to get the size
> that way.  I'm saying that this code in the kernel should also get the
> size from the driver, so that a malicious application cannot make the
> kernel allocate an excessively large buffer.
Makes sense. Will do so.

> 
> Also, you'll need to submit your implementation along with this, as
> David won't accept an extension to the API without a driver that
> implements it.
I have implemented it in qlcnic driver. Will submit it along with the v2 patches.
Thanks.

-Anirban

Re: [ethtool PATCH] FW dump support

[Ben Hutchings]

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.

-- 
Ben Hutchings, Senior Software 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.

Re: [ethtool PATCH] FW dump support

[Anirban Chakraborty]

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

Re: [ethtool PATCH] FW dump support

[Ben Hutchings]

One more thing: please can you update the manual page as well as the
built-in usage information.

Ben.

-- 
Ben Hutchings, Senior Software 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.

RE: [ethtool PATCH] FW dump support

[Ajit.Khaparde]

________________________________________
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?

-Ajit

RE: [ethtool PATCH] FW dump support

[Ben Hutchings]

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.

-- 
Ben Hutchings, Senior Software 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.