Re: [PATCH net-next v4 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] The notify_phy_detach hook is called unconditionally in phy_detach(), even if notify_phy_attach() was never called or previously failed.

Pre-existing issues:
- [High] phy_attach_direct() incorrectly tears down existing legitimate PHY attachments when returning -EBUSY.
--

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

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

This commit introduces notify_phy_attach and notify_phy_detach hooks to
struct mii_bus. These hooks allow the MDIO bus driver to perform
initialization and teardown steps when a PHY is attached or 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 0615228459ef4..676cbf183350d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1876,6 +1876,12 @@ 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 I noticed something about the error
handling here and earlier in the function.

If phy_attach_direct() detects the PHY is already attached:

	if (phydev->attached_dev) {
		dev_err(&dev->dev, "PHY already attached\n");
		err = -EBUSY;
		goto error;
	}

The error label unconditionally calls phy_detach().

Will this tear down the active, legitimate attachment state of the original
owner instead of just aborting the current failed attach attempt?

> +	}
> +
>  	phy_resume(phydev);
>  
>  	/**
> @@ -1919,6 +1925,9 @@ void phy_detach(struct phy_device *phydev)
>  	struct module *ndev_owner = NULL;
>  	struct mii_bus *bus;
>  
> +	if (phydev->mdio.bus->notify_phy_detach)
> +		phydev->mdio.bus->notify_phy_detach(phydev);
> +

[Severity: High]
If phy_attach_direct() fails early and jumps to the error label, it calls
phy_detach(), which completely bypasses notify_phy_attach() but
unconditionally invokes notify_phy_detach().

It also appears that if notify_phy_attach() itself returns an error,
phy_detach() will be called immediately.

Could this cause MDIO bus drivers to receive unbalanced detach events for a
PHY that was never successfully attached? Might this lead to NULL pointer
dereferences or double frees if the bus driver relies on the attach hook
to initialize state?

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

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