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

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

[Tariq Toukan]

From: Aya Levin <ayal@mellanox.com>

Introduce 50Gbps per lane link modes and 200Gbps speed, update print
functions and initialization functions accordingly.
In addition, update related man page accordingly.

Signed-off-by: Aya Levin <ayal@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 ethtool-copy.h | 19 ++++++++++++++++++-
 ethtool.8.in   | 15 +++++++++++++++
 ethtool.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 6bfbb85f9402..de459900b2d3 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -1455,15 +1455,31 @@ enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_FEC_NONE_BIT	= 49,
 	ETHTOOL_LINK_MODE_FEC_RS_BIT	= 50,
 	ETHTOOL_LINK_MODE_FEC_BASER_BIT	= 51,
+	ETHTOOL_LINK_MODE_50000baseKR_Full_BIT			= 52,
+	ETHTOOL_LINK_MODE_50000baseSR_Full_BIT			= 53,
+	ETHTOOL_LINK_MODE_50000baseCR_Full_BIT			= 54,
+	ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT		= 55,
+	ETHTOOL_LINK_MODE_50000baseDR_Full_BIT			= 56,
+	ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT		= 57,
+	ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT		= 58,
+	ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT		= 59,
+	ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT	= 60,
+	ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT		= 61,
+	ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT		= 62,
+	ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT		= 63,
+	ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT	= 64,
+	ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT		= 65,
+	ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT		= 66,
 
 	/* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
 	 * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
 	 * macro for bits > 31. The only way to use indices > 31 is to
 	 * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API.
 	 */
+	ETHTOOL_LINK_MODE_LAST,
 
 	__ETHTOOL_LINK_MODE_LAST
-	  = ETHTOOL_LINK_MODE_FEC_BASER_BIT,
+	  = ETHTOOL_LINK_MODE_LAST - 1,
 };
 
 #define __ETHTOOL_LINK_MODE_LEGACY_MASK(base_name)	\
@@ -1571,6 +1587,7 @@ enum ethtool_link_mode_bit_indices {
 #define SPEED_50000		50000
 #define SPEED_56000		56000
 #define SPEED_100000		100000
+#define SPEED_200000            200000
 
 #define SPEED_UNKNOWN		-1
 
diff --git a/ethtool.8.in b/ethtool.8.in
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
 .TE
 .TP
 .BI phyad \ N
diff --git a/ethtool.c b/ethtool.c
index fb4c0886ca84..1a23be818ec1 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -530,6 +530,21 @@ static void init_global_link_mode_masks(void)
 		ETHTOOL_LINK_MODE_10000baseER_Full_BIT,
 		ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
 		ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+		ETHTOOL_LINK_MODE_50000baseKR_Full_BIT,
+		ETHTOOL_LINK_MODE_50000baseSR_Full_BIT,
+		ETHTOOL_LINK_MODE_50000baseCR_Full_BIT,
+		ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
+		ETHTOOL_LINK_MODE_50000baseDR_Full_BIT,
+		ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT,
+		ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT,
+		ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT,
+		ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT,
+		ETHTOOL_LINK_MODE_100000baseDR2_Full_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,
 	};
 	static const enum ethtool_link_mode_bit_indices
 		additional_advertised_flags_bits[] = {
@@ -689,6 +704,36 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
 		  "2500baseT/Full" },
 		{ 0, ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
 		  "5000baseT/Full" },
+		{ 0, ETHTOOL_LINK_MODE_50000baseKR_Full_BIT,
+		  "50000baseKR/Full" },
+		{ 0, ETHTOOL_LINK_MODE_50000baseSR_Full_BIT,
+		  "50000baseSR/Full" },
+		{ 0, ETHTOOL_LINK_MODE_50000baseCR_Full_BIT,
+		  "50000baseCR/Full" },
+		{ 0, ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
+		  "50000baseLR_ER_FR/Full" },
+		{ 0, ETHTOOL_LINK_MODE_50000baseDR_Full_BIT,
+		  "50000baseDR/Full" },
+		{ 0, ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT,
+		  "100000baseKR2/Full" },
+		{ 0, ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT,
+		  "100000baseSR2/Full" },
+		{ 0, ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT,
+		  "100000baseCR2/Full" },
+		{ 0, ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT,
+		  "100000baseLR2_ER2_FR2/Full" },
+		{ 0, ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT,
+		  "100000baseDR2/Full" },
+		{ 0, ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT,
+		  "200000baseKR4/Full" },
+		{ 0, ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT,
+		  "200000baseSR4/Full" },
+		{ 0, ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT,
+		  "200000baseLR4_ER4_FR4/Full" },
+		{ 0, ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT,
+		  "200000baseDR4/Full" },
+		{ 0, ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT,
+		  "200000baseCR4/Full" },
 	};
 	int indent;
 	int did1, new_line_pend, i;
