[PATCH net v2 0/2] net: bcmgenet: fixes for a failed resume and the ring rebuild

[PATCH net v2 0/2] net: bcmgenet: fixes for a failed resume and the ring rebuild

[Nicolai Buchwitz]

This is v2 of my resume and ring fixes for genet. After some further
testing and local LLM review rounds I squashed v1 patches 1 to 3.

Mostly findings sashiko raised during my recent review sessions.

Patch 1 is a resume that fails in bcmgenet_init_dma(), which leaves the
interface up with no datapath and the next ifdown hanging in
napi_disable() with RTNL held. It is one patch because nothing that
resume leaves behind is reachable on its own, the second suspend never
returns until the hang is fixed.

Patch 2 (was patch 4 in v1) is the Tx coalescing threshold going back
to the driver default whenever the rings are rebuilt.

Patch 1 fixes the rebuild path, not the root cause. bcmasp for example
parks the PHY in its WoL suspend and has one deinit with a flag, which
looks like the direction for genet too. But one step at a time :)

Patch 1 is also prep for the next version of my MTU series [1], which
drops the "hallmark of a bad design" flag in favour of the ring pointers
it clears.

[1] https://patch.msgid.link/20261013-nb-genet-mtu-nn-v2-v5-0-076b6fc240be@tipi-net.de

For backports: the WoL hunks in patch 1 need 254f3239dd07, on older trees
they can be dropped. The rest applies on its own.

Patch 2 and the hang in patch 1 were tested on BCM2711 (Raspberry Pi CM4)
with the resume allocation forced to fail. Everything on the WoL path is
untested, the board has no wake source.

Florian, Justin: If you have a setup to test this, I'd appreciate it!

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Changes in v2:
- Squashed v1 patches 1 to 3 into one, each was either a no-op or left a
  broken tree on its own
- Turn HFB off on the suspend early return, it stayed armed as a wake
  source (sashiko)
- Second Fixes tag for the WoL hunks
- Let PHY_ERROR reach phy_stop() too (sashiko)
- Link to v1: https://patch.msgid.link/20261017-nb-genet-fini-dma-net-v1-0-47e7c57f6834@tipi-net.de

To: Doug Berger <opendmb@gmail.com>
To: Florian Fainelli <florian.fainelli@broadcom.com>
To: Nicolai Buchwitz <nb@tipi-net.de>
To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: Justin Chen <justin.chen@broadcom.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Nicolai Buchwitz (2):
      net: bcmgenet: recover from a failed resume
      net: bcmgenet: restore the Tx coalescing setting when the rings are rebuilt

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 56 +++++++++++++++++++++-----
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  1 +
 2 files changed, 46 insertions(+), 11 deletions(-)
---
base-commit: 8e2fbe0e75d38690d551456658194c632933cb57
change-id: 20261015-nb-genet-fini-dma-net-62a39ee14e65

Best regards,
--  
Nicolai Buchwitz <nb@tipi-net.de>

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

[Nicolai Buchwitz]

A resume that fails in bcmgenet_init_dma() returns with the interface
still IFF_UP and the datapath gone. The next ifdown runs the teardown
a second time and napi_disable() waits for a NAPI_STATE_SCHED bit that
nothing will clear:

  task:ip  state:D
    usleep_range_state+0xec/0x158
    napi_disable_locked+0x254/0x318
    napi_disable+0x3c/0x68
    bcmgenet_netif_stop+0x1c8/0x318
    bcmgenet_close+0x24/0xd0
    __dev_close_many+0x18c/0x310

RTNL is held across the wait, so everything else blocks behind it.

The same resume gates the clock and detaches the device. Close releases
the clock a second time and ifup fails with -ENODEV. The hardware filters
stay armed, so the next WoL suspend wakes on any frame. That suspend sets
CMD_RX_EN for magic packet detection, so the WoL resume reuses rings that
are gone.

