[PATCH] net: stmmac: Reorder cleanup sequence and disable IRQ earlier in reset path

[PATCH] net: stmmac: Reorder cleanup sequence and disable IRQ earlier in reset path

[yangg9]

The recent changes restructure resource cleanup in __stmmac_release() to
free IRQ lines, stop DMA, and release descriptors before stopping the PHY,
ensuring resources are properly cleaned up in the correct order.

Additionally, in stmmac_reset_subtask(), disable IRQ as early as possible
after acquiring the STMMAC_RESETING lock to prevent spurious interrupts
that could access device state during the DOWN transition, reducing race
conditions between interrupt handlers and device state transitions.

This improves the robustness of both the release and reset paths by
enforcing stricter ordering guarantees on resource cleanup and interrupt
handling.

Signed-off-by: yangg9 <yangg9@xiaopeng.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++--------
 1 file changed, 12 insertions(+), 9 deletions(-)

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);
-
 	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);
+
 	set_bit(STMMAC_DOWN, &priv->state);
 	dev_close(priv->dev);
 	dev_open(priv->dev, NULL);
-- 
2.43.0

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

[Paolo Abeni]

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