Re: [PATCH net 2/3] net: stmmac: close the interface after a failed MTU reopen

[netdev-bot+sashiko@kernel.org]

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

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

- [Critical] The failed-MTU-reopen path leaves the MAC Rx/Tx enabled and
  all RX/TX DMA channels in RUN state while the descriptor rings and RX…
- [High] The new priv->opened state is only maintained by
  __stmmac_open()/__stmmac_release(), but the driver contains a second,…
- [High] The fix is incomplete for the sibling live-reconfiguration paths
  and makes a new inconsistent state reachable there.

--

net: stmmac: close the interface after a failed MTU reopen

The patch adds a new priv->opened flag that tracks whether the data path
was successfully opened, makes __stmmac_release() bail out when it is
clear, and calls netif_close() on the MTU reopen error path so the PHY
and runtime-PM reference are dropped. A few questions below about paths
that are not covered by the new flag.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 7582fca637419..873331dbaf772 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -281,6 +281,8 @@ struct stmmac_priv {
>  	struct mutex lock;
>  
>  	struct stmmac_dma_conf dma_conf;
> +	/* Paired __stmmac_open()/__stmmac_release(), serialized by RTNL. */
> +	bool opened;

[Severity: High]
Does the comment's pairing invariant hold for the XDP release/open pair?

stmmac_xdp_release() performs the same teardown as __stmmac_release()
(stmmac_disable_all_queues(), hrtimer_cancel() per TX queue,
stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0), stmmac_stop_all_dma(),
free_dma_desc_resources(priv, &priv->dma_conf), stmmac_mac_set(false))
but never clears priv->opened, and stmmac_xdp_open() never sets it.

stmmac_xdp_set_prog() also ignores the reopen result:

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

so if stmmac_xdp_open() fails in alloc_dma_desc_resources(),
init_dma_desc_rings() or stmmac_request_irq(), the netdev stays up with
NAPI disabled, IRQs freed and priv->dma_conf holding freed pointers,
while priv->opened is still true.

A later "ip link set down" then passes the new guard and runs the whole
__stmmac_release() a second time. Isn't that the same repeated teardown
the commit message describes, i.e. napi_disable() on already disabled
NAPI while holding RTNL, stmmac_free_irq() on freed IRQs and a second
dma_free_coherent()/page_pool_destroy() over the same rings?

Should stmmac_xdp_release() clear priv->opened and stmmac_xdp_open() set
it on success as well?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..44b59d0f56b5b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4179,6 +4179,7 @@ static int __stmmac_open(struct net_device *dev,
>  	stmmac_enable_all_queues(priv);
>  	netif_tx_start_all_queues(priv->dev);
>  	stmmac_enable_all_dma_irq(priv);
> +	priv->opened = true;
>  
>  	return 0;
>  
> @@ -4249,6 +4250,11 @@ static void __stmmac_release(struct net_device *dev)
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	u8 chan;
>  
> +	/* A failed MTU reopen has already released the data path. */
> +	if (!priv->opened)
> +		return;
> +	priv->opened = false;
> +

[Severity: High]
Can this early return also be reached from the ethtool reconfiguration
paths, where the outer resources have already been dropped?

stmmac_reinit_ringparam() and stmmac_reinit_queues() call the ndo
handlers directly while the netdev stays administratively up:

	if (netif_running(dev))
		stmmac_release(dev);

	priv->dma_conf.dma_rx_size = rx_size;
	priv->dma_conf.dma_tx_size = tx_size;

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

stmmac_release() now clears priv->opened and its tail also runs
stmmac_legacy_serdes_power_down(), phylink_disconnect_phy() and
pm_runtime_put(). If the following stmmac_open() fails (for example in
stmmac_setup_dma_desc(), pm_runtime_resume_and_get() or
stmmac_init_phy()), the interface is left IFF_UP with priv->opened
false, no PHY attached and no runtime-PM reference, and unlike the MTU
path no netif_close() is done here.

>From that state, a later "ip link set down" reaches stmmac_release(),
__stmmac_release() returns at the new guard, and the unconditional tail
still runs:

	__stmmac_release(dev);

	stmmac_legacy_serdes_power_down(priv);
	phylink_disconnect_phy(priv->phylink);
	pm_runtime_put(priv->device);

Does the second pm_runtime_put() here produce a "Runtime PM usage count
underflow!" report?

And in the same state, an MTU change still passes netif_running(),
__stmmac_release() becomes a no-op and __stmmac_open() proceeds into
phylink_prepare_resume() and stmmac_hw_setup() register programming with
no runtime-PM reference held and no PHY connected. Since stmmac's
runtime suspend gates the bus clocks via stmmac_bus_clks_config(priv,
false), can those register accesses hit a suspended device?

>  	/* Stop and disconnect the PHY */
>  	phylink_stop(priv->phylink);
>  

[ ... ]

> @@ -6174,6 +6180,10 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  		if (ret) {
>  			free_dma_desc_resources(priv, dma_conf);
>  			kfree(dma_conf);
> +			/* Finish closing the PHY and PM state, but do not repeat
> +			 * the data-path teardown after the failed reopen.
> +			 */
> +			netif_close(dev);
>  			netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
>  			return ret;
>  		}

[Severity: Critical]
Is the premise of the comment, that the data path has already been
released, true for the hardware state when __stmmac_open() fails in
stmmac_request_irq()?

__stmmac_open() first copies the new configuration into priv->dma_conf
and then calls stmmac_hw_setup(), whose last steps are:

	/* Enable the MAC Rx/Tx */
	stmmac_mac_set(priv, priv->ioaddr, true);
	...
	/* Start the ball rolling... */
	stmmac_start_all_dma(priv);

If stmmac_request_irq() then fails, the unwind is only:

	irq_error:
		phylink_stop(priv->phylink);

		for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
			hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);

		stmmac_release_ptp(priv);
	init_error:
		return ret;

so the MAC stays enabled, all RX/TX DMA channels stay in RUN state, and
priv->opened stays false because it is only set after IRQ setup
succeeds.

stmmac_change_mtu() then calls free_dma_desc_resources(priv, dma_conf).
Since priv->dma_conf aliases dma_conf at this point, doesn't that free
the descriptor rings and RX page-pool pages whose addresses are still
live in the DMA channel base and tail registers?

The following netif_close() reaches __stmmac_release(), which now
returns at the new !priv->opened guard before stmmac_stop_all_dma(),
which was the only remaining caller that clears the RUN bits (and calls
stmmac_deinit_chan()). Can the device keep fetching descriptors and
writing received frames into the freed coherent memory and the returned
page-pool pages?

The commit message says:

	"A subsequent open can then allocate fresh resources and retry
	normally."

With the channels still armed, does the next stmmac_open() reliably
recover, given that the SWR poll in stmmac_init_dma_engine() is bounded
and can time out while the channels still have outstanding bus
transactions?

Would it be better to unwind the hardware where it was started, that is
stmmac_stop_all_dma() plus stmmac_mac_set(priv, priv->ioaddr, false) in
the irq_error path of __stmmac_open(), or before the rings are freed in
stmmac_change_mtu()? The follow-up patch in this series for the resume
path does exactly that (stmmac_stop_all_dma() and stmmac_mac_set(priv,
priv->ioaddr, false) before __stmmac_release()), but the MTU path is
left unchanged at the end of the series.

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