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

From: Tariq Toukan <tariqt@mellanox.com>
To: Andrew Lunn <andrew@lunn.ch>, Michal Kubecek <mkubecek@suse.cz>
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>,
	Aya Levin <ayal@mellanox.com>
Subject: Re: [PATCH ethtool] ethtool: Add support for 200Gbps (50Gbps per lane) link mode
Date: Mon, 25 Feb 2019 15:30:13 +0000	[thread overview]
Message-ID: <e34a57bf-af88-c691-e4f0-015555643fd3@mellanox.com> (raw)
In-Reply-To: <20190224194015.GM26626@lunn.ch>

On 2/24/2019 9:40 PM, Andrew Lunn wrote:
>>> 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.
> 
> Hi Michal
> 
> So are you doing a read/modify/write? In that case, off/on makes
> sense. For a pure write, i don't see the need for off/on.
> 
> I've not had to use this much, so i don't know how it is typically
> used. When i have used it, it is because i've got an SFP which can do
> 1G and 2.5G, but the peer can only do 1G. I've needed to remove the
> 2.5G in order to get link. So in that case, read/modify/write with an
> off would make sense.
> 
>      Andrew
> 
>> 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.
> 
> Oh, yes. Easy to miss. Maybe we should consider adding aliases,
> #define SPEED_200G SPEED_200000, and

Totally agree. This will help.

> 
> #define ETHTOOL_LINK_MODE_200GbaseCR4_Full_BIT ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT

In case you do it please consider adding an underscore after the speed, 
so it becomes
ETHTOOL_LINK_MODE_200G_baseCR4_Full_BIT.
I think it's more convenient for the human eye.