Re: [PATCH net-next 2/2] net: stmmac: disable TX checksum offload for jumbo frames on bugged cores

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

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

> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The commit message's premise doesn't match the tree.
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>   stmmac_xdp_xmit_xdpf() never looks at priv->plat->tx_coe when deciding…
> 
> --
> 
> net: stmmac: disable TX checksum offload for jumbo frames on bugged cores
> 
> This patch turns off TX checksum insertion in stmmac_xdp_xmit_xdpf() for
> frames longer than ETH_FRAME_LEN on bugged_jumbo cores. It is meant to
> match the stmmac_fix_features() workaround that the skb path already uses.
> 
> > The XDP TX
> > path bypasses that workaround, so once XDP is allowed at MTU >
> > ETH_DATA_LEN, jumbo frames sent via XDP_TX or redirected into the
> > device would leave the interface with a full checksum insertion enabled
> > on those cores - exactly what the workaround is meant to avoid.
> 
> [Severity: Low]
> Can this scenario happen in the current tree? XDP still can't be used with
> a jumbo MTU. stmmac_xdp_set_prog() refuses to attach a program when the MTU
> is above ETH_DATA_LEN:

This is actually a preliminary fix needed to remove the MTU limitation when the
device is running in XDP mode.

> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
>     ...
> 	if (prog && dev->mtu > ETH_DATA_LEN) {
> 		/* For now, the driver doesn't support XDP functionality with
> 		 * jumbo frames so we return error.
> 		 */
> 		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
> 		return -EOPNOTSUPP;
> 	}
>     ...
> }
> 
> stmmac_change_mtu() also refuses a jumbo MTU while XDP is enabled:
> 
> 	if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) {
> 		netdev_dbg(priv->dev, "Jumbo frames not supported for XDP\n");
> 		return -EINVAL;
> 	}
> 
> The redirect case looks limited too. Devmap checks every frame with
> xdp_ok_fwd_dev(dev, xdp_get_frame_len(xdpf)), which caps the length at
> mtu + hard_header_len + VLAN_HLEN. At MTU 1500 that is 1518 bytes.
> Redirected frames therefore reach the new ETH_FRAME_LEN check only when
> they are 1515 to 1518 bytes long, which is VLAN-tagged normal-size
> traffic, not jumbo.
> 
> The one path that seems to reach the new check today is XDP_TX after the
> program grows the frame. At MTU 1500, stmmac_rx() still sets frame_sz to
> at least PAGE_SIZE:
> 
> 	bufsz = DIV_ROUND_UP(priv->dma_conf.dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
> 
> A program can therefore use bpf_xdp_adjust_tail() to grow a frame past
> 1514 bytes and return XDP_TX. stmmac_xdp_xmit_xdpf() never checks that
> length against the MTU.
> 
> Could the commit message describe the trigger that exists today, and say
> whether the bugged_jumbo erratum covers it? If this patch depends on a
> separate change that allows XDP at a jumbo MTU, could the message say so?
> The Fixes: tag reads as if the problem can be hit now.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index ca61ab449e32b..0e29c5d04714c 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5276,6 +5276,14 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
> >  		return STMMAC_XDP_CONSUMED;
> >  	}
> >  
> > +	/* Disable TX checksum offload for jumbo frames on cores where
> > +	 * checksum insertion is broken for oversized frames, matching the
> > +	 * stmmac_fix_features() workaround used by the skb path.
> > +	 */
> > +	if (priv->plat->bugged_jumbo &&
> > +	    xdp_get_frame_len(xdpf) > ETH_FRAME_LEN)
> > +		csum = false;
> 
> [Severity: Medium]
> This is a pre-existing issue, not one introduced by this patch. The
> comment says this matches stmmac_fix_features(), but that function also
> turns off TX checksum offload when tx_coe is unset:

since this issue has not been introduced by this patch, it needs to be fixed
with a dedicated patch.

Regards,
Lorenzo

> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_fix_features() {
>     ...
> 	if (!priv->plat->tx_coe)
> 		features &= ~NETIF_F_CSUM_MASK;
>     ...
> }
> 
> In stmmac_xdp_xmit_xdpf(), csum comes only from the per-queue flag:
> 
> 	bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;
> 
> stmmac_hw_init() clears tx_coe when threshold DMA mode is forced:
> 
> 		/* TXCOE doesn't work in thresh DMA mode */
> 		if (priv->plat->force_thresh_dma_mode)
> 			priv->plat->tx_coe = false;
> 
> stmmac_dma_operation_mode() then runs TX in threshold mode
> (txmode = tc).
> 
> On platforms with snps,force_thresh_dma_mode, do XDP_TX and ndo_xdp_xmit
> frames still request full checksum insertion through
> stmmac_prepare_tx_desc() in a DMA mode where the driver treats TX COE as
> broken? Should the XDP path also check priv->plat->tx_coe here?
> 
> >  	while (true) {
> >  		skb_frag_t *frag = i ? &sinfo->frags[i - 1] : NULL;
> >  		int len = frag ? skb_frag_size(frag) : xdpf->len;
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-stmmac-xdp-tx-pre-fixes-v1-0-05fd0c90f7e1%40oss.qualcomm.com

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