Re: [PATCH net-next v3 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:
- [Medium] The newly added `notify_phy_detach` callback is invoked unconditionally during `phy_detach()`, violating attach/detach symmetry if attachment fails.

Pre-existing issues:
- [High] Calling `phy_attach_direct` on an already-attached PHY destructively removes its existing attachment.
--

commit 43b033c0e004225e5826f1197f795406afea2c07
Author: Daniel Golle <daniel@makrotopia.org>

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

This commit adds notify_phy_attach and notify_phy_detach hooks to
struct mii_bus. This allows MDIO bus drivers to handle PHY-specific
initialization and teardown without duplicating bus scanning logic.

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 does jumping to the error label
destructively tear down valid state if the PHY was already attached?

If phy_attach_direct() is mistakenly called on a PHY that is already
attached, it correctly catches it here:

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

However, by jumping to the error label, it calls phy_detach(). Since
phy_detach() unconditionally removes sysfs links, unbinds the PHY, and
clears phydev->attached_dev, won't this tear down the pre-existing
attachment and leave the original caller with dangling references?

> +	}
> +
>  	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: Medium]
Will this result in asymmetric attach/detach notifications on error paths?

If phy_init_hw() fails earlier in phy_attach_direct(), or if this new
notify_phy_attach callback itself fails, the code jumps to the error
label and calls phy_detach().

Because phy_detach() unconditionally calls notify_phy_detach(), won't
the bus driver receive a detach notification for a PHY that was never
successfully attached?

If future bus drivers rely on these callbacks to allocate resources or
increment reference counts, could this cause double-frees or underflows?

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

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