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

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

[Nicolai Buchwitz]

Mostly findings sashiko raised during my recent review sessions.

Patches 2 and 3 are the same failure, a resume that fails in
bcmgenet_init_dma(). The interface stays up with the datapath gone, and
the next ifdown hangs in napi_disable() with RTNL held. Patch 2 stops the
second teardown, patch 3 gets the interface open again instead of leaving
it detached with the clock released twice. Forcing the resume allocation
to fail reproduces the hang, the box then needs sysrq-b.

Patch 1 comes first because patch 2 sends one more case down the WoL
resume path, which calls phy_start() on a PHY the WoL suspend never
stopped.

Patch 4 is about the ring rebuild itself. The Tx coalescing threshold goes
back to the driver default whenever the rings are rebuilt, throwing away
what was set through ethtool. "ethtool -C eth0 tx-frames 16" followed by a
rebuild reports the default again.

Patch 1 fixes the rebuild path, not the cause. bcmasp 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 2 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

Patches 2 to 4 were tested on BCM2711 (Raspberry Pi CM4) with the resume
allocation forced to fail. The WoL paths in patches 1 and 2 are untested,
the board has no wake source.

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Nicolai Buchwitz (4):
      net: bcmgenet: stop the PHY when a WoL resume rebuilds the datapath
      net: bcmgenet: do not tear the datapath down twice after a failed resume
      net: bcmgenet: let the interface be reopened after a failed resume
      net: bcmgenet: restore the Tx coalescing setting when the rings are rebuilt

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 49 ++++++++++++++++++++------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  1 +
 2 files changed, 39 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 1/4] net: bcmgenet: stop the PHY when a WoL resume rebuilds the datapath

[Nicolai Buchwitz]

A WoL suspend leaves the PHY running. If the resume then finds the MAC
was reset it tears the rest down and rebuilds, which ends in phy_start().
That warns and does nothing unless the PHY is PHY_READY or PHY_HALTED.

Stop the PHY with the rest of the teardown, but only while it is still
running. A rebuild that failed can have stopped it already, and
phy_stop() warns about that too.

Fixes: 254f3239dd07 ("net: bcmgenet: revise suspend/resume")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 +++
 1 file changed, 3 insertions(+)

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);
 		enable_irq(priv->irq1);
 	}
 

-- 
2.53.0

[PATCH net 2/4] net: bcmgenet: do not tear the datapath down twice after a failed resume

[Nicolai Buchwitz]

If bcmgenet_resume() fails in bcmgenet_init_dma() it returns with the
interface still IFF_UP. The suspend before it already deleted the NAPIs and
freed rx_cbs and tx_cbs, so the next ifdown runs the whole teardown again
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 every other rtnl_lock() user blocks behind
it. A second suspend ends up in the same place.

Flagged in a sashiko review, found plausible and then reproduced on a CM4.

Clear rx_cbs and tx_cbs as they are freed, so close and suspend can tell
there is nothing left to tear down.

The WoL resume path needs the same check. It reuses the rings when it sees
the receiver enabled. But after a failed resume the MAC sits in software
reset, and the next link event takes it out and enables it. Reusing rings
that are gone reattaches the interface and the first transmit hits
BUG_ON(!tx_cb_ptr), so check the rings too.

Fixes: b6e978e50444 ("net: bcmgenet: add suspend/resume callbacks")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 308716cddc077..fe3338ba81980 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);
@@ -4258,7 +4265,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) {
+
+		/* A link event can enable the receiver after a failed
+		 * resume, so check the rings too
+		 */
+		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)
@@ -4343,6 +4354,15 @@ 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)
+			disable_irq(priv->irq1);
+		return 0;
+	}
+
 	netif_device_detach(dev);
 
 	if (device_may_wakeup(d) && priv->wolopts) {

-- 
2.53.0

[PATCH net 3/4] net: bcmgenet: let the interface be reopened after a failed resume

[Nicolai Buchwitz]

bcmgenet_suspend() detaches the device and a resume that fails in
bcmgenet_init_dma() never attaches it again, so an ifdown and ifup cannot
bring the interface back:

  # ip link set eth0 up
  RTNETLINK answers: No such device

That resume also gates the clock and powers the block down before it
returns. Close and the next suspend_noirq then release a clock that is
already released, which the clock core warns about. Leave the hardware as
bcmgenet_resume_noirq() set it up, so the clock stays on while the
interface is up and close or suspend releases it once.

Attach again in close, where __dev_close_many() has already cleared
__LINK_STATE_START, so no queue is woken.

Fixes: b6e978e50444 ("net: bcmgenet: add suspend/resume callbacks")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index fe3338ba81980..cb9951d6036fb 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3508,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);
 
