[ethtool 1/2] ethtool.h: implement new MDI-X set defines

[ethtool 1/2] ethtool.h: implement new MDI-X set defines

[Jeff Kirsher]

From: Jesse Brandeburg <jesse.brandeburg@intel.com>

These changes implement the kernel side of the interface
for allowing drivers to set MDI-X state on twisted pair.

Changes implemented as suggested by Ben Hutchings, thanks Ben!

see ethtool patches titled:
ethtool: allow setting MDI-X state

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool-copy.h |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 0e90e9b..6625eff 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -42,8 +42,10 @@ struct ethtool_cmd {
 				 * bits) in Mbps. Please use
 				 * ethtool_cmd_speed()/_set() to
 				 * access it */
-	__u8	eth_tp_mdix;
-	__u8	reserved2;
+	__u8	eth_tp_mdix;	/* twisted pair MDI-X status */
+	__u8	eth_tp_mdix_ctrl; /* twisted pair MDI-X control, when set,
+				   * link should be renegotiated if necessary
+				   */
 	__u32	lp_advertising;	/* Features the link partner advertises */
 	__u32	reserved[2];
 };
@@ -989,10 +991,13 @@ enum ethtool_sfeatures_retval_bits {
 #define AUTONEG_DISABLE		0x00
 #define AUTONEG_ENABLE		0x01
 
-/* Mode MDI or MDI-X */
-#define ETH_TP_MDI_INVALID	0x00
-#define ETH_TP_MDI		0x01
-#define ETH_TP_MDI_X		0x02
+/* MDI or MDI-X status/control - if MDI/MDI_X/AUTO is set then
+ * the driver is required to renegotiate link
+ */
+#define ETH_TP_MDI_INVALID	0x00 /* status: unknown; control: unsupported */
+#define ETH_TP_MDI		0x01 /* status: MDI;     control: force MDI */
+#define ETH_TP_MDI_X		0x02 /* status: MDI-X;   control: force MDI-X */
+#define ETH_TP_MDI_AUTO		0x03 /*                  control: auto-select */
 
 /* Wake-On-Lan options. */
 #define WAKE_PHY		(1 << 0)
-- 
1.7.10.4

Re: [ethtool 1/2] ethtool.h: implement new MDI-X set defines

[Ben Hutchings]

On Thu, 2012-07-19 at 23:25 -0700, Jeff Kirsher wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> These changes implement the kernel side of the interface
> for allowing drivers to set MDI-X state on twisted pair.
> 
> Changes implemented as suggested by Ben Hutchings, thanks Ben!
> 
> see ethtool patches titled:
> ethtool: allow setting MDI-X state
[...]

I don't see the corresponding kernel changes, but I'll hold onto these
for a while in the hope that they show up.

Ben.

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

Re: [ethtool 1/2] ethtool.h: implement new MDI-X set defines

[Jeff Kirsher]

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

On Fri, 2012-07-20 at 16:24 +0100, Ben Hutchings wrote:
> On Thu, 2012-07-19 at 23:25 -0700, Jeff Kirsher wrote:
> > From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > 
> > These changes implement the kernel side of the interface
> > for allowing drivers to set MDI-X state on twisted pair.
> > 
> > Changes implemented as suggested by Ben Hutchings, thanks Ben!
> > 
> > see ethtool patches titled:
> > ethtool: allow setting MDI-X state
> [...]
> 
> I don't see the corresponding kernel changes, but I'll hold onto these
> for a while in the hope that they show up.
> 
> Ben.
> 

I wanted to wait for Dave to pull the patches currently sitting in my
net-next tree before pushing out the corresponding kernel patches.

I will be sending them out here shortly now that Dave has pulled from
me.

Cheers,
Jeff

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[ethtool 1/2] ethtool.h: implement new MDI-X set defines

[Jeff Kirsher]

From: Jesse Brandeburg <jesse.brandeburg@intel.com>

These changes implement the kernel side of the interface
for allowing drivers to set MDI-X state on twisted pair.

Changes implemented as suggested by Ben Hutchings, thanks Ben!

see ethtool patches titled:
ethtool: allow setting MDI-X state

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
Tested-by: Aaron Brown aaron.f.brown@intel.com
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool-copy.h | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 0e90e9b..6625eff 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -42,8 +42,10 @@ struct ethtool_cmd {
 				 * bits) in Mbps. Please use
 				 * ethtool_cmd_speed()/_set() to
 				 * access it */
-	__u8	eth_tp_mdix;
-	__u8	reserved2;
+	__u8	eth_tp_mdix;	/* twisted pair MDI-X status */
+	__u8	eth_tp_mdix_ctrl; /* twisted pair MDI-X control, when set,
+				   * link should be renegotiated if necessary
+				   */
 	__u32	lp_advertising;	/* Features the link partner advertises */
 	__u32	reserved[2];
 };
