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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190703073802.GJ20101@unicorn.suse.cz>
Date:   Wed, 3 Jul 2019 09:38:02 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        David Miller <davem@...emloft.net>,
        Jiri Pirko <jiri@...nulli.us>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Johannes Berg <johannes@...solutions.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 11/15] ethtool: provide link mode names as a
 string set

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),
> > > +};  
>