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

[Simon Horman <horms@kernel.org>]

On Fri, Oct 23, 2026 at 02:04:34PM +0200, Nicolai Buchwitz wrote:
> 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.

FWIIW, I agree that is an appropriate approach here.

Reviewed-by: Simon Horman <horms@kernel.org>