@@ -989,10 +991,13 @@ enum ethtool_sfeatures_retval_bits {
 #define AUTONEG_DISABLE		0x00
 #define AUTONEG_ENABLE		0x01
 
-/* Mode MDI or MDI-X */
-#define ETH_TP_MDI_INVALID	0x00
-#define ETH_TP_MDI		0x01
-#define ETH_TP_MDI_X		0x02
+/* MDI or MDI-X status/control - if MDI/MDI_X/AUTO is set then
+ * the driver is required to renegotiate link
+ */
+#define ETH_TP_MDI_INVALID	0x00 /* status: unknown; control: unsupported */
+#define ETH_TP_MDI		0x01 /* status: MDI;     control: force MDI */
+#define ETH_TP_MDI_X		0x02 /* status: MDI-X;   control: force MDI-X */
+#define ETH_TP_MDI_AUTO		0x03 /*                  control: auto-select */
 
 /* Wake-On-Lan options. */
 #define WAKE_PHY		(1 << 0)
-- 
1.7.11.4

[ethtool 2/2] ethtool: allow setting MDI-X state

[Jeff Kirsher]

From: Jesse Brandeburg <jesse.brandeburg@intel.com>

A bit ago ethtool added support for reading MDI-X state, this
patch finishes the implementation, adding the complementary write
command.

Add support to ethtool for controlling the MDI-X (crossover)
state of a network port.  Most adapters correctly negotiate
MDI-X, but some ill-behaved switches have trouble and end up
picking the wrong MDI setting, which results in complete loss of
link.  Usually this error condition can be observed when multiple
ethtool -r ethX are required before link is achieved.

This patch allows the user to override the normal "auto" setting
and force the crossover state to on or off.

The set will fail if the driver doesn't support the get, as
suggested by Ben Hutchings.

# ./ethtool -s p1p1 mdix off
setting MDI not supported

In addition the do_gset output was changed slightly to report the
value set by the user (when the driver supports the set)

old:
MDI-X: on

new:
MDI-X: on (auto)
or
MDI-X: on (forced)

usage is ethtool -s eth0 mdix [auto|on|off]

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
Tested-by: Aaron Brown aaron.f.brown@intel.com
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool.8.in |  8 ++++++++
 ethtool.c    | 55 +++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index 7253832..3208d38 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -229,6 +229,7 @@ ethtool \- query or control network driver and hardware settings
 .BI speed \ N
 .B2 duplex half full
 .B4 port tp aui bnc mii fibre
+.B3 mdix auto on off
 .B2 autoneg on off
 .BN advertise
 .BN phyad
@@ -521,6 +522,13 @@ Sets full or half duplex mode.
 .A4 port tp aui bnc mii fibre
 Selects device port.
 .TP
+.A3 mdix auto on off
+Selects MDI-X mode for port. May be used to override the automatic detection
+feature of most adapters.  Auto means automatic detection of MDI status, on
+forces MDI-X (crossover) mode, while off means MDI (straight through) mode.
+The driver should guarantee that this command takes effect immediately, and
+if necessary may reset the link to cause the change to take effect.
+.TP
 .A2 autoneg on off
 Specifies whether autonegotiation should be enabled. Autonegotiation 
 is enabled by default, but in some network devices may have trouble
diff --git a/ethtool.c b/ethtool.c
index e573357..e21b7cb 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -18,6 +18,8 @@
  * Rx Network Flow Control configuration support <santwona.behera@sun.com>
  * Various features by Ben Hutchings <bhutchings@solarflare.com>;
  *	Copyright 2009, 2010 Solarflare Communications
+ * MDI-X set support by Jesse Brandeburg <jesse.brandeburg@intel.com>
+ *	Copyright 2012 Intel Corporation
  *
  * TODO:
  *   * show settings for all devices
@@ -602,16 +604,25 @@ static int dump_ecmd(struct ethtool_cmd *ep)
 
 	if (ep->port == PORT_TP) {
 		fprintf(stdout, "	MDI-X: ");
-		switch (ep->eth_tp_mdix) {
-		case ETH_TP_MDI:
-			fprintf(stdout, "off\n");
-			break;
-		case ETH_TP_MDI_X:
-			fprintf(stdout, "on\n");
-			break;
-		default:
-			fprintf(stdout, "Unknown\n");
-			break;
+		if (ep->eth_tp_mdix_ctrl == ETH_TP_MDI) {
+			fprintf(stdout, "off (forced)\n");
+		} else if (ep->eth_tp_mdix_ctrl == ETH_TP_MDI_X) {
+			fprintf(stdout, "on (forced)\n");
+		} else {
+			switch (ep->eth_tp_mdix) {
+			case ETH_TP_MDI:
+				fprintf(stdout, "off");
+				break;
+			case ETH_TP_MDI_X:
+				fprintf(stdout, "on");
+				break;
+			default:
+				fprintf(stdout, "Unknown");
+				break;
+			}
+			if (ep->eth_tp_mdix_ctrl == ETH_TP_MDI_AUTO)
+				fprintf(stdout, " (auto)");
+			fprintf(stdout, "\n");
 		}
 	}
 
@@ -2199,6 +2210,7 @@ static int do_sset(struct cmd_context *ctx)
 	int speed_wanted = -1;
 	int duplex_wanted = -1;
 	int port_wanted = -1;
+	int mdix_wanted = -1;
 	int autoneg_wanted = -1;
 	int phyad_wanted = -1;
 	int xcvr_wanted = -1;
@@ -2259,6 +2271,19 @@ static int do_sset(struct cmd_context *ctx)
 				port_wanted = PORT_FIBRE;
 			else
 				exit_bad_args();
+		} else if (!strcmp(argp[i], "mdix")) {
+			gset_changed = 1;
+			i += 1;
+			if (i >= argc)
+				exit_bad_args();
+			if (!strcmp(argp[i], "auto"))
+				mdix_wanted = ETH_TP_MDI_AUTO;
+			else if (!strcmp(argp[i], "on"))
+				mdix_wanted = ETH_TP_MDI_X;
+			else if (!strcmp(argp[i], "off"))
+				mdix_wanted = ETH_TP_MDI;
+			else
+				exit_bad_args();
 		} else if (!strcmp(argp[i], "autoneg")) {
 			i += 1;
 			if (i >= argc)
@@ -2380,6 +2405,13 @@ static int do_sset(struct cmd_context *ctx)
 				ecmd.duplex = duplex_wanted;
 			if (port_wanted != -1)
 				ecmd.port = port_wanted;
+			if (mdix_wanted != -1) {
+				/* check driver supports MDI-X */
+				if (ecmd.eth_tp_mdix_ctrl != ETH_TP_MDI_INVALID)
+					ecmd.eth_tp_mdix_ctrl = mdix_wanted;
+				else
+					fprintf(stderr, "setting MDI not supported\n");
+			}
 			if (autoneg_wanted != -1)
 				ecmd.autoneg = autoneg_wanted;
 			if (phyad_wanted != -1)
@@ -2439,6 +2471,8 @@ static int do_sset(struct cmd_context *ctx)
 				fprintf(stderr, "  not setting phy_address\n");
 			if (xcvr_wanted != -1)
 				fprintf(stderr, "  not setting transceiver\n");
+			if (mdix_wanted != -1)
+				fprintf(stderr, "  not setting mdix\n");
 		}
 	}
 