A failed resume should leave a state the ordinary paths can handle. Clear
rx_cbs and tx_cbs as they are freed so they answer whether a datapath
exists, and leave the hardware as bcmgenet_resume_noirq() set it up so
close and suspend finish the teardown once. The WoL resume rebuilds
instead, so stop the PHY first, phy_start() only takes PHY_READY or
PHY_HALTED.

Flagged in a sashiko review and reproduced on a CM4 by forcing the resume
allocation to fail.

Fixes: b6e978e50444 ("net: bcmgenet: add suspend/resume callbacks")
Fixes: 254f3239dd07 ("net: bcmgenet: revise suspend/resume")	# for the WoL parts
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 46 +++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index e8908916558b1..34acd3c47cc55 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3107,7 +3107,9 @@ static void bcmgenet_fini_dma(struct bcmgenet_priv *priv)
 	bcmgenet_free_rx_buffers(priv);
 	bcmgenet_destroy_rx_page_pools(priv);
 	kfree(priv->rx_cbs);
+	priv->rx_cbs = NULL;
 	kfree(priv->tx_cbs);
+	priv->tx_cbs = NULL;
 }
 
 /* init_edma: Initialize DMA control register */
@@ -3165,6 +3167,7 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx)
 	priv->tx_cbs = kzalloc_objs(struct enet_cb, priv->num_tx_bds);
 	if (!priv->tx_cbs) {
 		kfree(priv->rx_cbs);
+		priv->rx_cbs = NULL;
 		return -ENOMEM;
 	}
 
@@ -3184,7 +3187,9 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx)
 		bcmgenet_free_rx_buffers(priv);
 		bcmgenet_destroy_rx_page_pools(priv);
 		kfree(priv->rx_cbs);
+		priv->rx_cbs = NULL;
 		kfree(priv->tx_cbs);
+		priv->tx_cbs = NULL;
 		return ret;
 	}
 
@@ -3493,7 +3498,9 @@ static int bcmgenet_close(struct net_device *dev)
 
 	netif_dbg(priv, ifdown, dev, "bcmgenet_close\n");
 
-	bcmgenet_netif_stop(dev, false);
+	/* A failed resume has torn the datapath down already */
+	if (priv->tx_cbs)
+		bcmgenet_netif_stop(dev, false);
 
 	/* Really kill the PHY state machine and disconnect from it */
 	phy_disconnect(dev->phydev);
@@ -3501,6 +3508,9 @@ static int bcmgenet_close(struct net_device *dev)
 	free_irq(priv->irq0, priv);
 	free_irq(priv->irq1, priv);
 
+	/* A failed resume left the device detached, __dev_open() refuses that */
+	netif_device_attach(dev);
+
 	if (priv->internal_phy)
 		ret = bcmgenet_power_down(priv, GENET_POWER_PASSIVE);
 
@@ -4258,7 +4268,11 @@ static int bcmgenet_resume(struct device *d)
 
 	if (device_may_wakeup(d) && priv->wolopts) {
 		reg = bcmgenet_umac_readl(priv, UMAC_CMD);
-		if (reg & CMD_RX_EN) {
+
+		/* The WoL power down sets CMD_RX_EN itself, so the bit
+		 * alone does not mean the rings survived
+		 */
+		if (priv->tx_cbs && (reg & CMD_RX_EN)) {
 			/* Successfully exited WoL, just resume data flows */
 			list_for_each_entry(rule, &priv->rxnfc_list, list)
 				if (rule->state == BCMGENET_RXNFC_STATE_ENABLED)
@@ -4288,6 +4302,9 @@ static int bcmgenet_resume(struct device *d)
 		bcmgenet_rdma_disable(priv);
 		bcmgenet_intr_disable(priv);
 		bcmgenet_fini_dma(priv);
+		if (phy_is_started(dev->phydev) ||
+		    dev->phydev->state == PHY_ERROR)
+			phy_stop(dev->phydev);
 		enable_irq(priv->irq1);
 	}
 
@@ -4310,8 +4327,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;
 	}
 
 	if (!device_may_wakeup(d))
@@ -4322,12 +4340,6 @@ static int bcmgenet_resume(struct device *d)
 	netif_device_attach(dev);
 
 	return 0;
-
-out_clk_disable:
-	if (priv->internal_phy)
-		bcmgenet_power_down(priv, GENET_POWER_PASSIVE);
-	clk_disable_unprepare(priv->clk);
-	return ret;
 }
 
 static int bcmgenet_suspend(struct device *d)
