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

[David Decotigny <decot@google.com>]

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. 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. I have the patch ready but I would appreciate any feedback
on this.

Anyway, all your comments are really pertinent and ready to be sent.

Thank you!
Regards,