Message-ID: <CAKgT0UdhTT=g+ODpzR5uoTEOkC8u+cfCp7H-8718Zphd=24buw@mail.gmail.com>
Date: Thu, 3 Apr 2025 14:53:22 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Maxime Chevallier <maxime.chevallier@...tlin.com>, 
	"Russell King (Oracle)" <linux@...linux.org.uk>, 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 3, 2025 at 9:34 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> 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.

Another issue is that how you would define the connection between the
two endpoints is changing. Maxime is basing his data off of
speed/duplex however to source that he is pulling data from
link_mode_params that is starting to broaden including things like
lanes. I really think going forward lanes is going to start playing a
role as we get into the higher speeds and it is already becoming a
standard config item to use to strip out unsupported modes when
configuring the interface via autoneg.

> 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.

I am wondering about that. I know I specified we were XLGMII for fbnic
but that has proven problematic since we aren't actually 40G. So we
are still essentially just reporting link up/down using that. That is
why I was looking at going with a fixed mode as I can at least specify
the correct speed duplex for the one speed I am using if I want to use
ethtool_ksettings_get.

I have a patch to add the correct phy_interface_t modes for 50, and
100G links. However one thing I am seeing is that after I set the
initial interface type I cannot change the interface type without the
SFP code added. One thing I was wondering. Should I just ignore the
phy_interface_t on the pcs_config call and use the link mode mask
flags in autoneg and the speed/duplex/lanes in non-autoneg to
configure the link? It seems like that is what the SFP code itself is
doing based on my patch 2 in the set.

> 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?

How is it not a fixed link? If anything it was going to be more fixed
than what you described above. In our case the connection type is
indicated by the FW and we aren't meant to change it unless we want to
be without a link. The link goes up and down and that would be about
it. So we essentially advertise one link mode bit and are locked on
the interface configured at creation. I was basically taking that
config from the FW, creating a SW node with it, and then using that to
set up the link w/o permitting changes. The general idea was that I
would use that to limp along until I could get the QSFP support added
and then add support for configuring the link with the
ethtool_ksettings_set call. Mainly we just need that functionality for
our own testing as the production case is non-autoneg fixed links
only.

Powered by blists - more mailing lists