[PATCH] Force speed and duplex mode setting if link down

[PATCH] Force speed and duplex mode setting if link down

[stannous]

From: Sam Tannous <stannous@cumulusnetworks.com>

This patch forces the user to set both the speed and the duplex
mode if the user only specifies the speed while the interface is down.

If the interface is down, we'll get an ecmd speed
and duplex mode of 0. This has the ill effect of
setting the duplex to HALF and this often results in an
error since many higher speed will not support HALF duplex.

Signed-off-by: Sam Tannous <stannous@cumulusnetworks.com>
---
 ethtool.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 8e968a8..9cd3103 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2461,6 +2461,22 @@ static int do_sset(struct cmd_context *ctx)
 		if (err < 0) {
 			perror("Cannot get current device settings");
 		} else {
+			/* If the interface is down, we'll get an ecmd speed and
+			   duplex of 0.  This has the effect of defaulting
+			   the duplex mode to HALF.  This results in an
+			   error since many of our speeds will not support HALF
+			   any more.  To prevent this, we need to insist that
+			   the user provide us with specific duplex mode if
+			   they haven't already (likely FULL) if both the speed
+			   and duplex are 0 here.
+			*/
+			if (speed_wanted != -1 && duplex_wanted == -1
+			    && !(ecmd.speed || ecmd.duplex)) {
+			    fprintf(stderr, "Setting the speed while the link "
+				    "is down requires both the speed and the "
+				    "duplex mode to be set");
+			    exit_bad_args();
+			}
 			/* Change everything the user specified. */
 			if (speed_wanted != -1)
 				ethtool_cmd_speed_set(&ecmd, speed_wanted);
-- 
1.7.10.4

Re: [PATCH] Force speed and duplex mode setting if link down

[Florian Fainelli]

2014-06-02 16:07 GMT-07:00  <stannous@cumulusnetworks.com>:
> From: Sam Tannous <stannous@cumulusnetworks.com>
>
> This patch forces the user to set both the speed and the duplex
> mode if the user only specifies the speed while the interface is down.

Is there a valid use case for that, even with auto-negotiation disabled?

>
> If the interface is down, we'll get an ecmd speed
> and duplex mode of 0. This has the ill effect of
> setting the duplex to HALF and this often results in an
> error since many higher speed will not support HALF duplex.

Is this happening under the following scenario:

- interface is forced to a given speed and duplex parameter,
auto-negotiation is disabled
- interface is administratively brought down
- speed and parameters are changed while the interface is down
- interface is brought up with potentially invalid parameters, and
with auto-negotiation disabled resulting in no link

If that's the case, the driver implementing the set_settings()
callback should refuse changing the link parameters while the
interface is down to avoid creating such a situation.

Just curious...

>
> Signed-off-by: Sam Tannous <stannous@cumulusnetworks.com>
> ---
>  ethtool.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/ethtool.c b/ethtool.c
> index 8e968a8..9cd3103 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -2461,6 +2461,22 @@ static int do_sset(struct cmd_context *ctx)
>                 if (err < 0) {
>                         perror("Cannot get current device settings");
>                 } else {
> +                       /* If the interface is down, we'll get an ecmd speed and
> +                          duplex of 0.  This has the effect of defaulting
> +                          the duplex mode to HALF.  This results in an
> +                          error since many of our speeds will not support HALF
> +                          any more.  To prevent this, we need to insist that
> +                          the user provide us with specific duplex mode if
> +                          they haven't already (likely FULL) if both the speed
> +                          and duplex are 0 here.
> +                       */
> +                       if (speed_wanted != -1 && duplex_wanted == -1
> +                           && !(ecmd.speed || ecmd.duplex)) {
> +                           fprintf(stderr, "Setting the speed while the link "
> +                                   "is down requires both the speed and the "
> +                                   "duplex mode to be set");
> +                           exit_bad_args();
> +                       }
>                         /* Change everything the user specified. */
>                         if (speed_wanted != -1)
>                                 ethtool_cmd_speed_set(&ecmd, speed_wanted);
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Florian

Re: [PATCH] Force speed and duplex mode setting if link down

[David Miller]

From: stannous@cumulusnetworks.com
Date: Mon,  2 Jun 2014 19:07:00 -0400

> +			/* If the interface is down, we'll get an ecmd speed and
> +			   duplex of 0.  This has the effect of defaulting
> +			   the duplex mode to HALF.  This results in an
> +			   error since many of our speeds will not support HALF
> +			   any more.  To prevent this, we need to insist that
> +			   the user provide us with specific duplex mode if
> +			   they haven't already (likely FULL) if both the speed
> +			   and duplex are 0 here.
> +			*/

This comment is not formatted properly for the neworking, the proper layout
is:

	/* Like
	 * this.
	 */

Also, your subject line needs to be adjusted.  After "[PATCH] " you must
specify a subsystem prefix, which here would be something like
"ethtool: ".  This way people reading the commit log headers can see where
in the kernel your changes are happening.

Re: [PATCH] Force speed and duplex mode setting if link down

[Sam Tannous]

Hello Florian,

I probably didn't make this clear enough in the description.
I'm not describing a use case.   I'm simply changing the 
speed on two interfaces that are directly connected.  
I don't have autoneg on at all.

1. I start with a working link where the speed is
   10G and duplex is full on both sides.

2. I change the speed on one side to 1G with 
   
      ethtool -s speed 1000
  
   and the command appears to work (at least there is no error message).
   Once I issue this command, the link is no longer usable
   since the speed changed on one side but not the other.  Autoneg
   is disabled (by default).

3. I try to change the speed on the opposite side to 1G as well but
   I don't get very far and I get this error:

------------------------
   # ethtool -s swp29 speed 1000
Cannot set new settings: Operation not supported
  not setting speed
------------------------

In looking into ethtool.c, I found that before we ETHTOOL_SSET, 
we do an ETHTOOL_GSET to get everything.   

So while the link is up, we are getting duplex FULL, and the ETHTOOL_SSET is successful. 
Once the speed changes on one end, the link goes down.
The duplex setting goes to half.  

Then when we do an ETHTOOL_GSET on the second side, we get a duplex of half.
So we use this half duplex in ecmd.duplex (since the user never specified anything,
duplex_wanted == -1).

The net result is that ethtool attempts to set the speed to 1G with half duplex
and this port does not support half duplex so we get an error message with
no mention of the half duplex defaulting. 

This patch lets the user know that he/she must explicitly specify 
the duplex setting along with the speed because we received a duplex
config of 0 (half, ecmd.duplex == 0) with no speed (ecmd.speed == 0).

The motivation for this is simply that the error is not too informative:
the user changes the speed on one side...without problems.  Then the 
user tries to do the same thing on the other side and gets an error.
There is no hint about the duplex mode defaulting to half.  This simply
forces the user to be more explicit and specify both.  

There may be a simpler solution to this.  I thought it would be helpful
to be more explicit here.

Thanks,
--
Sam


----- Original Message -----
From: "Florian Fainelli" <f.fainelli@gmail.com>
To: stannous@cumulusnetworks.com
Cc: "netdev" <netdev@vger.kernel.org>, "Shrijeet Mukherjee" <shm@cumulusnetworks.com>, "Scott Feldman" <sfeldma@cumulusnetworks.com>
Sent: Monday, June 2, 2014 7:32:52 PM
Subject: Re: [PATCH] Force speed and duplex mode setting if link down

2014-06-02 16:07 GMT-07:00  <stannous@cumulusnetworks.com>:
> From: Sam Tannous <stannous@cumulusnetworks.com>
>
> This patch forces the user to set both the speed and the duplex
> mode if the user only specifies the speed while the interface is down.

Is there a valid use case for that, even with auto-negotiation disabled?

>
> If the interface is down, we'll get an ecmd speed
> and duplex mode of 0. This has the ill effect of
> setting the duplex to HALF and this often results in an
> error since many higher speed will not support HALF duplex.

Is this happening under the following scenario:

- interface is forced to a given speed and duplex parameter,
auto-negotiation is disabled
- interface is administratively brought down
- speed and parameters are changed while the interface is down
- interface is brought up with potentially invalid parameters, and
with auto-negotiation disabled resulting in no link

If that's the case, the driver implementing the set_settings()
callback should refuse changing the link parameters while the
interface is down to avoid creating such a situation.

Just curious...

>
> Signed-off-by: Sam Tannous <stannous@cumulusnetworks.com>
> ---
>  ethtool.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/ethtool.c b/ethtool.c
> index 8e968a8..9cd3103 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -2461,6 +2461,22 @@ static int do_sset(struct cmd_context *ctx)
>                 if (err < 0) {
>                         perror("Cannot get current device settings");
>                 } else {
> +                       /* If the interface is down, we'll get an ecmd speed and
> +                          duplex of 0.  This has the effect of defaulting
> +                          the duplex mode to HALF.  This results in an
> +                          error since many of our speeds will not support HALF
> +                          any more.  To prevent this, we need to insist that
> +                          the user provide us with specific duplex mode if
> +                          they haven't already (likely FULL) if both the speed
> +                          and duplex are 0 here.
> +                       */
> +                       if (speed_wanted != -1 && duplex_wanted == -1
> +                           && !(ecmd.speed || ecmd.duplex)) {
> +                           fprintf(stderr, "Setting the speed while the link "
> +                                   "is down requires both the speed and the "
> +                                   "duplex mode to be set");
> +                           exit_bad_args();
> +                       }
>                         /* Change everything the user specified. */
>                         if (speed_wanted != -1)
>                                 ethtool_cmd_speed_set(&ecmd, speed_wanted);
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Florian

RE: [PATCH] Force speed and duplex mode setting if link down

[David Laight]

From: Of Sam Tannous
> 
> I probably didn't make this clear enough in the description.
> I'm not describing a use case.   I'm simply changing the
> speed on two interfaces that are directly connected.
> I don't have autoneg on at all.
> 
> 1. I start with a working link where the speed is
>    10G and duplex is full on both sides.
> 
> 2. I change the speed on one side to 1G with
> 
>       ethtool -s speed 1000
> 
>    and the command appears to work (at least there is no error message).
>    Once I issue this command, the link is no longer usable
>    since the speed changed on one side but not the other.  Autoneg
>    is disabled (by default).

You really want to leave autoneg enabled and just limit the advertised
speeds.

I suspect there are plenty of ways ethtool can be used to make
non-working interfaces, some of them might be needed be developers
or in unusual circumstances.
So I don't think ethtool should be policing this specific condition.

Maybe the driver should just use FDX when HDX is invalid for the
speed.

	David

Re: [PATCH] Force speed and duplex mode setting if link down

[Scott Feldman]

On Jun 3, 2014, at 2:13 AM, David Laight <David.Laight@ACULAB.COM> wrote:

> From: Of Sam Tannous
>> 
>> I probably didn't make this clear enough in the description.
>> I'm not describing a use case.   I'm simply changing the
>> speed on two interfaces that are directly connected.
>> I don't have autoneg on at all.
>> 
>> 1. I start with a working link where the speed is
>>   10G and duplex is full on both sides.
>> 
>> 2. I change the speed on one side to 1G with
>> 
>>      ethtool -s speed 1000
>> 
>>   and the command appears to work (at least there is no error message).
>>   Once I issue this command, the link is no longer usable
>>   since the speed changed on one side but not the other.  Autoneg
>>   is disabled (by default).
> 
> You really want to leave autoneg enabled and just limit the advertised
> speeds.

This patch is about the forced speed/duplex case, which is one way to set settings.  Autoneg is another.

> I suspect there are plenty of ways ethtool can be used to make
> non-working interfaces, some of them might be needed be developers
> or in unusual circumstances.
> So I don't think ethtool should be policing this specific condition.

Why not?  The patch enforces the specific condition when input from the user is ambiguous.  When link is down, and user is forcing speed only, there is not enough information to know duplex, and ethtool app is defaulting to HDX, which is usually the wrong choice for most interfaces.  The patch forces the user to be explicit on speed and duplex in this ambiguous case.

> Maybe the driver should just use FDX when HDX is invalid for the
> speed.

No, the driver should fail an attempt to set HDX when not supported.

-scott