Re: [PATCH net v5 2/6] net: phy: unregister SFP upstream before port cleanup
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
Date: 2026-08-31 07:58:52
Also in:
lkml
Hi, On 8/23/26 05:55, Xuanqiang Luo wrote:
From: Xuanqiang Luo <redacted>
Commit 4497f5028675 ("net: phy: Clean the phy_ports after unregistering
the downstream SFP bus") established that an SFP upstream must be
unregistered before its phy_ports are destroyed because SFP callbacks
may access these ports.
phy_setup_ports() does not follow this order when a later port setup
step fails after phy_sfp_probe() succeeds. It destroys the SFP phy_port
and leaves phy_probe() to unregister the upstream later, creating a race
between port destruction and SFP upstream callbacks.
The error unwind is also split across three functions. If
phy_setup_sfp_port() fails, phy_sfp_probe() leaves the upstream
registered and relies on phy_probe() to remove it after
phy_setup_ports() returns.
Make each layer unwind the resources it successfully set up. Unregister
only the upstream in phy_sfp_probe() when SFP port setup fails, since
the failed port has already been destroyed. Add phy_sfp_release() for a
successful SFP probe, and make phy_setup_ports() use it before cleaning
up the remaining ports. Once phy_setup_ports() has rolled back all port
setup, make phy_probe() skip this cleanup.
Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Xuanqiang Luo <redacted>Thanks for this cleanup, Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com> Maxime
quoted hunk ↗ jump to hunk
--- drivers/net/phy/phy_device.c | 49 ++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 11 deletions(-)diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 2cf70471ae089..4b9b2300422fb 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c@@ -1723,12 +1723,41 @@ static int phy_sfp_probe(struct phy_device *phydev) phydev->sfp_bus = NULL; } - if (!ret && phydev->sfp_bus) + if (!ret && phydev->sfp_bus) { ret = phy_setup_sfp_port(phydev); + if (ret) { + sfp_bus_del_upstream(phydev->sfp_bus); + phydev->sfp_bus = NULL; + } + } return ret; } +/** + * phy_sfp_release - release resources set up by phy_sfp_probe() + * @phydev: the PHY device + * + * Release the SFP resources set up by a successful phy_sfp_probe(). Unregister + * the upstream before destroying its phy_port, so SFP upstream callbacks cannot + * race with port destruction. + */ +static void phy_sfp_release(struct phy_device *phydev) +{ + struct phy_port *port, *tmp; + + sfp_bus_del_upstream(phydev->sfp_bus); + phydev->sfp_bus = NULL; + + list_for_each_entry_safe(port, tmp, &phydev->ports, head) { + if (!port->is_sfp) + continue; + + phy_del_port(phydev, port); + phy_port_destroy(port); + } +} + static bool phy_drv_supports_irq(const struct phy_driver *phydrv) { return phydrv->config_intr && phydrv->handle_interrupt;@@ -3547,13 +3576,13 @@ static int phy_setup_ports(struct phy_device *phydev) if (!phydev->is_genphy_driven) { ret = phy_sfp_probe(phydev); if (ret) - goto out; + goto err_ports; } if (phydev->n_ports < phydev->max_n_ports) { ret = phy_default_setup_single_port(phydev); if (ret) - goto out; + goto err_sfp; } linkmode_zero(ports_supported);@@ -3580,7 +3609,9 @@ static int phy_setup_ports(struct phy_device *phydev) return 0; -out: +err_sfp: + phy_sfp_release(phydev); +err_ports: phy_cleanup_ports(phydev); return ret; }@@ -3744,7 +3775,7 @@ static int phy_probe(struct device *dev) err = phy_setup_ports(phydev); if (err) - goto out_sfp_release; + goto out_reset; phy_advertise_supported(phydev);@@ -3816,9 +3847,7 @@ static int phy_probe(struct device *dev) phy_led_triggers_unregister(phydev); out_sfp_release: - sfp_bus_del_upstream(phydev->sfp_bus); - phydev->sfp_bus = NULL; - + phy_sfp_release(phydev); phy_cleanup_ports(phydev); out_reset:@@ -3842,9 +3871,7 @@ static int phy_remove(struct device *dev) phydev->state = PHY_DOWN; - sfp_bus_del_upstream(phydev->sfp_bus); - phydev->sfp_bus = NULL; - + phy_sfp_release(phydev); phy_cleanup_ports(phydev); if (phydev->drv && phydev->drv->remove)