Re: [PATCH 2/2] net/dl2k: Don't reconfigure link @100Mbps whendisabling autoneg @1Gbps

[Ben Hutchings]

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@xxxxxxxxxx>
[...]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/