@@ -4324,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))
@@ -4336,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)

-- 
2.53.0

[PATCH net 4/4] 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 cb9951d6036fb..d4de88bbbfbad 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 1/4] net: bcmgenet: stop the PHY when a WoL resume rebuilds the datapath

[netdev-bot+sashiko]

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

Re: [PATCH net 2/4] net: bcmgenet: do not tear the datapath down twice after a failed resume

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 2

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] With this patch alone, bcmgenet_resume() still drops the
  priv->clk reference in out_clk_disable when bcmgenet_init_dma() fails,…
- [Medium] With this patch alone, after a failed resume the netdev stays
  detached.
- [Medium] Before calling bcmgenet_init_dma(), bcmgenet_resume() always
  calls bcmgenet_hfb_restore(), which goes through bcmgenet_hfb_clear().
- [Low] The patch fixes two bugs from different origins but carries only
  one Fixes tag.
- [Low] The commit message and the new comment in bcmgenet_resume() (`/*
  A link event can enable the receiver after a failed resume, so check…

--

net: bcmgenet: do not tear the datapath down twice after a failed resume

This patch sets priv->rx_cbs and priv->tx_cbs to NULL when they are freed.
bcmgenet_close() and bcmgenet_suspend() then check priv->tx_cbs and skip the
datapath teardown after a failed bcmgenet_resume(). The WoL resume path also
stops reusing rings that are gone when it sees CMD_RX_EN set.

> Fixes: b6e978e50444 ("net: bcmgenet: add suspend/resume callbacks")

[Severity: Low]

This isn't a bug, but should the WoL hunk have a second Fixes tag?

The priv->tx_cbs check added to the CMD_RX_EN test in bcmgenet_resume()
guards the "Successfully exited WoL, just resume data flows" branch, which
reuses the rings. That branch came from 254f3239dd07 ("net: bcmgenet: revise
suspend/resume"), not b6e978e50444. The previous patch in this series, "net:
bcmgenet: stop the PHY when a WoL resume rebuilds the datapath", already uses
Fixes: 254f3239dd07 for the same block.

A Fixes: 254f3239dd07 tag would record where the WoL hunk comes from. It
would also show that the hunk only applies to trees that contain that commit.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 308716cddc077..fe3338ba81980 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

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

[Severity: Medium]

Now that this check lets close go further, can priv->clk be released twice
after a failed resume?

When bcmgenet_init_dma() fails, bcmgenet_resume() still goes through
out_clk_disable, and the interface stays netif_running():

out_clk_disable:
	if (priv->internal_phy)
		bcmgenet_power_down(priv, GENET_POWER_PASSIVE);
	clk_disable_unprepare(priv->clk);
	return ret;

Before this patch, the next ifdown hung in napi_disable(). Now
bcmgenet_close() gets past the check and reaches this code again:

	if (priv->internal_phy)
		ret = bcmgenet_power_down(priv, GENET_POWER_PASSIVE);

	clk_disable_unprepare(priv->clk);

A second suspend has the same problem. The new early return in
bcmgenet_suspend() returns 0, so bcmgenet_suspend_noirq() calls
bcmgenet_power_down() and clk_disable_unprepare(priv->clk) once more.

Would this underflow the clk enable/prepare counts? And would the power-down
register writes (EXT_EXT_PWR_MGMT, or the WoL MPD/HFB/UMAC_CMD sequence) hit
a block that has no clock?

The next patch in the series, "net: bcmgenet: let the interface be reopened
after a failed resume", removes out_clk_disable and keeps the clock enabled
on this failure path. So this only affects this intermediate commit. Could
that change be folded in here or moved before this patch?

[Severity: Medium]

Is the netdev still detached once bcmgenet_close() completes after a failed
resume?

bcmgenet_suspend() called netif_device_detach(dev). The init_dma failure path
in bcmgenet_resume() returns without calling netif_device_attach(), and
bcmgenet_close() does not re-attach either. On the next ifup, __dev_open()
sees that the device is not present and returns -ENODEV.

The next patch in the series, "net: bcmgenet: let the interface be reopened
after a failed resume", adds netif_device_attach(dev) to bcmgenet_close()
after free_irq(). So this also only affects this intermediate commit.

[ ... ]

> @@ -4258,7 +4265,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) {
> +
> +		/* A link event can enable the receiver after a failed
> +		 * resume, so check the rings too
> +		 */

[Severity: Low]

Is a link event really what sets CMD_RX_EN here? The commit message gives
the same cause:

  But after a failed resume the MAC sits in software reset, and the
  next link event takes it out and enables it.

After a failed resume, the PHY state machine is not handling link changes:

- On the non-WoL path, bcmgenet_netif_stop(dev, true) in suspend already
  called phy_stop().
- On the WoL path, the fallback calls phy_stop() if the PHY was started.
  Otherwise bcmgenet_wol_power_down_cfg() leaves phydev->state = PHY_READY.

Either way, bcmgenet_mii_setup()->bcmgenet_mac_config() is not reached. That
is the code that clears CMD_SW_RESET and sets CMD_TX_EN | CMD_RX_EN on link
up.

The bit seems to come from the next WoL suspend instead. It takes the new
early return in bcmgenet_suspend(), and then runs
bcmgenet_suspend_noirq()->bcmgenet_power_down()->bcmgenet_wol_power_down_cfg():

	/* 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_wol_power_up_cfg() leaves RX_EN set when it puts the MAC back into
SW_RESET. bcmgenet_resume() then sees CMD_RX_EN with no rings.

The added priv->tx_cbs check covers this case anyway. Could the comment and
the commit message describe this path instead?

> +		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)

[ ... ]

> @@ -4343,6 +4354,15 @@ 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)
> +			disable_irq(priv->irq1);
> +		return 0;
> +	}

[Severity: Medium]

With WoL enabled, can this early return arm the default filter and the
non-wake HFB filters as wake sources?

bcmgenet_resume() calls bcmgenet_hfb_restore() before the
bcmgenet_init_dma() call that can fail. That goes through
bcmgenet_hfb_clear(), which enables the catch-all filter 0 and sets
RBUF_HFB_EN. It then restores every runtime rxnfc filter. Nothing undoes
this when init_dma fails.

On the next suspend, this return skips the "Suspend non-wake Rx data flows"
block further down. That block normally keeps only the RX_CLS_FLOW_WAKE
rules enabled and clears RBUF_HFB_EN when there are none:

		} else {
			bcmgenet_hfb_reg_writel(priv, hfb_enable << 1,
						HFB_FLT_ENABLE_V3PLUS + 4);
		}
		if (!hfb_enable)
			reg &= ~RBUF_HFB_EN;

bcmgenet_suspend_noirq() still calls
bcmgenet_power_down(GENET_POWER_WOL_MAGIC), and
bcmgenet_wol_power_down_cfg() then does:

	reg = UMAC_IRQ_MPD_R;
	if (hfb_ctrl_reg & RBUF_HFB_EN)
		reg |=  UMAC_IRQ_HFB_SM | UMAC_IRQ_HFB_MM;

Could ordinary received traffic that matches filter 0 then wake the system,
even when only magic-packet wake was requested?

This looks like it is still there at the end of the series. The later
patches keep the clock on after a failed resume, so these register writes
take effect.

>  	netif_device_detach(dev);
>  
>  	if (device_may_wakeup(d) && priv->wolopts) {

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