Message-ID: <20250401094153.43974af2@fedora.home>
Date: Tue, 1 Apr 2025 09:41:53 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>, Andrew Lunn
 <andrew@...n.ch>, davem@...emloft.net, Jakub Kicinski <kuba@...nel.org>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Heiner
 Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
 linux-arm-kernel@...ts.infradead.org, Christophe Leroy
 <christophe.leroy@...roup.eu>, Herve Codina <herve.codina@...tlin.com>,
 Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean
 <vladimir.oltean@....com>, Köry Maincent
 <kory.maincent@...tlin.com>, Oleksij Rempel <o.rempel@...gutronix.de>,
 Simon Horman <horms@...nel.org>, Romain Gantois
 <romain.gantois@...tlin.com>
Subject: Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for
 fixed-link configuration

On Mon, 31 Mar 2025 09:38:34 -0700
Alexander Duyck <alexander.duyck@...il.com> wrote:

> On Mon, Mar 31, 2025 at 9:20 AM Maxime Chevallier
> <maxime.chevallier@...tlin.com> wrote:
> >
> > On Mon, 31 Mar 2025 15:54:20 +0100
> > "Russell King (Oracle)" <linux@...linux.org.uk> wrote:
> >  
> > > On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote:  
> > > > On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote:  
> > > > > A serdes PHY is part of it, but not a traditional twisted pair PHY as
> > > > > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I
> > > > > agree it is a different beast, but are we saying that the fixed-link
> > > > > is supposed to be a twisted pair PHY only?  
> > > >
> > > > With phylink, the PCS enumerates its capabilities, the PHY enumerates
> > > > its capabilities, and the MAC enumerates it capabilities. phylink then
> > > > finds the subset which all support.
> > > >
> > > > As i said, historically, fixed_link was used in place of a PHY, since
> > > > it emulated a PHY. phylinks implementation of fixed_link is however
> > > > different. Can it be used in place of both a PCS and a PHY? I don't
> > > > know.  
> > >
> > > In fixed-link mode, phylink will use a PCS if the MAC driver says there
> > > is one, but it will not look for a PHY.  
> 
> Admittedly the documentation does reference much lower speeds as being
> the use case. I was a bit of an eager beaver and started assembling
> things without really reading the directions. I just kind of assumed
> what I could or couldn't get away with within the interface.
> 
> > > > You are pushing the envelope here, and maybe we need to take a step
> > > > back and consider what is a fixed link, how does it fit into the MAC,
> > > > PCS, PHY model of enumeration? Maybe fixed link should only represent
> > > > the PHY and we need a second sort of fixed_link object to represent
> > > > the PCS? I don't know?  
> > >
> > > As I previously wrote today in response to an earlier email, the
> > > link modes that phylink used were the first-match from the old
> > > settings[] array in phylib which is now gone. This would only ever
> > > return _one_ link mode, which invariably was a baseT link mode for
> > > the slower speeds.
> > >
> > > Maxime's first approach at adapting this to his new system was to
> > > set every single link mode that corresponded with the speed. I
> > > objected to that, because it quickly gets rediculous when we end
> > > up with lots of link modes being indicated for e.g. 10, 100M, 1G
> > > but the emulated PHY for these speeds only indicates baseT. That's
> > > just back-compatibility but... in principle changing the link modes
> > > that are reported to userspace for a fixed link is something we
> > > should not be doing - we don't know if userspace tooling has come
> > > to rely on that.
> > >
> > > Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X
> > > interface mode, but that's what we've always done in the past and
> > > phylink was coded to maintain that (following the principle that
> > > we shouldn't do gratuitous changes to the information exposed to
> > > userspace.)
> > >
> > > Maxime's replacement approach is to just expose baseT, which
> > > means that for the speeds which do not have a baseT mode, we go
> > > from supporting it but with a weird link mode (mostly baseCR*)
> > > based on first-match in the settings[] table, to not supporting the
> > > speed.  
> >
> > I very wrongfully considered that there was no >10G fixed-link users, I
> > plan to fix that with something like the proposed patch in the
> > discussion, that reports all linkmodes for speeds above 10G (looks less
> > like a randomly selected mode, you can kind-of see what's going on as
> > you get all the linkmodes) but is a change in what we expose to
> > userspace.  
> 
> I am not sure if there are any >10G users. I haven't landed anything
> in the kernel yet and like I said what I was doing was more of a hack
> to enable backwards compatibility on older kernels w/ the correct
> supported and advertised modes. If I have to patch one kernel to make
> it work for me that would be manageable.
> 
> One thing I was thinking about that it looks like this code might
> prevent would be reinterpreting the meaning of duplex. Currently we
> only have 3 values for it 0 (half), 1 (Full), and ~0 (Unknown). One
> thought I had is that once we are over 1G we don't really care about
> that anymore as everything is Full duplex and instead care about
> lanes. As it turns out the duplex values currently used would work
> well to be extended out to lanes. Essentially 0 would still be half, 1
> would be 1 lane full duplex, 2-8 could be the number of full duplex
> lanes the interface is using, and unknown lane count would still be ~0
> since it is unlikely we will end up with anything other than a power
> of 2 number of lanes anyway. With that you could greatly sort out a
> number of modes in your setup. We would then have to do some cleanups
> here and there to do something like "duplex == DUPLEX_UNKNOWN ? duplex
> : !!duplex" to clean up any cases where the legacy values are
> expected.

Funny you say that, the phy_port work I was referring to with the
mediums introduction also tracks the lanes for a given port 

previous outdated iteration here
: https://lore.kernel.org/netdev/20250213101606.1154014-4-maxime.chevallier@bootlin.com/

The idea is to represent physical interfaces to a NIC, as NICs can have
multiple ports for different types of connectors. Ports would be driven
by either a PHY, a NIC directly or an SFP.

The port contains, among other things, speed, duplex, n_lanes, and the
medium (with more granularity than just TP/Backplane/Fiber, as we list
the 802.3 media : BaseS / BaseC / BaseK / BaseT ...)

We already have information about whickh linkmode uses how many lanes,
but we don't do much with that information besides reporting it to
userspace.

I'll revamp the whole thing for the next iteration, however I really
only have a deep-ish understanding of embedded usecases, and I lack
insights on other classes of devices such as what your working on.

I'll be very happy to get your feedback on it, I plan to send that when
net-next reopens.

Maxime

Powered by blists - more mailing lists