Re: [PATCH net-next v6 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:
- [Medium] LIFO teardown violation in __phy_detach() clears phydev->attached_dev before invoking notify_phy_detach() hook.
- [Medium] Calling phy_attach_direct() on an already attached PHY destructively tears down the existing attachment without notifying the MDIO bus, risking hardware lockups.
--

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

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

This patch adds notify_phy_attach() and notify_phy_detach() hooks to 
struct mii_bus. This allows MDIO bus drivers to handle PHY specific 
initialization without reinventing bus scanning. It implements a symmetric
LIFO teardown where the detach hook is called only if the attach notification
succeeded.

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..38b31fc272d7d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1734,6 +1734,95 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->handle_interrupt;
>  }
>  
> +static void __phy_detach(struct phy_device *phydev, bool notify_bus)
> +{
> +	struct net_device *dev = phydev->attached_dev;
> +	struct module *ndev_owner = NULL;
> +	struct mii_bus *bus;

[ ... ]

> +	if (dev) {
> +		struct hwtstamp_provider *hwprov;
> +
> +		/* hwprov may technically be protected by ops lock but
> +		 * not for devices with a phydev, see phy_link_topo_add_phy()
> +		 */
> +		hwprov = rtnl_dereference(dev->hwprov);
> +		/* Disable timestamp if it is the one selected */
> +		if (hwprov && hwprov->phydev == phydev) {
> +			rcu_assign_pointer(dev->hwprov, NULL);
> +			kfree_rcu(hwprov, rcu_head);
> +		}
> +
> +		phydev->attached_dev->phydev = NULL;
> +		phydev->attached_dev = NULL;
> +		phy_link_topo_del_phy(dev, phydev);
> +	}
> +
> +	phydev->phy_link_change = NULL;
> +	phydev->phylink = NULL;
> +
> +	if (notify_bus && phydev->mdio.bus->notify_phy_detach)
> +		phydev->mdio.bus->notify_phy_detach(phydev);

[Severity: Medium]
The commit message mentions a symmetric LIFO teardown. However,
phydev->attached_dev is cleared before notify_phy_detach() is called here.

In phy_attach_direct(), phydev->attached_dev is set before
notify_phy_attach() is invoked. Does this break the LIFO symmetry,
preventing bus drivers from accessing the attached network interface during
teardown?

[ ... ]

> @@ -1890,8 +1985,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: Medium]
If phy_attach_direct() is called on an already attached PHY, it hits the
-EBUSY check and jumps to this error path.

This is a pre-existing issue, but destructively tearing down an existing
attachment upon a second attach attempt still occurs. With this patch, the
error path now explicitly passes false to __phy_detach(), suppressing the bus
notification.

Since the existing attachment would have previously triggered a successful
notify_phy_attach(), doesn't skipping the detach notification here leave the
bus out of sync, potentially causing MDIO timeouts if the bus continues
polling a reset PHY?

>  	return err;
>  
>  error_module_put:

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