@@ -2958,6 +3003,13 @@ static int do_sset(struct cmd_context *ctx)
 		else if (speed_wanted == SPEED_10000 &&
 			 duplex_wanted == DUPLEX_FULL)
 			adv_bit = ETHTOOL_LINK_MODE_10000baseT_Full_BIT;
+		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;
 
 		if (adv_bit >= 0) {
 			advertising_wanted = mask_advertising_wanted;
-- 
1.8.3.1

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

[Andrew Lunn]

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

	Andrew
> +			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.

	Andrew

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

[Michal Kubecek]

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

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

[Andrew Lunn]

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

#define ETHTOOL_LINK_MODE_200GbaseCR4_Full_BIT ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT

	Andrew

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

[Michal Kubecek]

On Sun, Feb 24, 2019 at 08:40:15PM +0100, 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.

When using netlink interface, the read/modify/write cycle is limited to
kernel code and is done under rtnl_lock. The netlink interface allows
userspace to send only attributes it wants to change and for bit sets
(like link modes) to tell kernel which bits it wants to change so that
there is no need to read current values first (and open a race window).

When using ioctl interface, ethtool does read the value first even now
as ETHTOOL_SLINKSETTINGS command uses struct ethtool_link_usettings
which has also other members and there is no way to say we only want to
change advertised link modes.

Michal

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

[Andrew Lunn]

> > 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.
> 
> When using netlink interface, the read/modify/write cycle is limited to
> kernel code and is done under rtnl_lock. The netlink interface allows
> userspace to send only attributes it wants to change and for bit sets
> (like link modes) to tell kernel which bits it wants to change so that
> there is no need to read current values first (and open a race window).

Nice.

But it would still be good to get feedback from people who use this
more often. I guess that is top of rack switches with all these odd
link modes.

     Andrew

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

[Tariq Toukan]

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.

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

[Aya Levin]

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

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

[Michal Kubecek]

On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote:
> From: Aya Levin <ayal@mellanox.com>
> 
> Introduce 50Gbps per lane link modes and 200Gbps speed, update print
> functions and initialization functions accordingly.
> In addition, update related man page accordingly.
> 
> Signed-off-by: Aya Levin <ayal@mellanox.com>
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> ---
>  ethtool-copy.h | 19 ++++++++++++++++++-
>  ethtool.8.in   | 15 +++++++++++++++
>  ethtool.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/ethtool-copy.h b/ethtool-copy.h
> index 6bfbb85f9402..de459900b2d3 100644
> --- a/ethtool-copy.h
> +++ b/ethtool-copy.h
> @@ -1455,15 +1455,31 @@ enum ethtool_link_mode_bit_indices {
>  	ETHTOOL_LINK_MODE_FEC_NONE_BIT	= 49,
>  	ETHTOOL_LINK_MODE_FEC_RS_BIT	= 50,
>  	ETHTOOL_LINK_MODE_FEC_BASER_BIT	= 51,
> +	ETHTOOL_LINK_MODE_50000baseKR_Full_BIT			= 52,
> +	ETHTOOL_LINK_MODE_50000baseSR_Full_BIT			= 53,
> +	ETHTOOL_LINK_MODE_50000baseCR_Full_BIT			= 54,
> +	ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT		= 55,
> +	ETHTOOL_LINK_MODE_50000baseDR_Full_BIT			= 56,
> +	ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT		= 57,
> +	ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT		= 58,
> +	ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT		= 59,
> +	ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT	= 60,
> +	ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT		= 61,
> +	ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT		= 62,
> +	ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT		= 63,
> +	ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT	= 64,
> +	ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT		= 65,
> +	ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT		= 66,
>  
>  	/* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
>  	 * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
>  	 * macro for bits > 31. The only way to use indices > 31 is to
>  	 * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API.
>  	 */
> +	ETHTOOL_LINK_MODE_LAST,
>  
>  	__ETHTOOL_LINK_MODE_LAST
> -	  = ETHTOOL_LINK_MODE_FEC_BASER_BIT,
> +	  = ETHTOOL_LINK_MODE_LAST - 1,
>  };

The name ETHTOOL_LINK_MODE_LAST is a bit misleading, maybe it should
rather be called ETHTOOL_LINK_MODE_COUNT. Also, there should be
parentheses around the expression to avoid surprises when
__ETHTOOL_LINK_MODE_LAST is used in an expression.

But this change is not in kernel include/uapi/linux/ethtool.h in
mainline, net or net-next. As ethtool-copy.h is supposed to be a copy of
sanitized kernel uapi header, you should make the change there first and
then sync the header to ethtool.

Michal Kubecek

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

[Michal Kubecek]

On Sun, Feb 24, 2019 at 07:11:04PM +0100, Michal Kubecek wrote:
> On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote:
> > From: Aya Levin <ayal@mellanox.com>
> > 
> > Introduce 50Gbps per lane link modes and 200Gbps speed, update print
> > functions and initialization functions accordingly.
> > In addition, update related man page accordingly.
> > 
> > Signed-off-by: Aya Levin <ayal@mellanox.com>
> > Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> > ---
> >  ethtool-copy.h | 19 ++++++++++++++++++-
> >  ethtool.8.in   | 15 +++++++++++++++
> >  ethtool.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 85 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ethtool-copy.h b/ethtool-copy.h
> > index 6bfbb85f9402..de459900b2d3 100644
> > --- a/ethtool-copy.h
> > +++ b/ethtool-copy.h
> > @@ -1455,15 +1455,31 @@ enum ethtool_link_mode_bit_indices {
> >  	ETHTOOL_LINK_MODE_FEC_NONE_BIT	= 49,
> >  	ETHTOOL_LINK_MODE_FEC_RS_BIT	= 50,
> >  	ETHTOOL_LINK_MODE_FEC_BASER_BIT	= 51,
> > +	ETHTOOL_LINK_MODE_50000baseKR_Full_BIT			= 52,
> > +	ETHTOOL_LINK_MODE_50000baseSR_Full_BIT			= 53,
> > +	ETHTOOL_LINK_MODE_50000baseCR_Full_BIT			= 54,
> > +	ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT		= 55,
> > +	ETHTOOL_LINK_MODE_50000baseDR_Full_BIT			= 56,
> > +	ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT		= 57,
> > +	ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT		= 58,
> > +	ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT		= 59,
> > +	ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT	= 60,
> > +	ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT		= 61,
> > +	ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT		= 62,
> > +	ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT		= 63,
> > +	ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT	= 64,
> > +	ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT		= 65,
> > +	ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT		= 66,
> >  
> >  	/* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
> >  	 * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
> >  	 * macro for bits > 31. The only way to use indices > 31 is to
> >  	 * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API.
> >  	 */
> > +	ETHTOOL_LINK_MODE_LAST,
> >  
> >  	__ETHTOOL_LINK_MODE_LAST
> > -	  = ETHTOOL_LINK_MODE_FEC_BASER_BIT,
> > +	  = ETHTOOL_LINK_MODE_LAST - 1,
> >  };
> 
> The name ETHTOOL_LINK_MODE_LAST is a bit misleading, maybe it should
> rather be called ETHTOOL_LINK_MODE_COUNT. Also, there should be
> parentheses around the expression to avoid surprises when
> __ETHTOOL_LINK_MODE_LAST is used in an expression.
> 
> But this change is not in kernel include/uapi/linux/ethtool.h in
> mainline, net or net-next. As ethtool-copy.h is supposed to be a copy of
> sanitized kernel uapi header, you should make the change there first and
> then sync the header to ethtool.

For the record, net-next tree now has simplified this even more with
commit e728fdf06289 ("net: phy: improve definition of
__ETHTOOL_LINK_MODE_MASK_NBITS").

Michal Kubecek