On Sun, 27 Jul 2025 10:56:52 +0100
"Russell King (Oracle)" [off-list ref] wrote:

On Tue, Jul 22, 2025 at 02:16:12PM +0200, Maxime Chevallier wrote:

quoted

+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);
+
+	__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);
+
+	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);  

I've been moving phylink away from using sfp_select_interface() because
it requires two stages of translation - one from the module capabilties
to linkmodes, and then linkmodes to interfaces.

sfp_parse_support() now provides the interfaces that the optical module
supports, and the possible interfaces that a copper module _might_
support (but we don't know for certain about that until we discover a
PHY.)

The only place in phylink where this function continues to be used is
when there's an optical module which supports multiple different
speeds, and we need to select it based on the advertising mask provided
by userspace. Everywhere else shouldn't use this function, but should
instead use the interfaces returned from sfp_parse_support().

Thanks for the input, it'll make things even simpler so I'm all in for
that. I'll rework this for the next iteration, thanks a lot for taking
a look at this !

Maxime