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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

On Tue, 01 Apr 2025 08:28:29 -0700
Alexander H Duyck <alexander.duyck@gmail.com> wrote:

> On Mon, 2025-03-31 at 09:14 +0200, Maxime Chevallier wrote:
> > On Fri, 28 Mar 2025 14:03:54 -0700
> > Alexander Duyck <alexander.duyck@gmail.com> wrote:
> >   
> > > On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
> > > <maxime.chevallier@bootlin.com> wrote:  
> > > > 
> > > > On Thu, 27 Mar 2025 18:16:00 -0700
> > > > Alexander H Duyck <alexander.duyck@gmail.com> 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@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.    
> > > > 
> > > > These baseT mode were for compatibility with the previous way to deal
> > > > with fixed links, but we can extend what's being report above 10G with
> > > > other modes. Indeed the above code unnecessarily restricts the
> > > > linkmodes. Can you tell me if the following patch works for you ?
> > > > 
> > > > Maxime
> > > > 
> > > > -----------
> > > > 
> > > > From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> > > > From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > Date: Fri, 28 Mar 2025 08:53:00 +0100
> > > > Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
> > > > 
> > > > The blamed commit introduced a helper to keep the linkmodes reported by
> > > > fixed links identical to what they were before phy_caps. This means
> > > > filtering linkmodes only to report BaseT modes.
> > > > 
> > > > Doing so, it also filtered out any linkmode above 10G. Reinstate the
> > > > reporting of linkmodes above 10G, where we report any linkmodes that
> > > > exist at these speeds.
> > > > 
> > > > Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> > > > Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> > > > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 17 +++++++++++++----
> > > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index 69ca765485db..de90fed9c207 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -715,16 +715,25 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> > > >                 phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
> > > >                              pl->link_config.speed);
> > > > 
> > > > -       linkmode_zero(pl->supported);
> > > > -       phylink_fill_fixedlink_supported(pl->supported);
> > > > +       linkmode_fill(pl->supported);
> > > > 
> > > >         linkmode_copy(pl->link_config.advertising, pl->supported);
> > > >         phylink_validate(pl, pl->supported, &pl->link_config);
> > > > 
> > > > +       phylink_fill_fixedlink_supported(match);
> > > > +    
> > > 
> > > I worry that this might put you back into the same problem again with
> > > the older drivers. In addition it is filling in modes that shouldn't
> > > be present after the validation.  
> > 
> > Note that I'm not directly filling pl->supported here.
> > 
> > [...]
> > 
> >  	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
> >  			    pl->supported, true);
> > -	if (c)
> > -		linkmode_and(match, pl->supported, c->linkmodes);
> > +	if (c) {
> > +		/* Compatbility with the legacy behaviour : Report one single
> > +		 * BaseT mode for fixed-link speeds under or at 10G, or all
> > +		 * linkmodes at the speed/duplex for speeds over 10G
> > +		 */
> > +		if (linkmode_intersects(match, c->linkmodes))
> > +			linkmode_and(match, match, c->linkmodes);
> > +		else
> > +			linkmode_copy(match, c->linkmodes);
> > +	}
> > 
> > [...]
> > 
> > 	if (c) {
> > 		linkmode_or(pl->supported, pl->supported, match);
> > 		linkmode_or(pl->link_config.lp_advertising,
> > 			    pl->link_config.lp_advertising, match);
> > 	}
> > 
> > For speeds above 10G, you will get all the modes for the requested
> > speed, so it should solve the issue in the next steps of your code
> > where you need matching linkmodes for your settings ? Did you give it a
> > try ?
> > 
> > You may end up with too many linkmodes, but for fixed-link we don't
> > really expect these modes to precisely represent any real linkmodes  
> 
> Here is more the quick-n-dirty approach that I think does what you were
> trying to do without breaking stuff:
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 16a1f31f0091..380e51c5bdaa 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -713,17 +713,24 @@ static int phylink_parse_fixedlink(struct phylink *pl,
>                 phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
>                              pl->link_config.speed);
>  
> -       linkmode_zero(pl->supported);
> -       phylink_fill_fixedlink_supported(pl->supported);
> -
> +       linkmode_fill(pl->supported);
>         linkmode_copy(pl->link_config.advertising, pl->supported);
>         phylink_validate(pl, pl->supported, &pl->link_config);
>  
>         c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
>                             pl->supported, true);
> -       if (c)
> +       if (c) {
>                 linkmode_and(match, pl->supported, c->linkmodes);
>  
> +               /* Compatbility with the legacy behaviour:
> +                * Report one single BaseT mode.
> +                */
> +               phylink_fill_fixedlink_supported(mask);
> +               if (linkmode_intersects(match, mask))
> +                       linkmode_and(match, match, mask);
> +               linkmode_zero(mask);
> +       }
> +
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
> 
> Basically we still need the value to be screened by the pl->supported.
> The one change is that we have to run the extra screening on the
> intersect instead of skipping the screening, or doing it before we even
> start providing bits.
> 
> With this approach we will even allow people to use non twisted pair
> setups regardless of speed as long as they don't provide any twisted
> pair modes in the standard set.
> 
> I will try to get this tested today and if it works out I will submit
> it for net. I just need to test this and an SFP ksettings_set issue I
> found when we aren't using autoneg.

That looks correct and indeed better than my patch, thanks for that :)

Feel free to send it indeed, I'll give it a try on the setups I have
here as well

Maxime