Re: [PATCH net-next v5 3/8] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus

[sashiko-bot@kernel.org]

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

New issues:
- [High] Inverted resource teardown order in `phy_detach` causes LIFO violation.

Pre-existing issues:
- [High] `phy_attach_direct` destroys pre-existing attachments when returning -EBUSY.
--

--- Patch [3]: [PATCH net-next v5 3/8] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus ---
commit 2e5a68c82441635d018ad6986569eeead2f1a154
Author: Daniel Golle <daniel@makrotopia.org>

net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus

Some MDIO buses require to program PHY polling registers depending on
the PHY type. This adds two simple hooks in struct mii_bus which are
called right after a PHY has been attached and just before it is
going to be detached.

Link: https://github.com/openwrt/openwrt/pull/21515#discussion_r2714069716
Link: https://lore.kernel.org/netdev/cover.1769053496.git.daniel@makrotopia.org/

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..a62c2ff6c2aab 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1876,6 +1876,13 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	if (err)
>  		goto error;
>  
> +	if (phydev->mdio.bus->notify_phy_attach) {
> +		err = phydev->mdio.bus->notify_phy_attach(phydev);
> +		if (err)
> +			goto error;

[Severity: High]
This is a pre-existing issue, but does jumping to the error label in this
function cause problems for already attached PHYs?

Earlier in phy_attach_direct(), if a second device attempts to attach to a
PHY that is already attached, it detects this (phydev->attached_dev is set),
sets err = -EBUSY, and jumps to the error label.

The error block then calls phy_detach(phydev), which tears down the fully
established attachment. This forcefully clears phydev->attached_dev and
removes the PHY from the original owner without its knowledge.

When the original owner later interacts with the PHY or tries to tear it down,
could it encounter a dangling pointer or mismatched refcounts?

> +		phydev->mdio_bus_notified = true;
> +	}
> +
>  	phy_resume(phydev);

[ ... ]

> @@ -1919,6 +1926,10 @@ void phy_detach(struct phy_device *phydev)
>  	struct module *ndev_owner = NULL;
>  	struct mii_bus *bus;
>  
> +	if (phydev->mdio_bus_notified && phydev->mdio.bus->notify_phy_detach)
> +		phydev->mdio.bus->notify_phy_detach(phydev);
> +	phydev->mdio_bus_notified = false;
> +

[Severity: High]
Does this tear down sequence violate LIFO order?

In phy_attach_direct(), the setup sequence calls notify_phy_attach() before
phy_resume() and device_link_add().

To follow strict LIFO teardown, shouldn't phy_detach() call device_link_del()
and phy_suspend() before notify_phy_detach()?

If an MDIO bus driver disables resources (like power, clocks, or polling) in
the detach hook, the subsequent call to phy_suspend() could attempt to issue
MDIO commands to an unconfigured bus, potentially leading to bus timeouts or
hangs.

>  	if (phydev->devlink) {
>  		device_link_del(phydev->devlink);
>  		phydev->devlink = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722062356.12291-1-markus.stockhausen@gmx.de?part=3