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

[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] After a failed non-WoL resume, the next non-WoL system suspend
  leaves the external PHY powered and linked.

--

net: bcmgenet: recover from a failed resume

This commit lets bcmgenet cope with a resume that fails in
bcmgenet_init_dma(). The driver now sets rx_cbs and tx_cbs to NULL when
it frees them, so they show whether a datapath exists, and the failure
path no longer disables the clock. Close, suspend and the WoL resume now
check that state, so they skip the datapath teardown that was already
done.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index f4cfbff49a1b..e7dec73d5b1c 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -4320,8 +4337,9 @@ static int bcmgenet_resume(struct device *d)
>  	/* Reinitialize TDMA and RDMA and SW housekeeping */
>  	ret = bcmgenet_init_dma(priv, false);
>  	if (ret) {
> +		/* Leave the block clocked, close and suspend tear it down */
>  		netdev_err(dev, "failed to initialize DMA\n");
> -		goto out_clk_disable;
> +		return ret;
>  	}

[ ... ]

>  static int bcmgenet_suspend(struct device *d)
>  {
>  	struct net_device *dev = dev_get_drvdata(d);
>  	struct bcmgenet_priv *priv = netdev_priv(dev);
> -	struct bcmgenet_rxnfc_rule *rule;
> -	u32 reg, hfb_enable = 0;
>
>  	if (!netif_running(dev))
>  		return 0;
>
> +	/* A failed resume has torn the datapath down already. Resume still
> +	 * enables irq1 on the WoL path, so it has to be disabled here.
> +	 */
> +	if (!priv->tx_cbs) {
> +		if (device_may_wakeup(d) && priv->wolopts) {
> +			bcmgenet_wol_arm_filters(priv);
> +			disable_irq(priv->irq1);
> +		}
> +		return 0;
> +	}

[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) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-nb-genet-fini-dma-net-v3-0-6e9fc89c0156%40tipi-net.de