Re: [PATCH net v3 08/10] net: stmmac: use the tracked datapath restart for XSK pool changes

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] Every live AF_XDP zero-copy pool bind or unbind now bounces
  the physical link, and the commit message does not say so.
- [Medium] Attaching an XSK pool now leaves rxtx NAPI repolling
  continuously for as long as carrier is down, even when there is no…
- [Low] stmmac.h keeps a stale external prototype for
  stmmac_xsk_wakeup().

Pre-existing issues:
- [Medium] stmmac_xdp_release() calls stmmac_stop_all_dma() and then
  immediately frees the descriptor rings and buffers with…

--

net: stmmac: use the tracked datapath restart for XSK pool changes

Live XSK pool attach and removal used per-queue disable/enable helpers.
This patch replaces them with the full stmmac_xdp_release() and
stmmac_xdp_open() pair. It undoes the bitmap bit and DMA mapping if
attach fails, and always returns success on removal. The unused per-queue
helpers are removed, and their TBS programming moves into
stmmac_xdp_open().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index ab5157fccc465..f07623a1285c9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -437,10 +437,6 @@ static inline bool stmmac_xdp_is_enabled(struct stmmac_priv *priv)
>  	return !!priv->xdp_prog;
>  }
>  
> -void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue);
> -void stmmac_enable_rx_queue(struct stmmac_priv *priv, u32 queue);
> -void stmmac_disable_tx_queue(struct stmmac_priv *priv, u32 queue);
> -void stmmac_enable_tx_queue(struct stmmac_priv *priv, u32 queue);
>  int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags);

[Severity: Low]
This isn't a bug, but should this prototype be removed as well?

The only caller outside stmmac_main.c was stmmac_xdp_enable_pool(), and
this patch replaces that call with napi_schedule(). After this patch,
stmmac_xsk_wakeup() is referenced only by its definition and the
.ndo_xsk_wakeup entry in stmmac_main.c. It could be made static there.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index 7ecb7addd2ea7..907ac49a1b764 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c

[ ... ]

> @@ -34,23 +33,23 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
>  	need_update = priv->datapath == STMMAC_DATAPATH_RUNNING &&
>  		      stmmac_xdp_is_enabled(priv);
>  
> -	if (need_update) {
> -		napi_disable(&ch->rx_napi);
> -		napi_disable(&ch->tx_napi);
> -		stmmac_disable_rx_queue(priv, queue);
> -		stmmac_disable_tx_queue(priv, queue);
> -	}
> +	if (need_update)
> +		stmmac_xdp_release(priv->dev);

[Severity: Medium]
Does this bounce the physical link on every live XSK pool bind and
unbind?

