Re: [PATCH net-next 08/13] net: phylink: Represent PHY-less SFP modules with phy_port

Romain Gantois
2026-01-29T08:58:50Z

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

On Tuesday, 27 January 2026 14:41:56 CET Maxime Chevallier wrote:
...
> @@ -1786,13 +1787,31 @@ static int phylink_create_sfp_port(struct phylink
> *pl) else
>
> pl->sfp_bus_port = port;
>
> + if (pl->mod_port) {
> + ret = phy_link_topo_add_port(pl->netdev, pl->mod_port);
> + if (ret)
> + goto out_bus_port;
> + }
> +
> + return 0;
> +out_bus_port:
> + phy_link_topo_del_port(pl->netdev, port);

This seems strange to me. Why clean up after phy_link_topo_add_port() if it
returned an error code? Presumably phy_link_topo_add_port() cleans up after
itself if it encounters an error doesn't it?

> + phy_port_destroy(port);
>
> return ret;
>
> }
>
> static void phylink_destroy_sfp_port(struct phylink *pl)
> {
>
> - if (pl->netdev && pl->sfp_bus_port)
> - phy_link_topo_del_port(pl->netdev, pl->sfp_bus_port);
> + if (pl->netdev) {
> + if (pl->sfp_bus_port)
> + phy_link_topo_del_port(pl->netdev, pl->sfp_bus_port);
> +
> + /* Only remove it from the topology, it will be destroyed at
> + * module removal.
> + */
> + if (pl->mod_port)
> + phy_link_topo_del_port(pl->netdev, pl->mod_port);
> + }
>
> if (pl->sfp_bus_port)
>
> phy_port_destroy(pl->sfp_bus_port);
>
> @@ -3998,6 +4017,49 @@ static void phylink_sfp_disconnect_phy(void
> *upstream, phylink_disconnect_phy(upstream);
>
> }
>
> +static int phylink_sfp_connect_nophy(void *upstream)

I'd name this "phylink_sfp_connect_no_phy" just to keep the name formatting
consistent.

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