Re: [PATCH net v2 2/2] 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: 1 (1) · Medium: 6 · 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] Changing the XDP program on a running interface releases and
  reopens the datapath but never updates the new priv->datapath state,…
- [Medium] The ethtool reinit paths are also release-then-reopen paths,
  and the patch does not handle their failure.
- [Medium] stmmac_bpf() calls `if (priv->datapath ==
  STMMAC_DATAPATH_SUSPENDED) __stmmac_release(dev);` for every…
- [Medium] The patch adds a lasting state where the interface is IFF_UP
  but datapath is DOWN, and in that state clk_ptp_ref has already been…
- [Medium] STMMAC_DATAPATH_SUSPENDED covers two different situations:
  'the system is asleep right now' and 'hardware resume failed'.
- [Medium] The patch leaves a failed resume in SUSPENDED so that 'a later
  successful resume can retry the retained datapath'.
- [Medium] On Tegra MGBE, tegra_mgbe_resume() calls
  clk_bulk_disable_unprepare() on all MGBE clocks when stmmac_resume()…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  touches it and leaves it reachable.

--

net: stmmac: keep datapath state coherent after reinitialization failure

This commit adds a priv->datapath state (DOWN, RUNNING, SUSPENDED) to
stmmac, so that closing after a failed MTU reopen or a failed hardware
resume only does the teardown that is still needed. It moves suspend and
resume under RTNL and detaches the netdev when an MTU reopen fails. The XDP,
TC, debugfs and reset paths now check either the new state or whether the
device is present.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..7b423c87314ca 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -4244,25 +4247,38 @@ static int stmmac_open(struct net_device *dev)

[ ... ]

> +static void __stmmac_release(struct net_device *dev)
> +{
> +	struct stmmac_priv *priv = netdev_priv(dev);
> +
> +	/* A failed MTU reopen has already released the data path. */
> +	if (priv->datapath == STMMAC_DATAPATH_DOWN)
> +		return;

[Severity: Medium]
Could this early return leak an enable of clk_ptp_ref?

With this patch the interface can stay IFF_UP while priv->datapath is
STMMAC_DATAPATH_DOWN and stmmac_release_ptp() has already released
clk_ptp_ref. That happens after a failed MTU reopen, a failed ethtool
reinit, or stmmac_bpf() releasing a suspended datapath.

stmmac_suspend() and stmmac_resume() now check priv->datapath. The
platform noirq callbacks in stmmac_platform.c still check
netif_running():

stmmac_pltfr_noirq_suspend():
	if (!netif_running(ndev))
		return 0;

	if (!priv->wolopts) {
		/* Disable clock in case of PWM is off */
		clk_disable_unprepare(priv->plat->clk_ptp_ref);

Here is what happens on a system suspend without WoL in this state:

- noirq suspend disables a clock that is already disabled, which
  triggers the clk core warning.
- stmmac_pltfr_noirq_resume() then calls clk_prepare_enable() on
  clk_ptp_ref.
- stmmac_resume() does nothing, because the datapath is not SUSPENDED.
- The later ndo_stop takes this early return and never calls
  stmmac_release_ptp().

Does the PTP reference clock then stay enabled for good, with each later
open/close cycle leaving one extra enable behind?

[ ... ]

> @@ -4296,6 +4312,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]
Do the ethtool reinit paths still end up in an inconsistent state when
the reopen fails?

stmmac_reinit_ringparam() (set_ringparam) and stmmac_reinit_queues()
(set_channels) call stmmac_release() and then reopen with:

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

If stmmac_open() fails, its own unwind already calls
phylink_disconnect_phy() and pm_runtime_put(). The interface is left
IFF_UP and present, with priv->datapath at STMMAC_DATAPATH_DOWN.

On the next ndo_stop, __stmmac_release() returns early. stmmac_release()
still calls phylink_disconnect_phy() and pm_runtime_put() unconditionally
here.

Is this pm_runtime_put() unbalanced, so that it triggers the "Runtime PM
usage count underflow!" warning? The detach and PHY/PM ownership handling
added to stmmac_change_mtu() does not seem to cover these callers.

[ ... ]

> @@ -6422,6 +6445,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 is netif_device_present()
the right check here?

__stmmac_disable_all_queues() calls napi_disable() on every rx/tx/rxtx
NAPI. In several states the device is present but its NAPIs are already
disabled, or were never enabled:

- the interface is administratively down, including after the failed
  MTU state is closed and stmmac_release() calls netif_device_attach();
- stmmac_reinit_ringparam() or stmmac_reinit_queues() failed in
  stmmac_open();
- an XDP reopen failed.

tc_cls_can_offload_and_chain0() does not check the admin state, so a
sequence such as:

  ip link set dev X down
  tc qdisc add dev X clsact
  tc filter add dev X ingress flower ... skip_sw

would reach napi_disable_locked(), which then spins forever on
NAPIF_STATE_SCHED with RTNL held.

The new state tracks whether NAPI is enabled and the resources are
owned. Would checking priv->datapath == STMMAC_DATAPATH_RUNNING here be
more accurate?

[ ... ]

> @@ -6959,6 +6987,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 only happen for requests that actually change the
pool or the program?

It runs for every XDP_SETUP_PROG and XDP_SETUP_XSK_POOL request, before
any validation. That includes requests that are later rejected or do
nothing:

- stmmac_xdp_set_prog() returns -EOPNOTSUPP for a prog when
  dev->mtu > ETH_DATA_LEN
- one program replaces another, so need_update is false
- stmmac_xdp_enable_pool() rejects a small frame_size, or
  xsk_pool_dma_map() fails

In each of these cases the retained rings are freed and the datapath
becomes DOWN. A later stmmac_resume() then does:

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

It never calls netif_device_attach(), so the interface stays detached
until someone runs a manual down/up.

The commit message only gives a reason for this in the AF_XDP pool
removal case. It also says "A later successful resume can retry the
retained datapath".

[Severity: Medium]
Can this also run during a normal system sleep, and not only after a
failed resume?

stmmac_suspend() sets STMMAC_DATAPATH_SUSPENDED before it drops RTNL. A
failed resume leaves the same state. AF_XDP pool teardown can reach
ndo_bpf from the non-freezable system_wq:

xp_put_pool()
  schedule_work(xp_release_deferred)
    rtnl_lock()
    xp_clear_dev()
      ndo_bpf(XDP_SETUP_XSK_POOL, NULL)
        stmmac_bpf()
          __stmmac_release()

If that work runs after stmmac_suspend() releases RTNL, __stmmac_release()
would:

- call phylink_stop();
- free the IRQs;
- write DMA registers that stmmac_pltfr_noirq_suspend() or
  pm_runtime_force_suspend() may already have clock-gated;
- disable clk_ptp_ref a second time via stmmac_release_ptp().

On wake, stmmac_resume() sees STMMAC_DATAPATH_DOWN and returns 0 without
calling netif_device_attach(). Before this patch, removing a pool with
no XDP program loaded did not touch the hardware.

Is there a way to tell the two SUSPENDED cases apart here?

[ ... ]

> @@ -8198,26 +8246,24 @@ 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 (priv->datapath != STMMAC_DATAPATH_RUNNING) {
> +		rtnl_unlock();
>  		goto suspend_bsp;
> +	}

[Severity: Medium]
After a failed resume, does the next suspend skip re-arming WoL?

The failed stmmac_resume() has already cleared PMT with
stmmac_pmt(priv, priv->hw, 0) and set irq_wake to 0, or has switched
pinctrl to the default state.

The datapath is left SUSPENDED so that a later resume can retry. On the
next system suspend, though, this check sends stmmac_suspend() straight
to suspend_bsp and skips:

	if (priv->wolopts) {
		stmmac_pmt(priv, priv->hw, priv->wolopts);
		priv->irq_wake = 1;
	} else {
		stmmac_mac_set(priv, priv->ioaddr, false);
		pinctrl_pm_select_sleep_state(priv->device);
	}

With WoL configured, the noirq suspend keeps the clocks on, but the MAC
wake mode is never armed again, so the system will not wake on LAN.
Without WoL, the pins stay in the active pinctrl state through suspend.

Is that intended?

[ ... ]

> @@ -8371,11 +8415,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]
On Tegra MGBE, is it safe to recover from this failure with an ordinary
down?