@@ -3604,6 +3638,7 @@ static const struct option {
 	  "		[ speed %d ]\n"
 	  "		[ duplex half|full ]\n"
 	  "		[ port tp|aui|bnc|mii|fibre ]\n"
+	  "		[ mdix auto|on|off ]\n"
 	  "		[ autoneg on|off ]\n"
 	  "		[ advertise %x ]\n"
 	  "		[ phyad %d ]\n"
-- 
1.7.11.4

Re: [ethtool 1/2] ethtool.h: implement new MDI-X set defines

[Jeff Kirsher]

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

On Tue, 2012-08-21 at 01:37 -0700, Kirsher, Jeffrey T wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> These changes implement the kernel side of the interface
> for allowing drivers to set MDI-X state on twisted pair.
> 
> Changes implemented as suggested by Ben Hutchings, thanks Ben!
> 
> see ethtool patches titled:
> ethtool: allow setting MDI-X state
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> CC: Ben Hutchings <bhutchings@solarflare.com>
> Tested-by: Aaron Brown aaron.f.brown@intel.com
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  ethtool-copy.h | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-) 

Ben - ping?  I did not see a response that you queued up this 2 patch
series for Ethtool yet.  Dave has already pulled the corresponding
kernel patches into net-next.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [ethtool 2/2] ethtool: allow setting MDI-X state

[Ben Hutchings]

On Tue, 2012-08-21 at 01:37 -0700, Jeff Kirsher wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> A bit ago ethtool added support for reading MDI-X state, this
> patch finishes the implementation, adding the complementary write
> command.
[...]

Applied.  I also commited the following changes:

---
Subject: ethtool.8: Mark-up mdix arguments properly

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 ethtool.8.in |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index 3208d38..2799e25 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -523,11 +523,12 @@ Sets full or half duplex mode.
 Selects device port.
 .TP
 .A3 mdix auto on off
