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

[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

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.

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);
>  	}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-nb-genet-fini-dma-net-v1-0-47e7c57f6834%40tipi-net.de