[PATCH net v2] net: stmmac: mask the MAC interrupt while resume resets the MAC

[PATCH net v2] net: stmmac: mask the MAC interrupt while resume resets the MAC

[Igor Velkov]

stmmac_resume() resets the MAC in stmmac_hw_setup(). On dwmac1000 the
reset sets GMAC_INT_MASK to 0, which unmasks every core interrupt, until
dwmac1000_core_init() writes the mask again. Since commit 2e2c878a3141
("net: stmmac: remove SGMII/RGMII/SMII interrupt handling") nothing on
the RGMII path reads GMAC_RGSMIIIS (only the PCS code does), so RGSMIIIS
stays pending behind the mask from the first link change on, and the
reset lets it through. The line storms:

  dwmac_dma_interrupt: unexpected status 04000000

A register dump at the first message on ODROID-HC4 showed it:

  core int_status 00000001 int_mask 00000000 pmt 00000000 rgsmiiis 0000000d

The mask is written by the resume thread itself. When that thread runs
on the CPU that takes the MAC interrupt, the storm starves it and the
board hangs until a watchdog reset or a soft-lockup panic; on another
CPU the storm ends after a few interrupts.

Disable the interrupt line for the whole resume and enable it again on
every exit path. This matches open(), which resets the MAC before it
requests the interrupt; resume resets it with the line live. The line
is IRQF_SHARED, so leave it enabled during suspend, where another user
of the line may wake the system.

Wake-on-LAN resume with the resume thread on the CPU that takes the MAC
interrupt, 7.3-rc5: without this patch the storm hit the first resume
on Helios64 (dwmac-rk) twice, ROCK Pi 4A (dwmac-rk) once and ODROID-HC4
(dwmac-meson8b) twice; with it, no storm in 10 resumes on Helios64 and
10 on ODROID-HC4. With the thread on another CPU and no patch, the
storm printed at most 3 messages per resume in 30 resumes.

Fixes: 2e2c878a3141 ("net: stmmac: remove SGMII/RGMII/SMII interrupt handling")
Assisted-by: LLM
Signed-off-by: Igor Velkov <iav@iav.lv>
---
v2:
- target net with a Fixes tag; v1 was posted for net-next
- describe the RGSMIIIS bit pending since link-up and the resume thread
  on the interrupt CPU instead of a link change in the reset window;
  the code comment is fixed the same way
- results on Helios64, ROCK Pi 4A and ODROID-HC4
v1: https://lore.kernel.org/r/20260930-stmmac-irq-shut-v1-1-104d1a1dcb28@iav.lv

Build-tested on net: allmodconfig and allyesconfig with W=1, no new
warnings.

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 27 ++++++++++++++-----
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ec62fa7418f4..b275b6234b55 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -8313,15 +8313,25 @@ int stmmac_resume(struct device *dev)
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct stmmac_priv *priv = netdev_priv(ndev);
 	int ret;
