| Message ID | 1304986748-15809-3-git-send-email-decot@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2011-05-09 at 17:19 -0700, David Decotigny wrote: > The initial version of the driver used to force the link to 100Mbps > when auto-negociation was disabled on a 1Gbps link, ignoring the > requested link speed. Instead, this change refuses to change anything > when it is asked to configure the link speed at 1Gbps without > auto-negociation, but acts as requested in all the other cases. > > IMPORTANT: Previously, the return value from mii_set_media() was > ignored. This patch uses it for its own return value. > > Tested: module compiling, NOT tested on real hardware. > Signed-off-by: David Decotigny <decot@google.com> [...] The changes to validation look fine. However, I noticed that there's a call to netif_carrier_off() at the top of this function. This means that in the error and shortcut cases, the interface will be left disabled! It's an existing bug but might be made slightly worse by this change. Please also move the call to netif_carrier_off() down to the end, just before the call to mii_set_media() which actually alters the link. Ben.
Hi all, Yes, right, I will send the updated patch together with the stmmac update (if any). I just hope that changing the netdev_private fields without committing to hardware and before calling netif_carrier_off() will not create too much confusion. I don't think so, but I still wish I could test. Regards, -- David Decotigny On Tue, May 10, 2011 at 11:55 AM, Ben Hutchings <bhutchings@solarflare.com> wrote: > On Mon, 2011-05-09 at 17:19 -0700, David Decotigny wrote: >> The initial version of the driver used to force the link to 100Mbps >> when auto-negociation was disabled on a 1Gbps link, ignoring the >> requested link speed. Instead, this change refuses to change anything >> when it is asked to configure the link speed at 1Gbps without >> auto-negociation, but acts as requested in all the other cases. >> >> IMPORTANT: Previously, the return value from mii_set_media() was >> ignored. This patch uses it for its own return value. >> >> Tested: module compiling, NOT tested on real hardware. >> Signed-off-by: David Decotigny <decot@google.com> > [...] > > The changes to validation look fine. However, I noticed that there's a > call to netif_carrier_off() at the top of this function. This means > that in the error and shortcut cases, the interface will be left > disabled! It's an existing bug but might be made slightly worse by this > change. > > Please also move the call to netif_carrier_off() down to the end, just > before the call to mii_set_media() which actually alters the link. > > Ben. > > -- > Ben Hutchings, Senior Software 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. > > -- 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
* David Decotigny:
> Tested: module compiling, NOT tested on real hardware.
To my knowledge, dl2k is broken. Some sort of synchronization
primitives are missing. Under load, the NIC's notion of ring buffer
status diverges from the host's view. 8-(
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi all, An update on the patch series I posted earlier (dl2k/stmmac autoneg minor changes: see my posts on 05/09/11 17:19 PST)... I'd suggest dropping the patch concerning dl2k, as I don't have any way to claim it's any good at all (though I'm pretty confident it doesn't make things any worse). However, for the other patch of the same series (stmmac), please consider for inclusion the new version prepared by Giuseppe instead of my initial patch, for his version readily integrates my patch and has been tested on real hardware: stmmac: don't go through ethtool to start auto-negotiation stmmac: fix autoneg in set_pauseparam Regards, Thank you! On 05/11/11 02:47, Florian Weimer wrote: > * David Decotigny: > >> Tested: module compiling, NOT tested on real hardware. > > To my knowledge, dl2k is broken. Some sort of synchronization > primitives are missing. Under load, the NIC's notion of ring buffer > status diverges from the host's view. 8-( > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk3NVCYACgkQld7vhusVrCHUEACggeKyCmoEHy9AzYec/aW8cDjU GyAAoIiESxUAFWuKBmCOA31X/V0fvC+Y =6hXY -----END PGP SIGNATURE----- -- 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
diff --git a/drivers/net/dl2k.c b/drivers/net/dl2k.c index c445457..1a4856b 100644 --- a/drivers/net/dl2k.c +++ b/drivers/net/dl2k.c @@ -1211,24 +1211,17 @@ static int rio_set_settings(struct net_device *dev, struct ethtool_cmd *cmd) if (cmd->autoneg == AUTONEG_ENABLE) { if (np->an_enable) return 0; - else { - np->an_enable = 1; - mii_set_media(dev); - return 0; - } + + np->an_enable = 1; } else { - np->an_enable = 0; - if (np->speed == 1000) { - ethtool_cmd_speed_set(cmd, SPEED_100); - cmd->duplex = DUPLEX_FULL; - printk("Warning!! Can't disable Auto negotiation in 1000Mbps, change to Manual 100Mbps, Full duplex.\n"); - } switch (ethtool_cmd_speed(cmd)) { case SPEED_10: + np->an_enable = 0; np->speed = 10; np->full_duplex = (cmd->duplex == DUPLEX_FULL); break; case SPEED_100: + np->an_enable = 0; np->speed = 100; np->full_duplex = (cmd->duplex == DUPLEX_FULL); break; @@ -1236,9 +1229,9 @@ static int rio_set_settings(struct net_device *dev, struct ethtool_cmd *cmd) default: return -EINVAL; } - mii_set_media(dev); } - return 0; + + return mii_set_media(dev); } static u32 rio_get_link(struct net_device *dev)
The initial version of the driver used to force the link to 100Mbps when auto-negociation was disabled on a 1Gbps link, ignoring the requested link speed. Instead, this change refuses to change anything when it is asked to configure the link speed at 1Gbps without auto-negociation, but acts as requested in all the other cases. IMPORTANT: Previously, the return value from mii_set_media() was ignored. This patch uses it for its own return value. Tested: module compiling, NOT tested on real hardware. Signed-off-by: David Decotigny <decot@google.com> --- drivers/net/dl2k.c | 19 ++++++------------- 1 files changed, 6 insertions(+), 13 deletions(-)