Re: [PATCH v4 net 8/8] net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

[netdev-bot+sashiko]

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

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

- [High] In enetc_xdp_xmit()
  (drivers/net/ethernet/freescale/enetc/enetc.c), the doorbell condition…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it comes from
  the earlier commit in the same series, 'net: enetc: serialize access…

--

net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

This commit changes the doorbell condition in enetc_xdp_xmit(). The TX ring
tail is now updated only when at least one frame was sent and either
XDP_XMIT_FLUSH was requested or the frames array could not be fully
enqueued. The redundant xdp_tx_frm_cnt variable is removed in favour of k.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 218065b9a3777..3003a415418ba 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -1928,22 +1928,20 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  						    &xdp_redirect_arr[i]);
>  			break;
>  		}
> -
> -		xdp_tx_frm_cnt++;
>  	}
>  
> -	if (unlikely((flags & XDP_XMIT_FLUSH) || k != xdp_tx_frm_cnt))
> +	if (unlikely(k && ((flags & XDP_XMIT_FLUSH) || k < num_frames)))
>  		enetc_update_tx_ring_tail(tx_ring);

[Severity: High]

Does the new k && term skip the doorbell when an XDP_XMIT_FLUSH call
enqueues zero frames?

The old condition rang on every FLUSH call, because k != xdp_tx_frm_cnt was
always false. XDP_XMIT_FLUSH asks the driver to publish everything queued so
far. That includes frames from earlier ndo_xdp_xmit() calls made with
flags == 0.

devmap makes calls like that when the bulk queue fills up:

kernel/bpf/devmap.c:bq_enqueue() {
    ...
	if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
		bq_xmit_all(bq, 0);
    ...
}

Only the later __dev_flush() passes XDP_XMIT_FLUSH:

kernel/bpf/devmap.c:__dev_flush() {
    ...
		bq_xmit_all(bq, XDP_XMIT_FLUSH);
    ...
}

The flags == 0 call advances tx_ring->next_to_use without writing TPIR. If
the first frame of the next FLUSH batch fails, k is 0 and
enetc_update_tx_ring_tail() is never called.

The first frame can fail in these cases:

  - enetc_xdp_tx() finds too few free BDs. This is likely right after the
    previous batch used up the ring.
  - The head buffer is shorter than ENETC_MIN_BUFF_SIZE.
  - The frame fails the ENETC_MAX_SKB_FRAGS check.
  - DMA mapping fails.

On a ring reserved for XDP, the only other TPIR writer is the XDP_TX tail
update in enetc_clean_rx_ring_xdp(). It rings only when it has enqueued
XDP_TX frames itself:

	if (xdp_tx_frm_cnt) {
		spin_lock(&tx_ring->xdp_lock);
		enetc_update_tx_ring_tail(tx_ring);
		spin_unlock(&tx_ring->xdp_lock);
	}

So the stranded BDs stay unpublished until a later call both succeeds and
flushes. If traffic stops, they never go out.

If they fill the ring, every later enqueue fails on its first frame. TX
completion cannot reclaim BDs the hardware never saw. Can the XDP TX queue
then stall permanently, with the xdp_frames and pages still pinned?

Other drivers ring the tail on XDP_XMIT_FLUSH no matter how many frames
that call sent. For example, i40e_xdp_xmit():

	if (unlikely(flags & XDP_XMIT_FLUSH))
		i40e_xdp_ring_update_tail(xdp_ring);

Would something like this be closer to the intent?

	if (unlikely((flags & XDP_XMIT_FLUSH) || (k && k < num_frames)))

The commit message says:

    The correct condition on which we should ring the doorbell is when at
    least one frame was sent, and either the caller required us to flush, or
    we couldn't enqueue the entire passed array.

The subject says the patch ensures the tail update happens, and the patch
has a Fixes: tag. Even so, FLUSH calls with k == 0 no longer ring the
doorbell, and the commit message does not mention this change.

>  
>  	enetc_unlock_mdio();
>  
>  out:
> -	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
> -	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
> +	tx_ring->stats.xdp_tx += k;
> +	tx_ring->stats.xdp_tx_drops += num_frames - k;
>  
>  	spin_unlock(&tx_ring->xdp_lock);

[Severity: High]

This isn't a bug introduced by this patch. It comes from the earlier patch
in this series, "net: enetc: serialize access to the same TX ring from NAPI
XDP_TX and ndo_xdp_xmit()".

Can tx_ring->xdp_lock and enetc_mdio_lock deadlock on PREEMPT_RT when the
ERR050089 workaround (enetc_has_err050089) is enabled?

enetc_xdp_xmit() takes xdp_lock first and then the MDIO read lock:

	spin_lock(&tx_ring->xdp_lock);
	...
	enetc_lock_mdio();

enetc_clean_rx_ring_xdp() drops xdp_lock at the out: label but keeps the
MDIO read lock. It then takes xdp_lock again, so the order is reversed:

out:
	spin_unlock(&tx_ring->xdp_lock);
	...
	if (xdp_tx_frm_cnt) {
		spin_lock(&tx_ring->xdp_lock);
		enetc_update_tx_ring_tail(tx_ring);
		spin_unlock(&tx_ring->xdp_lock);
	}
	...
	enetc_unlock_mdio();

On PREEMPT_RT, rwbase_write_lock() takes the rtmutex and removes
READER_BIAS. New readers then block behind a pending writer. That would
allow this cycle:

T1 (RX NAPI, enetc_clean_rx_ring_xdp())
  holds the MDIO read lock after out:

T3 (_enetc_rd_mdio_reg_wa())
  write_lock_irqsave(&enetc_mdio_lock, flags);
  waits for T1 to drop the read lock

T2 (enetc_xdp_xmit() on another CPU, targeting the same XDP TX ring,
    e.g. after an IRQ affinity change)
  spin_lock(&tx_ring->xdp_lock);
  enetc_lock_mdio();          <- blocks behind T3

T1
  spin_lock(&tx_ring->xdp_lock);  <- blocks behind T2

Non-RT testing would not catch this. On non-RT, qrwlock readers in
interrupt context ignore a waiting writer, and lockdep treats this
read_lock as recursive.

Would it help to use one lock order in the RX tail update path? For
example, the MDIO read lock could be dropped before re-taking xdp_lock and
then taken again inside it.

>  
> -	return err ?: xdp_tx_frm_cnt;
> +	return err ?: k;
>  }
>  EXPORT_SYMBOL_GPL(enetc_xdp_xmit);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015105536.1692348-1-vladimir.oltean%40nxp.com