Re: [PATCH net v3 03/10] net: stmmac: keep datapath state coherent after reinitialization failure

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 (2) · Medium: 4 (1) · 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).

- [High] stmmac_resume_hw() now returns the error from
  stmmac_mdio_reset().
- [High] (Baseline: fixed later in the series.) In this commit,
  stmmac_xdp_set_prog() gates on priv->datapath == RUNNING, but…
- [High] (Baseline: fixed later in the series.) In this commit, the
  irq_error path of stmmac_xdp_open() frees the descriptor rings after…
- [Medium] stmmac_bpf() calls __stmmac_release() whenever datapath ==
  STMMAC_DATAPATH_SUSPENDED (and also HALTED at the end of the series).
- [Medium] In stmmac_suspend(), the early exit on `priv->hw_suspended`
  (goto suspend_bsp) comes before the new check that rejects suspend…
- [Medium] After an early resume failure, the new close-time sleep
  cleanup touches MAC registers without confirming that the hardware's…
- [Medium] (Baseline: fixed later in the series.) In this commit,
  stmmac_reinit_queues() and stmmac_reinit_ringparam() call the full…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  a guard at this exact spot and the guard does not cover it.
- [High] (Baseline: preexisting and fixed later in the series.)
  stmmac_quiesce() cancels the TX coalescing hrtimers before…
- [Medium] (Baseline: preexisting, not introduced.) If
  stmmac_pltfr_noirq_resume() fails in pm_runtime_force_resume() or in…

--

net: stmmac: keep datapath state coherent after reinitialization failure

