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 <a...@mellanox.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
