Re: [PATCH net v1 1/2] net: phy: split phy_probe() error paths

Hi,

On 8/12/26 15:41, Andrew Lunn wrote:

On Wed, Aug 12, 2026 at 08:51:26PM +0800, Xuanqiang Luo wrote:

quoted

From: Xuanqiang Luo <redacted>

phy_probe() uses one cleanup path for failures at every initialization
stage. This runs cleanup for resources that have not been initialized
and leaves phy_setup_ports() relying on its caller to remove an SFP
upstream after a partial failure.

Make phy_setup_ports() unwind the SFP upstream before removing its ports.
Then split the phy_probe() cleanup so each failure path unwinds only the
resources that may have been initialized.

Signed-off-by: Xuanqiang Luo <redacted>
---
 drivers/net/phy/phy_device.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0615228459ef4..f8e434daab66e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -3556,6 +3556,9 @@ static int phy_setup_ports(struct phy_device *phydev)
 	return 0;
 
 out:
+	sfp_bus_del_upstream(phydev->sfp_bus);
+	phydev->sfp_bus = NULL;
+
 	phy_cleanup_ports(phydev);
 	return ret;

This does not look correct. phy_sfp_probe() may fail, and you then
call sfp_bus_del_upstream() on something which never happened.

Also, it is not obvious that sfp_bus_del_upstream() is the correct
thing to do. You are trying to undo phy_sfp_probe() so i would expect
you to call a function like phy_sfp_release().

I agree with having phy_sfp_release(), we have even more to do with phy SFP
in the future with phy_port, this is less error prone.

It also looks like phy_sfp_probe() does not correctly clean up on
phy_setup_sfp_port() returning an error. But that is a different
issue.

In practise, this is cleaned in the phy_probe's "out" failure label,
but indeed this is not pretty.

Maxime