From: Aya Levin <ayal@mellanox.com>
To: Michal Kubecek <mkubecek@suse.cz>, Andrew Lunn <andrew@lunn.ch>
Cc: Tariq Toukan <tariqt@mellanox.com>,
"John W. Linville" <linville@tuxdriver.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Eran Ben Elisha <eranbe@mellanox.com>
Subject: Re: [PATCH ethtool] ethtool: Add support for 200Gbps (50Gbps per lane) link mode
Date: Mon, 25 Feb 2019 12:42:44 +0000 [thread overview]
Message-ID: <110b268a-e5c7-8e2c-04d3-00d8ac1231c5@mellanox.com> (raw)
In-Reply-To: <20190224184046.GB1914@unicorn.suse.cz>
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@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
next prev parent reply other threads:[~2019-02-25 12:42 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-24 15:08 [PATCH ethtool] ethtool: Add support for 200Gbps (50Gbps per lane) link mode Tariq Toukan
2019-02-24 16:47 ` Andrew Lunn
2019-02-24 18:40 ` Michal Kubecek
2019-02-24 19:40 ` Andrew Lunn
2019-02-24 20:33 ` Michal Kubecek
2019-02-24 20:43 ` Andrew Lunn
2019-02-25 15:30 ` Tariq Toukan
2019-02-25 12:42 ` Aya Levin [this message]
2019-02-24 18:11 ` Michal Kubecek
2019-02-25 11:48 ` Michal Kubecek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=110b268a-e5c7-8e2c-04d3-00d8ac1231c5@mellanox.com \
--to=ayal@mellanox.com \
--cc=andrew@lunn.ch \
--cc=eranbe@mellanox.com \
--cc=linville@tuxdriver.com \
--cc=mkubecek@suse.cz \
--cc=netdev@vger.kernel.org \
--cc=tariqt@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.