Re: [PATCH net-next v2 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 `notify_phy_detach` hook can be invoked on an MDIO bus even if `notify_phy_attach` was never called or failed, leading to unbalanced setup/teardown symmetry.

Pre-existing issues:
- [High] Calling `phy_attach_direct` on an already attached PHY returns `-EBUSY` but incorrectly executes the `phy_detach` error path, destroying the existing valid attachment.
--

commit c461faab1a569fa9ec7dc7cc9fc30615fc89408c
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. Provide two simple hooks in struct mii_bus which are called
right after a PHY has been attached and just before the PHY 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 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 calling phy_attach_direct() on an already
attached PHY seems to tear down the existing valid attachment. Earlier in
this function:

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

Jumping to the error label unconditionally executes phy_detach(), which
clears phydev->attached_dev (the original owner) and severs the
dev->phydev link.

Should this path return an error directly without calling phy_detach()?

> +	}
> +
>  	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 notify_phy_detach() be invoked even if notify_phy_attach() was never
called or failed?

If phy_init_hw() or any other initialization step prior to
notify_phy_attach() fails in phy_attach_direct(), the code jumps to the
error label and calls phy_detach(), which now unconditionally executes
notify_phy_detach().

Could this lead to unbalanced setup/teardown symmetry if a bus driver
assumes notify_phy_detach() is only called after a successful
notify_phy_attach()?

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

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