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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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 fixes a divide-by-zero crash in the XDP TX path
  without saying so and without a Fixes: tag.
- [Low] In stmmac_xdp_xmit_xdpf(), the new coalescing check reads
  priv->tx_coal_frames[queue] twice with plain loads: once for the zero…

--

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

This commit extends stmmac_xdp_xmit_xdpf() so it can transmit XDP frames
that carry fragments. The frame head and each frag get their own TX
descriptor. The driver now also advertises NDO_XMIT_SG for the redirect
target.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3ad9252bf6aea..f1e9ac2f98598 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5241,77 +5241,118 @@ 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)
>  {

[ ... ]

> +	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 (priv->tx_coal_frames[queue] &&
> +	    (tx_q->tx_count_frames % priv->tx_coal_frames[queue]) < num_frames) {

[Severity: Medium]
The old code here divided by priv->tx_coal_frames[queue] without checking
it for zero. That code came in with be8b38a722e6 ("net: stmmac: Add support
for XDP_TX action").

Can tx_coal_frames[queue] be 0 here? __stmmac_set_coalesce() only rejects
the request when both values are zero:

	if ((ec->tx_coalesce_usecs == 0) &&
	    (ec->tx_max_coalesced_frames == 0))
		return -EINVAL;

So a command like "ethtool -C ethX tx-usecs 100 tx-frames 0" is accepted
and stores 0 in priv->tx_coal_frames[]. Before this patch, the next XDP_TX
or ndo_xdp_xmit frame would then do a modulo by zero in softirq context.

The new priv->tx_coal_frames[queue] guard fixes that, but the commit
message only describes multi-buff TX support. stmmac_xmit(),
stmmac_tso_xmit() and stmmac_xdp_xmit_zc() already have this check.

Could the fix be split into its own patch with a
"Fixes: be8b38a722e6" tag, so it can reach stable?

[Severity: Low]
This check does two plain loads of priv->tx_coal_frames[queue]: one for the
zero test and one for the divisor.

__stmmac_set_coalesce() writes this field under RTNL. It does not hold the
TX queue lock that stmmac_xdp_xmit() and stmmac_xdp_xmit_back() take, and
it can store 0.

If the compiler reloads the field between the two reads and a 0 is stored
in that window, can the modulo still divide by zero? Would taking a single
snapshot help, for example:

	u32 coal = READ_ONCE(priv->tx_coal_frames[queue]);

and then using coal for both the test and the division?

stmmac_xmit(), stmmac_tso_xmit() and stmmac_xdp_xmit_zc() read the field
twice in the same way.

> +		struct stmmac_txq_stats *txq_stats;
>  
> -	if (set_ic) {
> +		desc = stmmac_get_tx_desc(priv, tx_q, entry);
> +		stmmac_set_tx_ic(priv, desc);
>  		tx_q->tx_count_frames = 0;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-b4-stmmac-xmit-mb-v3-1-ca08f029e81c%40oss.qualcomm.com