[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