* [PATCH net v1 0/2] net: phy: fix driver cleanup after probe failure @ 2026-08-12 12:51 Xuanqiang Luo 2026-08-12 12:51 ` [PATCH net v1 1/2] net: phy: split phy_probe() error paths Xuanqiang Luo 2026-08-12 12:51 ` [PATCH net v1 2/2] net: phy: call driver remove when core initialization fails Xuanqiang Luo 0 siblings, 2 replies; 5+ messages in thread From: Xuanqiang Luo @ 2026-08-12 12:51 UTC (permalink / raw) To: netdev Cc: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni, linux-kernel, Xuanqiang Luo From: Xuanqiang Luo <luoxuanqiang@kylinos.cn> Patch 1 splits the phy_probe() error paths and makes phy_setup_ports() unwind its own resources. Patch 2 calls the PHY driver remove callback when phy_probe() fails after the driver probe callback succeeds. Xuanqiang Luo (2): net: phy: split phy_probe() error paths net: phy: call driver remove when core initialization fails drivers/net/phy/phy_device.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) base-commit: db2ddb87143519e20a95aa36c60b36107b736a58 -- 2.43.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net v1 1/2] net: phy: split phy_probe() error paths 2026-08-12 12:51 [PATCH net v1 0/2] net: phy: fix driver cleanup after probe failure Xuanqiang Luo @ 2026-08-12 12:51 ` Xuanqiang Luo 2026-08-12 13:41 ` Andrew Lunn 2026-08-12 12:51 ` [PATCH net v1 2/2] net: phy: call driver remove when core initialization fails Xuanqiang Luo 1 sibling, 1 reply; 5+ messages in thread From: Xuanqiang Luo @ 2026-08-12 12:51 UTC (permalink / raw) To: netdev Cc: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni, linux-kernel, Xuanqiang Luo From: Xuanqiang Luo <luoxuanqiang@kylinos.cn> 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 <luoxuanqiang@kylinos.cn> --- 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; } @@ -3681,7 +3684,7 @@ static int phy_probe(struct device *dev) if (phydev->drv->probe) { err = phydev->drv->probe(phydev); if (err) - goto out; + goto out_reset; } phy_disable_interrupts(phydev); @@ -3702,7 +3705,7 @@ static int phy_probe(struct device *dev) err = genphy_read_abilities(phydev); if (err) - goto out; + goto out_reset; if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported)) @@ -3719,7 +3722,7 @@ static int phy_probe(struct device *dev) err = phy_setup_ports(phydev); if (err) - goto out; + goto out_reset; phy_advertise_supported(phydev); @@ -3728,7 +3731,7 @@ static int phy_probe(struct device *dev) */ err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee); if (err) - goto out; + goto out_ports; /* Get the EEE modes we want to prohibit. */ of_set_phy_eee_broken(phydev); @@ -3781,20 +3784,22 @@ static int phy_probe(struct device *dev) if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev)) { err = of_phy_leds(phydev); if (err) - goto out; + goto out_led_triggers; } return 0; -out: +out_led_triggers: + if (!phydev->is_on_sfp_module) + phy_led_triggers_unregister(phydev); + +out_ports: sfp_bus_del_upstream(phydev->sfp_bus); phydev->sfp_bus = NULL; phy_cleanup_ports(phydev); - if (!phydev->is_on_sfp_module) - phy_led_triggers_unregister(phydev); - +out_reset: /* Re-assert the reset signal on error */ phy_device_reset(phydev, 1); -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v1 1/2] net: phy: split phy_probe() error paths 2026-08-12 12:51 ` [PATCH net v1 1/2] net: phy: split phy_probe() error paths Xuanqiang Luo @ 2026-08-12 13:41 ` Andrew Lunn 2026-08-12 16:05 ` Maxime Chevallier 0 siblings, 1 reply; 5+ messages in thread From: Andrew Lunn @ 2026-08-12 13:41 UTC (permalink / raw) To: Xuanqiang Luo Cc: netdev, hkallweit1, linux, davem, edumazet, kuba, pabeni, linux-kernel, Xuanqiang Luo On Wed, Aug 12, 2026 at 08:51:26PM +0800, Xuanqiang Luo wrote: > From: Xuanqiang Luo <luoxuanqiang@kylinos.cn> > > 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 <luoxuanqiang@kylinos.cn> > --- > 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(). 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. Andrew --- pw-bot: cr ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v1 1/2] net: phy: split phy_probe() error paths 2026-08-12 13:41 ` Andrew Lunn @ 2026-08-12 16:05 ` Maxime Chevallier 0 siblings, 0 replies; 5+ messages in thread From: Maxime Chevallier @ 2026-08-12 16:05 UTC (permalink / raw) To: Andrew Lunn, Xuanqiang Luo Cc: netdev, hkallweit1, linux, davem, edumazet, kuba, pabeni, linux-kernel, Xuanqiang Luo Hi, On 8/12/26 15:41, Andrew Lunn wrote: > On Wed, Aug 12, 2026 at 08:51:26PM +0800, Xuanqiang Luo wrote: >> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn> >> >> 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 <luoxuanqiang@kylinos.cn> >> --- >> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net v1 2/2] net: phy: call driver remove when core initialization fails 2026-08-12 12:51 [PATCH net v1 0/2] net: phy: fix driver cleanup after probe failure Xuanqiang Luo 2026-08-12 12:51 ` [PATCH net v1 1/2] net: phy: split phy_probe() error paths Xuanqiang Luo @ 2026-08-12 12:51 ` Xuanqiang Luo 1 sibling, 0 replies; 5+ messages in thread From: Xuanqiang Luo @ 2026-08-12 12:51 UTC (permalink / raw) To: netdev Cc: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni, linux-kernel, Xuanqiang Luo From: Xuanqiang Luo <luoxuanqiang@kylinos.cn> phy_probe() may fail while querying features or completing other core initialization after the PHY driver probe callback has succeeded. The driver core does not run the remove path after a probe error, so resources that the PHY driver releases in its remove callback are leaked. Call the PHY driver remove callback on these failures. Fixes: efbdfdc29bdd ("net: phy: Add support for asking the PHY its abilities") Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn> --- drivers/net/phy/phy_device.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index f8e434daab66e..be714758360b2 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -3705,7 +3705,7 @@ static int phy_probe(struct device *dev) err = genphy_read_abilities(phydev); if (err) - goto out_reset; + goto out_remove; if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported)) @@ -3722,7 +3722,7 @@ static int phy_probe(struct device *dev) err = phy_setup_ports(phydev); if (err) - goto out_reset; + goto out_remove; phy_advertise_supported(phydev); @@ -3799,6 +3799,10 @@ static int phy_probe(struct device *dev) phy_cleanup_ports(phydev); +out_remove: + if (phydev->drv->remove) + phydev->drv->remove(phydev); + out_reset: /* Re-assert the reset signal on error */ phy_device_reset(phydev, 1); -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-08-12 16:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-12 12:51 [PATCH net v1 0/2] net: phy: fix driver cleanup after probe failure Xuanqiang Luo 2026-08-12 12:51 ` [PATCH net v1 1/2] net: phy: split phy_probe() error paths Xuanqiang Luo 2026-08-12 13:41 ` Andrew Lunn 2026-08-12 16:05 ` Maxime Chevallier 2026-08-12 12:51 ` [PATCH net v1 2/2] net: phy: call driver remove when core initialization fails Xuanqiang Luo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.