linux-kernel - Re: [PATCH net-next v3 08/47] net: phylink: Support differing link speeds and interface speeds

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]

Date:   Mon, 18 Jul 2022 11:49:25 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>, netdev@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        linux-arm-kernel@...ts.infradead.org,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org,
        Alexandru Marginean <alexandru.marginean@....com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH net-next v3 08/47] net: phylink: Support differing link
 speeds and interface speeds

On 7/16/22 9:26 PM, Andrew Lunn wrote:
>> > This seem error prone when new PHY_INTERFACE_MODES are added. I would
>> > prefer a WARN_ON_ONCE() in the default: so we get to know about such
>> > problems.
>> 
>> Actually, this is the reason I did not add a default: clause to the
>> switch (and instead listed everything out). If a new interface mode is
>> added, there will be a warning (as I discovered when preparing this
>> patch).
> 
> Ah, the compiler produces a warning. O.K. that is good. Better than an
> WARN_ON_ONCE at runtime.
> 
>> > Bike shedding a bit, but would it be better to use host_side_speed and
>> > line_side_speed? When you say link_speed, which link are your
>> > referring to? Since we are talking about the different sides of the
>> > PHY doing different speeds, the naming does need to be clear.
>> When I say "link" I mean the thing that the PMD speaks. That is, one of
>> the ethtool link mode bits. I am thinking of a topology like
>> 
>> 
>> MAC (+PCS) <-- phy interface mode (MII) --> phy <-- link mode --> far-end phy
>> 
>> The way it has been done up to now, the phy interface mode and the link
>> mode have the same speed. For some MIIs, (such as MII or GMII) this is
>> actually the case, since the data clock changes depending on the data
>> speed. For others (SGMII/USXGMII) the data is repeated, but the clock
>> rate stays the same. In particular, the MAC doesn't care what the actual
>> link speed is, just what configuration it has to use (so it selects the
>> right clock etc).
>> 
>> The exception to the above is when you have no phy (such as for
>> 1000BASE-X):
>> 
>> MAC (+PCS) <-- MDI --> PMD <-- link mode --> far-end PMD
>> 
>> All of the phy interface modes which can be used this way are
>> "non-adaptive." That is, in the above case they have a fixed speed.
>> 
>> That said, I would like to keep the "phy interface mode speed" named
>> "speed" so I don't have to write up a semantic patch to rename it in all
>> the drivers.
> 
> So you want phydev->speed to be the host side speed. That leaves the
> line side speed as a new variable, so it can be called line_side_speed?
> 
> I just find link_speed ambiguous, and line_side_speed less so.

I would rather use something with "link" to match up with
ETHTOOL_LINK_MODE_*. Ideally "speed" would be something like
"interface_speed" to match up with PHY_INTERFACE_MODE_*.

> The documentation for phydev->speed needs updating to make it clear it
> is the host side speed.

OK

--Sean