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

[Michal Kubecek <mkubecek@suse.cz>]

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