Re: net: stmmac: Reorder cleanup sequence and disable IRQ earlier in reset path

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: Reorder cleanup sequence and disable IRQ earlier in reset path

The recent changes restructure resource cleanup in __stmmac_release() to
free IRQ lines, stop DMA, and release descriptors before stopping the PHY.
Additionally, stmmac_reset_subtask() now disables IRQ earlier to prevent
spurious interrupts during the DOWN transition.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 124d7a00f9f0..fa06b2eb33f2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4202,6 +4202,15 @@ static void __stmmac_release(struct net_device *dev)
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	u8 chan;
>
> +	/* Free the IRQ lines */
> +	stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
> +
> +	/* Stop TX/RX DMA and clear the descriptors */
> +	stmmac_stop_all_dma(priv);
> +
> +	/* Release and free the Rx/Tx resources */
> +	free_dma_desc_resources(priv, &priv->dma_conf);
> +
>  	/* Stop and disconnect the PHY */
>  	phylink_stop(priv->phylink);
>
> @@ -4212,15 +4221,6 @@ static void __stmmac_release(struct net_device *dev)
>
>  	netif_tx_disable(dev);
>
> -	/* Free the IRQ lines */
> -	stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
> -
> -	/* Stop TX/RX DMA and clear the descriptors */
> -	stmmac_stop_all_dma(priv);
> -
> -	/* Release and free the Rx/Tx resources */
> -	free_dma_desc_resources(priv, &priv->dma_conf);
> -
                ^^^^

Moving free_dma_desc_resources() before stmmac_disable_all_queues() looks
problematic. Can a TX coalesce hrtimer fire after the DMA resources are
freed?

The stmmac driver has TX coalesce timers (stmmac_tx_timer) that can call
napi_schedule(). If free_dma_desc_resources() frees tx_q->tx_skbuff_dma
and tx_q->tx_skbuff via __free_dma_tx_desc_resources():

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__free_dma_tx_desc_resources() {
    ...
    kfree(tx_q->tx_skbuff_dma);
    tx_q->tx_skbuff_dma = NULL;
    kfree(tx_q->tx_skbuff);
    tx_q->tx_skbuff = NULL;
    ...
}

But the hrtimer hasn't been cancelled yet (hrtimer_cancel is in
stmmac_disable_all_queues), and NAPI hasn't been disabled yet, then:

stmmac_tx_timer() fires
  -> napi_schedule_prep()
  -> __napi_schedule()
  -> NAPI poll eventually runs
  -> stmmac_napi_poll_tx()
  -> stmmac_tx_clean()
  -> accesses freed tx_q->tx_skbuff_dma[entry] and tx_q->tx_skbuff[entry]

The original order disabled NAPI and cancelled hrtimers before freeing DMA
resources. The stmmac_xdp_release() function in this same file shows the
correct pattern:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_xdp_release() {
    ...
    stmmac_disable_all_queues(priv);
    for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
        hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
    ...
    free_dma_desc_resources(priv, &priv->dma_conf);
    ...
}


>  	stmmac_release_ptp(priv);
>
>  	if (stmmac_fpe_supported(priv))

[ ... ]

> @@ -7331,6 +7331,9 @@ static void stmmac_reset_subtask(struct stmmac_priv *priv)
>  	while (test_and_set_bit(STMMAC_RESETING, &priv->state))
>  		usleep_range(1000, 2000);
>
> +	/* Close IRQ entry as early as possible before DOWN short-circuit path. */
> +	disable_irq_nosync(priv->dev->irq);
> +
                ^^^^

disable_irq_nosync() increments the IRQ descriptor's depth counter, but
where is the corresponding enable_irq()?

The stmmac driver doesn't call enable_irq() anywhere. After dev_close()
and dev_open() complete:

stmmac_reset_subtask()
  -> disable_irq_nosync(dev->irq)   // depth++
  -> dev_close()
      -> __stmmac_release()
          -> stmmac_free_irq()
              -> free_irq(dev->irq, dev)
  -> dev_open()
      -> stmmac_request_irq_single()
          -> request_irq(dev->irq, ..., IRQF_SHARED, ...)

For non-shared IRQs, this works by coincidence: free_irq's last-handler
removal calls irq_shutdown (depth++), then request_irq's first-handler
registration calls irq_startup which resets depth to 0.

However, when dev->irq is shared with another device (IRQF_SHARED is set
in stmmac_request_irq_single), free_irq does NOT call irq_shutdown because
other handlers remain on desc->action. The depth stays at 1. The
subsequent request_irq takes the shared path in __setup_irq which does NOT
call irq_startup, leaving depth at 1.

The IRQ line remains disabled after the reset completes, breaking
interrupts for all devices on that shared line. Should enable_irq() be
added after dev_open()?


>  	set_bit(STMMAC_DOWN, &priv->state);
>  	dev_close(priv->dev);
>  	dev_open(priv->dev, NULL);