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

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

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