Message ID | 1298044598.2570.4.camel@bwh-desktop |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Fri, 2011-02-18 at 15:56 +0000, Ben Hutchings wrote: > On Thu, 2011-02-17 at 19:06 +0000, Ben Hutchings wrote: > > 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. > > > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> > > --- > > 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. [...] > + 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. > + ecmd.advertising = advertising_wanted; > } > > /* Try to perform the update. */
diff --git a/ethtool.c b/ethtool.c index 1afdfe4..2f4b6ee 100644 --- a/ethtool.c +++ b/ethtool.c @@ -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. */