Message-ID: <20250523145457.07b1e7db@2a02-8428-0f40-1901-f412-2f85-a503-26ba.rev.sfr.net>
Date: Fri, 23 May 2025 14:54:57 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Romain Gantois <romain.gantois@...tlin.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 thomas.petazzoni@...tlin.com, Andrew Lunn <andrew@...n.ch>, Jakub Kicinski
 <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Russell King <linux@...linux.org.uk>,
 linux-arm-kernel@...ts.infradead.org, Christophe Leroy
 <christophe.leroy@...roup.eu>, Herve Codina <herve.codina@...tlin.com>,
 Florian Fainelli <f.fainelli@...il.com>, Heiner Kallweit
 <hkallweit1@...il.com>, Vladimir Oltean <vladimir.oltean@....com>,
 Köry Maincent <kory.maincent@...tlin.com>, Marek
 Behún <kabel@...nel.org>, Oleksij Rempel
 <o.rempel@...gutronix.de>, Nicolò Veronese
 <nicveronese@...il.com>, Simon Horman <horms@...nel.org>,
 mwojtas@...omium.org, Antoine Tenart <atenart@...nel.org>,
 devicetree@...r.kernel.org, Conor Dooley <conor+dt@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Rob Herring <robh@...nel.org>, Daniel Golle
 <daniel@...rotopia.org>, Dimitri Fedrau <dimitri.fedrau@...bherr.com>
Subject: Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP
 handling for PHY drivers

Hi Romain,

On Mon, 12 May 2025 10:38:52 +0200
Romain Gantois <romain.gantois@...tlin.com> wrote:

> Hi Maxime,
> 
> On Wednesday, 7 May 2025 15:53:22 CEST Maxime Chevallier wrote:
> > There are currently 4 PHY drivers that can drive downstream SFPs:
> > marvell.c, marvell10g.c, at803x.c and marvell-88x2222.c. Most of the
> > logic is boilerplate, either calling into generic phylib helpers (for
> > SFP PHY attach, bus attach, etc.) or performing the same tasks with a
> > bit of validation :
> >  - Getting the module's expected interface mode
> >  - Making sure the PHY supports it
> >  - Optionnaly perform some configuration to make sure the PHY outputs
> >    the right mode
> > 
> > This can be made more generic by leveraging the phy_port, and its
> > configure_mii() callback which allows setting a port's interfaces when
> > the port is a serdes.
> > 
> > Introduce a generic PHY SFP support. If a driver doesn't probe the SFP
> > bus itself, but an SFP phandle is found in devicetree/firmware, then the
> > generic PHY SFP support will be used, relying on port ops.
> > 
> > PHY driver need to :
> >  - Register a .attach_port() callback
> >  - When a serdes port is registered to the PHY, drivers must set
> >    port->interfaces to the set of PHY_INTERFACE_MODE the port can output
> >  - If the port has limitations regarding speed, duplex and aneg, the
> >    port can also fine-tune the final linkmodes that can be supported
> >  - The port may register a set of ops, including .configure_mii(), that
> >    will be called at module_insert time to adjust the interface based on
> >    the module detected.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
> > ---
> >  drivers/net/phy/phy_device.c | 107 +++++++++++++++++++++++++++++++++++
> >  include/linux/phy.h          |   2 +
> >  2 files changed, 109 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index aaf0eccbefba..aca3a47cbb66 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1450,6 +1450,87 @@ void phy_sfp_detach(void *upstream, struct sfp_bus
> > *bus) }
> >  EXPORT_SYMBOL(phy_sfp_detach);
> > 
> > +static int phy_sfp_module_insert(void *upstream, const struct sfp_eeprom_id
> > *id) +{
> > +	struct phy_device *phydev = upstream;
> > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > +  
> 
> RCT

Can't be done here, it won't build if in the other order...

> 
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> > +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> > +	phy_interface_t iface;
> > +
> > +	linkmode_zero(sfp_support);
> > +
> > +	if (!port)
> > +		return -EINVAL;
> > +
> > +	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
> > +
> > +	if (phydev->n_ports == 1)
> > +		phydev->port = sfp_parse_port(phydev->sfp_bus, id,   
> sfp_support);
> 
> As mentionned below, this check looks a bit strange to me. Why are we only 
> parsing the SFP port if the PHY device only has one registered port?

Because phydev->port is global to the PHY. If we have another port,
then phydev->port must be handled differently so that SFP insertion /
removal doesn't overwrite what the other port is.

Handling of phydev->port is still fragile in this state of the series,
I'll try to improve on that for V7 and document it better.

> > +
> > +	linkmode_and(sfp_support, port->supported, sfp_support);
> > +
> > +	if (linkmode_empty(sfp_support)) {
> > +		dev_err(&phydev->mdio.dev, "incompatible SFP module   
> inserted\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
> > +
> > +	/* Check that this interface is supported */
> > +	if (!test_bit(iface, port->interfaces)) {
> > +		dev_err(&phydev->mdio.dev, "incompatible SFP module   
> inserted\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (port->ops && port->ops->configure_mii)
> > +		return port->ops->configure_mii(port, true, iface);  
> 
> The name "configure_mii()" seems a bit narrow-scoped to me, as this callback 
> might have to configure something else than a MII link. For example, if a DAC 
> SFP module is inserted, the downstream side of the transciever will have to be 
> configured to 1000Base-X or something similar.

In that regard, you can consider 1000BaseX as a MII mode (we do have
PHY_INTERFACE_MODE_1000BASEX).

> I'd suggest something like "post_sfp_insert()", please let me know what you 
> think.

That's not intended to be SFP-specific though. post_sfp_insert() sounds
lke the narrow-scoped name to me :) Here we are dealing with a PHy that
has a media-side port that isn't a MDI port, but an MII interface like
a MAC would usually export. There may be an SFP here, or something else
entirely :)

One thing though is that this series uses a mix of "is_serdes" and
"configure_mii" to mean pretty-much the same thing, I'll make the names
a bit more homogenous.

> > +
> > +	return 0;
> > +}
> > +
> > +static void phy_sfp_module_remove(void *upstream)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > +
> > +	if (port && port->ops && port->ops->configure_mii)
> > +		port->ops->configure_mii(port, false, PHY_INTERFACE_MODE_NA);
> > +
> > +	if (phydev->n_ports == 1)
> > +		phydev->port = PORT_NONE;  
> 
> This check is a bit confusing to me. Could you please explain why you're only 
> setting the phydev's SFP port to PORT_NONE if the PHY device only has one 
> registered port? Shouldn't this be done regardless?

So that we don't overwrite what the other port would have set :) but,
that's a bit fragile as I said and probably not correct anyways, let me
double-check that.

Maxime

Powered by blists - more mailing lists