-Selects MDI-X mode for port. May be used to override the automatic detection
-feature of most adapters.  Auto means automatic detection of MDI status, on
-forces MDI-X (crossover) mode, while off means MDI (straight through) mode.
-The driver should guarantee that this command takes effect immediately, and
-if necessary may reset the link to cause the change to take effect.
+Selects MDI-X mode for port. May be used to override the automatic
+detection feature of most adapters. An argument of \fBauto\fR means
+automatic detection of MDI status, \fBon\fR forces MDI-X (crossover)
+mode, while \fBoff\fR means MDI (straight through) mode.  The driver
+should guarantee that this command takes effect immediately, and if
+necessary may reset the link to cause the change to take effect.
 .TP
 .A2 autoneg on off
 Specifies whether autonegotiation should be enabled. Autonegotiation 
---
Subject: test-cmdline: Test -s mdix keyword

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 test-cmdline.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/test-cmdline.c b/test-cmdline.c
index 6a60ed4..85b4ce0 100644
--- a/test-cmdline.c
+++ b/test-cmdline.c
@@ -22,12 +22,14 @@ static struct test_case {
 	{ 1, "16_char_devname!" },
 	/* Argument parsing for -s is specialised */
 	{ 0, "-s devname" },
-	{ 0, "--change devname speed 100 duplex half" },
+	{ 0, "--change devname speed 100 duplex half mdix auto" },
 	{ 1, "-s devname speed foo" },
 	{ 1, "--change devname speed" },
 	{ 0, "-s devname duplex half" },
 	{ 1, "--change devname duplex foo" },
 	{ 1, "-s devname duplex" },
+	{ 1, "--change devname mdix foo" },
+	{ 1, "-s devname mdix" },
 	{ 0, "--change devname port tp" },
 	{ 1, "-s devname port foo" },
 	{ 1, "--change devname port" },
---

Please include at least basic test cases like this for any new feature.
I did it this time because I've already kept you waiting and didn't
think it would be fair to request changes.

Ben.

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

Re: [ethtool 2/2] ethtool: allow setting MDI-X state

[Jeff Kirsher]

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

On Thu, 2012-09-06 at 19:25 +0100, Ben Hutchings wrote:
> On Tue, 2012-08-21 at 01:37 -0700, Jeff Kirsher wrote:
> > From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > 
> > A bit ago ethtool added support for reading MDI-X state, this
> > patch finishes the implementation, adding the complementary write
> > command.
> [...]
> 
> Applied.  I also commited the following changes:
> 
> ---
> Subject: ethtool.8: Mark-up mdix arguments properly
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  ethtool.8.in |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 3208d38..2799e25 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -523,11 +523,12 @@ Sets full or half duplex mode.
>  Selects device port.
>  .TP
>  .A3 mdix auto on off
> -Selects MDI-X mode for port. May be used to override the automatic detection
> -feature of most adapters.  Auto means automatic detection of MDI status, on
> -forces MDI-X (crossover) mode, while off means MDI (straight through) mode.
> -The driver should guarantee that this command takes effect immediately, and
> -if necessary may reset the link to cause the change to take effect.
> +Selects MDI-X mode for port. May be used to override the automatic
> +detection feature of most adapters. An argument of \fBauto\fR means
> +automatic detection of MDI status, \fBon\fR forces MDI-X (crossover)
> +mode, while \fBoff\fR means MDI (straight through) mode.  The driver
> +should guarantee that this command takes effect immediately, and if
> +necessary may reset the link to cause the change to take effect.
>  .TP
>  .A2 autoneg on off
>  Specifies whether autonegotiation should be enabled. Autonegotiation 
> ---
> Subject: test-cmdline: Test -s mdix keyword
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  test-cmdline.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/test-cmdline.c b/test-cmdline.c
> index 6a60ed4..85b4ce0 100644
> --- a/test-cmdline.c
> +++ b/test-cmdline.c
> @@ -22,12 +22,14 @@ static struct test_case {
>  	{ 1, "16_char_devname!" },
>  	/* Argument parsing for -s is specialised */
>  	{ 0, "-s devname" },
> -	{ 0, "--change devname speed 100 duplex half" },
> +	{ 0, "--change devname speed 100 duplex half mdix auto" },
>  	{ 1, "-s devname speed foo" },
>  	{ 1, "--change devname speed" },
>  	{ 0, "-s devname duplex half" },
>  	{ 1, "--change devname duplex foo" },
>  	{ 1, "-s devname duplex" },
> +	{ 1, "--change devname mdix foo" },
> +	{ 1, "-s devname mdix" },
>  	{ 0, "--change devname port tp" },
>  	{ 1, "-s devname port foo" },
>  	{ 1, "--change devname port" },
> ---
> 
> Please include at least basic test cases like this for any new feature.
> I did it this time because I've already kept you waiting and didn't
> think it would be fair to request changes.
> 
> Ben.
> 

Thanks Ben, I will make sure that we cover that with any new features
going forward.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]