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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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