Re: [PATCH net v3 1/2] net: bcmgenet: recover from a failed resume

[Nicolai Buchwitz <nb@tipi-net.de>]

On 23.10.2026 13:55, netdev-bot+sashiko@kernel.org wrote:
> [...]

> [Severity: Medium]
> 
> After a failed non-WoL resume, can the next non-WoL suspend leave an
> external PHY powered up with its link active?
> 
> On the normal non-WoL path, bcmgenet_suspend() halts and suspends the
> PHY:
> 
> bcmgenet_netif_stop(dev, true)
>   phy_stop()
>     PHY_HALTED -> phy_suspend()   (BMCR power-down)
> 
> The failed resume has already done this before bcmgenet_init_dma()
> fails:
> 
> bcmgenet_resume()
>   phy_init_hw()          drv->soft_reset() clears BMCR_PDOWN and sets
>                          phydev->suspended = 0
>   genphy_config_aneg()   restarts autoneg
>   bcmgenet_init_dma()    fails -> return ret
>                          (phy_resume() is never reached)
> 
> This new early return then skips any PHY power-down. Calling phy_stop()
> here would not work either, because the PHY is still in PHY_HALTED.
> 
> Nothing else seems to power the PHY down. bcmgenet_mii_probe() sets
> mac_managed_pm = true, so mdio_bus_phy_suspend() returns early, and
> bcmgenet_suspend_noirq() only powers down an internal GPHY via
> GENET_POWER_PASSIVE.
> 
> So an external PHY whose driver has both .soft_reset and .suspend (for
> example genphy_soft_reset with genphy_suspend) would stay powered with
> its link up through system sleep.
> 
> This would not show up with the bcm54xx PHY on CM4, which has no
> .soft_reset, or with the Generic PHY driver.
> 
> Does the !priv->tx_cbs path need a phy_suspend() call (or similar) when
> WoL is not in use?
> 
>> 
>>  	netif_device_detach(dev);
>> 
>>  	if (device_may_wakeup(d) && priv->wolopts) {
> 
> [ ... ]

Same as in v2: pre-existing, phy_init_hw() powers it up in the failed
resume and nothing starts it again. Fair that it's reachable now, before
this series that suspend just hung. Nobody seems to have hit it either, 
it
needs a failed resume plus a PHY with soft_reset, so pretty much an edge
case, don't you think?

I'd rather not add a phy_suspend() I can't test ... the CM4's bcm54xx 
has
no soft_reset, so the PHY never gets powered up there anyway. The proper
fix is to stop leaving the half state behind, that's on my list after 
the
MTU series.

Thanks,
Nicolai