[PATCH] Make possible speeds known to ethtool

[PATCH] Make possible speeds known to ethtool

[Rick Jones]

Certain Broadcom 10Gb Ethernet solutions (e.g. the 57711E) can have a
10Gb port split into multiple virtual NICs each with an instance of
the bnx2x driver.  These virtual NICs can be configured for any speed
which is an integer multiple of 100 Mb/s from 100 to 10,000 Mbit/s
inclusive.  Since this is "normal" for such systems an "Unknown!" is
not indicated.

Signed-off-by: Rick Jones <rick.jones2@hp.com>

---

Rather than make a wholesale change to the existing "speed vetting"
code, it seemed least invasive to escape-out to a new routine in the
default case.  Should other "unfamiliar" speeds surface in the future
the checks can be put there.

Where the original would say this:

	Speed: Unknown! (1600)

for a FlexNIC link set to 1600 Mbit/s the changed version will
say:

	Speed: 1600Mb/s

There is presently no way to alter this speed setting from within the
host, so there is no need/point to altering the ethtool "set" path.

--- ethtool.c.orig	2008-11-17 11:53:40.000000000 -0800
+++ ethtool.c	2008-11-17 13:14:55.000000000 -0800
@@ -806,6 +806,14 @@ static void dump_advertised(struct ethto
 		fprintf(stdout, "No\n");
 }
 
+static void vet_unfamiliar_speed(struct ethtool_cmd *ep)
+{
+	if ((!ep->speed) || (ep->speed % 100))
+		fprintf(stdout, "Unknown! (%i)\n", ep->speed);
+	else
+		fprintf(stdout,"%dMb/s\n",ep->speed); 
+}
+
 static int dump_ecmd(struct ethtool_cmd *ep)
 {
 	dump_supported(ep);
@@ -829,7 +837,7 @@ static int dump_ecmd(struct ethtool_cmd 
 		fprintf(stdout, "10000Mb/s\n");
 		break;
 	default:
-		fprintf(stdout, "Unknown! (%i)\n", ep->speed);
+		vet_unfamiliar_speed(ep);
 		break;
 	};
 

Re: [PATCH] Make possible speeds known to ethtool

[Ben Hutchings]

On Wed, 2009-01-07 at 18:03 -0800, Rick Jones wrote:
> Certain Broadcom 10Gb Ethernet solutions (e.g. the 57711E) can have a
> 10Gb port split into multiple virtual NICs each with an instance of
> the bnx2x driver.  These virtual NICs can be configured for any speed
> which is an integer multiple of 100 Mb/s from 100 to 10,000 Mbit/s
> inclusive.  Since this is "normal" for such systems an "Unknown!" is
> not indicated.
[...]

The vetting of speeds is kind of silly.  Given that speed is established
as being a number of Mbit/s (hence the need for speed_hi), why not
remove the warning and the checks for known values and report it as
such?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] Make possible speeds known to ethtool

[Jeff Garzik]

Ben Hutchings wrote:
> On Wed, 2009-01-07 at 18:03 -0800, Rick Jones wrote:
>> Certain Broadcom 10Gb Ethernet solutions (e.g. the 57711E) can have a
>> 10Gb port split into multiple virtual NICs each with an instance of
>> the bnx2x driver.  These virtual NICs can be configured for any speed
>> which is an integer multiple of 100 Mb/s from 100 to 10,000 Mbit/s
>> inclusive.  Since this is "normal" for such systems an "Unknown!" is
>> not indicated.
> [...]
> 
> The vetting of speeds is kind of silly.  Given that speed is established
> as being a number of Mbit/s (hence the need for speed_hi), why not
> remove the warning and the checks for known values and report it as
> such?

I'm ok with that route.  Historically it made sense, but AFAICS the 
driver _must_ verify the speed anyway, so removing the limitation in the 
userspace tool seems reasonable.

The next release of ethtool is coming in about 4 weeks, and we can 
definitely get something like this in there.

	Jeff

Re: [PATCH] Make possible speeds known to ethtool

[Rick Jones]

Jeff Garzik wrote:
> Ben Hutchings wrote:
>> The vetting of speeds is kind of silly.  Given that speed is established
>> as being a number of Mbit/s (hence the need for speed_hi), why not
>> remove the warning and the checks for known values and report it as
>> such?
> 
> 
> I'm ok with that route.  Historically it made sense, but AFAICS the 
> driver _must_ verify the speed anyway, so removing the limitation in the 
> userspace tool seems reasonable.
> 
> The next release of ethtool is coming in about 4 weeks, and we can 
> definitely get something like this in there.

I have a simple patch which does just that ready to post, but will 
point-out that removing the checks entirely will result in the speed 
being reported as 65535 (without Unknown) for an interface with its 
cable disconnected.  This however is is based only on "testing" on a 
2.6.24-22-generic (hardy) kernel with 7.3.20-k2-NAPI of the e1000 driver 
driving an Intel 82566MM (rev 03).

rick jones

Re: [PATCH] Make possible speeds known to ethtool

[Ben Hutchings]

