[PATCH] net: stmmac: remove STMMAC_DOWN check to avoid irq storm

[PATCH] net: stmmac: remove STMMAC_DOWN check to avoid irq storm

[Xiaochuan Li]

The stmmac interrupt handlers return IRQ_HANDLED immediately when
STMMAC_DOWN is set, without reading or clearing any device status
register.

STMMAC_DOWN is set by stmmac_reset_subtask() before dev_close(). The
device interrupt is level triggered, so if it fires after STMMAC_DOWN
is set and before the IRQs are masked/freed in __stmmac_release(), the
pending status is never cleared, the line stays asserted and the handler
is re-entered forever. Since the handler returns IRQ_HANDLED,
note_interrupt() never flags the IRQ as spurious and never disables it,
resulting in an interrupt storm.

Signed-off-by: Xiaochuan Li <chuanx2070@163.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 33 ++++++++-----------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 56d89a696272..520c1b8d736c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3091,6 +3091,13 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
 
 	ret = stmmac_safety_feat_irq_status(priv, priv->dev,
 			priv->ioaddr, priv->dma_cap.asp, &priv->sstats);
+
+	/* Status is read/cleared; do not escalate to a global error reset
+	 * while the interface is going down.
+	 */
+	if (test_bit(STMMAC_DOWN, &priv->state))
+		return false;
+
 	if (ret && (ret != -EINVAL)) {
 		stmmac_global_err(priv);
 		return true;
@@ -3113,6 +3120,12 @@ static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan, u32 dir)
 	rx_napi = rx_q->xsk_pool ? &ch->rxtx_napi : &ch->rx_napi;
 	tx_napi = tx_q->xsk_pool ? &ch->rxtx_napi : &ch->tx_napi;
 
+	/* Status was already read (and clear) above, so the irq line is
+	 * deasserted, Drop the event while the interface is going down.
+	 */
+	if (test_bit(STMMAC_DOWN, &priv->state))
+		return 0;
+
 	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
 		if (napi_schedule_prep(rx_napi)) {
 			spin_lock_irqsave(&ch->lock, flags);
@@ -6408,10 +6421,6 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct stmmac_priv *priv = netdev_priv(dev);
 
-	/* Check if adapter is up */
-	if (test_bit(STMMAC_DOWN, &priv->state))
-		return IRQ_HANDLED;
-
 	/* Check ASP error if it isn't delivered via an individual IRQ */
 	if (priv->sfty_irq <= 0 && stmmac_safety_feat_interrupt(priv))
 		return IRQ_HANDLED;
@@ -6430,10 +6439,6 @@ static irqreturn_t stmmac_mac_interrupt(int irq, void *dev_id)
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct stmmac_priv *priv = netdev_priv(dev);
 
-	/* Check if adapter is up */
-	if (test_bit(STMMAC_DOWN, &priv->state))
-		return IRQ_HANDLED;
-
 	/* To handle Common interrupts */
 	stmmac_common_interrupt(priv);
 
@@ -6445,10 +6450,6 @@ static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id)
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct stmmac_priv *priv = netdev_priv(dev);
 
-	/* Check if adapter is up */
-	if (test_bit(STMMAC_DOWN, &priv->state))
-		return IRQ_HANDLED;
-
 	/* Check if a fatal error happened */
 	stmmac_safety_feat_interrupt(priv);
 
@@ -6466,10 +6467,6 @@ static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
 	dma_conf = container_of(tx_q, struct stmmac_dma_conf, tx_queue[chan]);
 	priv = container_of(dma_conf, struct stmmac_priv, dma_conf);
 