This adds a datapath ownership state and a hw_suspended flag to
stmmac_priv. Close, suspend, resume and XDP paths use them to tell which
IRQ, DMA and NAPI resources are still owned after a failed MTU reopen or
a failed resume. It splits queue quiescing out of __stmmac_release(),
serializes suspend and resume with RTNL, and adds guards to the TC,
debugfs, XDP xmit and reset task paths.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 4f3d452c35036..43a6154fcf6d7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4127,6 +4127,37 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
>  	return ERR_PTR(ret);
>  }
>  
> +/* Finish core sleep state even if the platform resume callback failed. */
> +static int stmmac_resume_hw(struct stmmac_priv *priv)
> +{
> +	int ret;
> +
> +	if (!priv->hw_suspended)
> +		return 0;
> +
> +	/* Use the state installed by suspend, not a subsequently changed WoL
> +	 * setting. Clear PMT even when a different device caused the wakeup.
> +	 */
> +	if (priv->irq_wake) {
> +		mutex_lock(&priv->lock);
> +		stmmac_pmt(priv, priv->hw, 0);
> +		mutex_unlock(&priv->lock);
> +		priv->irq_wake = 0;
> +	} else {
> +		ret = pinctrl_pm_select_default_state(priv->device);
> +		if (ret)
> +			return ret;
> +		if (priv->mii) {
> +			ret = stmmac_mdio_reset(priv->mii);
> +			if (ret)
> +				return ret;

[Severity: High]
On boards that use the snps,reset GPIO, could this return -EBUSY on
every resume without WoL?

stmmac_mdio_reset() requests the GPIO each time it runs:

drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c:stmmac_mdio_reset() {
    ...
		reset_gpio = devm_gpiod_get_optional(priv->device,
						     "snps,reset",
						     GPIOD_OUT_LOW);
		if (IS_ERR(reset_gpio))
			return PTR_ERR(reset_gpio);
    ...
}

The first call happens at probe through __mdiobus_register()->bus->reset().
Devres then keeps the line requested until unbind. Later calls reach
gpiod_request_commit(), which returns -EBUSY for a line that is already
requested, because GPIOD_OUT_LOW is not a nonexclusive request.

Before this patch, stmmac_resume() ignored this return value. Now it does:

	if (priv->hw_suspended) {
		ret = stmmac_resume_hw(priv);
		if (ret)
			goto out_unlock;

On this error it skips stmmac_hw_setup(). The datapath stays
STMMAC_DATAPATH_SUSPENDED, the netdev stays detached and hw_suspended
stays set.

After that, stmmac_open() calls stmmac_resume_hw() and fails the same
way, while stmmac_release() only logs the error. Would the interface stay
unusable until the driver is rebound?

> +		}
> +	}
> +	priv->hw_suspended = false;
> +
> +	return 0;
> +}

[ ... ]

> @@ -4192,6 +4224,8 @@ static int __stmmac_open(struct net_device *dev,
>  
>  	stmmac_release_ptp(priv);
>  init_error:
> +	stmmac_stop_all_dma(priv);
> +	stmmac_mac_set(priv, priv->ioaddr, false);
>  	return ret;
>  }

[Severity: High]
This stops DMA on the __stmmac_open() failure path. Does stmmac_xdp_open()
still have the same problem? In this commit its error path is:

irq_error:
	for (chan = 0; ...)
		hrtimer_cancel(...);
init_error:
	free_dma_desc_resources(priv, &priv->dma_conf);

By this point stmmac_mac_set(priv, priv->ioaddr, true) and
stmmac_start_all_dma() have already run.

If the IRQ request fails during an XDP reopen, can the running DMA write
into the freed descriptor rings?

The later commit "net: stmmac: account for failed live XDP reopening" in
this series adds stmmac_stop_tx_queues(), stmmac_stop_all_dma() and
stmmac_mac_set(false) to that path.

[ ... ]

> @@ -4246,25 +4284,38 @@ static int stmmac_open(struct net_device *dev)
>  	return ret;
>  }
>  
> -static void __stmmac_release(struct net_device *dev)
> +/* Quiesce NAPI and transmit queues without releasing their resources. */
> +static void stmmac_quiesce(struct stmmac_priv *priv)
>  {
> -	struct stmmac_priv *priv = netdev_priv(dev);
>  	u8 chan;
>  
> -	/* Stop and disconnect the PHY */
> -	phylink_stop(priv->phylink);
> -
>  	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);
>  
> -	netif_tx_disable(dev);
> +	netif_tx_disable(priv->dev);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but stmmac_quiesce() keeps the
old ordering. It cancels the TX coalescing hrtimers before
netif_tx_disable(), and it does not drain NAPI poll tails. Can
stmmac_xmit() or stmmac_napi_poll_tx()->stmmac_tx_timer_arm() re-arm a
txtimer after its final hrtimer_cancel()?

On the MTU path, __stmmac_open() then copies the new dma_conf over the
embedded timer. That could overwrite a queued hrtimer.

Later in this series, stmmac_quiesce() uses stmmac_stop_tx_queues(),
which calls netif_tx_disable() and synchronize_net() before
hrtimer_cancel(). The commit "net: stmmac: keep DMA configurations at
stable addresses" also replaces the dma_conf copy with a stable pointer.

[ ... ]

> @@ -4285,6 +4336,15 @@ static void __stmmac_release(struct net_device *dev)
>  static int stmmac_release(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> +	int ret;
> +
> +	/* Resume may have failed before restoring pins or disabling MAC wake.
> +	 * Complete that cleanup without restarting the link or the datapath.
> +	 * If it fails, keep hw_suspended set so a fresh open can retry it.
> +	 */
> +	ret = stmmac_resume_hw(priv);
> +	if (ret)
> +		netdev_err(dev, "failed to restore hardware sleep state: %d\n", ret);

[Severity: Medium]
Is it safe to touch MAC registers here after priv->plat->resume() has
failed? In that case stmmac_resume() returns early and leaves
hw_suspended set:

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

Glue suspend callbacks, such as the one in dwmac-rk, turn off clocks and
regulators. Only the resume callback that just failed turns them back on.

A later close then runs stmmac_release()->stmmac_resume_hw(). That does
the PMT register write, or the dummy writel() in stmmac_mdio_reset().
Next, __stmmac_release() runs stmmac_stop_all_dma() and stmmac_mac_set()
on the retained datapath. Neither step takes pm_runtime_resume_and_get()
or checks the power state first.

Before this patch, the same close hung in napi_disable() before it got
here. Depending on how far the glue resume got, could this now cause a
bus abort or silently dropped writes?

[ ... ]

> @@ -4298,6 +4358,8 @@ static int stmmac_release(struct net_device *dev)
>  	stmmac_legacy_serdes_power_down(priv);
>  	phylink_disconnect_phy(priv->phylink);
>  	pm_runtime_put(priv->device);
> +	/* Allow a fresh open after a failed MTU reopen or resume. */
> +	netif_device_attach(dev);
>  
>  	return 0;
>  }

