Re: [PATCH net-next] 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 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] The defect is specific to dwmac1000, but stmmac_resume() now
  calls disable_irq(ndev->irq) on every core for the whole resume.
- [Medium] stmmac_resume() now re-enables ndev->irq on every exit path,
  including error exits where GMAC_INT_MASK was never restored.
- [Low] The commit message names commit 2e2c878a3141 ("net: stmmac:
  remove SGMII/RGMII/SMII interrupt handling") as the point after which…
- [Low] The new comment in the generic stmmac_resume() says 'Resume
  resets the MAC, leaving every core interrupt unmasked until the mask…

--

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

This patch disables ndev->irq for the whole of stmmac_resume() when the
interface is running, and enables it again on every exit path. The aim is
to stop an RGMII interrupt storm on dwmac1000. There, the MAC reset in
stmmac_hw_setup() clears GMAC_INT_MASK, and it stays clear until
dwmac1000_core_init() writes it again.

> stmmac_resume() resets the MAC in stmmac_hw_setup(). On dwmac1000 the
> reset clears the interrupt mask, and all core interrupts stay unmasked
> until dwmac1000_core_init() writes it again. A link change in that
> window raises an RGMII interrupt, and no handler clears it since
> commit 2e2c878a3141 ("net: stmmac: remove SGMII/RGMII/SMII interrupt handling").
> The line storms:

[ ... ]

> The message repeats every few milliseconds and the network stays down
> until a power cycle or a watchdog reset.

[Severity: Low]
Should this carry a Fixes: tag for the commit it names?

Fixes: 2e2c878a3141 ("net: stmmac: remove SGMII/RGMII/SMII interrupt handling")

That commit removed dwmac1000_rgsmii() and its call from
dwmac1000_irq_status(). That call read GMAC_RGSMIIIS to ack the interrupt:

-	if (intr_status & PCS_RGSMIIIS_IRQ)
-		dwmac1000_rgsmii(ioaddr, x);

It also moved GMAC_INT_DISABLE_RGMII into GMAC_INT_DEFAULT_MASK. Since
then, the mask written by dwmac1000_core_init() is the only protection.

The only trailers are Assisted-by and Signed-off-by, and the note after
them says the patch was ported to net-next. Both 7.2 and 7.3-rc5 were
tested as affected. Would net, with a Fixes: tag and possibly Cc: stable,
be the better target?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3ad9252bf6aea..6e0415538327b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -8356,16 +8356,26 @@ int stmmac_resume(struct device *dev)
>  {
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> +	bool irq_off;
>  	int ret;
>  
> +	/* Resume resets the MAC, leaving every core interrupt unmasked until
> +	 * the mask is written again: a link change in that window would storm.
> +	 */

[Severity: Low]
Does this comment hold for every core that uses stmmac_resume()?

Only dwmac1000 writes a mask register, where 0 means everything is
unmasked:

drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:dwmac1000_core_init() {
    ...
	writel(GMAC_INT_DEFAULT_MASK, ioaddr + GMAC_INT_MASK);
    ...
}

dwmac4 and xgmac write enable registers instead. On those cores a reset
leaves the core interrupts disabled, not unmasked:

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:dwmac4_core_init() {
    ...
	writel(GMAC_INT_DEFAULT_ENABLE, ioaddr + GMAC_INT_EN);
    ...
}

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_core_init() {
    ...
	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
    ...
}

Could the comment say that this is a workaround specific to dwmac1000?

> +	irq_off = netif_running(ndev);
> +	if (irq_off)
> +		disable_irq(ndev->irq);

[Severity: Medium]
What happens to other devices on this line while resume runs? 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 line for every handler on it. The line then stays
masked through all of these:

  plat->resume()
  stmmac_mdio_reset()
  SerDes power-up
  rtnl_lock()
  mutex_lock(&priv->lock)
  stmmac_hw_setup(), including the DMA soft reset poll of up to 200ms
  in dwmac_dma_reset()
  PTP init
  phylink_resume()

No other device on the line gets interrupts during that time. Suppose one
of them waits for its own interrupt while holding RTNL, or another lock
that stmmac_resume() needs. Can that stall until a timeout, or forever if
the wait has no timeout? Lockdep would not see this dependency.

The commit message mentions IRQF_SHARED only as the reason to leave the
line enabled during suspend.

The problem is specific to dwmac1000. Could the fix avoid the shared line
altogether? Some options:

  rewrite GMAC_INT_MASK right after the reset
  restore the read-to-clear of GMAC_RGSMIIIS in dwmac1000
  disable the line only between the reset and core_init

[ ... ]

> @@ -8451,6 +8462,9 @@ int stmmac_resume(struct device *dev)
>  	stmmac_legacy_serdes_power_down(priv);
>  	mutex_unlock(&priv->lock);
>  	rtnl_unlock();
> +out:
> +	if (irq_off)
> +		enable_irq(ndev->irq);

[Severity: Medium]
Can this enable the line again while GMAC_INT_MASK is still clear?

If stmmac_init_dma_engine() fails, stmmac_hw_setup() returns before
stmmac_core_init(). That is the only place where dwmac1000_core_init()
restores the mask:

stmmac_hw_setup() {
    ...
	ret = stmmac_init_dma_engine(priv);
	if (ret < 0) {
		phylink_rx_clk_stop_unblock(priv->phylink);
		...
		return ret;
	}
    ...
	stmmac_core_init(priv, priv->hw, dev);
    ...
}

The failure can come from stmmac_prereset_configure(), or from
stmmac_reset() timing out ("Failed to reset the dma"). The timeout is a
known case when clk_rx_i from the PHY is not running yet.

dwmac_dma_reset() returns -ETIMEDOUT and leaves SFT_RESET set:

dwmac_dma_reset() {
    ...
	return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value,
				 !(value & DMA_BUS_MODE_SFT_RESET),
				 10000, 200000);
}

So the hardware finishes the reset once the RX clock starts. That usually
happens when the PHY link comes up, which phylink_prepare_resume() has
already started. The reset clears the mask just as the RGMII link change
event arrives.

The error then takes this path:

stmmac_resume()
  stmmac_hw_setup() fails
  goto error_unlock
  out:
    enable_irq(ndev->irq)

stmmac_interrupt() never acks the RGMII source and always returns
IRQ_HANDLED:

stmmac_interrupt() {
    ...
	stmmac_common_interrupt(priv);
	stmmac_dma_interrupt(priv);
	return IRQ_HANDLED;
}

Because of that, the spurious IRQ detector will not disable the line
either. Does the storm this patch fixes come back on these exits, with no
later core_init to close the window?

The commit message says the line is enabled again "on every exit path".
On these exits, though, it is enabled inside the window the patch is
meant to close.

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930-stmmac-irq-shut-v1-1-104d1a1dcb28%40iav.lv