Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Romain Gantois <romain.gantois@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 6691 bytes --]

On Friday, 23 May 2025 14:54:57 CEST Maxime Chevallier wrote:
> Hi Romain,
> 
> On Mon, 12 May 2025 10:38:52 +0200
> 
> Romain Gantois <romain.gantois@bootlin.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@bootlin.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...
> 

You could always separate the declaration from the assignment, I've seen that 
done quite a lot to keep things in RCT.

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

Okay, I see, thanks for explaining.

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

Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes you're 
right.

> > 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 :)
> 

Is that callback really not meant to be SFP-specific? It's only called from 
phy_sfp_module_insert() though.

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

Sure, sounds good.

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

All right, that makes sense given what you've already told me.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]