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

[Alexander Duyck]

On Mon, Mar 31, 2025 at 5:51 AM Russell King (Oracle)
<linux at armlinux.org.uk> wrote:
>
> On Thu, Mar 27, 2025 at 06:16:00PM -0700, Alexander H Duyck wrote:
> > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > > When phylink creates a fixed-link configuration, it finds a matching
> > > linkmode to set as the advertised, lp_advertising and supported modes
> > > based on the speed and duplex of the fixed link.
> > >
> > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > phy_lookup_settings(). This has the side effect that the matched
> > > settings and configured linkmodes may now contain several linkmodes (the
> > > intersection of supported linkmodes from the phylink settings and the
> > > linkmodes that match speed/duplex) instead of the one from
> > > phy_lookup_settings().
> > >
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier at bootlin.com>
> > > ---
> > >  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index cf9f019382ad..8e2b7d647a92 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > >     return phylink_validate_mac_and_pcs(pl, supported, state);
> > >  }
> > >
> > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > +{
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > +}
> > > +
> >
> > Any chance we can go with a different route here than just locking
> > fixed mode to being only for BaseT configurations?
> >
> > I am currently working on getting the fbnic driver up and running and I
> > am using fixed-link mode as a crutch until I can finish up enabling
> > QSFP module support for phylink and this just knocked out the supported
> > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> >
> > Seems like this should really be something handled by some sort of
> > validation function rather than just forcing all devices using fixed
> > link to assume that they are BaseT. I know in our direct attached
> > copper case we aren't running autoneg so that plan was to use fixed
> > link until we can add more flexibility by getting QSFP support going.
>
> Please look back at phylink's historical behaviour. Phylink used to
> use phy_lookup_setting() to locate the linkmode for the speed and
> duplex. That is the behaviour that we should be aiming to preserve.
>
> We were getting:
>
> speed   duplex  linkmode
> 10M     Half    10baseT_Half
> 10M     Full    10baseT_Full
> 100M    Half    100baseT_Half
> 100M    Full    100baseT_Full
> 1G      Half    1000baseT_Half
> 1G      Full    1000baseT_Full (this changed over time)
> 2.5G    Full    2500baseT_Full
> 5G      Full    5000baseT_Full
>
> At this point, things get weird because of the way linkmodes were
> added, as we return the _first_ match. Before commit 3c6b59d6f07c
> ("net: phy: Add more link modes to the settings table"):
>
> 10G     Full    10000baseKR_Full
> Faster speeds not supported
>
> After the commit:
> 10G     Full    10000baseCR_Full
> 20G     Full    20000baseKR2_Full
> 25G     Full    25000baseCR_Full
> 40G     Full    40000baseCR4_Full
> 50G     Full    50000baseCR2_Full
> 56G     Full    56000baseCR4_Full
> 100G    Full    100000baseCR4_Full
>
> It's all a bit random. :(

I agree. I was using pcs_validate to overcome some of that by limiting
myself to *mostly* one link type at each speed. The only spot that got
a bit iffy was the 50G as I support 50GAUI and LAUI-2. I was
overcoming that by tracking the number of lanes expected and filtering
for one or the other.

One thought I had proposed in another email was to look at adding more
data to the fields. In the case of duplex we could overload it to also
be the number of lanes as they represent full duplex lanes anyway.
With that you could sort out some of the CR vs CR2 noise.

In addition I wonder if we shouldn't also look at including port type
in the data. Using that you could essentially go through the
post-validated supported values and OR in the types that belong to the
various link modes for TP, FIBER, DA, ect. That would sort out some of
the KR vs CR vs SR vs LR noise.