Re: [PATCH net v4 09/15] net: stmmac: track datapath and power ownership across failed reopening

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 4 · Medium: 4 · Low: 1

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

- [High] Platforms that have a resume hook but no suspend hook no longer
  run the resume hook on system resume.
- [High] The new 'retry pending power transitions' logic misuses
  pm_runtime_force_resume(), which only does its work once.
- [High] While hw_unavailable is set, stmmac_interrupt() and
  stmmac_mac_interrupt() (and the safety and MSI rx/tx handlers) return…
- [High] With MAC WoL enabled, stmmac_suspend() calls phylink_suspend(pl,
  true), which only sets PHYLINK_DISABLE_MAC_WOL and does not call…
- [Medium] A failed system resume leaves the PHY powered after it was
  resumed only to supply the MAC reset clock.
- [Medium] stmmac_bpf() calls __stmmac_release() on a retained SUSPENDED
  datapath (and, at the end of the series, HALTED) for every…
- [Medium] On platforms without plat->suspend (so bsp_suspended is never
  set), a resume where only re-enabling the optional PTP reference clock…
- [Medium] stmmac_suspend() sets priv->bsp_suspended = true before
  calling priv->plat->suspend(), and does not clear it if the callback…
- [Low] Submission scope and stable backportability: one commit carries
  five Fixes: tags and many independent behaviour changes, and it…

--

net: stmmac: track datapath and power ownership across failed reopening

This commit tracks datapath state (down, running, suspended) separately
from IFF_UP. On failure, the MTU, XDP and ethtool reopen paths now leave
the device detached instead of tearing it down. It also splits core sleep
cleanup from platform and clock restoration, and adds an hw_unavailable
gate on MMIO, MDIO, PHC and IRQ access while power is not restored.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index c4ba97d57b47e..22a65c1c16810 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -959,7 +966,26 @@ static void stmmac_setup_ptp(struct stmmac_priv *priv)
>  static void stmmac_release_ptp(struct stmmac_priv *priv)
>  {
>  	stmmac_ptp_unregister(priv);
> -	clk_disable_unprepare(priv->plat->clk_ptp_ref);
> +	mutex_lock(&priv->pm_mutex);
> +	if (priv->ptp_clock_enabled) {
> +		clk_disable_unprepare(priv->plat->clk_ptp_ref);
> +		priv->ptp_clock_enabled = false;
> +	}
> +	/* A later noirq resume must not reacquire a released reference. */
> +	priv->ptp_clock_suspended = false;
> +	mutex_unlock(&priv->pm_mutex);
> +}

[Severity: Medium]
Can clearing ptp_clock_suspended here stop the interface from opening
until the next system sleep?

Take a platform with no plat->suspend, so bsp_suspended is never set.
Suppose noirq resume restores the bus clocks but
clk_prepare_enable(clk_ptp_ref) fails. ptp_clock_suspended stays true.
stmmac_resume()->stmmac_resume_power(priv, true) fails the same way and
returns before hw_unavailable is cleared.

If the interface is then closed,
stmmac_release()->__stmmac_release()->stmmac_release_ptp() clears
ptp_clock_suspended here. On the next open,
stmmac_open()->stmmac_resume_power(priv, false) evaluates:

	bool pending = priv->bus_clks_suspended || priv->bsp_suspended ||
		       priv->ptp_clock_suspended;

as false. hw_unavailable is never cleared, so every open attempt returns
-EHOSTDOWN.

stmmac_setup_ptp() treats a PTP reference clock failure as only a
warning. Should this optional clock be able to block the whole MAC?

[ ... ]

> @@ -4164,6 +4217,89 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
>  	return ERR_PTR(ret);
>  }
>  
> +/* The freezer excludes pool teardown and userspace from noirq callbacks.
> + * Outside system sleep these flags are changed under RTNL.
> + */
> +int stmmac_resume_clocks(struct stmmac_priv *priv)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&priv->pm_mutex);
> +	if (priv->bus_clks_suspended) {
> +		ret = pm_runtime_force_resume(priv->device);
> +		if (ret)
> +			goto out;
> +		priv->bus_clks_suspended = false;
> +	}

[Severity: High]
Is pm_runtime_force_resume() safe to retry like this?

In drivers/base/power/runtime.c it clears needs_force_resume and calls
pm_runtime_enable() on both success and failure:

	if (!dev->power.needs_force_resume && (!dev_pm_smart_suspend(dev) ||
	    pm_runtime_status_suspended(dev)))
		goto out;
	...
out:
	dev->power.needs_force_resume = false;
	pm_runtime_enable(dev);
	return ret;

Suppose the noirq call fails, for example when stmmac_runtime_resume()
fails a clock enable. bus_clks_suspended stays true. The retry from
stmmac_resume(), stmmac_open() or stmmac_release() then skips the
callback, does an unbalanced pm_runtime_enable(), and returns 0.

stmmac_resume_clocks() then clears bus_clks_suspended and
stmmac_resume_power() clears hw_unavailable. After that,
stmmac_resume_hw() and stmmac_hw_setup() access MMIO while the bus
clocks are still off.

