Re: [PATCH net v2] net: stmmac: mask the MAC interrupt while resume resets the MAC

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 0

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

- [Medium] If stmmac_hw_setup() fails because stmmac_reset() times out in
  stmmac_init_dma_engine() ("Failed to reset the dma"),…
- [Medium] stmmac_resume() disables ndev->irq at the very top and re-
  enables it only after netif_device_attach().
- [Medium] The disable_irq()/enable_irq() pair in stmmac_resume() is not
  serialized against stmmac_reset_subtask().

--

net: stmmac: mask the MAC interrupt while resume resets the MAC

This patch disables ndev->irq for all of stmmac_resume() when the
interface is running, and re-enables it on every exit path. The goal is
to stop an RGSMIIIS status that is already pending from storming the line
between the MAC reset in stmmac_hw_setup() and the GMAC_INT_MASK write in
dwmac1000_core_init().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec62fa7418f41..b275b6234b557 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -8313,15 +8313,25 @@ int stmmac_resume(struct device *dev)
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  	int ret;
> +	bool irq_off;
> +
> +	/* Resume resets the MAC, leaving every core interrupt unmasked until
> +	 * the mask is written again: a status bit already pending would storm.
> +	 */
> +	irq_off = netif_running(ndev);
> +	if (irq_off)
> +		disable_irq(ndev->irq);

[Severity: Medium]
In single-IRQ mode the line is requested as shared:

stmmac_request_irq_single() {
	ret = request_irq(dev->irq, stmmac_interrupt,
			  IRQF_SHARED, dev->name, dev);
}

disable_irq() masks the descriptor for every action on that line. The
commit message uses the same sharing argument to leave the line enabled
during suspend.

Is it intended that resume keeps the line masked for every sharer through
priv->plat->resume(), stmmac_mdio_reset(), stmmac_legacy_serdes_power_up(),
rtnl_lock(), mutex_lock(&priv->lock), phylink_prepare_resume(), the DMA
SWR poll (up to 200 ms) and phylink_resume()?

The window that needs protection seems to run only from the SWR in
stmmac_init_dma_engine() to stmmac_core_init(), and both are inside
stmmac_hw_setup().

Could another device on the line that resumes at the same time (for
example an async-resumed PCI function on the same INTx line with
stmmac_pci, or the Loongson glue without MSI) have its completion
interrupts held off long enough to time out?

The masked window also covers blocking on rtnl_lock(). Could a sharer
that holds rtnl and waits for its own interrupt on this line with no
timeout deadlock here? Lockdep cannot see that dependency.

>  
>  	if (priv->plat->resume) {
>  		ret = priv->plat->resume(dev, priv->plat->bsp_priv);

[ ... ]

> @@ -8367,7 +8377,7 @@ int stmmac_resume(struct device *dev)
>  		stmmac_legacy_serdes_power_down(priv);
>  		mutex_unlock(&priv->lock);
>  		rtnl_unlock();
> -		return ret;
> +		goto out;
>  	}

[Severity: Medium]
If stmmac_hw_setup() fails because stmmac_reset() times out, can this
path turn the line back on while the core interrupts are still unmasked?

stmmac_hw_setup()
  stmmac_init_dma_engine()
    stmmac_reset()        <- SWR asserted, poll times out
    netdev_err(priv->dev, "Failed to reset the dma\n");
    return ret;
  return ret;             <- before stmmac_core_init()

On this path, stmmac_core_init()->dwmac1000_core_init() is the only code
that writes GMAC_INT_DEFAULT_MASK back to GMAC_INT_MASK, so the mask is
never restored. The goto out then calls enable_irq(ndev->irq)
unconditionally.

Suppose the mask has already returned to its reset value of 0, or the
reset finishes late after the 200 ms poll in dwmac_dma_reset() has given
up. Then the latched RGSMIIIS bit reaches a live line.
stmmac_interrupt() never clears that source and still returns
IRQ_HANDLED. Wouldn't that be the same storm this patch is meant to
prevent?

The comment in stmmac_hw_setup() says clk_rx_i must be running for the
reset to complete, and a stopped clk_rx_i is the known cause of SWR
timeouts.

This path behaved the same way before the patch, because the line was
always live. It is still a resume exit that the fix does not cover.
Would re-masking the core interrupts, or acknowledging the source, on
this failure path close it?

>  
>  	stmmac_init_timestamping(priv);

[ ... ]

> @@ -8393,7 +8403,12 @@ int stmmac_resume(struct device *dev)
>  
>  	netif_device_attach(ndev);
>  
> -	return 0;
> +	ret = 0;
> +out:
> +	if (irq_off)
> +		enable_irq(ndev->irq);

[Severity: Medium]
Can this enable_irq() become unbalanced if stmmac_reset_subtask() runs
between rtnl_unlock() and here?

priv->wq comes from create_singlethread_workqueue() and is not
freezable. stmmac_suspend() does not cancel or flush the reset work
either.

A reset can be requested around resume, for example from
stmmac_safety_interrupt() on a separate sfty_irq line, which resume does
not disable. That goes through stmmac_global_err() ->
stmmac_service_event_schedule(), and STMMAC_DOWN is not set across
suspend/resume.

The worker blocks on rtnl while resume holds it, then:

stmmac_resume()                    stmmac_reset_subtask()
  disable_irq(ndev->irq)
  ...
  rtnl_unlock()
  netif_device_attach()
  <preempted>
                                     dev_close()
                                       stmmac_release()
                                         free_irq()
                                     dev_open()
                                       request_irq()
                                         irq_startup()
                                           desc->depth = 0
  enable_irq(ndev->irq)
    __enable_irq() with depth 0

Wouldn't this trigger the "Unbalanced enable for IRQ %d" WARN, which
becomes a panic with panic_on_warn? The line itself ends up enabled,
because request_irq() already started it. Nothing serializes the window
between rtnl_unlock() and this enable_irq() against the worker.

> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(stmmac_resume);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002043558.1302590-1-iav%40iav.lv