Re: [PATCH net-next] net: stmmac: add XDP multi-buff support for TX side

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] stmmac: exact-match coalescing logic skips TX completion interrupts
- [High] stmmac: missing memory barrier before setting TX OWN bit
- [Medium] stmmac: XDP frame check removes SKB TX threshold reservation
--

commit 4955fe9fec58eed4009939eaf9ed4fcf2a742ee7
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

net: stmmac: add XDP multi-buff support for TX side

This patch extends stmmac_xdp_xmit_xdpf to transmit XDP frames with fragments
by mapping and programming each buffer into a dedicated TX descriptor.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9e7e4cb1c31..41ccde6371ecd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5239,77 +5239,114 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
[ ... ]
> -	if (stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv))
> +	if (stmmac_tx_avail(priv, queue) < num_frames)

[Severity: Medium]
Could this change lead to TX ring starvation for standard SKB traffic?

The STMMAC_TX_THRESH check was designed to reserve a portion of the shared TX
ring for standard SKB traffic. By replacing the threshold with a check against
just num_frames, heavy XDP loads could exhaust all available TX descriptors,
potentially leaving 0 descriptors for SKBs and monopolizing the TX ring.

>  		return STMMAC_XDP_CONSUMED;
[ ... ]
> +	tx_q->tx_count_frames += num_frames;
>  
> -	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
> -		set_ic = true;
> -	else
> -		set_ic = false;
> -
> -	if (set_ic) {
> +	if (!(tx_q->tx_count_frames % priv->tx_coal_frames[queue])) {

[Severity: High]
Will this exact-match coalescing logic reliably trigger TX completion
interrupts?

When transmitting XDP multi-buffer frames, tx_count_frames increases by
num_frames, which can be greater than 1. The modulo condition relies on an
exact match to 0 to trigger the Interrupt on Completion (IC) bit.

If num_frames causes the counter to overshoot a multiple of tx_coal_frames
(for example, jumping from 24 to 26 when the threshold is 25), the IC bit
will not be set. This could indefinitely delay TX completion handling and
exhaust the TX ring.

> +		struct stmmac_txq_stats *txq_stats;
>  
> +		desc = stmmac_get_tx_desc(priv, tx_q, entry);
> +		stmmac_set_tx_ic(priv, desc);
>  		tx_q->tx_count_frames = 0;
> -		stmmac_set_tx_ic(priv, tx_desc);
> +
> +		txq_stats = &priv->xstats.txq_stats[queue];
>  		u64_stats_update_begin(&txq_stats->q_syncp);
>  		u64_stats_inc(&txq_stats->q.tx_set_ic_bit);
>  		u64_stats_update_end(&txq_stats->q_syncp);
>  	}
>  
> +	/* Set the OWN bit on the first descriptor now that all descriptors
> +	 * for this skb are populated.
> +	 */
> +	desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
> +	stmmac_set_tx_owner(priv, desc);

[Severity: High]
Is a dma_wmb() needed here before setting the TX OWN bit on the first
descriptor?

The new logic loops over fragments, calling stmmac_prepare_tx_desc() with
tx_own=0 for the first descriptor. This effectively bypasses the internal
dma_wmb() inside stmmac_prepare_tx_desc(), which typically requires both
is_fs and tx_own to be true.

Without a preceding dma_wmb() before manually setting the OWN bit here,
the CPU might reorder the write to the first descriptor's OWN bit before
the writes that populated the fragment descriptors. The DMA engine could
then process stale or uninitialized memory.

> +	tx_q->cur_tx = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
>  	stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
>  
> -	entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
> -	tx_q->cur_tx = entry;
> -
>  	return STMMAC_XDP_TX;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-b4-stmmac-xmit-mb-v1-1-a71a1c522142@oss.qualcomm.com?part=1