On Thu, 2009-01-08 at 11:11 -0800, Rick Jones wrote:
> Jeff Garzik wrote:
> > Ben Hutchings wrote:
> >> The vetting of speeds is kind of silly.  Given that speed is established
> >> as being a number of Mbit/s (hence the need for speed_hi), why not
> >> remove the warning and the checks for known values and report it as
> >> such?
> > 
> > 
> > I'm ok with that route.  Historically it made sense, but AFAICS the 
> > driver _must_ verify the speed anyway, so removing the limitation in the 
> > userspace tool seems reasonable.
> > 
> > The next release of ethtool is coming in about 4 weeks, and we can 
> > definitely get something like this in there.
> 
> I have a simple patch which does just that ready to post, but will 
> point-out that removing the checks entirely will result in the speed 
> being reported as 65535 (without Unknown) for an interface with its 
> cable disconnected.  This however is is based only on "testing" on a 
> 2.6.24-22-generic (hardy) kernel with 7.3.20-k2-NAPI of the e1000 driver 
> driving an Intel 82566MM (rev 03).

I think 0, (u32)(-1) and (u16)(-1) may have to be special-cased as
unknown, but everything else can be treated as a number of Mbit/s.  I
don't know what a driver should do about an interface that really runs
at 65.535 Gbit/s though...

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] Make possible speeds known to ethtool

[Rick Jones]

> 
> I think 0, (u32)(-1) and (u16)(-1) may have to be special-cased as
> unknown, but everything else can be treated as a number of Mbit/s.  I
> don't know what a driver should do about an interface that really runs
> at 65.535 Gbit/s though...

