netdev - Re: [PATCH ethtool] ethtool: Add support for 200Gbps (50Gbps per lane) link mode

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 for Android: free password hash cracker in your pocket

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

Message-ID: <110b268a-e5c7-8e2c-04d3-00d8ac1231c5@mellanox.com>
Date:   Mon, 25 Feb 2019 12:42:44 +0000
From:   Aya Levin <ayal@...lanox.com>
To:     Michal Kubecek <mkubecek@...e.cz>, Andrew Lunn <andrew@...n.ch>
CC:     Tariq Toukan <tariqt@...lanox.com>,
        "John W. Linville" <linville@...driver.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Eran Ben Elisha <eranbe@...lanox.com>
Subject: Re: [PATCH ethtool] ethtool: Add support for 200Gbps (50Gbps per
 lane) link mode



On 2/24/2019 8:40 PM, Michal Kubecek wrote:
> On Sun, Feb 24, 2019 at 05:47:51PM +0100, Andrew Lunn wrote:
>> On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote:
>>> From: Aya Levin <ayal@...lanox.com>
>>> index 5a26cff5fb33..64ce0711ad5f 100644
>>> --- a/ethtool.8.in
>>> +++ b/ethtool.8.in
>>> @@ -650,6 +650,11 @@ lB	l	lB.
>>>   0x400000000	50000baseCR2 Full
>>>   0x800000000	50000baseKR2 Full
>>>   0x10000000000	50000baseSR2 Full
>>> +0x10000000000000	50000baseKR Full
>>> +0x20000000000000	50000baseSR Full
>>> +0x40000000000000	50000baseCR Full
>>> +0x80000000000000	50000baseLR_ER_FR Full
>>> +0x100000000000000	50000baseDR Full
>>>   0x8000000	56000baseKR4 Full
>>>   0x10000000	56000baseCR4 Full
>>>   0x20000000	56000baseSR4 Full
>>> @@ -658,6 +663,16 @@ lB	l	lB.
>>>   0x2000000000	100000baseSR4 Full
>>>   0x4000000000	100000baseCR4 Full
>>>   0x8000000000	100000baseLR4_ER4 Full
>>> +0x200000000000000	100000baseKR2 Full
>>> +0x400000000000000	100000baseSR2 Full
>>> +0x800000000000000	100000baseCR2 Full
>>> +0x1000000000000000	100000baseLR2_ER2_FR2 Full
>>> +0x2000000000000000	100000baseDR2 Full
>>> +0x4000000000000000	200000baseKR4 Full
>>> +0x8000000000000000	200000baseSR4 Full
>>> +0x10000000000000000	200000baseLR4_ER4_FR4 Full
>>> +0x20000000000000000	200000baseDR4 Full
>>> +0x40000000000000000	200000baseCR4 Full
>>
>> This is getting less friendly all the time, and it was never very
>> friendly to start with. We have the strings which represent these link
>> modes in the table used for dumping caps. How about allowing the user
>> to list a comma separate list of modes.
>>
>> ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full
> 
> In my preliminary netlink patchset, I'm adding support for e.g.
> 
>    ethtool -s eth0 advertise 100baseT/Half off 1000baseT/Full on
> 
> I'm not sure what would be more useful, if switching individual modes or
> listing the whole set. After all, we can also support both. But I fully
> agree that the numeric bitmaps are already too inconvenient.
> 
>>> +			adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT &
>>> +				  ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT &
>>> +				  ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT &
>>> +				  ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT &
>>> +				  ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT;
>>
>> Maybe i'm wrong, but this looks odd.
>>
>> enum ethtool_link_mode_bit_indices {
>>          ETHTOOL_LINK_MODE_10baseT_Half_BIT      = 0,
>>          ETHTOOL_LINK_MODE_10baseT_Full_BIT      = 1,
>>          ETHTOOL_LINK_MODE_100baseT_Half_BIT     = 2,
>>          ETHTOOL_LINK_MODE_100baseT_Full_BIT     = 3,
>>          ETHTOOL_LINK_MODE_1000baseT_Half_BIT    = 4,
>>          ETHTOOL_LINK_MODE_1000baseT_Full_BIT    = 5,
>>
>> These are numbers, not bitmasks, so & them together does not look
>> correct.
> 
> Yes, this is wrong. Even if bit masks were used, the operator should be
> "|" but here adv_bit is interpreted as an index, not mask. It's obvious
> this part of the code was designed when speed and duplex identified the
> mode uniquely which is no longer the case. (Which is probably also why
> there are no branches for speeds over 10G.)
> 
> And there is another problem:
> 
> +		else if (speed_wanted == SPEED_20000 &&
> +			 duplex_wanted == DUPLEX_FULL)
> +			adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT &
> +				  ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT &
> +				  ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT &
> +				  ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT &
> +				  ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT;
> 
> The test is for SPEED_20000 but then 200G modes are added.
> 
> Michal
> 
Thanks Michal and Andrew for your comments - I will fix them in the next 
version.
The code section (setting advertisement of 200G) will be removed 
completely, advertisement of 200G will be set to the supported 
link-modes that is handled below.
In addition I will make sure ethtool-copy.h is synced with current 
kernel version without additions.
As for the man page, I agree with you completely - I thought of 
replacing the LONG hex values with BIT(x) but since this can not be 
applied in the command line - I didn't implement it.

Aya