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

[Daniel Machon]

Hi Igor,

Just a question:

> stmmac_resume() resets the MAC in stmmac_hw_setup(). On dwmac1000 the
> reset sets GMAC_INT_MASK to 0, which unmasks every core interrupt, until
> dwmac1000_core_init() writes the mask again. Since commit 2e2c878a3141
> ("net: stmmac: remove SGMII/RGMII/SMII interrupt handling") nothing on
> the RGMII path reads GMAC_RGSMIIIS (only the PCS code does), so RGSMIIIS
> stays pending behind the mask from the first link change on, and the
> reset lets it through. The line storms:
> 
>   dwmac_dma_interrupt: unexpected status 04000000
> 
> A register dump at the first message on ODROID-HC4 showed it:
> 
>   core int_status 00000001 int_mask 00000000 pmt 00000000 rgsmiiis 0000000d
> 
> The mask is written by the resume thread itself. When that thread runs
> on the CPU that takes the MAC interrupt, the storm starves it and the
> board hangs until a watchdog reset or a soft-lockup panic; on another
> CPU the storm ends after a few interrupts.
> 
> Disable the interrupt line around stmmac_hw_setup(). This matches
> open(), which resets the MAC before it requests the interrupt; resume
> resets it with the line live. The window sits under rtnl_lock(), which
> stmmac_reset_subtask() also takes, so the reset task cannot free and
> request the interrupt between the disable and the enable. If the setup
> fails, it may not have written the mask: mask the core interrupts before
> enabling the line.
> 
> Wake-on-LAN resume with the resume thread on the CPU that takes the MAC
> interrupt, 7.3-rc5: without this patch the storm hit the first resume
> on Helios64 (dwmac-rk) twice, ROCK Pi 4A (dwmac-rk) once and ODROID-HC4
> (dwmac-meson8b) twice. With the thread on another CPU and no patch, the
> storm printed at most 3 messages per resume in 30 resumes. 7.3-rc6 with
> this patch: no storm in 10 resumes on ODROID-HC4, 10 on Helios64 with
> the GMAC kept powered across suspend (an Armbian patch) and 10 on
> Helios64 with mainline dwmac-rk plus "net: phy: postpone PHY interrupts
> during sleep with MAC-managed PM"; int_mask read 00000001 after the
> glue resume and before stmmac_hw_setup() in all 30.
> 

The fix seems OK, but it could have implications on other drivers, for an issue
that is dwmac1000 specific. Did you consider a fix that would only touch
dwmac1000?

> Fixes: 2e2c878a3141 ("net: stmmac: remove SGMII/RGMII/SMII interrupt handling")
> Assisted-by: LLM
> Signed-off-by: Igor Velkov <iav@xxxxxx>
> ---
> v3:
> - disable the line only around stmmac_hw_setup(), under rtnl_lock() and
>   priv->lock, not for the whole resume (Sashiko)
> - mask the core interrupts if stmmac_hw_setup() fails (Sashiko)
> - results with this window on 7.3-rc6, including Helios64 with mainline
>   dwmac-rk
> v2: https://lore.kernel.org/r/20261002043558.1302590-1-iav@xxxxxx
> v1: https://lore.kernel.org/r/20260930-stmmac-irq-shut-v1-1-104d1a1dcb28@xxxxxx
> 
> Build-tested on net: allmodconfig and allyesconfig with W=1, no new
> warnings.
> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b837e8e27a35..6091dc2d4ab4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -8361,7 +8361,15 @@ int stmmac_resume(struct device *dev)
>         stmmac_free_tx_skbufs(priv);
>         stmmac_clear_descriptors(priv, &priv->dma_conf);
> 
> +       /* On dwmac1000 the MAC reset unmasks every core interrupt until the
> +        * mask is written again; we keep a pending status bit from storming.
> +        */
> +       disable_irq(ndev->irq);
>         ret = stmmac_hw_setup(ndev);
> +       /* A failed setup can stop before it writes the mask. */
> +       if (ret < 0)
> +               stmmac_mac_irq_modify(priv, ~0U, 0);
> +       enable_irq(ndev->irq);
>         if (ret < 0) {
>                 netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
>                 stmmac_legacy_serdes_power_down(priv);
> --
> 2.43.0
> 
> 

/Daniel