Re: [PATCH net-next v6 11/15] ethtool: provide link mode names as a string set

[Michal Kubecek <mkubecek@suse.cz>]

On Tue, Jul 02, 2019 at 07:11:24PM -0700, Jakub Kicinski wrote:
> On Tue, 2 Jul 2019 19:04:19 -0700, Jakub Kicinski wrote:
> > On Tue,  2 Jul 2019 13:50:34 +0200 (CEST), Michal Kubecek wrote:
> > > +const char *const link_mode_names[] = {
> > > +	__DEFINE_LINK_MODE_NAME(10, T, Half),
> > > +	__DEFINE_LINK_MODE_NAME(10, T, Full),
> > > +	__DEFINE_LINK_MODE_NAME(100, T, Half),
> > > +	__DEFINE_LINK_MODE_NAME(100, T, Full),
> > > +	__DEFINE_LINK_MODE_NAME(1000, T, Half),
> > > +	__DEFINE_LINK_MODE_NAME(1000, T, Full),
> > > +	__DEFINE_SPECIAL_MODE_NAME(Autoneg, "Autoneg"),
> > > +	__DEFINE_SPECIAL_MODE_NAME(TP, "TP"),
> > > +	__DEFINE_SPECIAL_MODE_NAME(AUI, "AUI"),
> > > +	__DEFINE_SPECIAL_MODE_NAME(MII, "MII"),
> > > +	__DEFINE_SPECIAL_MODE_NAME(FIBRE, "FIBRE"),
> > > +	__DEFINE_SPECIAL_MODE_NAME(BNC, "BNC"),  
> > 
> > > +	__DEFINE_LINK_MODE_NAME(10000, T, Full),
> > > +	__DEFINE_SPECIAL_MODE_NAME(Pause, "Pause"),
> > > +	__DEFINE_SPECIAL_MODE_NAME(Asym_Pause, "Asym_Pause"),
> > > +	__DEFINE_LINK_MODE_NAME(2500, X, Full),
> > > +	__DEFINE_SPECIAL_MODE_NAME(Backplane, "Backplane"),
> > > +	__DEFINE_LINK_MODE_NAME(1000, KX, Full),  
> > ...
> > > +	__DEFINE_LINK_MODE_NAME(5000, T, Full),
> > > +	__DEFINE_SPECIAL_MODE_NAME(FEC_NONE, "None"),
> > > +	__DEFINE_SPECIAL_MODE_NAME(FEC_RS, "RS"),
> > > +	__DEFINE_SPECIAL_MODE_NAME(FEC_BASER, "BASER"),  
> > 
> > Why are port types and FEC params among link mode strings?
> 
> Ah, FEC for autoneg, but port type?

The bits in supported bitmap are used to pass information which port
types the device supports (but the information which port is selected
is passed in a different way :-( ). It is used by ethtool to provide the
"Supported ports:" line in "ethtool <dev>" output.

I don't like this design where link modes are mixed with few different
and only loosely related bitmaps. Maybe it would be cleaner to split it
into multiple bitmaps and later change the backend (ethtool_ops) too and
only translate to/from this combined bitmap for legacy ioctl interface.

Michal

> 
> > > +	__DEFINE_LINK_MODE_NAME(50000, KR, Full),  
> > ...
> > > +	__DEFINE_LINK_MODE_NAME(1000, T1, Full),
> > > +};  
>