-	/* Check if adapter is up */
-	if (test_bit(STMMAC_DOWN, &priv->state))
-		return IRQ_HANDLED;
-
 	status = stmmac_napi_check(priv, chan, DMA_DIR_TX);
 
 	if (unlikely(status & tx_hard_error_bump_tc)) {
@@ -6492,10 +6489,6 @@ static irqreturn_t stmmac_msi_intr_rx(int irq, void *data)
 	dma_conf = container_of(rx_q, struct stmmac_dma_conf, rx_queue[chan]);
 	priv = container_of(dma_conf, struct stmmac_priv, dma_conf);
 
-	/* Check if adapter is up */
-	if (test_bit(STMMAC_DOWN, &priv->state))
-		return IRQ_HANDLED;
-
 	stmmac_napi_check(priv, chan, DMA_DIR_RX);
 
 	return IRQ_HANDLED;
-- 
2.43.0

Re: [PATCH] net: stmmac: remove STMMAC_DOWN check to avoid irq storm

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net: stmmac: remove STMMAC_DOWN check to avoid irq storm

[Maxime Chevallier]

Hi,

First thing first, you're missing the target git tree, net or net-next. It's
likely net-next, unless this is a real crash you're facing.

On 10/15/26 13:24, Xiaochuan Li wrote:
> The stmmac interrupt handlers return IRQ_HANDLED immediately when
> STMMAC_DOWN is set, without reading or clearing any device status
> register.
> 
> STMMAC_DOWN is set by stmmac_reset_subtask() before dev_close(). The
> device interrupt is level triggered, so if it fires after STMMAC_DOWN
> is set and before the IRQs are masked/freed in __stmmac_release(), the
> pending status is never cleared, the line stays asserted and the handler
> is re-entered forever. Since the handler returns IRQ_HANDLED,
> note_interrupt() never flags the IRQ as spurious and never disables it,
> resulting in an interrupt storm.
> 
> Signed-off-by: Xiaochuan Li <chuanx2070@163.com>

This STMMAC_DOWN flag is quite misleading :( it's only set in the
stmmac_reset_subtask() and there's no explanation at all in the git history about
why this flag is even here...

How did you find that problem ? Do you have a use-case that triggered the
storm ?

I'm wondering if the right move isn't just to get rid of the flag entirely, I'm
failing to see what it brings.

> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 33 ++++++++-----------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 56d89a696272..520c1b8d736c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3091,6 +3091,13 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
>  
>  	ret = stmmac_safety_feat_irq_status(priv, priv->dev,
>  			priv->ioaddr, priv->dma_cap.asp, &priv->sstats);
> +
> +	/* Status is read/cleared; do not escalate to a global error reset
> +	 * while the interface is going down.
> +	 */
> +	if (test_bit(STMMAC_DOWN, &priv->state))
> +		return false;
> +
>  	if (ret && (ret != -EINVAL)) {
>  		stmmac_global_err(priv);
>  		return true;
> @@ -3113,6 +3120,12 @@ static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan, u32 dir)
>  	rx_napi = rx_q->xsk_pool ? &ch->rxtx_napi : &ch->rx_napi;
>  	tx_napi = tx_q->xsk_pool ? &ch->rxtx_napi : &ch->tx_napi;
>  
> +	/* Status was already read (and clear) above, so the irq line is
> +	 * deasserted, Drop the event while the interface is going down.
> +	 */
> +	if (test_bit(STMMAC_DOWN, &priv->state))
> +		return 0;
> +

I don't quite understand why this is added here, can you elaborate ?

Maxime

Re: [PATCH] net: stmmac: remove STMMAC_DOWN check to avoid irq storm

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

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

- [High] The patch moves the STMMAC_DOWN check so it runs after the
  hardware interrupt status has been read and cleared.
- [Low] The commit fixes a user-visible bug: an interrupt storm (a hard-
  IRQ livelock on UP, or on the CPU running the stmmac_wq reset worker)…

--

net: stmmac: remove STMMAC_DOWN check to avoid irq storm

This patch removes the early STMMAC_DOWN return from the stmmac interrupt
handlers, so the device status registers are always read and cleared. The
STMMAC_DOWN check now sits inside stmmac_safety_feat_interrupt() and
stmmac_napi_check(), after the status has been read, and events are dropped
while the bit is set. The goal is to avoid an interrupt storm on
level-triggered IRQ lines during an adapter reset.

[ ... ]

> is re-entered forever. Since the handler returns IRQ_HANDLED,
> note_interrupt() never flags the IRQ as spurious and never disables it,
> resulting in an interrupt storm.
>
> Signed-off-by: Xiaochuan Li <chuanx2070@163.com>

[Severity: Low]
Should this have a Fixes: tag? The STMMAC_DOWN early return in
stmmac_interrupt() was added by:

34877a15f787 ("net: stmmac: Rework and fix TX Timeout code")

The same check was later copied into stmmac_mac_interrupt(),
stmmac_safety_interrupt(), stmmac_msi_intr_tx() and stmmac_msi_intr_rx() by:

8532f613bc78 ("net: stmmac: introduce MSI Interrupt routines for mac, safety, RX & TX")

Without a Fixes: tag, or a Cc: stable, stable maintainers can't easily tell
that this is a fix or which trees need it.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b837e8e27a352..ec7ca45a9ba61 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3063,6 +3063,13 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
>  
>  	ret = stmmac_safety_feat_irq_status(priv, priv->dev,
>  			priv->ioaddr, priv->dma_cap.asp, &priv->sstats);
> +
> +	/* Status is read/cleared; do not escalate to a global error reset
> +	 * while the interface is going down.
> +	 */
> +	if (test_bit(STMMAC_DOWN, &priv->state))
> +		return false;
> +
>  	if (ret && (ret != -EINVAL)) {
>  		stmmac_global_err(priv);
>  		return true;
> @@ -3085,6 +3092,12 @@ static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan, u32 dir)
>  	rx_napi = rx_q->xsk_pool ? &ch->rxtx_napi : &ch->rx_napi;
>  	tx_napi = tx_q->xsk_pool ? &ch->rxtx_napi : &ch->tx_napi;
>  
> +	/* Status was already read (and clear) above, so the irq line is
> +	 * deasserted, Drop the event while the interface is going down.
> +	 */
> +	if (test_bit(STMMAC_DOWN, &priv->state))
> +		return 0;
> +

[Severity: High]
Is "while the interface is going down" accurate here? STMMAC_DOWN is set and
cleared only in stmmac_reset_subtask(), and it stays set across dev_open()
as well as dev_close():

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_reset_subtask() {
    ...
	set_bit(STMMAC_DOWN, &priv->state);
	dev_close(priv->dev);
	dev_open(priv->dev, NULL);
	clear_bit(STMMAC_DOWN, &priv->state);
    ...
}

By the time dev_open() returns, __stmmac_open() has already set up DMA,
called phylink_start(), requested the IRQs, enabled NAPI and the TX queues,
and called stmmac_enable_all_dma_irq(). The NETDEV_UP notifiers also run
before clear_bit().

Can interrupts from the reopened, running hardware in that window be
acknowledged and then lost? stmmac_dma_interrupt_status() has already
written the W1C status, for example in dwmac4_dma_interrupt():

	writel(intr_status & intr_en,
	       ioaddr + DMA_CHAN_STATUS(dwmac4_addrs, chan));

Then stmmac_napi_check() returns 0 without scheduling NAPI. Before this
patch, the status stayed latched and was serviced once STMMAC_DOWN was
cleared.

Here are some of the cases.

If RI is dropped and the RX ring fills inside the window, RX DMA suspends
with RBU. dwmac4_dma_interrupt() only counts RBU in rx_buf_unav_irq and
never returns handle_rx. Does RX then stay stalled, because
stmmac_rx_refill() never runs, until the next ifdown/up or reset?

If TI is dropped and tx_coal_timer is 0, stmmac_tx_timer_arm() returns
early. What reclaims those TX descriptors in that case?

Because the status is 0, stmmac_dma_interrupt() and stmmac_msi_intr_tx()
also skip this for a stopped TX DMA channel:

	if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
		stmmac_bump_dma_threshold(priv, chan);
	} else if (unlikely(status[chan] == tx_hard_error)) {
		stmmac_tx_err(priv, chan);

That leaves recovery to the netdev watchdog.

In stmmac_safety_feat_interrupt(), an uncorrectable ASP error reported by
the reinitialised hardware never reaches stmmac_global_err(). In
stmmac_interrupt(), the false return also falls through into
stmmac_common_interrupt() and stmmac_dma_interrupt() on hardware that has
just reported a fatal error.

On the close side, __stmmac_release() calls stmmac_disable_all_queues()
(napi_disable) before stmmac_free_irq(), so napi_schedule_prep() already
fails there. That suggests the new check only changes behaviour during the
reopen half of the reset.

Would it work to clear STMMAC_DOWN, or use a separate close-only state,
before the DMA IRQs are re-enabled? Or to keep handling events normally and
rely on napi_disable() during close?

>  	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
>  		if (napi_schedule_prep(rx_napi)) {
>  			spin_lock_irqsave(&ch->lock, flags);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015112415.437900-1-chuanx2070%40163.com