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;
 	};
 
--

[ message continues ]

[...]

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.

--

[ message continues ]

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



--

[ message continues ]

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

[ message continues ]

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.

--

[ message continues ]

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

[ message continues ]

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

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)
 ...
[ message continues ]

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.


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

rick jones
--

[ message continues ]

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.

--

[ message continues ]

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

rick
--

[ message continues ]

ACK, can you rediff against current git repo?


--

[ message continues ]

applied


--

[ message continues ]

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"
@@ ...
[ message continues ]

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




--

[ message continues ]

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




--

[ message continues ]

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

[ message continues ]