Re: [PATCH v2 10/19] net: phy: Use the compatibility driver attachment API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net: phy: loss of phydev->irq in phy_attach_direct() on
  -EBUSY probe failure
--

commit 89d719155ade2a67c02be5cec992ec1828452066
Author: Leon Romanovsky <leon@kernel.org>

net: phy: Use the compatibility driver attachment API

This commit switches generic PHY driver attachment in phy_attach_direct()
to device_driver_attach_full() and updates phy_probe() and phy_remove() to
manage phydev->is_genphy_driven.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5d4508bcfbe99..5f0ca981644ec 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1912,7 +1912,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	struct device *d = &phydev->mdio.dev;
>  	struct module *ndev_owner = NULL;
>  	struct module *drv_owner;
> +	struct device_driver *drv;
>  	int irq = phydev->irq;
> +	bool bind_genphy;
>  	int err;
>  
>  	/* For Ethernet device drivers that register their own MDIO bus, we
> @@ -1939,16 +1941,16 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	/* Assume that if there is no driver, that it doesn't
>  	 * exist, and we should use the genphy driver.
>  	 */
> -	if (!d->driver) {
> +	drv = d->driver;
> +	bind_genphy = !drv;
> +	if (bind_genphy) {
>  		if (phydev->is_c45)
> -			d->driver = &genphy_c45_driver.mdiodrv.driver;
> +			drv = &genphy_c45_driver.mdiodrv.driver;
>  		else
> -			d->driver = &genphy_driver.mdiodrv.driver;
> -
> -		phydev->is_genphy_driven = 1;
> +			drv = &genphy_driver.mdiodrv.driver;
>  	}
>  
> -	drv_owner = d->driver->owner;
> +	drv_owner = drv->owner;
>  	if (!try_module_get(drv_owner)) {
>  		phydev_err(phydev, "failed to get the device driver module\n");
>  		err = -EIO;
> @@ -1956,11 +1958,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	}
>  	phydev->drv_owner = drv_owner;
>  
> -	if (phydev->is_genphy_driven) {
> -		err = d->driver->probe(d);
> -		if (err >= 0)
> -			err = device_bind_driver(d);
> -
> +	if (bind_genphy) {
> +		err = device_driver_attach_full(drv, d);
> +		if (err == -EBUSY)
> +			irq = phydev->irq;
>  		if (err)
>  			goto error_module_put;
>  	}

[Severity: Medium]
Can this overwrite the saved irq with PHY_POLL when phy_probe() fails with
-EBUSY?

At the start of phy_probe(), !phy_drv_supports_irq(phydrv) sets phydev->irq
to PHY_POLL for the generic PHY drivers.  If a later step in phy_probe()
returns -EBUSY (for example via phy_setup_ports() -> of_phy_ports() ->
phy_add_port(), or an MDIO bus read returning -EBUSY),
device_driver_attach_full() propagates -EBUSY after unbinding the driver.
Overwriting irq = phydev->irq on -EBUSY then causes the error_module_put
path to write PHY_POLL back to phydev->irq instead of restoring the
original interrupt number, leaving the PHY in polling mode on subsequent
attachments.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-device-trust-core-v2-0-3b12d9a9f861@nvidia.com?part=10