@@ -4340,6 +4352,22 @@ static int bcmgenet_suspend(struct device *d)
 	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) {
+			/* No rings to receive into, so no filter wake */
+			bcmgenet_hfb_reg_writel(priv, 0, HFB_CTRL);
+			bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF,
+						 INTRL2_CPU_MASK_SET);
+			bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF,
+						 INTRL2_CPU_CLEAR);
+			disable_irq(priv->irq1);
+		}
+		return 0;
+	}
+
 	netif_device_detach(dev);
 
 	if (device_may_wakeup(d) && priv->wolopts) {

-- 
2.53.0

[PATCH net v2 2/2] net: bcmgenet: restore the Tx coalescing setting when the rings are rebuilt

[Nicolai Buchwitz]

bcmgenet_set_coalesce() writes tx_max_coalesced_frames straight into
DMA_MBUF_DONE_THRESH and keeps no copy of it, while
bcmgenet_init_tx_ring() programs a fixed 1. An ifdown and ifup or a resume
then puts the Tx coalescing back to one frame per interrupt.

Rx keeps its value per ring and reapplies it on init. Do the same for Tx.

Fixes: 2f9130709d2c ("net: bcmgenet: Implement TX coalescing control knobs")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 ++++++++--
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 34acd3c47cc55..8db82d924d218 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -931,10 +931,13 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
 	/* Program all TX queues with the same values, as there is no
 	 * ethtool knob to do coalescing on a per-queue basis
 	 */
-	for (i = 0; i <= priv->hw_params->tx_queues; i++)
+	for (i = 0; i <= priv->hw_params->tx_queues; i++) {
+		priv->tx_rings[i].tx_max_coalesced_frames =
+			ec->tx_max_coalesced_frames;
 		bcmgenet_tdma_ring_writel(priv, i,
 					  ec->tx_max_coalesced_frames,
 					  DMA_MBUF_DONE_THRESH);
+	}
 
 	for (i = 0; i <= priv->hw_params->rx_queues; i++)
 		bcmgenet_set_ring_rx_coalesce(&priv->rx_rings[i], ec);
@@ -2748,7 +2751,8 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
 
 	bcmgenet_tdma_ring_writel(priv, index, 0, TDMA_PROD_INDEX);
 	bcmgenet_tdma_ring_writel(priv, index, 0, TDMA_CONS_INDEX);
-	bcmgenet_tdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH);
+	bcmgenet_tdma_ring_writel(priv, index, ring->tx_max_coalesced_frames,
+				  DMA_MBUF_DONE_THRESH);
 	/* Disable rate control for now */
 	bcmgenet_tdma_ring_writel(priv, index, flow_period_val,
 				  TDMA_FLOW_PERIOD);
@@ -4166,6 +4170,8 @@ static int bcmgenet_probe(struct platform_device *pdev)
 		priv->rx_rings[i].index = i;
 		priv->rx_rings[i].rx_max_coalesced_frames = 1;
 	}
