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

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> 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);
>>
> 

Agreed. PHY teardown can re-enable the MAC through MDIO accesses. I’ll 
update patch #3 to call au1000_reset_mac() after MDIO bus teardown, 
matching the probe error path.

The RTNL annotation concern appears to be a separate phylib issue shared 
by other drivers.

pw-bot: cr
-- 
Florian