[Severity: Medium]
How do the ethtool reinit paths behave in this commit?
stmmac_reinit_queues() and stmmac_reinit_ringparam() still do:

	if (netif_running(dev))
		stmmac_release(dev);
	...
	if (netif_running(dev))
		ret = stmmac_open(dev);

If that stmmac_open() fails, stmmac_release() has already dropped the PHY
and runtime PM references. IFF_UP is still set and the device is still
attached.

Would the next ndo_stop call pm_runtime_put() a second time and underflow
the usage count? In that state the netif_device_present() guards in the
TC callback and the reset task would not fire either.

The later commit "net: stmmac: retain PHY and PM ownership during ethtool
reopening" changes both functions to use netif_device_detach(),
__stmmac_release() and stmmac_reopen().

[ ... ]

> @@ -6425,6 +6492,8 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  
>  	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
>  		return ret;
> +	if (!netif_device_present(priv->dev))
> +		return -ENETDOWN;
>  
>  	__stmmac_disable_all_queues(priv);

[Severity: High]
This isn't a bug introduced by this patch, but does this guard cover an
interface that is administratively down? In that state the device is
present and NAPI is disabled. That holds for a device that was never
opened, and stmmac_release() now calls netif_device_attach() explicitly.

Adding a u32 or flower filter in that state still reaches
__stmmac_disable_all_queues():

	if (queue < rx_queues_cnt)
		napi_disable(&ch->rx_napi);

napi_disable_locked() then loops forever with RTNL held:

	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);

Would checking priv->datapath == STMMAC_DATAPATH_RUNNING here work better
than netif_device_present()?

[ ... ]

> @@ -6962,6 +7035,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]
Can this run in the middle of an ordinary system suspend? The check does
not look at hw_suspended, so it fires for any suspended datapath. Later in
the series the same applies to the halted state.

XSK pool teardown runs from xp_release_deferred(). That work is queued with
schedule_work() on system_wq, which is not frozen during suspend, and it
takes only RTNL:

	rtnl_lock();
	xp_clear_dev(pool);
	rtnl_unlock();

So the work can run after stmmac_suspend() has dropped RTNL and called
plat->suspend, or after stmmac_pltfr_noirq_suspend(). The noirq callbacks
also read priv->datapath without RTNL, although the new stmmac_priv
comment says the field is serialized by RTNL.

Suppose noirq suspend has already done clk_disable_unprepare(clk_ptp_ref)
and pm_runtime_force_suspend(). __stmmac_release() would then call
stmmac_free_irq(), stmmac_stop_all_dma() and stmmac_mac_set() on a
clock-gated MAC. stmmac_release_ptp() would also underflow the clk_ptp_ref
enable and prepare counts.

[ ... ]

> @@ -8193,34 +8286,41 @@ EXPORT_SYMBOL_GPL(stmmac_dvr_remove);

[ ... ]

