Re: [PATCH net-next v8 3/9] 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `phy_attach_direct` on an already attached PHY destructively tears down the existing legitimate attachment and causes module refcount imbalances.
--

--- Patch [3]: [PATCH net-next v8 3/9] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus ---
commit 93555b251005e09563b58618551521a23eeca40d
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 for MDIO bus drivers. It also relocates code from
phy_detach() into a new __phy_detach() helper to ensure symmetrical bus
notification during PHY attach and detach phases.

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..233622e9ba61f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1890,8 +1986,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	return err;
>  
>  error:
> -	/* phy_detach() does all of the cleanup below */
> -	phy_detach(phydev);
> +	/* cleanup without bus notification */
> +	__phy_detach(phydev, false);

[Severity: High]
This is a pre-existing issue, but since the error path is being modified here,
I wanted to ask about how we handle failures when a PHY is already attached.

If phy_attach_direct() is called on a PHY that is already attached to another
device, it returns -EBUSY and jumps to the error label:

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

Because this error path unconditionally calls __phy_detach(), and 
__phy_detach() uses the current phydev->attached_dev as its target, 
does this perform a full teardown on the existing active connection?

static void __phy_detach(struct phy_device *phydev, bool notify_bus)
{
	struct net_device *dev = phydev->attached_dev;
...
		phydev->attached_dev->phydev = NULL;
		phydev->attached_dev = NULL;


Could this break the live attachment and cause module refcount imbalances, 
since the original MAC driver is completely unaware its PHY was detached and 
its dev->phydev pointer was set to NULL?

Should the error path cleanly roll back only the references taken for the 
new failed attachment instead of destroying the pre-existing state?

>  	return err;
>  
>  error_module_put:

[ ... ]

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