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

From: Michal Kubecek <mkubecek@suse.cz>
To: Tariq Toukan <tariqt@mellanox.com>
Cc: "John W. Linville" <linville@tuxdriver.com>,
	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 12:48:28 +0100	[thread overview]
Message-ID: <20190225114828.GD1914@unicorn.suse.cz> (raw)
In-Reply-To: <20190224181104.GA1914@unicorn.suse.cz>

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