Something along these lines then? (assuming my mailer doesn't fubar this
:( - I normally send matches via mailx)

--- ethtool.c.orig      2008-11-17 11:53:40.000000000 -0800
+++ ethtool.c   2009-01-08 11:41:54.000000000 -0800
@@ -813,23 +813,12 @@ static int dump_ecmd(struct ethtool_cmd

         fprintf(stdout, "       Speed: ");
         switch (ep->speed) {
-       case SPEED_10:
-               fprintf(stdout, "10Mb/s\n");
-               break;
-       case SPEED_100:
-               fprintf(stdout, "100Mb/s\n");
-               break;
-       case SPEED_1000:
-               fprintf(stdout, "1000Mb/s\n");
-               break;
-       case SPEED_2500:
-               fprintf(stdout, "2500Mb/s\n");
-               break;
-       case SPEED_10000:
-               fprintf(stdout, "10000Mb/s\n");
+       case 0:
+       case (u16)(-1):
+               fprintf(stdout, "Unknown! (%i)\n", ep->speed);
                 break;
         default:
-               fprintf(stdout, "Unknown! (%i)\n", ep->speed);
+               fprintf(stdout, "%dMb/s\n", ep->speed);
                 break;
         };

If that looks reasonable I'll post a proper one with the apropriate text and such with mailx...

rick jones

[PATCH] ethtool: Support arbitrary speeds

[Ben Hutchings]

The speed and speed_hi fields of struct ethtool_cmd together represent
a value in units of Mbit/s.  The valid speed settings are hardware-
dependent and should be checked by the driver.  Remove our validation
and allow arbitrary positive values.  Continue to report 0 and -1 as
"Unknown!" since some drivers will report these invalid values when
the link is down.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
On Thu, 2009-01-08 at 11:50 -0800, Rick Jones wrote:
> > 
> > I think 0, (u32)(-1) and (u16)(-1) may have to be special-cased as
> > unknown, but everything else can be treated as a number of Mbit/s.  I
> > don't know what a driver should do about an interface that really runs
> > at 65.535 Gbit/s though...
> 
> Something along these lines then? (assuming my mailer doesn't fubar this
> :( - I normally send matches via mailx)

That's kind of incomplete.  Here's my attempt.

In a quick test I found that the tg3 driver *doesn't* validate the speed
setting if autonegotiation is off, and will accept and report back e.g.
99.  But this patch doesn't create a new problem as you could already
set it to the unsupported speeds of 2500 and 10000.

Ben.

 ethtool.8 |    4 ++--
 ethtool.c |   42 ++++++++++--------------------------------
 2 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/ethtool.8 b/ethtool.8
index 1beb387..1504a23 100644
--- a/ethtool.8
+++ b/ethtool.8
@@ -183,7 +183,7 @@ ethtool \- Display or change ethernet card settings
 
 .B ethtool \-s
 .I ethX
-.B4 speed 10 100 1000 2500 10000
+.BI speed \ N
 .B2 duplex half full
 .B4 port tp aui bnc mii fibre
 .B2 autoneg on off
@@ -343,7 +343,7 @@ All following options only apply if
 .B \-s
 was specified.
 .TP
-.A4 speed 10 100 1000 2500 10000
+.BI speed \ N
 Set speed in Mb/s.
 .B ethtool
 with just the device name as an argument will show you the supported device speeds.
diff --git a/ethtool.c b/ethtool.c
index a7c02d0..bcc865e 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -107,7 +107,7 @@ static struct option {
     char *opthelp;
 } args[] = {
     { "-s", "--change", MODE_SSET, "Change generic options",
-		"		[ speed 10|100|1000|2500|10000 ]\n"
+		"		[ speed %%d ]\n"
 		"		[ duplex half|full ]\n"
 		"		[ port tp|aui|bnc|mii|fibre ]\n"
 		"		[ autoneg on|off ]\n"
@@ -608,17 +608,8 @@ static void parse_cmdline(int argc, char **argp)
 				i += 1;
 				if (i >= argc)
 					show_usage(1);
-				if (!strcmp(argp[i], "10"))
-					speed_wanted = SPEED_10;
-				else if (!strcmp(argp[i], "100"))
-					speed_wanted = SPEED_100;
-				else if (!strcmp(argp[i], "1000"))
-					speed_wanted = SPEED_1000;
-				else if (!strcmp(argp[i], "2500"))
-					speed_wanted = SPEED_2500;
-				else if (!strcmp(argp[i], "10000"))
-					speed_wanted = SPEED_10000;
-				else
+				speed_wanted = strtol(argp[i], NULL, 10);
+				if (speed_wanted <= 0)
 					show_usage(1);
 				break;
 			} else if (!strcmp(argp[i], "duplex")) {
@@ -893,30 +884,17 @@ static void dump_advertised(struct ethtool_cmd *ep)
 
 static int dump_ecmd(struct ethtool_cmd *ep)
 {
+	u32 speed;
+
 	dump_supported(ep);
 	dump_advertised(ep);
 
 	fprintf(stdout, "	Speed: ");
-	switch (ethtool_cmd_speed(ep)) {
-	case SPEED_10:
-		fprintf(stdout, "10Mb/s\n");
-		break;
-	case SPEED_100:
-		fprintf(stdout, "100Mb/s\n");
-		break;
-	case SPEED_1000:
-		fprintf(stdout, "1000Mb/s\n");
-		break;
-	case SPEED_2500:
-		fprintf(stdout, "2500Mb/s\n");
-		break;
-	case SPEED_10000:
-		fprintf(stdout, "10000Mb/s\n");
-		break;
-	default:
-		fprintf(stdout, "Unknown! (%i)\n", ethtool_cmd_speed(ep));
-		break;
-	};
+	speed = ethtool_cmd_speed(ep);
+	if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
+		fprintf(stdout, "Unknown!\n");
+	else
+		fprintf(stdout, "%uMb/s\n", speed);
 
 	fprintf(stdout, "	Duplex: ");
 	switch (ep->duplex) {

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] ethtool: Support arbitrary speeds

[Rick Jones]

Ben Hutchings wrote:
> The speed and speed_hi fields of struct ethtool_cmd together represent
> a value in units of Mbit/s.  The valid speed settings are hardware-
> dependent and should be checked by the driver.  Remove our validation
> and allow arbitrary positive values.  Continue to report 0 and -1 as
> "Unknown!" since some drivers will report these invalid values when
> the link is down.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> On Thu, 2009-01-08 at 11:50 -0800, Rick Jones wrote:
> 
>>>I think 0, (u32)(-1) and (u16)(-1) may have to be special-cased as
>>>unknown, but everything else can be treated as a number of Mbit/s.  I
>>>don't know what a driver should do about an interface that really runs
>>>at 65.535 Gbit/s though...
>>
>>Something along these lines then? (assuming my mailer doesn't fubar this
>>:( - I normally send matches via mailx)
> 
> 
> That's kind of incomplete.  Here's my attempt.
> 
> In a quick test I found that the tg3 driver *doesn't* validate the speed
> setting if autonegotiation is off, and will accept and report back e.g.
> 99.  But this patch doesn't create a new problem as you could already
> set it to the unsupported speeds of 2500 and 10000.

I'm fine with yanking the vetting on set - didn't do it initially 
because what got me patching in the first place does the setting of the 
speeds "elsewhere" so set support wasn't an issue.

WRT the get part:

> @@ -893,30 +884,17 @@ static void dump_advertised(struct ethtool_cmd *ep)
>  
>  static int dump_ecmd(struct ethtool_cmd *ep)
>  {
> +	u32 speed;
> +
>  	dump_supported(ep);
>  	dump_advertised(ep);
>  
>  	fprintf(stdout, "	Speed: ");
> -	switch (ethtool_cmd_speed(ep)) {
> -	case SPEED_10:
> -		fprintf(stdout, "10Mb/s\n");
> -		break;
> -	case SPEED_100:
> -		fprintf(stdout, "100Mb/s\n");
> -		break;
> -	case SPEED_1000:
> -		fprintf(stdout, "1000Mb/s\n");
> -		break;
> -	case SPEED_2500:
> -		fprintf(stdout, "2500Mb/s\n");
> -		break;
> -	case SPEED_10000:
> -		fprintf(stdout, "10000Mb/s\n");
> -		break;
> -	default:
> -		fprintf(stdout, "Unknown! (%i)\n", ethtool_cmd_speed(ep));
> -		break;
> -	};
> +	speed = ethtool_cmd_speed(ep);
> +	if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
> +		fprintf(stdout, "Unknown!\n");

Doesn't that need to keep the reporting of the unknown speed in parens 
like the original?

rick jones

Re: [PATCH] ethtool: Support arbitrary speeds

[Ben Hutchings]

On Fri, 2009-01-09 at 09:55 -0800, Rick Jones wrote:
> Ben Hutchings wrote:
[...]
> > +	speed = ethtool_cmd_speed(ep);
> > +	if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
> > +		fprintf(stdout, "Unknown!\n");
> 
> Doesn't that need to keep the reporting of the unknown speed in parens 
> like the original?

We'll only be doing it for known invalid values, so the value isn't
useful information.  I suppose it's conceivable that there are tools out
there that are parsing the output and expect "Unknown! (0)" when link is
down.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] ethtool: Support arbitrary speeds

[Rick Jones]

>>Doesn't that need to keep the reporting of the unknown speed in parens 
>>like the original?
> 
> 
> We'll only be doing it for known invalid values, so the value isn't
> useful information.  I suppose it's conceivable that there are tools out
> there that are parsing the output and expect "Unknown! (0)" when link is
> down.

That was my paranoid assumption :) (albeit for the 65535 value :)

rick

Re: [PATCH] ethtool: Support arbitrary speeds

[Jeff Garzik]

Ben Hutchings wrote:
> The speed and speed_hi fields of struct ethtool_cmd together represent
> a value in units of Mbit/s.  The valid speed settings are hardware-
> dependent and should be checked by the driver.  Remove our validation
> and allow arbitrary positive values.  Continue to report 0 and -1 as
> "Unknown!" since some drivers will report these invalid values when
> the link is down.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> On Thu, 2009-01-08 at 11:50 -0800, Rick Jones wrote:
>>> I think 0, (u32)(-1) and (u16)(-1) may have to be special-cased as
>>> unknown, but everything else can be treated as a number of Mbit/s.  I
>>> don't know what a driver should do about an interface that really runs
>>> at 65.535 Gbit/s though...
>> Something along these lines then? (assuming my mailer doesn't fubar this
>> :( - I normally send matches via mailx)
> 
> That's kind of incomplete.  Here's my attempt.
> 
> In a quick test I found that the tg3 driver *doesn't* validate the speed
> setting if autonegotiation is off, and will accept and report back e.g.
> 99.  But this patch doesn't create a new problem as you could already
> set it to the unsupported speeds of 2500 and 10000.
> 
> Ben.
> 
>  ethtool.8 |    4 ++--
>  ethtool.c |   42 ++++++++++--------------------------------
>  2 files changed, 12 insertions(+), 34 deletions(-)

ACK, can you rediff against current git repo?

Re: [PATCH] ethtool: Support arbitrary speeds

[Jeff Garzik]

Ben Hutchings wrote:
> The speed and speed_hi fields of struct ethtool_cmd together represent
> a value in units of Mbit/s.  The valid speed settings are hardware-
> dependent and should be checked by the driver.  Remove our validation
> and allow arbitrary positive values.  Continue to report 0 and -1 as
> "Unknown!" since some drivers will report these invalid values when
> the link is down.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> On Thu, 2009-01-08 at 11:50 -0800, Rick Jones wrote:
>>> I think 0, (u32)(-1) and (u16)(-1) may have to be special-cased as
>>> unknown, but everything else can be treated as a number of Mbit/s.  I
>>> don't know what a driver should do about an interface that really runs
>>> at 65.535 Gbit/s though...
>> Something along these lines then? (assuming my mailer doesn't fubar this
>> :( - I normally send matches via mailx)
> 
> That's kind of incomplete.  Here's my attempt.
> 
> In a quick test I found that the tg3 driver *doesn't* validate the speed
> setting if autonegotiation is off, and will accept and report back e.g.
> 99.  But this patch doesn't create a new problem as you could already
> set it to the unsupported speeds of 2500 and 10000.
> 
> Ben.
> 
>  ethtool.8 |    4 ++--
>  ethtool.c |   42 ++++++++++--------------------------------
>  2 files changed, 12 insertions(+), 34 deletions(-)
> 

applied

Re: [PATCH] Make possible speeds known to ethtool

[Herbert Xu]

On Thu, Jan 08, 2009 at 03:12:01AM +0000, Jeff Garzik wrote:
>
> The next release of ethtool is coming in about 4 weeks, and we can  
> definitely get something like this in there.

Jeff, any chance you can stick this patch in there to add the GRO
toggle?

ethtool: Add GRO toggle

This patch adds the -k toggles to query and set GRO on a device.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ethtool-copy.h b/ethtool-copy.h
index eadba25..3ca4e2c 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -336,6 +336,8 @@ struct ethtool_rxnfc {
 
 #define	ETHTOOL_GRXFH		0x00000029 /* Get RX flow hash configuration */
 #define	ETHTOOL_SRXFH		0x0000002a /* Set RX flow hash configuration */
+#define	ETHTOOL_GGRO		0x0000002b /* Get GRO enable (ethtool_value) */
+#define	ETHTOOL_SGRO		0x0000002c /* Set GRO enable (ethtool_value) */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/ethtool.c b/ethtool.c
index a7c02d0..502fc8f 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -160,6 +160,7 @@ static struct option {
 	        "		[ tso on|off ]\n"
 	        "		[ ufo on|off ]\n"
 		"		[ gso on|off ]\n"
+		"		[ gro on|off ]\n"
 		"               [ lro on|off ]\n"
     },
     { "-i", "--driver", MODE_GDRV, "Show driver information" },
@@ -218,6 +219,7 @@ static int off_sg_wanted = -1;
 static int off_tso_wanted = -1;
 static int off_ufo_wanted = -1;
 static int off_gso_wanted = -1;
+static int off_gro_wanted = -1;
 static int off_lro_wanted = -1;
 
 static struct ethtool_pauseparam epause;
@@ -333,6 +335,7 @@ static struct cmdline_info cmdline_offload[] = {
 	{ "tso", CMDL_BOOL, &off_tso_wanted, NULL },
 	{ "ufo", CMDL_BOOL, &off_ufo_wanted, NULL },
 	{ "gso", CMDL_BOOL, &off_gso_wanted, NULL },
+	{ "gro", CMDL_BOOL, &off_gro_wanted, NULL },
 	{ "lro", CMDL_BOOL, &off_lro_wanted, NULL },
 };
 
@@ -1387,7 +1390,8 @@ static int dump_coalesce(void)
 	return 0;
 }
 
-static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso, int lro)
+static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso,
+			int gro, int lro)
 {
 	fprintf(stdout,
 		"rx-checksumming: %s\n"
@@ -1396,6 +1400,7 @@ static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso, int l
 		"tcp segmentation offload: %s\n"
 		"udp fragmentation offload: %s\n"
 		"generic segmentation offload: %s\n"
+		"generic receive offload: %s\n"
 		"large receive offload: %s\n",
 		rx ? "on" : "off",
 		tx ? "on" : "off",
@@ -1403,6 +1408,7 @@ static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso, int l
 		tso ? "on" : "off",
 		ufo ? "on" : "off",
 		gso ? "on" : "off",
+		gro ? "on" : "off",
 		lro ? "on" : "off");
 
 	return 0;
@@ -1714,7 +1720,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 {
 	struct ethtool_value eval;
 	int err, allfail = 1, rx = 0, tx = 0, sg = 0;
-	int tso = 0, ufo = 0, gso = 0, lro = 0;
+	int tso = 0, ufo = 0, gso = 0, gro = 0, lro = 0;
 
 	fprintf(stdout, "Offload parameters for %s:\n", devname);
 
@@ -1778,6 +1784,16 @@ static int do_goffload(int fd, struct ifreq *ifr)
 		allfail = 0;
 	}
 
+	eval.cmd = ETHTOOL_GGRO;
+	ifr->ifr_data = (caddr_t)&eval;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err)
+		perror("Cannot get device generic receive offload settings");
+	else {
+		gro = eval.data;
+		allfail = 0;
+	}
+
 	eval.cmd = ETHTOOL_GFLAGS;
 	ifr->ifr_data = (caddr_t)&eval;
 	err = ioctl(fd, SIOCETHTOOL, ifr);
@@ -1793,7 +1809,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 		return 83;
 	}
 
-	return dump_offload(rx, tx, sg, tso, ufo, gso, lro);
+	return dump_offload(rx, tx, sg, tso, ufo, gso, gro, lro);
 }
 
 static int do_soffload(int fd, struct ifreq *ifr)
@@ -1870,6 +1886,17 @@ static int do_soffload(int fd, struct ifreq *ifr)
 			return 90;
 		}
 	}
+	if (off_gro_wanted >= 0) {
+		changed = 1;
+		eval.cmd = ETHTOOL_SGRO;
+		eval.data = (off_gro_wanted == 1);
+		ifr->ifr_data = (caddr_t)&eval;
+		err = ioctl(fd, SIOCETHTOOL, ifr);
+		if (err) {
+			perror("Cannot set device generic receive offload settings");
+			return 93;
+		}
+	}
 	if (off_lro_wanted >= 0) {
 		changed = 1;
 		eval.cmd = ETHTOOL_GFLAGS;

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make possible speeds known to ethtool

[Jeff Garzik]

Herbert Xu wrote:
> On Thu, Jan 08, 2009 at 03:12:01AM +0000, Jeff Garzik wrote:
>> The next release of ethtool is coming in about 4 weeks, and we can  
>> definitely get something like this in there.
> 
> Jeff, any chance you can stick this patch in there to add the GRO
> toggle?
> 
> ethtool: Add GRO toggle
> 
> This patch adds the -k toggles to query and set GRO on a device.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/ethtool-copy.h b/ethtool-copy.h
> index eadba25..3ca4e2c 100644
> --- a/ethtool-copy.h
> +++ b/ethtool-copy.h
> @@ -336,6 +336,8 @@ struct ethtool_rxnfc {
>  
>  #define	ETHTOOL_GRXFH		0x00000029 /* Get RX flow hash configuration */
>  #define	ETHTOOL_SRXFH		0x0000002a /* Set RX flow hash configuration */
> +#define	ETHTOOL_GGRO		0x0000002b /* Get GRO enable (ethtool_value) */
> +#define	ETHTOOL_SGRO		0x0000002c /* Set GRO enable (ethtool_value) */

Ugh...  is it too late to change this interface?

The person who added this should have used the new flags interface, and 
added ETH_FLAG_GRO right next to the pre-existing ETH_FLAG_LRO.

It is incorrect to add new ioctls just to toggle a boolean value.

	Jeff

Re: [PATCH] Make possible speeds known to ethtool

[Jeff Garzik]

Herbert Xu wrote:
> On Thu, Jan 08, 2009 at 03:12:01AM +0000, Jeff Garzik wrote:
>> The next release of ethtool is coming in about 4 weeks, and we can  
>> definitely get something like this in there.
> 
> Jeff, any chance you can stick this patch in there to add the GRO
> toggle?
> 
> ethtool: Add GRO toggle
> 
> This patch adds the -k toggles to query and set GRO on a device.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

ugh.  this got buried in my mailbox, sorry.

I missed it completely... and recreated it from scratch just now.  Check 
the git repo, it should work now.

	Jeff

Re: [PATCH] Make possible speeds known to ethtool

[Herbert Xu]

On Fri, Mar 06, 2009 at 06:19:04AM -0500, Jeff Garzik wrote:
>
> I missed it completely... and recreated it from scratch just now.  Check  
> the git repo, it should work now.

Thanks Jeff!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make possible speeds known to ethtool

[Herbert Xu]

Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> The person who added this should have used the new flags interface, and 
> added ETH_FLAG_GRO right next to the pre-existing ETH_FLAG_LRO.
> 
> It is incorrect to add new ioctls just to toggle a boolean value.

Well you missed my earlier explanation.  GRO is a stack flag,
it's not something we want the device drivers to touch at all.

The generic flags interface appears to be designed for flags
that the device driver directly controls, such as LRO.  That's
why it is inappropriate for GRO, which like GSO is entirely done
in software.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make possible speeds known to ethtool

[Jeff Garzik]

Herbert Xu wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
>> The person who added this should have used the new flags interface, and 
>> added ETH_FLAG_GRO right next to the pre-existing ETH_FLAG_LRO.
>>
>> It is incorrect to add new ioctls just to toggle a boolean value.
> 
> Well you missed my earlier explanation.  GRO is a stack flag,
> it's not something we want the device drivers to touch at all.
> 
> The generic flags interface appears to be designed for flags
> that the device driver directly controls, such as LRO.  That's
> why it is inappropriate for GRO, which like GSO is entirely done
> in software.

Nope, the generic flags interface is for any time you have a boolean 
flag to control on a per-interface basis.

The generic flags interface was created precisely because it is silly to 
keep creating _two_ ethtool sub-ioctls (get, set) just for single 
boolean flags.

For generic net stack flags outside the driver's control, that can 
easily be added to ethtool_{get,set}_flags() overriding or ignoring 
whatever the driver may have done.

	Jeff

Re: [PATCH] Make possible speeds known to ethtool

[Herbert Xu]

Jeff Garzik <jgarzik@pobox.com> wrote:
>
> For generic net stack flags outside the driver's control, that can 
> easily be added to ethtool_{get,set}_flags() overriding or ignoring 
> whatever the driver may have done.

To use the flags interface as is, you'd have to go through every
single driver to get them to call ethtool_op_set_flags.  I'm sorry
but I'm sticking with the current interface.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make possible speeds known to ethtool

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 9 Jan 2009 15:19:53 +1100

> Jeff Garzik <jgarzik@pobox.com> wrote:
> >
> > For generic net stack flags outside the driver's control, that can 
> > easily be added to ethtool_{get,set}_flags() overriding or ignoring 
> > whatever the driver may have done.
> 
> To use the flags interface as is, you'd have to go through every
> single driver to get them to call ethtool_op_set_flags.  I'm sorry
> but I'm sticking with the current interface.

Right, the flags thing is inappropriate without major reworking for
Herbert's GRO case and therefore his choice was completely
appropriate.

Re: [PATCH] Make possible speeds known to ethtool

[Jeff Garzik]

David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Fri, 9 Jan 2009 15:19:53 +1100
> 
>> Jeff Garzik <jgarzik@pobox.com> wrote:
>>> For generic net stack flags outside the driver's control, that can 
>>> easily be added to ethtool_{get,set}_flags() overriding or ignoring 
>>> whatever the driver may have done.
>> To use the flags interface as is, you'd have to go through every
>> single driver to get them to call ethtool_op_set_flags.  I'm sorry
>> but I'm sticking with the current interface.
> 
> Right, the flags thing is inappropriate without major reworking for
> Herbert's GRO case and therefore his choice was completely
> appropriate.

It took about ten minutes of coding to disprove this...

	Jeff

Re: [PATCH] Make possible speeds known to ethtool

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

Herbert Xu wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
>> For generic net stack flags outside the driver's control, that can 
>> easily be added to ethtool_{get,set}_flags() overriding or ignoring 
>> whatever the driver may have done.
> 
> To use the flags interface as is, you'd have to go through every
> single driver to get them to call ethtool_op_set_flags.  I'm sorry
> but I'm sticking with the current interface.

I think you misunderstand.  You don't have touch any drivers at all... 
see attached demonstration patch.

The more general point is that it is silly to add two ethtool ioctls 
each time you want to twiddle a single boolean flag (whatever that flag 
may be, generic or driver-specific or whatnot).

If you still desire separation from ->{get,set}_flags() ops, then at 
least create an ETHTOOL_[GS]STACK_FLAGS.

	Jeff




[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2511 bytes --]

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 27c67a5..75fab70 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -285,6 +285,7 @@ struct ethtool_perm_addr {
  */
 enum ethtool_flags {
 	ETH_FLAG_LRO		= (1 << 15),	/* LRO is enabled */
+	ETH_FLAG_GRO		= (1 << 14),	/* GRO is enabled */
 };
 
 struct ethtool_rxnfc {
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 947710a..a114afa 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -864,6 +864,60 @@ static int ethtool_set_value(struct net_device *dev, char __user *useraddr,
 	return actor(dev, edata.data);
 }
 
+static int ethtool_get_generic_flags(struct net_device *dev, u32 *val_out)
+{
+	if (dev->features & NETIF_F_GRO)
+		*val_out |= ETH_FLAG_GRO;
+
+	return 0;
+}
+
+static int ethtool_set_generic_flags(struct net_device *dev, u32 val_in)
+{
+	if (val_in & ETH_FLAG_GRO) {
+		if (!dev->ethtool_ops->get_rx_csum ||
+		    !dev->ethtool_ops->get_rx_csum(dev))
+			return -EINVAL;
+		dev->features |= NETIF_F_GRO;
+	} else
+		dev->features &= ~NETIF_F_GRO;
+
+	return 0;
+}
+
+static int ethtool_get_flags(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value edata = { ETHTOOL_GFLAGS };
+	int rc;
+
+	if (dev->ethtool_ops->get_flags)
+		edata.data = dev->ethtool_ops->get_flags(dev);
+
+	rc = ethtool_get_generic_flags(dev, &edata.data);
+	if (rc)
+		return rc;
+
+	if (copy_to_user(useraddr, &edata, sizeof(edata)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_set_flags(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value edata;
+	int rc;
+
+	if (copy_from_user(&edata, useraddr, sizeof(edata)))
+		return -EFAULT;
+
+	rc = ethtool_set_generic_flags(dev, edata.data);
+
+	if (rc == 0 && dev->ethtool_ops->set_flags)
+		rc = dev->ethtool_ops->set_flags(dev, edata.data);
+
+	return rc;
+}
+
 /* The main entry point in this file.  Called from net/core/dev.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -1036,12 +1090,10 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 		rc = ethtool_set_gso(dev, useraddr);
 		break;
 	case ETHTOOL_GFLAGS:
-		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       dev->ethtool_ops->get_flags);
+		rc = ethtool_get_flags(dev, useraddr);
 		break;
 	case ETHTOOL_SFLAGS:
-		rc = ethtool_set_value(dev, useraddr,
-				       dev->ethtool_ops->set_flags);
+		rc = ethtool_set_flags(dev, useraddr);
 		break;
 	case ETHTOOL_GPFLAGS:
 		rc = ethtool_get_value(dev, useraddr, ethcmd,

Re: [PATCH] Make possible speeds known to ethtool

[Herbert Xu]

On Fri, Jan 09, 2009 at 12:03:57AM -0500, Jeff Garzik wrote:
>
> I think you misunderstand.  You don't have touch any drivers at all...  
> see attached demonstration patch.
>
> The more general point is that it is silly to add two ethtool ioctls  
> each time you want to twiddle a single boolean flag (whatever that flag  
> may be, generic or driver-specific or whatnot).
>
> If you still desire separation from ->{get,set}_flags() ops, then at  
> least create an ETHTOOL_[GS]STACK_FLAGS.

OK, however I'm still not convinced that this is a good idea.
First of all we don't have a shortage in the ethtool name space,
we've only used up two hex digits worth of a 32-bit integer field.

More importantly, making multiple bit changes at the same time
may create semantic nightmares in future.

For example, imagine if we started out with this generic flag
function and TX checksum offload, SG, TSO were done using it.
Now the user issues a request changing all of these bits, we'd
then have to either validate it for contradictory settings, or
devise some ad-hoc ordering in which the settings are applied.

This just seems to be unnecessary penny-pinching that doesn't
save much but may end up costing us down the road.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make possible speeds known to ethtool

[Jeff Garzik]

Herbert Xu wrote:
> On Fri, Jan 09, 2009 at 12:03:57AM -0500, Jeff Garzik wrote:
>> I think you misunderstand.  You don't have touch any drivers at all...  
>> see attached demonstration patch.
>>
>> The more general point is that it is silly to add two ethtool ioctls  
>> each time you want to twiddle a single boolean flag (whatever that flag  
>> may be, generic or driver-specific or whatnot).
>>
>> If you still desire separation from ->{get,set}_flags() ops, then at  
>> least create an ETHTOOL_[GS]STACK_FLAGS.
> 
> OK, however I'm still not convinced that this is a good idea.
> First of all we don't have a shortage in the ethtool name space,
> we've only used up two hex digits worth of a 32-bit integer field.
> 
> More importantly, making multiple bit changes at the same time
> may create semantic nightmares in future.
> 
> For example, imagine if we started out with this generic flag
> function and TX checksum offload, SG, TSO were done using it.
> Now the user issues a request changing all of these bits, we'd
> then have to either validate it for contradictory settings, or
> devise some ad-hoc ordering in which the settings are applied.

Actually it's just the opposite -- _the_ most common complaint from 
users and driver developers of the ethtool interface, over the years, 
has been that there is no way to collect all the modifications and then 
commit it to hardware all in one go.

Each new ethtool command added often winds up pausing and resetting the 
hardware completely, and ETHTOOL_SGRO is no exception.

Now consider what happens when you have a lot of settings to tweak... 
reset, reset, reset, reset, reset.  That's a lot of needless hardware 
banging.

If I could redesign everything from scratch, I would make the interface 
flexible enough to input a list of changed settings, and then commit 
those all at once.  Lists of flags make that possible to a limited 
extent, at least.


> This just seems to be unnecessary penny-pinching that doesn't
> save much but may end up costing us down the road.

This sort of interface was added because the current "multi-reset" 
interface has already cost us.

A multi-flag interface is also more flexible and future-proof, as you 
can deploy new flags without having to update any userland utility... 
ethtool can permit input of the raw value as well as a more refined 
command interface.

That's another facet of the driver-private flag interfaces, too.  The 
ETHTOOL_[GS]PFLAGS interface is flexible enough to permit a wide variety 
of feature flags to be exported by various drivers; userland ethtool 
automatically supports all flags a driver supports, and never needs 
updating even if the list of supported driver-private flags change.

	Jeff

Re: [PATCH] Make possible speeds known to ethtool

[Herbert Xu]

On Fri, Jan 09, 2009 at 12:30:21AM -0500, Jeff Garzik wrote:
>
> Actually it's just the opposite -- _the_ most common complaint from  
> users and driver developers of the ethtool interface, over the years,  
> has been that there is no way to collect all the modifications and then  
> commit it to hardware all in one go.

Yes that's a problem for flags that require the drivers to reset
itself.

> Each new ethtool command added often winds up pausing and resetting the  
> hardware completely, and ETHTOOL_SGRO is no exception.

But as I explained before, GRO (like GSO) is purely a software
setting, it has nothing to do with the driver at all.  In other
words we don't need the driver to reset or do anything.

If anything by going into the driver's set_flags function as you
suggested may cause a spurious reset that wouldn't have happened
otherwise.

So for software flags like GSO/GRO at least, I don't see any
benefit in going to a multi-bit interface.  On the flip side,
I see potential complications with a multi-bit interfaces that
simply don't exist with a single-bit interface.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make possible speeds known to ethtool

[Jeff Garzik]

Herbert Xu wrote:
> On Fri, Jan 09, 2009 at 12:30:21AM -0500, Jeff Garzik wrote:
>> Actually it's just the opposite -- _the_ most common complaint from  
>> users and driver developers of the ethtool interface, over the years,  
>> has been that there is no way to collect all the modifications and then  
>> commit it to hardware all in one go.
> 
> Yes that's a problem for flags that require the drivers to reset
> itself.
> 
>> Each new ethtool command added often winds up pausing and resetting the  
>> hardware completely, and ETHTOOL_SGRO is no exception.
> 
> But as I explained before, GRO (like GSO) is purely a software
> setting, it has nothing to do with the driver at all.  In other

Not quite true...  it touches the driver's rx-csum hook.


> If anything by going into the driver's set_flags function as you
> suggested may cause a spurious reset that wouldn't have happened
> otherwise.
> 
> So for software flags like GSO/GRO at least, I don't see any
> benefit in going to a multi-bit interface.  On the flip side,
> I see potential complications with a multi-bit interfaces that
> simply don't exist with a single-bit interface.

Well, whichever.  Overall, if [GS]GRO remains I am happy to take patches 
supporting it in the userspace utility...

	Jeff

Re: [PATCH] Make possible speeds known to ethtool

[Herbert Xu]

On Fri, Jan 09, 2009 at 01:28:22AM -0500, Jeff Garzik wrote:
>
> Not quite true...  it touches the driver's rx-csum hook.

Only because we never put the RX offload flag in a place that's
independent of the driver.  Also GRO never sets the RX offload
flag (or clears it).

> Well, whichever.  Overall, if [GS]GRO remains I am happy to take patches  
> supporting it in the userspace utility...

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt