Re: [PATCH net v4 10/15] 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 5 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 3 (2) · 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] The all-queue restart now reports AF_XDP TX completions a second
  time for buffers that were already returned on another, still-attached…
- [Medium] In stmmac_xdp_xmit_zc()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the patch changes…
- [Medium] stmmac_xdp_enable_pool() and stmmac_xdp_disable_pool() in
  drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c now restart the whole…
- [Medium] Because pool attach and detach now restart every queue,
  pending AF_XDP TX work on an unrelated zero-copy queue can be left…
- [Low] stmmac_xdp_xmit_zc() now returns true at entry when carrier is
  down, even if XSK TX descriptors are still pending in the user TX ring.

Pre-existing issues:
- [Medium] At this commit, stmmac_xdp_release() calls
  stmmac_stop_all_dma(), which only clears the SR/ST bits and does not…
- [Medium] stmmac_set_queue_rx_buf_size() picks the XSK frame size only
  when `rx_q->xsk_pool && rx_q->buf_alloc_num`.

--

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

This commit drops the per-queue XSK pool enable/disable helpers. When a
pool is attached or removed while XDP is running, it now does a full
stmmac_xdp_release()/stmmac_xdp_open() restart instead. It also makes
stmmac_xdp_xmit_zc() return early while carrier is down, restores TBS in
stmmac_xdp_open(), and unwinds a failed reopen.

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

[ ... ]

> @@ -2756,6 +2757,12 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
>  	bool work_done = true;
>  	u32 tx_set_ic_bit = 0;
>  
> +	/* Nothing can be submitted while the link is down. Let NAPI complete;
> +	 * userspace can retry ndo_xsk_wakeup() once carrier has returned.
> +	 */
> +	if (!netif_carrier_ok(priv->dev))
> +		return true;
> +

[Severity: Medium]
Can AF_XDP zero-copy TX stall after a link flap with this change?

Before this patch, the carrier check inside the loop set work_done = false.
stmmac_tx_clean() then returned budget, and rxtx_napi kept polling until
carrier came back. With the early return, stmmac_tx_clean() takes the other
branch:

stmmac_tx_clean() {
    ...
	work_done = stmmac_xdp_xmit_zc(priv, queue, STMMAC_XSK_TX_BUDGET_MAX);
	if (work_done)
		xmits = budget - 1;
	else
		xmits = budget;
    ...
}

NAPI then completes. Nothing seems to reschedule rxtx_napi on link up,
because stmmac_mac_link_up() doesn't call napi_schedule(). A kick made
during the outage also fails:

stmmac_xsk_wakeup() {
    ...
	if (test_bit(STMMAC_DOWN, &priv->state) ||
	    !netif_carrier_ok(priv->dev))
		return -ENETDOWN;
    ...
}

Descriptors queued during the outage stay unsent until userspace kicks
again after carrier returns, or until RX traffic raises an IRQ on that
channel. An application blocked in poll() with no RX on the channel would
not recover.

This applies to every carrier-down period, not only pool changes, and the
commit message doesn't mention it. Would it be better as a separate patch
that describes the new behaviour?

[Severity: Low]
This isn't a bug, but a true return can now also mean "carrier down,
nothing drained". The comment in stmmac_tx_clean() still says:

		/* For XSK TX, we try to send as many as possible.
		 * If XSK work done (XSK TX desc empty and budget still
		 * available), return "budget - 1" to reenable TX IRQ.

The trailing comment in stmmac_xdp_xmit_zc() also still says true is only
returned when budget remains and the XSK TX desc peek is empty. Should
both comments be updated?

[ ... ]

> 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]
Is a full device restart intended on every AF_XDP bind and unbind?

stmmac_xdp_release() does the following:
- detaches the netdev;
- calls phylink_replay_link_begin(), which runs phylink_link_down() and
  netif_carrier_off();
- frees all IRQs, stops all DMA channels and frees every RX/TX ring.

stmmac_xdp_open() then rebuilds everything, and phylink_replay_link_end()
turns carrier back on.

Before this patch, only the target queue was restarted. Now each bind or
unbind appears to cause a carrier off/on transition: RTM_NEWLINK events,
operstate changes, a carrier_down_count increment, and possible bonding or
team failover. It also drops in-flight traffic on all unrelated queues.

The operation can now fail for reasons that have nothing to do with the
target queue, such as re-requesting IRQs or allocating rings for other
queues. On failure, the whole interface stays detached, with phylink
stopped and PTP released.

In the attach path, if the socket didn't force XDP_ZEROCOPY,
xp_assign_dev() falls back to copy mode. bind() can then succeed while the
interface is dead.

The commit message says "Pause all queues and MAC link resolution". It
doesn't mention the carrier flap or the effect on other traffic. Could that
be spelled out?

>  
>  	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]
What happens to pending XSK TX work on other zero-copy queues after this
restart?

Take a queue A that already has an active pool, with its NAPI polling
because work_done == false (for example, the TX ring was nearly full).
stmmac_xdp_release() disables A's NAPI, cancels the TX hrtimers and frees
A's TX ring.

stmmac_xdp_open() re-enables NAPI via stmmac_enable_all_queues(), but
napi_enable() doesn't schedule a poll. netif_device_attach() only wakes the
netdev TX queues.

Only channel[queue].rxtx_napi for the newly attached queue is scheduled
here, and stmmac_xdp_disable_pool() schedules nothing. Queue A is left with
no pending completions, no armed timer and no NAPI poll. Its XSK TX work
stays stalled until userspace kicks again or RX traffic arrives on
channel A.

xsk_poll() only calls xsk_wakeup() on entry, so a TX-only application
already blocked in poll() for POLLOUT would not be woken. Should every
queue with an XSK pool be rescheduled after a successful reopen?

[Severity: Medium]
This is a pre-existing issue, but now that this napi_schedule() drives the
initial ZC refill, can RBSZ end up programmed with the wrong size?

stmmac_set_queue_rx_buf_size() only uses the XSK frame size when buffers
have already been allocated:

	if (rx_q->xsk_pool && rx_q->buf_alloc_num)
		buf_size = xsk_pool_get_rx_frame_size(rx_q->xsk_pool);
	else
		buf_size = priv->dma_conf->dma_buf_sz;

__init_dma_rx_desc_rings() ignores the return value of
stmmac_alloc_rx_buffers_zc(). If the fill ring is still empty at bind time,
buf_alloc_num is 0 and stmmac_xdp_open() programs dma_buf_sz. The later
stmmac_rx_refill_zc() only advances the tail pointer and never reprograms
RBSZ.

The programmed size would only exceed the real buffer size for XSK frame
sizes from 1522 to 1535 bytes. The removed stmmac_enable_rx_queue() used
the same ordering.

[ ... ]

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

[Severity: High]
Could this restart report TX completions a second time to a pool that
stays attached on another queue?

When stmmac_tx_clean() completes an XSK TX descriptor, it counts it but
leaves buf_type set:

		if (tx_q->tx_skbuff_dma[entry].buf_type == STMMAC_TXBUF_T_XSK_TX)
			tx_q->xsk_frames_done++;

The slot stays STMMAC_TXBUF_T_XSK_TX until an skb transmit reuses it.

A pool change on queue B now goes through stmmac_xdp_release() ->
free_dma_desc_resources() for every queue. For queue A,
dma_free_tx_skbufs() walks all dma_tx_size entries:

	for (i = 0; i < dma_conf->dma_tx_size; i++)
		stmmac_free_tx_buffer(priv, dma_conf, queue, i);

	if (tx_q->xsk_pool && tx_q->xsk_frames_done) {
		xsk_tx_completed(tx_q->xsk_pool, tx_q->xsk_frames_done);

stmmac_free_tx_buffer() counts every stale XSK_TX slot again.
xsk_tx_completed() ends in xskq_prod_submit_n(), which advances the
producer without checking cached_prod:

static inline void xskq_prod_submit_n(struct xsk_queue *q, u32 nb_entries)
{
	__xskq_prod_submit(q, q->ring->producer + nb_entries);
}

Userspace would then see stale addresses in queue A's completion ring as
newly completed, and later completions would be misaligned. The
application could then reuse UMEM frames the kernel still owns. The old
per-queue restart never touched queue A's ring.

Should buf_type be reset on completion, or should teardown count only the
dirty_tx..cur_tx range?

>  
>  	xsk_pool_dma_unmap(pool, STMMAC_RX_DMA_ATTR);

[Severity: Medium]
This is a pre-existing issue, but is DMA guaranteed to be idle before the
rings are freed and the pool is unmapped here?

At this commit, 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 don't wait for the stopped state. An in-flight frame or descriptor
write-back could therefore land in freed rings or unmapped UMEM.

The removed per-queue helpers used the same stop-then-free pattern, and
their synchronize_rcu() never drained DMA.

A later patch in this series appears to address this: "net: stmmac: retain
DMA memory until hardware shutdown completes" adds stmmac_drain_dma()
before free_dma_desc_resources() in stmmac_xdp_release().

[ ... ]

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