+	for (i = 0; i <= priv->hw_params->tx_queues; i++)
+		priv->tx_rings[i].tx_max_coalesced_frames = 1;
 
 	/* Initialize u64 stats seq counter for 32bit machines */
 	for (i = 0; i <= GENET_MAX_MQ_CNT; i++) {
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 86f2aed20dbe8..e48ce39d3e7c8 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -553,6 +553,7 @@ struct bcmgenet_tx_ring {
 	unsigned int	prod_index;	/* Tx ring producer index SW copy */
 	unsigned int	cb_ptr;		/* Tx ring initial CB ptr */
 	unsigned int	end_ptr;	/* Tx ring end CB ptr */
+	unsigned int	tx_max_coalesced_frames;
 	struct bcmgenet_priv *priv;
 };
 

-- 
2.53.0

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · 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 resume, the next Wake-on-LAN suspend can no
  longer wake on the user's configured wake filters.
- [Medium] When WoL is not in use, the new `!priv->tx_cbs` early return
  in bcmgenet_suspend() (bcmgenet.c:4358-4368) does nothing to the PHY.
- [Medium] On the new ringless WoL suspend path after a failed DMA
  rebuild, the receiver is enabled while UniMAC still has its reset-…

--

net: bcmgenet: recover from a failed resume

This patch changes what happens when a resume fails in bcmgenet_init_dma(),
so that close and suspend can still handle the device. NULL rx_cbs/tx_cbs
now means there is no datapath. The block stays clocked on failure, close
reattaches the device, bcmgenet_suspend() gets a new path for the no-rings
case, and the WoL resume path stops the PHY before it rebuilds.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index e8908916558b1..34acd3c47cc55 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -4288,6 +4302,9 @@ static int bcmgenet_resume(struct device *d)
>  		bcmgenet_rdma_disable(priv);
>  		bcmgenet_intr_disable(priv);
>  		bcmgenet_fini_dma(priv);
> +		if (phy_is_started(dev->phydev) ||
> +		    dev->phydev->state == PHY_ERROR)
> +			phy_stop(dev->phydev);
>  		enable_irq(priv->irq1);
>  	}
>  
> @@ -4310,8 +4327,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;
>  	}

[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.

> @@ -4340,6 +4352,22 @@ static int bcmgenet_suspend(struct device *d)
>  	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) {
> +			/* No rings to receive into, so no filter wake */
> +			bcmgenet_hfb_reg_writel(priv, 0, HFB_CTRL);

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

The commit message says:

    leave the hardware as bcmgenet_resume_noirq() set it up so
    close and suspend finish the teardown once.

Is that accurate? In bcmgenet_resume(), init_umac(), phy_init_hw(),
genphy_config_aneg() and bcmgenet_hfb_restore() all run before
bcmgenet_init_dma() fails. bcmgenet_hfb_restore() has already re-armed
every rule that is not unused.

The commit message also doesn't mention that configured filter wake is lost
after a failed resume.

> +			bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF,
> +						 INTRL2_CPU_MASK_SET);
> +			bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF,
> +						 INTRL2_CPU_CLEAR);
> +			disable_irq(priv->irq1);
> +		}
> +		return 0;
> +	}

[Severity: Medium]

With WoL not in use, can this early return leave an external PHY powered
for the whole sleep?

The non-WoL path that this skips calls bcmgenet_netif_stop(dev, true). That
calls phy_stop(), which ends in phy_suspend() and powers the PHY down.

bcmgenet_mii_probe() sets phydev->mac_managed_pm = true, so MDIO bus PM
won't power the PHY down either. bcmgenet_suspend_noirq() handles only the
internal PHY.

In the failed resume, phy_init_hw() calls the PHY driver's soft_reset and
sets phydev->suspended = 0. With genphy_soft_reset, BMCR_RESET clears
BMCR_PDOWN. genphy_config_aneg() then restarts autonegotiation.

bcmgenet_init_dma() then fails before phy_resume()/phy_start() run. The
PHY is now powered and linking while phylib still has it as PHY_HALTED.

This doesn't show on the CM4's BCM54213PE, because its phy_driver entry has
no soft_reset. It looks reachable when wolopts is 0 and the external PHY's
driver has a real soft_reset (such as genphy_soft_reset).

Does this match the commit message's statement that suspend will "finish
the teardown once"?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-nb-genet-fini-dma-net-v2-0-b85f6b92654a%40tipi-net.de

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

[Nicolai Buchwitz]

On 20.10.2026 14:55, netdev-bot+sashiko@kernel.org 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

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

[Florian Fainelli]

On 10/20/26 07:16, Nicolai Buchwitz wrote:
[snip]
>> [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 ....

It should be somehow possible to exercise that code path on a CM4 by 
having the system enter the "freeze" sleep state and have GENET 
configured for WoL. We can discuss offline how to get there if you are 
having issues with that.
-- 
Florian