Re: [net PATCH 1/2] net: phy: Cleanup handling of recent changes to phy_lookup_setting

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Apr 01, 2025 at 02:30:06PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> The blamed commit introduced an issue where it was limiting the link
> configuration so that we couldn't use fixed-link mode for any settings
> other than twisted pair modes 10G or less. As a result this was causing the
> driver to lose any advertised/lp_advertised/supported modes when setup as a
> fixed link.
> 
> To correct this we can add a check to identify if the user is in fact
> enabling a TP mode and then apply the mask to select only 1 of each speed
> for twisted pair instead of applying this before we know the number of bits
> set.
> 
> Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> ---
>  drivers/net/phy/phylink.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> 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);
> +	}
> +

I'm still wondering about the wiseness of exposing more than one link
mode for something that's supposed to be fixed-link.

For gigabit fixed links, even if we have:

	phy-mode = "1000base-x";
	speed = <1000>;
	full-duplex;

in DT, we still state to ethtool:

        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  1000baseT/Full
        Link partner advertised auto-negotiation: No
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on

despite it being a 1000base-X link. This is perfectly reasonable,
because of the origins of fixed-links - these existed as a software
emulated baseT PHY no matter what the underlying link was.

So, is getting the right link mode for the underlying link important
for fixed-links? I don't think it is. Does it make sense to publish
multiple link modes for a fixed-link? I don't think it does, because
if multiple link modes are published, it means that it isn't fixed.

As for arguments about the number of lanes, that's a property of the
PHY_INTERFACE_MODE_xxx. There's a long history of this, e.g. MII/RMII
is effectively a very early illustration of reducing the number of
lanes, yet we don't have separate link modes for these.

So, I'm still uneasy about this approach.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!