An earlier patch in this series ("net: stmmac: account for failed live
XDP reopening") made stmmac_xdp_release() call phylink_stop() and
stmmac_xdp_open() call phylink_start(). The attach path looks like this:

stmmac_xdp_enable_pool()
  stmmac_xdp_release()
    phylink_stop()
      phy_stop()
        PHY_HALTED: phy_link_down(), PHY_STATE_WORK_SUSPEND
        _phy_state_machine_post_work(): phy_suspend()
  stmmac_xdp_open()
    phylink_start()
      netif_carrier_off()
      autonegotiation restarts

stmmac_xdp_disable_pool() follows the same path on every socket close.

Each bind or unbind would then cause a carrier down/up event and a PHY
power-down followed by renegotiation, which typically takes seconds at
1000BASE-T. Traffic stops on every queue. The rings of all other queues
are freed and rebuilt, including queues serving other live AF_XDP
sockets.

The previous per-queue path touched only one queue and left the link
alone.

The commit message only says "This pauses all queues during a pool
change". Since the patch carries a Fixes: tag, could the commit message
mention the link flap? Alternatively, could pool changes avoid the
phylink stop/start?

>  
>  	set_bit(queue, priv->af_xdp_zc_qps);
>  
>  	if (need_update) {
> -		stmmac_enable_rx_queue(priv, queue);
> -		stmmac_enable_tx_queue(priv, queue);
> -		napi_enable(&ch->rxtx_napi);
> -
> -		err = stmmac_xsk_wakeup(priv->dev, queue, XDP_WAKEUP_RX);
> -		if (err)
> +		err = stmmac_xdp_open(priv->dev);
> +		if (err) {
> +			clear_bit(queue, priv->af_xdp_zc_qps);
> +			xsk_pool_dma_unmap(pool, STMMAC_RX_DMA_ATTR);
> +			netdev_err(priv->dev, "failed reopening after XSK pool attach: %pe; interface remains detached\n",
> +				   ERR_PTR(err));
>  			return err;
> +		}
> +
> +		/* The pool is installed even if link resolution is still pending. */
> +		napi_schedule(&priv->channel[queue].rxtx_napi);

[Severity: Medium]
Can this leave rxtx_napi repolling nonstop while the carrier is down?

The old code went through stmmac_xsk_wakeup(), which returns -ENETDOWN
when !netif_carrier_ok(). At this point the carrier is always down: the
PHY was just halted, and phylink_start() inside stmmac_xdp_open() has just
called netif_carrier_off(). If no cable is connected, it stays down
indefinitely.

Because tx_q->xsk_pool is set, the poll goes through:

stmmac_napi_poll_rxtx()
  stmmac_tx_clean()
    stmmac_xdp_xmit_zc()

stmmac_xdp_xmit_zc() checks the carrier before it looks at the XSK TX
ring:

	if (unlikely(stmmac_tx_avail(priv, queue) < STMMAC_TX_XSK_AVAIL) ||
	    !netif_carrier_ok(priv->dev)) {
		work_done = false;
		break;
	}

stmmac_tx_clean() then turns that into a full budget:

	if (work_done)
		xmits = budget - 1;
	else
		xmits = budget;
	...
	return max(count, xmits);

stmmac_napi_poll_rxtx() then returns without completing NAPI:

	if (rxtx_done >= budget)
		return budget;

Wouldn't this spin a CPU in softirq or ksoftirqd, even with an empty XSK
TX ring, until the carrier comes up or the interface is closed?

>  	}
>  
>  	return 0;

[ ... ]

> @@ -73,24 +72,21 @@ static int stmmac_xdp_disable_pool(struct stmmac_priv *priv, u16 queue)
>  	need_update = priv->datapath == STMMAC_DATAPATH_RUNNING &&
>  		      stmmac_xdp_is_enabled(priv);
>  
> -	if (need_update) {
> -		napi_disable(&ch->rxtx_napi);
> -		stmmac_disable_rx_queue(priv, queue);
> -		stmmac_disable_tx_queue(priv, queue);
> -		synchronize_rcu();
> -	}
> +	if (need_update)
> +		stmmac_xdp_release(priv->dev);
>  
>  	xsk_pool_dma_unmap(pool, STMMAC_RX_DMA_ATTR);

[Severity: Medium]
This is a pre-existing issue, but is the hardware DMA guaranteed to be
idle when the pool is unmapped here?

stmmac_xdp_release() does:

	stmmac_stop_all_dma(priv);

	/* Release and free the Rx/Tx resources */
	free_dma_desc_resources(priv, priv->dma_conf);

	/* Disable the MAC Rx/Tx */
	stmmac_mac_set(priv, priv->ioaddr, false);

dwmac4_dma_stop_rx() and dwmac4_dma_stop_tx() only clear the SR/ST bits.
They never poll for the Stopped state, and the MAC receiver is disabled
only after the rings have been freed.

A later commit in this series ("net: stmmac: retain DMA resources across
MTU changes") notes that "clearing a DMA enable bit need not finish an
in-flight frame immediately". An in-flight frame or descriptor writeback
could therefore land in freed descriptors or in the XSK buffers unmapped
here.

__stmmac_release(), the XDP program change path and the removed
per-queue helpers all follow the same stop-then-free pattern, so this
patch did not introduce it. The old synchronize_rcu() before the unmap
did not drain DMA either.

>  
>  	clear_bit(queue, priv->af_xdp_zc_qps);
>  

[ ... ]

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