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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 4532 bytes --]

> 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.

ack, I will fix it in v2.

> 
> >  		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.

ack, I will fix it in v2.

> 
> > +		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.

I do not think we need dma_wmb() here since this is the same logic used in
stmmac_xmit() and the barrier is already in stmmac_flush_tx_descriptors().

Regards,
Lorenzo

> 
> > +	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]