[PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Alexander Duyck]

On Mon, Mar 31, 2025 at 9:20 AM Maxime Chevallier
<maxime.chevallier at bootlin.com> wrote:
>
> On Mon, 31 Mar 2025 15:54:20 +0100
> "Russell King (Oracle)" <linux at armlinux.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.

Likewise if you were to look at adding the port type that might allow
for further division and cleanup. With that someone could specify the
speed, duplex, and port type and they would be able to pretty
precisely pick out a specific fixed mode.

> Or maybe simpler, I could extend the list of compat fixed-link linkmodes
> to all speeds with the previous arbitrary values that Russell listed in
> the other mail (that way, no user-visible changes :) )
>
> I was hoping Alexander could give option 1 a try, but let me know if
> you think we should instead adopt option 2, which is probably the safer
> on.

I can try to get to it, but I have a number of meetings today so I may
not be able to get to it until tomorrow morning.

Also I suspect this may have an impact outside of just the fixed link
setup. I will have to try some other spots to see if I see anything
odd pop up as I suspect that I will have issues with 50R2/50R running
over top of each other after these changes.

Thanks,

- Alex