Re: [PATCH net 1/4] net: bcmgenet: stop the PHY when a WoL resume rebuilds the datapath

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

On 18.10.2026 14:02, 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
> 
> 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 bcmgenet_resume()
>   (drivers/net/ethernet/broadcom/genet/bcmgenet.c), the new guard on 
> the…
> 
> --
> 
> net: bcmgenet: stop the PHY when a WoL resume rebuilds the datapath
> 
> This patch adds a phy_stop() to the WoL teardown path in
> bcmgenet_resume(), which runs when the MAC was found reset, so the
> phy_start() in the rebuild does not warn and do nothing. The stop is
> skipped when phy_is_started() says the PHY is not running, because a
> failed earlier rebuild may already have stopped it.
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index e8908916558b1..308716cddc077 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -4288,6 +4288,9 @@ static int bcmgenet_resume(struct device *d)
>>  		bcmgenet_rdma_disable(priv);
>>  		bcmgenet_intr_disable(priv);
>>  		bcmgenet_fini_dma(priv);
>> +		/* An earlier rebuild of this datapath may have stopped it */
>> +		if (phy_is_started(dev->phydev))
>> +			phy_stop(dev->phydev);
> 
> [Severity: Medium]
> 
> If the PHY is in PHY_ERROR at this point, does it get past this guard
> and still hit the phy_start() warning during the rebuild?
> 
> phy_is_started() only returns true for states at or above PHY_UP:
> 
> include/linux/phy.h:phy_is_started() {
> 	return phydev->state >= PHY_UP;
> }
> 
> In enum phy_state, PHY_ERROR comes before PHY_UP, so a PHY in that 
> state
> skips phy_stop(). phy_stop() itself accepts PHY_ERROR without warning
> and moves the PHY to PHY_HALTED:
> 
> drivers/net/phy/phy.c:phy_stop() {
> 	if (!phy_is_started(phydev) && phydev->state != PHY_DOWN &&
> 	    phydev->state != PHY_ERROR) {
> 		WARN(1, "called from state %s\n",
> 		...
> 	phydev->state = PHY_HALTED;
> 	...
> }
> 
> The kernel-doc for PHY_ERROR in enum phy_state also says "phy_stop 
> moves
> to @PHY_HALTED".
> 
> Next, the rebuild calls bcmgenet_netif_start()->phy_start(), which only
> accepts READY or HALTED:
> 
> drivers/net/phy/phy.c:phy_start() {
> 	if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
> 		WARN(1, "called from state %s\n",
> 		     phy_state_to_str(phydev->state));
> 		goto out;
> 	}
> 	...
> }
> 
> When that happens, the PHY is not resumed and the state machine is not
> restarted. bcmgenet_resume() still returns 0, but the link stays down
> until the interface is taken down and up again.
> 
> Nothing stops the PHY state machine during a WoL suspend:
> 
>   - bcmgenet_mii_probe() sets mac_managed_pm = true, so
>     mdio_bus_phy_suspend() and mdio_bus_phy_resume() return early.
>   - The WoL branch of bcmgenet_suspend() does not call phy_stop().
>   - bcmgenet_wol_power_up_cfg() writes PHY_RUNNING or PHY_NOLINK
>     without stopping the machine.
> 
> So two things in that window could leave the PHY in PHY_ERROR when this
> check runs:
> 
>   - an MDIO error in _phy_state_machine(), via phy_error_precise()
>   - a phy_error() call from a PHY driver's interrupt handler
> 
> Nobody has seen this happen in practice yet. It follows from the code
> paths above.

Agreed, but lets fix it anyway as Clashiko will flag this once the path 
is
touched again in the future anyway ...

> 
> The commit message says:
> 
>     A rebuild that failed can have stopped it already, and
>     phy_stop() warns about that too.
> 
> That holds for PHY_READY and PHY_HALTED, but not for PHY_ERROR. Would 
> it
> make sense to have the guard accept the same states phy_stop() does?
> For example:
> 
> 	if (phy_is_started(dev->phydev) ||
> 	    dev->phydev->state == PHY_ERROR)
> 		phy_stop(dev->phydev);
> 
> The non-WoL path does not have this gap, because bcmgenet_netif_stop()
> always calls phy_stop().
> 
>>  		enable_irq(priv->irq1);
>>  	}
>> 

Will change this in v2.

--
pw-bot: cr