Message-ID: <de19e9f1-4ae3-4193-981c-e366c243352d@lunn.ch>
Date: Thu, 3 Apr 2025 18:34:20 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>,
	Alexander Duyck <alexander.duyck@...il.com>, netdev@...r.kernel.org,
	hkallweit1@...il.com, davem@...emloft.net, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [net PATCH 1/2] net: phy: Cleanup handling of recent changes to
 phy_lookup_setting

On Thu, Apr 03, 2025 at 05:29:53PM +0200, Maxime Chevallier wrote:
> On Thu, 3 Apr 2025 15:55:45 +0100
> "Russell King (Oracle)" <linux@...linux.org.uk> wrote:
> 
> > On Tue, Apr 01, 2025 at 02:30:06PM -0700, Alexander Duyck wrote:
> > > From: Alexander Duyck <alexanderduyck@...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@...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.
> 
> That's a good point. The way I saw that was :
> 
>   "we report all the modes because, being fixed-link, it can be
>   any of these modes."
> 
> But I agree with you in that this doesn't show that "this is fixed,
> don't try to change that, this won't work". So, I do agree with you now.
> 
> > 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.
> 
> So, how about extending the compat list of "first link of each speed"
> to all the modes, then once the "mediums" addition from the phy_port
> lands, we simplify it down the following way :
> 
> Looking at the current list of elegible fixed-link linkmodes, we have
> (I'm taking this from one of your mails) :
> 
> 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
> 10G	Full	10000baseCR_Full (used to be 10000baseKR_Full)
> 20G	Full	20000baseKR2_Full => there's no 20GBaseCR*
> 25G	Full	25000baseCR_Full
> 40G	Full	40000baseCR4_Full
> 50G	Full	50000baseCR2_Full
> 56G	Full	56000baseCR4_Full
> 100G	Full	100000baseCR4_Full
> 
> To avoid maintaining a hardcoded list, we could clearly specifying
> what we report in fixed-link :
> 
>  1 : Any BaseT mode for the given speed duplex (BaseT and not BaseT1)
>  2 : If there's none, Any BaseK mode for that speed/duplex
>  3 : If there's none, Any BaseC mode for that speed/duplex
> 
> That's totally arbitrary of course, and if one day someone adds, say,
> 25GBaseT, fixed-link linkmode will change. Another issue us 10G,
> 10GBaseT exists, but wasn't the first choice.

Maybe go back to why fixed-link exists? It is basically a hack to make
MAC configuration easier. It was originally used for MAC to MAC
connections, e.g. a NIC connected to a switch, without PHYs in the
middle. By faking a PHY, there was no need to add any special
configuration API to the MAC, the phylib adjust_link callback would be
sufficient to tell the MAC to speed and duplex to use. For {R}{G}MII,
or SGMII, that is all you need to know. The phy-mode told you to
configure the MAC to MII, GMII, SGMII.

But things evolved since then. We started having PHYs which change
their host side depending on their media side. SGMII for <= 1G,
2500BaseX, 5GBaseX, 10GBaseX. It became necessary for the adjust_link
callback to look at more than just the speed/duplex, it also needed to
look at the phy_interface_t. phy-mode looses its meaning, it might be
considered the default until we know better.

But consider the use case, a hack to allow configuration of a MAC to
MAC connection. The link mode does not change depending on the media,
there is no media. The switch will not be changing its port
configuration. The link really is fixed. phy-mode tells you the basic
configuration, and then adjust_link/mac_link_up tells you the
speed/dupex if there are multiple speeds/duplex supported,
e.g. RGMII/SGMII.

What Alex is trying to do is abuse fixed link for something which is
not a MAC-MAC connection, something which is not fixed. Do we want to
support that?

	Andrew

Powered by blists - more mailing lists