ethtool: Warn if we cannot set the advertising mask for specified speed/duplex
Subsystems (2)
PATCH 1/1 ethtool: Warn if we cannot set the advertising mask for specified speed/duplex
When autonegotiation is enabled, drivers must determine link speed and duplex through the autonegotiation process and will generally ignore the speed and duplex specified in struct ethtool_cmd. If the user specifies a recognised combination of speed and duplex, we set the advertising mask to include only the matching mode. Otherwise, we advertise all supported modes. We should really limit the advertised modes separately by speed and duplex, but for now we just warn.
---
This is a longstanding bug in ethtool which people keep getting bitten by. Naveen was just the latest to report it. I have been working on some changes to improve link advertising and reporting, but until those are ready I'm adding this warning.
Ben.
ethtool.c | 46 ++++++++++++++++++++++++++++++++-------------- 1 files changed, 32 insertions(+), 14 deletions(-)
ethtool.c +32 -14
@@ -1126,7 +1126,7 @@ static void parse_cmdline(int argc, char **argp) } } -if ((autoneg_wanted ==AUTONEG_ENABLE ) && (advertising_wanted <0 )) { +if (advertising_wanted <0 ) {if (speed_wanted ==SPEED_10 && duplex_wanted ==DUPLEX_HALF ) advertising_wanted = ADVERTISED_10baseT_Half; else if (speed_wanted == SPEED_10 &&
@@ -2528,19 +2528,37 @@ static int do_sset(int fd, struct ifreq *ifr) ecmd.phy_address = phyad_wanted;if (xcvr_wanted !=-1 ) ecmd.transceiver = xcvr_wanted; -if (advertising_wanted !=-1 ) { -if (advertising_wanted ==0 ) - ecmd.advertising = ecmd.supported & - (ADVERTISED_10baseT_Half | - ADVERTISED_10baseT_Full | - ADVERTISED_100baseT_Half | - ADVERTISED_100baseT_Full | - ADVERTISED_1000baseT_Half | - ADVERTISED_1000baseT_Full | - ADVERTISED_2500baseX_Full | - ADVERTISED_10000baseT_Full); -else - ecmd.advertising = advertising_wanted; +if (advertising_wanted ==0 ) { +/* XXX If autoneg is on and the user +* specified speed or duplex then we +* should mask the advertised modes +* accordingly. For now, warn that we +* aren't doing that. +*/ +if (ecmd.autoneg && + (speed_wanted >=0 || duplex_wanted >=0 )) { +fprintf (stderr,"Cannot advertise" ); +if (speed_wanted >=0 ) +fprintf (stderr," speed %d" , + speed_wanted); +if (duplex_wanted >=0 ) +fprintf (stderr," duplex %s" , + duplex_wanted ? +"full" :"half" ); +fprintf (stderr,"; advertising all" +" supported modes\n" ); + } + ecmd.advertising = ecmd.supported & + (ADVERTISED_10baseT_Half | + ADVERTISED_10baseT_Full | + ADVERTISED_100baseT_Half | + ADVERTISED_100baseT_Full | + ADVERTISED_1000baseT_Half | + ADVERTISED_1000baseT_Full | + ADVERTISED_2500baseX_Full | + ADVERTISED_10000baseT_Full); + }else if (advertising_wanted !=-1 ) { + ecmd.advertising = advertising_wanted; }/* Try to perform the update. */
Signature · 7 lines
1.7.3.4 -- 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.
Quoted 8 lines · Ben Hutchings
When autonegotiation is enabled, drivers must determine link speed and duplex through the autonegotiation process and will generally ignore the speed and duplex specified in struct
ethtool_cmd. If the user specifies a recognised combination of speed and duplex, we set the advertising mask to include only the matching mode. Otherwise, we advertise all supported modes. We should really limit the advertised modes separately by speed and duplex, but for now we just warn.
Quoted 6 lines
Signed-off-by Ben Hutchings <[email protected]>---
This is a longstanding bug in ethtool which people keep getting bitten by. Naveen was just the latest to report it. I have been working on some changes to improve link advertising and reporting, but until those are ready I'm adding this warning.
That patch fixes an additional bug: we will now update the advertising mask based on speed/duplex when autoneg is already on. Unfortunately it introduces a bug: we will update advertising even if none of autoneg, speed or duplex are specified!
Here's a second version which I think will do the right thing in all cases, though I haven't tested it yet. I'll include this in ethtool 2.6.38 if no-one can spot a flaw.
Ben.
---
From: Ben Hutchings <[email protected]>
Date: Thu, 17 Feb 2011 18:51:15 +0000
Subject: [PATCH ethtool] ethtool: Don't silently ignore speed/duplex when autoneg is on
When autonegotiation is enabled, drivers must determine link speed and duplex through the autonegotiation process and will generally ignore the speed and duplex specified in struct ethtool_cmd. Currently, if the user specifies autoneg on but does not specify the advertising mask then:
- If the user specifies a recognised combination of speed and duplex, we set the advertising mask to the flag for that mode. (Currently only one mode is recognised per combination of speed and duplex.)
- Otherwise, we advertise all recognised and supported modes.
But we should also set the advertising mask if autoneg is *already* on. Also, we should be able to limit the advertised modes separately by speed and duplex. For now, we just warn if we fail to do that.
--- ethtool.c | 45 +++++++++++++++++++++++++++++++-------------- 1 files changed, 31 insertions(+), 14 deletions(-)
ethtool.c +31 -14
@@ -1126,7 +1126,7 @@ static void parse_cmdline(int argc, char **argp) } } -if ((autoneg_wanted ==AUTONEG_ENABLE ) && (advertising_wanted <0 )) { +if (advertising_wanted <0 ) {if (speed_wanted ==SPEED_10 && duplex_wanted ==DUPLEX_HALF ) advertising_wanted = ADVERTISED_10baseT_Half; else if (speed_wanted == SPEED_10 &&
@@ -2528,19 +2528,36 @@ static int do_sset(int fd, struct ifreq *ifr) ecmd.phy_address = phyad_wanted;if (xcvr_wanted !=-1 ) ecmd.transceiver = xcvr_wanted; -if (advertising_wanted !=-1 ) { -if (advertising_wanted ==0 ) - ecmd.advertising = ecmd.supported & - (ADVERTISED_10baseT_Half | - ADVERTISED_10baseT_Full | - ADVERTISED_100baseT_Half | - ADVERTISED_100baseT_Full | - ADVERTISED_1000baseT_Half | - ADVERTISED_1000baseT_Full | - ADVERTISED_2500baseX_Full | - ADVERTISED_10000baseT_Full); -else - ecmd.advertising = advertising_wanted; +/* XXX If the user specified speed or duplex +* then we should mask the advertised modes +* accordingly. For now, warn that we aren't +* doing that. +*/ +if ((speed_wanted !=-1 || duplex_wanted !=-1 ) && + ecmd.autoneg && advertising_wanted ==0 ) { +fprintf (stderr,"Cannot advertise" ); +if (speed_wanted >=0 ) +fprintf (stderr," speed %d" , + speed_wanted); +if (duplex_wanted >=0 ) +fprintf (stderr," duplex %s" , + duplex_wanted ? +"full" :"half" ); +fprintf (stderr,"\n" ); + } +if (autoneg_wanted ==AUTONEG_ENABLE && + advertising_wanted ==0 ) { + ecmd.advertising = ecmd.supported & + (ADVERTISED_10baseT_Half | + ADVERTISED_10baseT_Full | + ADVERTISED_100baseT_Half | + ADVERTISED_100baseT_Full | + ADVERTISED_1000baseT_Half | + ADVERTISED_1000baseT_Full | + ADVERTISED_2500baseX_Full | + ADVERTISED_10000baseT_Full); + }else if (advertising_wanted !=-1 ) { + ecmd.advertising = advertising_wanted; }/* Try to perform the update. */
Signature · 8 lines
1.7.3.4 -- 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.
Quoted 24 lines · Ben Hutchings
Quoted 8 lines · Ben Hutchings
When autonegotiation is enabled, drivers must determine link speed and duplex through the autonegotiation process and will generally ignore the speed and duplex specified in struct
ethtool_cmd. If the user specifies a recognised combination of speed and duplex, we set the advertising mask to include only the matching mode. Otherwise, we advertise all supported modes. We should really limit the advertised modes separately by speed and duplex, but for now we just warn.Quoted 6 lines
Signed-off-by Ben Hutchings <[email protected]>---
This is a longstanding bug in ethtool which people keep getting bitten by. Naveen was just the latest to report it. I have been working on some changes to improve link advertising and reporting, but until those are ready I'm adding this warning.That patch fixes an additional bug: we will now update the advertising mask based on speed/duplex when autoneg is already on. Unfortunately it introduces a bug: we will update advertising even if none of autoneg, speed or duplex are specified!
Here's a second version which I think will do the right thing in all cases, though I haven't tested it yet. I'll include this in ethtool 2.6.38 if no-one can spot a flaw.
[...]
ethtool.c static int do_sset(int fd, struct ifreq *ifr) · line 2559
Quoted 12 lines
ethtool.c +12
-0+if (autoneg_wanted ==AUTONEG_ENABLE && + advertising_wanted ==0 ) { + ecmd.advertising = ecmd.supported & + (ADVERTISED_10baseT_Half | + ADVERTISED_10baseT_Full | + ADVERTISED_100baseT_Half | + ADVERTISED_100baseT_Full | + ADVERTISED_1000baseT_Half | + ADVERTISED_1000baseT_Full | + ADVERTISED_2500baseX_Full | + ADVERTISED_10000baseT_Full); + }else if (advertising_wanted !=-1 ) {
This condition should be (advertising_wanted > 0). With that last change, ethtool seems to do the right thing.
Ben.
Quoted 4 lines
ethtool.c +1
-0+ ecmd.advertising = advertising_wanted; }/* Try to perform the update. */
Signature · 3 lines
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.