+	bool irq_off;
+
+	/* Resume resets the MAC, leaving every core interrupt unmasked until
+	 * the mask is written again: a status bit already pending would storm.
+	 */
+	irq_off = netif_running(ndev);
+	if (irq_off)
+		disable_irq(ndev->irq);
 
 	if (priv->plat->resume) {
 		ret = priv->plat->resume(dev, priv->plat->bsp_priv);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
-	if (!netif_running(ndev))
-		return 0;
+	if (!netif_running(ndev)) {
+		ret = 0;
+		goto out;
+	}
 
 	/* Power Down bit, into the PM register, is cleared
 	 * automatically as soon as a magic packet or a Wake-up frame
@@ -8344,7 +8354,7 @@ int stmmac_resume(struct device *dev)
 	if (!(priv->plat->flags & STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP)) {
 		ret = stmmac_legacy_serdes_power_up(priv);
 		if (ret < 0)
-			return ret;
+			goto out;
 	}
 
 	rtnl_lock();
@@ -8367,7 +8377,7 @@ int stmmac_resume(struct device *dev)
 		stmmac_legacy_serdes_power_down(priv);
 		mutex_unlock(&priv->lock);
 		rtnl_unlock();
-		return ret;
+		goto out;
 	}
 
 	stmmac_init_timestamping(priv);
@@ -8393,7 +8403,12 @@ int stmmac_resume(struct device *dev)
 
 	netif_device_attach(ndev);
 
-	return 0;
+	ret = 0;
+out:
+	if (irq_off)
+		enable_irq(ndev->irq);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(stmmac_resume);
 
-- 
2.43.0

Re: [PATCH net v2] net: stmmac: mask the MAC interrupt while resume resets the MAC

[Igor Velkov]

This conflicts with net-next commit 8181678a92f0 ("net: stmmac:
propagate PTP init failures in __stmmac_open() and stmmac_resume()").
The success path has to jump to out:, or the MAC IRQ stays disabled
after every resume and the build does not catch it. Resolution, built
with W=1 on net-next 071876fd5048:

diff --cc drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0c381ae0d0ff,b275b6234b55..000000000000
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@@ -8467,15 -8403,10 +8477,19 @@@ init_coalesce
  
  	netif_device_attach(ndev);
  
- 	return 0;
+ 	ret = 0;
++	goto out;
 +
 +error_stop_dma:
 +	stmmac_stop_all_dma(priv);
 +	stmmac_mac_set(priv, priv->ioaddr, false);
 +error_unlock:
 +	stmmac_legacy_serdes_power_down(priv);
 +	mutex_unlock(&priv->lock);
 +	rtnl_unlock();
+ out:
+ 	if (irq_off)
+ 		enable_irq(ndev->irq);
  
  	return ret;
  }

+Cc Lorenzo.

--
Igor Velkov

Re: [PATCH net v2] net: stmmac: mask the MAC interrupt while resume resets the MAC

[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] If stmmac_hw_setup() fails because stmmac_reset() times out in
  stmmac_init_dma_engine() ("Failed to reset the dma"),…
- [Medium] stmmac_resume() disables ndev->irq at the very top and re-
  enables it only after netif_device_attach().
- [Medium] The disable_irq()/enable_irq() pair in stmmac_resume() is not
  serialized against stmmac_reset_subtask().

--

net: stmmac: mask the MAC interrupt while resume resets the MAC

This patch disables ndev->irq for all of stmmac_resume() when the
interface is running, and re-enables it on every exit path. The goal is
to stop an RGSMIIIS status that is already pending from storming the line
between the MAC reset in stmmac_hw_setup() and the GMAC_INT_MASK write in
dwmac1000_core_init().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec62fa7418f41..b275b6234b557 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -8313,15 +8313,25 @@ int stmmac_resume(struct device *dev)
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  	int ret;
> +	bool irq_off;
> +
> +	/* Resume resets the MAC, leaving every core interrupt unmasked until
> +	 * the mask is written again: a status bit already pending would storm.
> +	 */
> +	irq_off = netif_running(ndev);
> +	if (irq_off)
> +		disable_irq(ndev->irq);

[Severity: Medium]
In single-IRQ mode the line is requested as shared:

stmmac_request_irq_single() {
	ret = request_irq(dev->irq, stmmac_interrupt,
			  IRQF_SHARED, dev->name, dev);
}

disable_irq() masks the descriptor for every action on that line. The
commit message uses the same sharing argument to leave the line enabled
during suspend.

Is it intended that resume keeps the line masked for every sharer through
priv->plat->resume(), stmmac_mdio_reset(), stmmac_legacy_serdes_power_up(),
rtnl_lock(), mutex_lock(&priv->lock), phylink_prepare_resume(), the DMA
SWR poll (up to 200 ms) and phylink_resume()?

The window that needs protection seems to run only from the SWR in
stmmac_init_dma_engine() to stmmac_core_init(), and both are inside
stmmac_hw_setup().

Could another device on the line that resumes at the same time (for
example an async-resumed PCI function on the same INTx line with
stmmac_pci, or the Loongson glue without MSI) have its completion
interrupts held off long enough to time out?

The masked window also covers blocking on rtnl_lock(). Could a sharer
that holds rtnl and waits for its own interrupt on this line with no
timeout deadlock here? Lockdep cannot see that dependency.

>  
>  	if (priv->plat->resume) {
>  		ret = priv->plat->resume(dev, priv->plat->bsp_priv);

[ ... ]

> @@ -8367,7 +8377,7 @@ int stmmac_resume(struct device *dev)
>  		stmmac_legacy_serdes_power_down(priv);
>  		mutex_unlock(&priv->lock);
>  		rtnl_unlock();
> -		return ret;
> +		goto out;
>  	}

[Severity: Medium]
If stmmac_hw_setup() fails because stmmac_reset() times out, can this
path turn the line back on while the core interrupts are still unmasked?

stmmac_hw_setup()
  stmmac_init_dma_engine()
    stmmac_reset()        <- SWR asserted, poll times out
    netdev_err(priv->dev, "Failed to reset the dma\n");
    return ret;
  return ret;             <- before stmmac_core_init()

On this path, stmmac_core_init()->dwmac1000_core_init() is the only code
that writes GMAC_INT_DEFAULT_MASK back to GMAC_INT_MASK, so the mask is
never restored. The goto out then calls enable_irq(ndev->irq)
unconditionally.

Suppose the mask has already returned to its reset value of 0, or the
reset finishes late after the 200 ms poll in dwmac_dma_reset() has given
up. Then the latched RGSMIIIS bit reaches a live line.
stmmac_interrupt() never clears that source and still returns
IRQ_HANDLED. Wouldn't that be the same storm this patch is meant to
prevent?

The comment in stmmac_hw_setup() says clk_rx_i must be running for the
reset to complete, and a stopped clk_rx_i is the known cause of SWR
timeouts.

This path behaved the same way before the patch, because the line was
always live. It is still a resume exit that the fix does not cover.
Would re-masking the core interrupts, or acknowledging the source, on
this failure path close it?

>  
>  	stmmac_init_timestamping(priv);

[ ... ]

> @@ -8393,7 +8403,12 @@ int stmmac_resume(struct device *dev)
>  
>  	netif_device_attach(ndev);
>  
> -	return 0;
> +	ret = 0;
> +out:
> +	if (irq_off)
> +		enable_irq(ndev->irq);

[Severity: Medium]
Can this enable_irq() become unbalanced if stmmac_reset_subtask() runs
between rtnl_unlock() and here?

priv->wq comes from create_singlethread_workqueue() and is not
freezable. stmmac_suspend() does not cancel or flush the reset work
either.

A reset can be requested around resume, for example from
stmmac_safety_interrupt() on a separate sfty_irq line, which resume does
not disable. That goes through stmmac_global_err() ->
stmmac_service_event_schedule(), and STMMAC_DOWN is not set across
suspend/resume.

The worker blocks on rtnl while resume holds it, then:

stmmac_resume()                    stmmac_reset_subtask()
  disable_irq(ndev->irq)
  ...
  rtnl_unlock()
  netif_device_attach()
  <preempted>
                                     dev_close()
                                       stmmac_release()
                                         free_irq()
                                     dev_open()
                                       request_irq()
                                         irq_startup()
                                           desc->depth = 0
  enable_irq(ndev->irq)
    __enable_irq() with depth 0

Wouldn't this trigger the "Unbalanced enable for IRQ %d" WARN, which
becomes a panic with panic_on_warn? The line itself ends up enabled,
because request_irq() already started it. Nothing serializes the window
between rtnl_unlock() and this enable_irq() against the worker.

> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(stmmac_resume);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002043558.1302590-1-iav%40iav.lv