Re: [PATCH net v5 1/2] net: phylink: unwind the PHY binding when bringup fails late

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Commit message misstates the impact of the bug being fixed on
  the SFP path.

--

net: phylink: unwind the PHY binding when bringup fails late

phylink_bringup_phy() stores the PHY in pl->phydev before calling
phy_eee_rx_clock_stop(), which can fail on a MAC implementing LPI, leaving
a stale pl->phydev behind after the callers unwind with phy_detach().  The
patch clears the binding on that failure path and factors the clearing out
of phylink_disconnect_phy() into a new phylink_clear_phydev() helper.

The code change looks right to me; the question below is only about the
description of the bug being fixed.

> What that costs depends on how the caller got here.
> phylink_connect_phy() and the SFP path go through
> phylink_attach_phy(), which refuses to attach while pl->phydev is set
> and turns a transient MDIO error into a permanent -EBUSY.

[Severity: Medium]
Is the -EBUSY description accurate for the SFP path?  On that path the
phy_device is created by sfp_sm_probe_phy() and destroyed when the connect
fails, so the stale pl->phydev looks like it points at freed memory rather
than just blocking a later attach.

phylink_sfp_config_phy() propagates the bringup error:

	ret = phylink_bringup_phy(pl, phy, config.interface);
	if (ret < 0) {
		phy_detach(phy);
		return ret;
	}

That error travels back through sfp_add_phy() (ops->connect_phy) into
sfp_sm_probe_phy():

	err = sfp_add_phy(sfp->sfp_bus, phy);
	if (err) {
		phy_device_remove(phy);
		phy_device_free(phy);
		dev_err(sfp->dev, "sfp_add_phy failed: %pe\n", ERR_PTR(err));
		return err;
	}

phy_detach() drops the reference taken by phy_attach_direct(), then
phy_device_free() -> put_device() drops the last one and
phy_device_release() does kfree(to_phy_device(dev)).  sfp->mod_phy is never
assigned in this path either, so no later disconnect_phy callback would
clear pl->phydev.

Wouldn't pre-patch consumers of pl->phydev then touch freed memory, for
instance phylink_resolve():

	mutex_lock(&pl->phydev_mutex);
	phy = pl->phydev;
	if (phy)
		mutex_lock(&phy->lock);

along with the ethtool ksettings/EEE helpers that test pl->phydev, and a
subsequent phylink_disconnect_phy() -> phy_disconnect()?

If that reading is correct, could the commit message describe the SFP path
as a dangling pl->phydev pointing at a freed phy_device instead of a
permanent -EBUSY, since that affects how the fix is triaged for stable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906174643.4107607-1-f%40lex.la