Re: [PATCH net-next v18 04/10] net: phy: Create SFP phy_port before registering upstream

["Christophe Leroy (CS GROUP)" <chleroy@kernel.org>]

Le 27/09/2026 à 15:36, Maxime Chevallier a écrit :
> When dealing with PHY-driven SFP, we create a phy_port representing the
> SFP bus when we know we have such a bus.
> 
> We can move the port creation before registering the sfp upstream ops,
> as long as we know the SFP bus is there. This will allow passing the
> phy_port along with the upstream information to the SFP bus.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Aleksei Sviridkin <f@lex.la>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>

> ---
>   drivers/net/phy/phy_device.c | 58 +++++++++++++++++++++++-------------
>   1 file changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c4d2c03b8282..3c20d4cc1a79 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1586,13 +1586,13 @@ static void phy_del_port(struct phy_device *phydev, struct phy_port *port)
>   	phydev->n_ports--;
>   }
>   
> -static int phy_setup_sfp_port(struct phy_device *phydev)
> +static struct phy_port *phy_setup_sfp_port(struct phy_device *phydev)
>   {
>   	struct phy_port *port = phy_port_alloc();
>   	int ret;
>   
>   	if (!port)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>   
>   	port->parent_type = PHY_PORT_PHY;
>   	port->phy = phydev;
> @@ -1607,10 +1607,12 @@ static int phy_setup_sfp_port(struct phy_device *phydev)
>   	 * when attaching the port to the phydev.
>   	 */
>   	ret = phy_add_port(phydev, port);
> -	if (ret)
> +	if (ret) {
>   		phy_port_destroy(port);
> +		return ERR_PTR(ret);
> +	}
>   
> -	return ret;
> +	return port;
>   }
>   
>   /**
> @@ -1619,30 +1621,46 @@ static int phy_setup_sfp_port(struct phy_device *phydev)
>    */
>   static int phy_sfp_probe(struct phy_device *phydev)
>   {
> +	struct phy_port *port = NULL;
>   	struct sfp_bus *bus;
> -	int ret = 0;
> +	int ret;
>   
> -	if (phydev->mdio.dev.fwnode) {
> -		bus = sfp_bus_find_fwnode(phydev->mdio.dev.fwnode);
> -		if (IS_ERR(bus))
> -			return PTR_ERR(bus);
> +	if (!phydev->mdio.dev.fwnode)
> +		return 0;
>   
> -		phydev->sfp_bus = bus;
> +	bus = sfp_bus_find_fwnode(phydev->mdio.dev.fwnode);
> +	if (IS_ERR(bus))
> +		return PTR_ERR(bus);
>   
> -		ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
> -		sfp_bus_put(bus);
> +	phydev->sfp_bus = bus;
>   
> -		if (ret)
> -			phydev->sfp_bus = NULL;
> +	if (bus) {
> +		port = phy_setup_sfp_port(phydev);
> +		if (IS_ERR(port)) {
> +			ret = PTR_ERR(port);
> +			goto out_sfp;
> +		}
>   	}
>   
> -	if (!ret && phydev->sfp_bus) {
> -		ret = phy_setup_sfp_port(phydev);
> -		if (ret) {
> -			sfp_bus_del_upstream(phydev->sfp_bus);
> -			phydev->sfp_bus = NULL;
> -		}
> +	ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
> +	if (ret)
> +		goto out_port;
> +
> +	/* sfp_bus_add_upstream() grabs a ref to the sfp bus on success, it's
> +	 * safe to release it now.
> +	 */
> +	sfp_bus_put(bus);
> +
> +	return ret;
> +
> +out_port:
> +	if (port) {
> +		phy_del_port(phydev, port);
> +		phy_port_destroy(port);
>   	}
> +out_sfp:
> +	sfp_bus_put(bus);
> +	phydev->sfp_bus = NULL;
>   
>   	return ret;
>   }