When the common resume fails, tegra_mgbe_resume() in dwmac-tegra.c turns
the clocks off:

	err = stmmac_resume(dev);
	if (err < 0)
		clk_bulk_disable_unprepare(ARRAY_SIZE(mgbe_clks), mgbe->clks);

The datapath stays STMMAC_DATAPATH_SUSPENDED. A later ndo_stop, or the
release in stmmac_bpf(), then runs __stmmac_release() with the MGBE
clocks off:

- phylink_stop() takes the MAC link down and uses MDIO;
- stmmac_stop_all_dma() accesses the XGMAC channel-control and MAC TX
  configuration registers.

Nothing turns these clocks back on first, and the runtime-PM reference
does not cover them. Part of this predates the patch, since the old
ndo_stop also accessed registers through phylink_stop() before it hung.

stmmac_suspend() now returns early when the datapath is not RUNNING. The
next tegra_mgbe_suspend() would therefore call
clk_bulk_disable_unprepare() a second time.

Can this lead to register access on unclocked hardware and to
unbalanced clock disables?

> 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;

[Severity: High]
Does attaching or detaching an XDP program leave priv->datapath stale?

When need_update is true on a running interface, stmmac_xdp_set_prog()
does:

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

stmmac_xdp_release() disables NAPI, frees the IRQs, stops DMA and frees
the rings.

stmmac_xdp_open() can fail in alloc_dma_desc_resources(),
init_dma_desc_rings() or stmmac_request_irq(). Its error labels free the
new rings and return with NAPI disabled and no IRQs requested.

The return value is ignored here, and neither function writes
priv->datapath. It therefore stays STMMAC_DATAPATH_RUNNING, and the
netdev is still present.

After that, would ndo_stop hang?

stmmac_release()
  __stmmac_release()
    if (priv->datapath == STMMAC_DATAPATH_RUNNING)
        stmmac_quiesce(priv);
          stmmac_disable_all_queues()
            napi_disable()   <- already disabled

napi_disable_locked() would spin forever on NAPIF_STATE_SCHED with RTNL
held. It would then be followed by a second stmmac_free_irq() and
free_dma_desc_resources().

Other paths would hit the same state:

- stmmac_suspend(), stmmac_xdp_enable_pool() and
  stmmac_xdp_disable_pool() would reach the same napi_disable();
- stmmac_rings_status_show() only skips the dump for
  STMMAC_DATAPATH_DOWN, so it would read the freed descriptor rings.

The failed-reopen problem here existed before this patch. However, this
commit targets this kind of failed-reopen state and moves this function
onto priv->datapath.

Should stmmac_xdp_release() and stmmac_xdp_open() update priv->datapath?
Should a failed stmmac_xdp_open() also be handled the same way as the
MTU failure path?

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