netdev - Re: [PATCH net-next 05/13] ethtool,net/mlx4_en: Add 100M, 20G, 56G speeds ethtool reporting support

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] [day] [month] [year] [list]

Message-ID: <546093AA.8050005@dev.mellanox.co.il>
Date:	Mon, 10 Nov 2014 12:30:02 +0200
From:	Saeed Mahameed <saeedm@....mellanox.co.il>
To:	Ben Hutchings <ben@...adent.org.uk>,
	Amir Vadai <amirv@...lanox.com>
CC:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Yevgeny Petrilin <yevgenyp@...lanox.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: [PATCH net-next 05/13] ethtool,net/mlx4_en: Add 100M, 20G, 56G
 speeds ethtool reporting support


On 11/6/2014 12:38 AM, Ben Hutchings wrote:
> On Mon, 2014-10-27 at 11:37 +0200, Amir Vadai wrote:
>> From: Saeed Mahameed <saeedm@...lanox.com>
>>
>> Added 100M, 20G and 56G ethtool speed reporting support.
>> Update mlx4_en_test_speed self test with the new speeds.
>>
>> Defined new link speeds in include/uapi/linux/ethtool.h:
>> +#define SPEED_20000	20000
>> +#define SPEED_40000	40000
>> +#define SPEED_56000	56000
>>
>> Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
>> Signed-off-by: Amir Vadai <amirv@...lanox.com>
> [...]
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -1213,6 +1213,10 @@ enum ethtool_sfeatures_retval_bits {
>>   #define SUPPORTED_40000baseCR4_Full	(1 << 24)
>>   #define SUPPORTED_40000baseSR4_Full	(1 << 25)
>>   #define SUPPORTED_40000baseLR4_Full	(1 << 26)
>> +#define SUPPORTED_56000baseKR4_Full	(1 << 27)
>> +#define SUPPORTED_56000baseCR4_Full	(1 << 28)
>> +#define SUPPORTED_56000baseSR4_Full	(1 << 29)
>> +#define SUPPORTED_56000baseLR4_Full	(1 << 30)
>>   
>>   #define ADVERTISED_10baseT_Half		(1 << 0)
>>   #define ADVERTISED_10baseT_Full		(1 << 1)
>> @@ -1241,6 +1245,10 @@ enum ethtool_sfeatures_retval_bits {
>>   #define ADVERTISED_40000baseCR4_Full	(1 << 24)
>>   #define ADVERTISED_40000baseSR4_Full	(1 << 25)
>>   #define ADVERTISED_40000baseLR4_Full	(1 << 26)
>> +#define ADVERTISED_56000baseKR4_Full	(1 << 27)
>> +#define ADVERTISED_56000baseCR4_Full	(1 << 28)
>> +#define ADVERTISED_56000baseSR4_Full	(1 << 29)
>> +#define ADVERTISED_56000baseLR4_Full	(1 << 30)
> Can these modes be auto-negotiated?  If not then they don't need
> advertised/supported bits.
Hi Ben,
At least 56000baseKR4_Full and 56000baseCR4_Full can be negotiated for sure,
regarding SR and LR i will check with arch team.
>>   /* The following are all involved in forcing a particular link
>>    * mode for the device for setting things.  When getting the
>> @@ -1248,12 +1256,16 @@ enum ethtool_sfeatures_retval_bits {
>>    * it was forced up into this mode or autonegotiated.
>>    */
>>   
>> -/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE. */
>> +/* The forced speed, 10Mb, 100Mb, gigabit, [2.5|10|20|40|56]GbE. */
>>   #define SPEED_10		10
>>   #define SPEED_100		100
>>   #define SPEED_1000		1000
>>   #define SPEED_2500		2500
>>   #define SPEED_10000		10000
>> +#define SPEED_20000		20000
>> +#define SPEED_40000		40000
>> +#define SPEED_56000		56000
> We shouldn't add new SPEED macros.  The speed is just a number of Mbit/s
> and we don't need to enumerate the possible values.
It is a little bit confusing that some speeds still use the defines 
above, and new speeds should be hardcoded.
Why those speeds still there? is it some kind of work that need to be 
done (i.e remove all of those defines) or are they needed in other 
situations.
I thought it is nice that well-known speeds are organized in such MACROs 
and this will prevent drivers from using magic numbers in their code.

Thanks for your time,
-Saeed.

>
> Ben.
>
>>   #define SPEED_UNKNOWN		-1
>>   
>>   /* Duplex, half or full. */

--
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