Re: [PATCH net 03/11] net: au1000: disconnect the PHY before freeing the MDIO bus

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/net/ethernet/amd/au1000_eth.c, au1000_remove() now
  calls phy_disconnect() after unregister_netdev().

Pre-existing issues:
- [Low] The two new phy_disconnect() calls (au1000_remove() after
  unregister_netdev(), and the au1000_probe() err_out path) run without…

--

net: au1000: disconnect the PHY before freeing the MDIO bus

This patch adds phy_disconnect() calls to the au1000_probe() error path
and to au1000_remove(). This balances the phy_connect() done in
au1000_mii_probe(). In both paths the disconnect runs before the MDIO bus
is unregistered.

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index 58bc8e1b539d5..70319b432182f 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c
> @@ -1287,6 +1287,8 @@ static int au1000_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_out:
> +	if (dev->phydev)
> +		phy_disconnect(dev->phydev);
>  	if (aup->mii_bus)
>  		mdiobus_unregister(aup->mii_bus);
>  

[ ... ]

> @@ -1334,6 +1336,8 @@ static void au1000_remove(struct platform_device *pdev)
>  	struct resource *base, *macen;
>  
>  	unregister_netdev(dev);
> +	if (dev->phydev)
> +		phy_disconnect(dev->phydev);

[Severity: Medium]
Can this leave the MAC clocked and out of reset after the driver unbinds?

By the time au1000_remove() reaches this point, the MAC is already in
reset with its clock gated, and aup->mac_enabled is 0. If the interface
was never opened, au1000_probe() did this by calling au1000_reset_mac()
just before register_netdev(). If the interface was opened,
au1000_close() did it.

If the interface was never opened, phydev->suspended is still false,
because phy_attach_direct() resumed the PHY. In that case phy_disconnect()
does MDIO accesses:

phy_disconnect()
  phy_detach()
    phy_detach_internal()
      phy_suspend()
        genphy_suspend()
          phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN)

Each of these accesses goes through au1000_mdiobus_read() or
au1000_mdiobus_write(). Both call au1000_enable_mac(dev, 0) first:

	if (force_reset || (!aup->mac_enabled)) {
		writel(MAC_EN_CLOCK_ENABLE, aup->enable);
		...
		writel((MAC_EN_RESET0 | MAC_EN_RESET1 | MAC_EN_RESET2
				| MAC_EN_CLOCK_ENABLE), aup->enable);
		...
		aup->mac_enabled = 1;
	}

Nothing later in au1000_remove() calls au1000_reset_mac() again before
the registers are unmapped and the netdev is freed. Before this patch,
remove left the MAC in reset with its clock gated.

The err_out path in au1000_probe() does call au1000_reset_mac(dev) after
phy_disconnect() and mdiobus_unregister(). Should au1000_remove() also
call au1000_reset_mac(dev) after phy_disconnect(), to match it?

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch, but both
new phy_disconnect() calls run without RTNL held. In au1000_remove(),
unregister_netdev() has already dropped RTNL. au1000_probe() never takes
it.

phy_disconnect()->phy_detach()->phy_detach_internal() does:

		hwprov = rtnl_dereference(dev->hwprov);

With CONFIG_PROVE_RCU enabled, could this emit a one-time "suspicious
rcu_dereference_protected() usage" warning? The read itself looks
harmless, since au1000 never installs a hwtstamp provider.

Other drivers already call phy_disconnect() after unregister_netdev()
without RTNL, for example r6040_remove_one(), et131x_pci_remove(),
bgmac_enet_remove(), mlxbf_gige_remove(), hisi_femac_drv_remove() and
ixp4xx_eth_remove(). So this looks like a problem with the phylib
annotation, not with this driver.

>  	mdiobus_unregister(aup->mii_bus);
>  	mdiobus_free(aup->mii_bus);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com