Re: [PATCHv2 3/4] ethtool: Use the full 32 bit speed range inethtool's set_settings

[Ben Hutchings]

On Wed, 2011-04-27 at 15:05 -0700, David Decotigny wrote:
> Hi,
> 
> On 04/27/11 12:27, Ben Hutchings wrote:
> >> diff --git a/drivers/net/tulip/de2104x.c b/drivers/net/tulip/de2104x.c
> >> index b13c6b0..f8d26bf 100644
> >> --- a/drivers/net/tulip/de2104x.c
> >> +++ b/drivers/net/tulip/de2104x.c
> >> @@ -1549,10 +1549,11 @@ static int __de_set_settings(struct de_private *de, struct ethtool_cmd *ecmd)
> >>  {
> >>  	u32 new_media;
> >>  	unsigned int media_lock;
> >> +	u32 speed = ethtool_cmd_speed(ecmd);
> >>  
> >> -	if (ecmd->speed != SPEED_10 && ecmd->speed != 5 && ecmd->speed != 2)
> >> +	if (speed != SPEED_10 && speed != 5 && speed != 2)
> >>  		return -EINVAL;
> >> -	if (de->de21040 && ecmd->speed == 2)
> >> +	if (de->de21040 && speed == 2)
> >>  		return -EINVAL;
> >>  	if (ecmd->duplex != DUPLEX_HALF && ecmd->duplex != DUPLEX_FULL)
> >>  		return -EINVAL;
> > [...]
> > 
> > This implementation is absolute crap - it's using speed values as
> > mnemonics for different physical ports!  Please change it to report and
> > accept only speed = 10.  (Both get_settings and set_settings have to be
> > changed at the same time.)
> 
> Well, I am really not sure about this. At first I thought you meant that
> these non standard throughputs were a weird way to switch between
> different ports because of some limitation in the ethtool API at the
> time the driver was written. But looking at the history, this behavior
> is there right from the very first revision of the driver checked in in
> v2.5.0.9
> (http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=b1507c9acd944c8703612c0e38ac580bf9064e8a),
> and the ports could be switched the normal way with ecmd->port.

ethtool was quite new then.  It's not that surprising that people found
odd ways to implement the API - though a little surprising that Jeff did
so, since he was also maintaining ethtool...

> So this
> leads me to think that these speeds are actual speeds and make sense as
> such, they are not a work-around over some ancient ethtool API
> limitation.
[...]

If you care to dig out the datasheets for these chips (DEC 21040 and
21041) you'll see they support 10BASE-T, 10BASE-2 and 10BASE-5.  There's
no such thing as 2M or 5M Ethernet (not counting 'wireless Ethernet').

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/