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

[Nicolai Buchwitz]

On 20.10.2026 14:55, netdev-bot+sashiko@xxxxxxxxxx wrote:
> [...]

> [Severity: Medium]
>
> Does this return leave UMAC_CMD at the reset-default 10 Mb/s speed when 
> the
> next WoL suspend runs?
>
> Earlier in bcmgenet_resume(), init_umac()->reset_umac() writes only
> CMD_SW_RESET to UMAC_CMD. That zeroes the speed field (CMD_SPEED_10 is 
> 0)
> and the duplex and pause bits.
>
> On the WoL rebuild path, the new phy_stop() above also leads to
> phy_link_down()->bcmgenet_mii_setup() with the link down, which clears
> RGMII_LINK.
>
> bcmgenet_mac_config() is the only code that restores the speed and 
> duplex
> bits and RGMII_LINK, and it runs only from the phylib adjust_link 
> callback.
> After this early return, bcmgenet_netif_start()/phy_start() never run, 
> so
> that callback never fires. Meanwhile phy_init_hw() and 
> genphy_config_aneg()
> have already restarted autonegotiation on the PHY, which is still 
> powered.
>
> On the next WoL suspend, the new !priv->tx_cbs branch in 
> bcmgenet_suspend()
> returns 0. Then bcmgenet_suspend_noirq()->bcmgenet_wol_power_down_cfg()
> does this:
>
> 	/* Can't suspend with WoL if MAC is still in reset */
> 	if (reg & CMD_SW_RESET)
> 		reg &= ~CMD_SW_RESET;
>
> 	/* Receiver must be enabled for WOL MP detection */
> 	reg |= CMD_RX_EN;
> 	bcmgenet_umac_writel(priv, reg, UMAC_CMD);
>
> None of this corrects the speed.
>
> If the PHY links at 1000 or 100 Mb/s, can magic packet reception fail 
> even
> though WoL looks armed? Together with the HFB_CTRL write below, a 
> failed
> resume could leave GENET with no working wake source.

Yes, but IMHO pre-existing: the init resets UMAC_CMD to 10M and only
restored by adjust_link (which doesn't run anymore once init_dma() 
fails).
Without my patch the next suspend hangs in napi_disable(), so there is 
no
wake source either way. It makes the interface recoverable, not WoL 
work.

The real fix is probably to allocate before tearing the rings down...

> [...]

> [Severity: Medium]
>
> Does writing 0 to HFB_CTRL here turn off WAKE_FILTER wake completely?
>
> bcmgenet_wol_power_down_cfg() reads HFB_CTRL after this point. It 
> unmasks
> the filter wake interrupts only if RBUF_HFB_EN is still set:
>
> 	reg = UMAC_IRQ_MPD_R;
> 	if (hfb_ctrl_reg & RBUF_HFB_EN)
> 		reg |=  UMAC_IRQ_HFB_SM | UMAC_IRQ_HFB_MM;
>
> MPD is enabled only for WAKE_MAGIC/WAKE_MAGICSECURE. With wolopts set 
> to
> WAKE_FILTER only, the system would suspend with device_may_wakeup() 
> true
> and no GENET wake source.
>
> A few lines below, the normal WoL path keeps only the RX_CLS_FLOW_WAKE
> filters armed:
>
> 		if (priv->wolopts & WAKE_FILTER)
> 			list_for_each_entry(rule, &priv->rxnfc_list, list)
> 				if (rule->fs.ring_cookie == RX_CLS_FLOW_WAKE &&
> 				    rule->state == BCMGENET_RXNFC_STATE_ENABLED)
> 					hfb_enable |= 1 << rule->fs.location;
> 		...
> 		if (!hfb_enable)
> 			reg &= ~RBUF_HFB_EN;
>
> Could this path use the same selection? Because tx_cbs is NULL, the
> following resume rebuilds the rings anyway. The device is detached 
> here,
> so the WoL setting can't be changed from ethtool (it returns -ENODEV).

Yes, keeping just the RX_CLS_FLOW_WAKE rules is better than my off.
Will do this in v3, together with the clock wording.

> [...]

> [Severity: Medium]
>
> With WoL not in use, can this early return leave an external PHY 
> powered
> for the whole sleep?

Yes, but IMHO also pre-existing: phy_init_hw() in the failed resume 
powers
it up and it isn't started again. Before this patch suspend hung 
instead.
Not leaving the half state behind is arguably a better way.

> [...]

I really need to find a WOL capable genet board, so I can test these 
code paths too ....

Nicolai

--
pw-bot: cr