>  int stmmac_suspend(struct device *dev)
>  {
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> -	u8 chan;
>  
> -	if (!ndev || !netif_running(ndev))
> +	rtnl_lock();
> +	if (!netif_running(ndev) || priv->hw_suspended) {
> +		rtnl_unlock();
>  		goto suspend_bsp;
> +	}
> +
> +	/* A failed datapath cannot provide a working MAC wake path. It may
> +	 * even have released its wake IRQ. Do not silently suspend without WoL.
> +	 */
> +	if (priv->wolopts && priv->datapath != STMMAC_DATAPATH_RUNNING) {
> +		netdev_err(ndev, "cannot suspend failed datapath with MAC WoL enabled\n");
> +		rtnl_unlock();
> +		return -EBUSY;
> +	}

[Severity: Medium]
Can the hw_suspended shortcut above skip this WoL rejection? Consider
this sequence:

  1. Suspend with MAC WoL sets irq_wake = 1, sets hw_suspended = true and
     moves the datapath to STMMAC_DATAPATH_SUSPENDED.
  2. priv->plat->resume() fails, so stmmac_resume() returns before it
     clears hw_suspended.
  3. An XDP program change or AF_XDP pool teardown calls
     stmmac_bpf()->__stmmac_release(). That frees all IRQs, including the
     wake IRQ, and sets the datapath to STMMAC_DATAPATH_DOWN.
  4. On the next system suspend, hw_suspended is still set, so this
     function jumps straight to suspend_bsp.

The -EBUSY check never runs, and the system suspends with MAC WoL
configured but no wake IRQ. The commit message says this case is rejected
"rather than silently losing the configured wake source".

Should the WoL check come before the hw_suspended shortcut?

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 6128ed1bd521a..9bd622517389d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c

[ ... ]

> @@ -984,12 +985,14 @@ static int __maybe_unused stmmac_pltfr_noirq_resume(struct device *dev)
>  		if (ret)
>  			return ret;
>  
> -		ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> -		if (ret < 0) {
> -			netdev_warn(priv->dev,
> -				    "failed to enable PTP reference clock: %pe\n",
> -				    ERR_PTR(ret));
> -			return ret;
> +		if (priv->datapath != STMMAC_DATAPATH_DOWN) {
> +			ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> +			if (ret < 0) {
> +				netdev_warn(priv->dev,
> +					    "failed to enable PTP reference clock: %pe\n",
> +					    ERR_PTR(ret));
> +				return ret;
> +			}
>  		}
>  	}

[Severity: Medium]
This is a pre-existing issue, but consider a failure in
pm_runtime_force_resume() or clk_prepare_enable(clk_ptp_ref) here. The
function then returns without holding the PTP clock reference, while the
datapath is still not STMMAC_DATAPATH_DOWN. Would a later close, through
stmmac_release_ptp(), or the next noirq suspend then disable clk_ptp_ref
a second time?

The pre-patch code had the same sequence. The new datapath checks only
cover the XDP cleanup case.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index d7e4db7224b0c..909219775507b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c

[ ... ]

> @@ -107,7 +109,7 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
>  	bool need_update;
>  	bool if_running;
>  
> -	if_running = netif_running(dev);
> +	if_running = priv->datapath == STMMAC_DATAPATH_RUNNING;
>  
>  	if (prog && dev->mtu > ETH_DATA_LEN) {
>  		/* For now, the driver doesn't support XDP functionality with

[Severity: High]
In this commit, stmmac_xdp_release() and stmmac_xdp_open() never update
priv->datapath. Further down, the return value of stmmac_xdp_open() is
ignored:

	if (if_running && need_update)
		stmmac_xdp_open(dev);

If that reopen fails, NAPI is disabled and the IRQs and rings are freed,
but the datapath stays STMMAC_DATAPATH_RUNNING.

Would a later close then call stmmac_quiesce() from __stmmac_release() and
hang in napi_disable() on NAPI that is already disabled? It could also
free the IRQs and rings a second time. Suspend, AF_XDP pool changes and
the debugfs ring dump would all trust the stale running state too.

The later commit "net: stmmac: account for failed live XDP reopening" in
this series fixes this. It sets the datapath to STMMAC_DATAPATH_DOWN in
stmmac_xdp_release(), sets it to running only when stmmac_xdp_open()
succeeds, and handles the reopen error in stmmac_xdp_set_prog().

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