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

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

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

> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 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).
> 
> - [Medium] The patch adds multi-buffer (frag) handling to
>   stmmac_xdp_xmit_xdpf() in…
> - [Medium] In stmmac_xdp_xmit_xdpf()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the patch removes…
> - [Low] A new comment in stmmac_xdp_xmit_xdpf()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:5332-5334) reads…

I will fix sashiko's reported issues in v3.

Regard,
Lorenzo

> 
> --
> 
> net: stmmac: add XDP multi-buff support for TX side
> 
> This commit extends stmmac_xdp_xmit_xdpf() so it can send XDP frames that
> carry fragments. The frame head and each frag are mapped and placed in
> their own TX descriptor. The OWN bit on the first descriptor is set only
> after all the descriptors are filled in.
> 
> Link: https://lore.kernel.org/r/20260923-b4-stmmac-xmit-mb-v1-1-a71a1c522142@oss.qualcomm.com
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index bf9e7e4cb1c31..089281ef9e612 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5239,77 +5239,117 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> >  static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
> >  				struct xdp_frame *xdpf, bool dma_map)
> >  {
> 
> [ ... ]
> 
> > -	if (stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv))
> > +	if (unlikely(xdp_frame_has_frags(xdpf)))
> > +		num_frames += sinfo->nr_frags;
> 
> [Severity: Medium]
> Can a frame with frags reach this function from either caller?
> 
> On the ndo_xdp_xmit path, stmmac_xdp_set_prog() still registers the
> redirect target without SG support:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
>     ...
> 	if (prog)
> 		xdp_features_set_redirect_target(dev, false);
>     ...
> }
> 
> The static mask in stmmac_dvr_probe() is only NETDEV_XDP_ACT_BASIC,
> NETDEV_XDP_ACT_REDIRECT and NETDEV_XDP_ACT_XSK_ZEROCOPY. Because
> NETDEV_XDP_ACT_NDO_XMIT_SG is never set, devmap drops fragmented frames
> before they get to stmmac_xdp_xmit():
> 
> kernel/bpf/devmap.c:__xdp_enqueue() {
>     ...
> 	if (unlikely(!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG) &&
> 		     xdp_frame_has_frags(xdpf)))
> 		return -EOPNOTSUPP;
>     ...
> }
> 
> is_valid_dst() does the same check on the broadcast path.
> 
> On the XDP_TX path, stmmac_xdp_xmit_back() only gets frames built by the
> stmmac RX path, and that path never builds a multi-buffer xdp_buff. The
> driver has no xdp_buff_add_frag() or xdp_buff_set_frags() calls.
> stmmac_xdp_set_prog() also rejects XDP when mtu > ETH_DATA_LEN,
> stmmac_change_mtu() blocks jumbo MTU while XDP is loaded, and SPH is turned
> off for XDP.
> 
> If so, the frag loop, the skb_frag_dma_map() and skb_frag_off() handling,
> and the multi-descriptor error_dma_unmap unwind can't be run or tested. The
> subject "add XDP multi-buff support for TX side" also doesn't match what
> the driver advertises.
> 
> Should this patch advertise NDO_XMIT_SG as well, for example by calling
> xdp_features_set_redirect_target(dev, true)?
> 
> [ ... ]
> 
> > +		stmmac_set_tx_dma_entry(tx_q, entry, buf_type, dma_addr, len,
> > +					dma_map && frag);
> > +		stmmac_set_desc_addr(priv, desc, dma_addr);
> > +		stmmac_prepare_tx_desc(priv, desc, !i, len, csum,
> > +				       priv->descriptor_mode, !!i, last_frame,
> > +				       xdp_get_frame_len(xdpf));
> > +		tx_q->xdpf[entry] = last_frame ? xdpf : NULL;
> > +		if (last_frame) {
> > +			stmmac_set_tx_dma_last_segment(tx_q, entry);
> > +			break;
> > +		}
> >  
> > -	stmmac_prepare_tx_desc(priv, tx_desc, 1, xdpf->len,
> > -			       csum, priv->descriptor_mode, true, true,
> > -			       xdpf->len);
> 
> [Severity: Medium]
> Does this drop the dma_wmb() that used to come before the OWN bit on the
> normal single-buffer XDP_TX and ndo_xdp_xmit path?
> 
> The old call passed is_fs=1 and tx_own=true. That hit the barrier in
> enh_desc_prepare_tx_desc(), and the dwmac4 and dwxgmac2 versions have the
> same one:
> 
> drivers/net/ethernet/stmicro/stmmac/enh_desc.c:enh_desc_prepare_tx_desc() {
>     ...
> 	if (is_fs && tx_own)
> 		/* When the own bit, for the first frame, has to be set, all
> 		 * descriptors for the same frame has to be set before, to
> 		 * avoid race condition.
> 		 */
> 		dma_wmb();
> 
> 	p->des0 = cpu_to_le32(tdes0);
> }
> 
> Now the first descriptor is prepared with is_fs=1 and tx_own=0, and the
> frag descriptors with is_fs=0. Neither combination reaches the barrier.
> OWN on first_entry is set later by stmmac_set_tx_owner(). Every
> implementation of that is a plain read-modify-write with no barrier:
> 
> static void enh_desc_set_tx_owner(struct dma_desc *p)
> {
> 	p->des0 |= cpu_to_le32(ETDES0_OWN);
> }
> 
> dwmac100 and dwmac1000 cores have no tail pointer.
> dwmac_enable_dma_transmission() only writes DMA_CHAN_XMT_POLL_DEMAND, and a
> DMA that is already running keeps fetching owned descriptors. On a weakly
> ordered CPU, can the DMA see OWN=1 before the new des1/des2 buffer length
> and address? It could then send a stale address left over from the slot's
> earlier use.
> 
> stmmac_xmit() has the same pattern, but that is not new. Would adding a
> dma_wmb() just before stmmac_set_tx_owner() on first_entry restore the old
> ordering?
> 
> [ ... ]
> 
> > +	/* Set the OWN bit on the first descriptor now that all descriptors
> > +	 * for this skb are populated.
> > +	 */
> 
> [Severity: Low]
> This isn't a bug, but this comment appears to come from stmmac_xmit(). This
> function transmits an xdp_frame and does not use an skb.
> 
> > +	desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
> > +	stmmac_set_tx_owner(priv, desc);
> > +	tx_q->cur_tx = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
> >  	stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-b4-stmmac-xmit-mb-v2-1-003347b7bc25%40oss.qualcomm.com

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