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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 27 Apr 2011 15:05:09 -0700
From:	David Decotigny <decot@...gle.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	"David S. Miller" <davem@...emloft.net>, mirq-linux@...e.qmqm.pl,
	Stanislaw Gruszka <sgruszka@...hat.com>,
	Alexander Duyck <alexander.h.duyck@...el.com>,
	Eilon Greenstein <eilong@...adcom.com>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCHv2 3/4] ethtool: Use the full 32 bit speed range in ethtool's
 set_settings

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,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html