stmmac_pltfr_noirq_suspend() has the same problem. It sets the flag
before the call and does not roll it back if the call fails:

		priv->bus_clks_suspended = true;
		ret = pm_runtime_force_suspend(dev);
		if (ret) {
			mutex_unlock(&priv->pm_mutex);
			return ret;
		}

The later retry would then hit the same unbalanced enable and false
success.

[ ... ]

> +static int stmmac_resume_power(struct stmmac_priv *priv, bool system_resume)
> +{
> +	bool pending = priv->bus_clks_suspended || priv->bsp_suspended ||
> +		       priv->ptp_clock_suspended;
> +	int ret;
> +
> +	ret = stmmac_resume_clocks(priv);
> +	if (ret)
> +		return ret;
> +	if (priv->bsp_suspended && priv->plat->resume) {
> +		ret = priv->plat->resume(priv->device, priv->plat->bsp_priv);
> +		if (ret)
> +			return ret;
> +		priv->bsp_suspended = false;
> +	}

[Severity: High]
Does this skip plat->resume() on platforms that have a resume hook but
no suspend hook?

stmmac_suspend() sets bsp_suspended only when plat->suspend is non-NULL:

	if (priv->plat->suspend && !priv->bsp_suspended) {
		priv->bsp_suspended = true;

Before this patch, stmmac_resume() called plat->resume() unconditionally.

dwmac-mediatek sets plat->resume = mediatek_dwmac_init and has no suspend
hook. stmmac_pltfr_probe() does:

	if (!plat->suspend && plat->exit)
		plat->suspend = stmmac_plat_suspend;
	if (!plat->resume && plat->init)
		plat->resume = stmmac_plat_resume;

So glue drivers with init but no exit (dwmac-thead, dwmac-socfpga,
dwmac-loongson1) get only a resume hook.

On those boards, PHY interface selection, delays and syscon setup would
not be reprogrammed after sleep. stmmac_resume_power(priv, true) still
clears hw_unavailable, and the core goes on into stmmac_hw_setup().

The commit message doesn't mention this change. Was it intended?

[ ... ]

> @@ -6377,6 +6571,12 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
>  	struct net_device *dev = (struct net_device *)dev_id;
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  
> +	if (READ_ONCE(priv->hw_unavailable)) {
> +		if (priv->irq_wake)
> +			pm_wakeup_event(priv->device, 0);
> +		return IRQ_NONE;
> +	}
> +

[Severity: High]
Can this cause an interrupt storm on a MAC WoL wake?

stmmac_suspend() sets hw_unavailable after PMT has been armed, and only
stmmac_resume() clears it. With irq_wake set,
stmmac_pltfr_noirq_suspend() keeps the clocks on, so the registers are
still accessible in this window.

The PMT status is cleared by reading it in the host IRQ status path,
which this early return now skips. A magic packet can arrive after
stmmac_suspend(), or the wake IRQ can be replayed after
resume_device_irqs() but before stmmac_resume(). Either way, a
level-triggered MAC/WoL line would stay asserted.

Each pass calls pm_wakeup_event() and returns IRQ_NONE, until
note_interrupt() disables the line as "nobody cared". After that the
interface gets no interrupts following resume.

stmmac_mac_interrupt() has the same early return. When irq_wake keeps the
MAC clocked, would it be better to service the PMT status here?

[ ... ]

> @@ -7029,6 +7283,18 @@ static int stmmac_bpf(struct net_device *dev, struct netdev_bpf *bpf)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  
> +	if (bpf->command != XDP_SETUP_PROG &&
> +	    bpf->command != XDP_SETUP_XSK_POOL)
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Pool removal must succeed even after a failed resume. Release the
> +	 * suspended rings before their pool or XDP buffer layout can change.
> +	 * Leave the interface detached until it is closed and reopened.
> +	 */
> +	if (priv->datapath == STMMAC_DATAPATH_SUSPENDED)
> +		__stmmac_release(dev);
> +

[Severity: Medium]
Should this release happen only for pool removal, as the comment says?

As written, it runs for every XDP_SETUP_PROG and XDP_SETUP_XSK_POOL
request before any validation. That includes program installs, swaps
from one program to another, and requests that are later rejected by
stmmac_xdp_set_prog() (jumbo MTU) or stmmac_xdp_enable_pool() (bad queue,
small frame size).

Once datapath is DOWN, a later stmmac_resume() takes this path:

	if (priv->datapath != STMMAC_DATAPATH_SUSPENDED) {
		ret = 0;
		goto out_unlock;
	}

It returns 0 with the netdev still detached. The "A later resume may
retry" behaviour described in the resume error path is lost, even when
the XDP request itself failed.

[ ... ]

> @@ -8317,18 +8677,41 @@ int stmmac_suspend(struct device *dev)
>  
>  	mutex_unlock(&priv->lock);
>  
> -	rtnl_lock();
>  	phylink_suspend(priv->phylink, !!priv->wolopts);
> -	rtnl_unlock();
> +	if (priv->datapath == STMMAC_DATAPATH_RUNNING)
> +		priv->datapath = STMMAC_DATAPATH_SUSPENDED;
> +	priv->hw_suspended = true;
>  
>  	if (stmmac_fpe_supported(priv))
>  		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
>  
>  suspend_bsp:
> -	if (priv->plat->suspend)
> -		return priv->plat->suspend(dev, priv->plat->bsp_priv);
> +	accessible = !priv->hw_unavailable;
> +	mutex_lock(&priv->ptp_mutex);
> +	stmmac_block_ptp(priv, true);
> +	mutex_unlock(&priv->ptp_mutex);
> +	/* Drain MDIO transactions before the platform can remove power. */
> +	if (priv->mii)
> +		mutex_lock(&priv->mii->mdio_lock);
> +	/* ndo_set_rx_mode() can run without RTNL under the address lock. */
> +	netif_addr_lock_bh(ndev);
> +	WRITE_ONCE(priv->hw_unavailable, true);
> +	netif_addr_unlock_bh(ndev);
> +	if (priv->mii)
> +		mutex_unlock(&priv->mii->mdio_lock);

[Severity: High]
With MAC WoL enabled, what happens to a polled PHY once this flag is set?

phylink_suspend(priv->phylink, true) only sets PHYLINK_DISABLE_MAC_WOL
and does not call phy_stop(). stmmac sets mac_managed_pm, so
mdio_bus_phy_suspend() does not stop the PHY state machine either.

>From here until stmmac_resume() clears the flag, stmmac_mdio_access()
and the xgmac2 c22/c45 accessors return -EHOSTDOWN. The PHY state machine
work runs on a workqueue that is not freezable. A poll in this window
would get -EHOSTDOWN from phy_read_status(), and phylib would then WARN,
move the PHY to PHY_ERROR and stop polling.

On resume, the irq_wake branch in stmmac_resume_hw() only clears PMT, and
phylink_resume() only clears the disable bit. Is anything left that
restarts the PHY?

Before this patch MDIO stayed usable in this window, because the clocks
are kept on for irq_wake.

> +	if (priv->datapath == STMMAC_DATAPATH_SUSPENDED) {
> +		stmmac_synchronize_irq(priv);
> +		stmmac_stop_tx_queues(priv);
> +		if (accessible)
> +			stmmac_stop_all_dma(priv);
> +	}
> +	if (priv->plat->suspend && !priv->bsp_suspended) {
> +		priv->bsp_suspended = true;
> +		ret = priv->plat->suspend(dev, priv->plat->bsp_priv);
> +	}
> +	rtnl_unlock();

[Severity: Medium]
What happens to bsp_suspended if plat->suspend() fails?

It stays true. The PM core won't call ->resume for a device whose
->suspend failed. Even so, stmmac_open() or
stmmac_release()->stmmac_resume_power() will call plat->resume() for a
platform suspend that never completed.

If no open or close happens first, the next system suspend skips
plat->suspend() because of the !priv->bsp_suspended check, while
stmmac_resume() still calls plat->resume().

For hooks such as the dwmac-intel or PCI glue (pci_enable_device, clock
enables), would this leave the platform enable counts unbalanced? Should
bsp_suspended be set only when the callback succeeds?

[ ... ]

> @@ -8430,13 +8811,13 @@ int stmmac_resume(struct device *dev)
>  	ret = stmmac_hw_setup(ndev);
>  	if (ret < 0) {
>  		netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
> -		stmmac_legacy_serdes_power_down(priv);
> -		mutex_unlock(&priv->lock);
> -		rtnl_unlock();
> -		return ret;
> +		goto error_stop_dma;
>  	}

[ ... ]

> @@ -8455,11 +8836,25 @@ int stmmac_resume(struct device *dev)

[ ... ]

> +error_stop_dma:
> +	stmmac_stop_all_dma(priv);
> +	stmmac_mac_set(priv, priv->ioaddr, false);
> +	stmmac_legacy_serdes_power_down(priv);
> +	mutex_unlock(&priv->lock);
> +	/*
> +	 * Keep the suspended data path detached. A later resume may retry, or
> +	 * ndo_stop() can release its resources without disabling NAPI again.
> +	 */
> +out_unlock:
> +	rtnl_unlock();
> +
> +	return ret;
>  }

[Severity: Medium]
Does this leave the PHY powered after a failed resume?

stmmac sets mac_requires_rxc. So the earlier phylink_prepare_resume() in
stmmac_resume() calls phy_resume() on a PHY that the non-WoL
phylink_suspend() had stopped.

If stmmac_hw_setup() then fails, this path does not call phylink_stop().
With the earlier "allow stopping a suspended instance" change in the
series, phylink_stop() would call phy_suspend() here.

__stmmac_open() does this unwind in its init_error path ("Undo
phylink_prepare_resume() even if hardware setup failed"). Should the
resume failure path do the same?

Otherwise the PHY stays powered, and the link partner may see link up
while the datapath is detached, until close or the next suspend.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260926-submit-stmmac-reset-fixes-v1-v4-0-ec1c0250b3c9%40gmail.com