Re: [PATCH net-next v7 6/9] net: bcmgenet: split frames that end just past the transmit threshold

[netdev-bot+sashiko@kernel.org]

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] ENET_TX_THLD_WINDOW is fixed at 64 bytes for every platform.
- [Medium] bcmgenet_tx_pkt_rdy_thld() returns ENET_THLD_MAX (0xf0, 3840
  bytes) for every GENET version once the MTU exceeds ETH_DATA_LEN, and…
- [Low] The commit message describes the transmitter stall as a live bug
  that IP fragmentation hits on every datagram, and says the interface…

--

net: bcmgenet: split frames that end just past the transmit threshold

This patch splits a frame across two descriptors that share one DMA
mapping when the frame ends within ENET_TX_THLD_WINDOW bytes past the
transmit packet ready threshold. It programs TBUF_PKT_RDY_THLD from the
MTU and falls back to a software checksum for frames longer than the
threshold. It also reserves one more descriptor in ring->stop_thld.

> A frame that ends a few bytes past the transmit packet ready threshold
> stops the transmitter as soon as a shorter frame follows. Tx DMA then
> refuses to halt, so every later bcmgenet_init_dma() fails and the
> interface cannot be opened again. IP fragmentation generates that pattern
> on every datagram, full frames and a short tail.

[Severity: Low]
Can this happen at this point in the series?

genet does not set dev->max_mtu and has no ndo_change_mtu, so
ether_setup() leaves max_mtu at ETH_DATA_LEN. There is no TSO, so GSO
segments down to the MTU.

That means bcmgenet_tx_pkt_rdy_thld() always returns ENET_THLD_DEFAULT
and priv->tx_thld_len is always 2048. No transmitted frame, which is
about 1518 bytes at most, can satisfy this check in bcmgenet_xmit():

	split = unlikely(skb->len > priv->tx_thld_len &&
			 skb->len < priv->tx_thld_len + ENET_TX_THLD_WINDOW);

So the split path, the skb_checksum_help() fallback and the
ENET_THLD_MAX branch cannot run until "net: bcmgenet: allow the MTU to
be changed". Today the only runtime changes appear to be two:
TBUF_PKT_RDY_THLD gets its reset value written, and stop_thld goes from
MAX_SKB_FRAGS + 1 to MAX_SKB_FRAGS + 2.

Earlier patches in this series state this directly ("The MTU cannot
produce a frame past the threshold yet, so nothing hits this today. It
is preparation for the larger MTU").

Could this message say the same, so that readers and stable backporters
don't take it for a fix for a hang that can happen today?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 1bb5fed4a8b4d..d70079832dd24 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -59,6 +59,11 @@
>  #define ENET_THLD_DEFAULT	0x80
>  #define ENET_THLD_MAX		0xf0
>  
> +/* A frame ending this far past the transmit threshold stops the transmitter
> + * once a shorter frame follows. Widest window measured is 46 bytes.
> + */
> +#define ENET_TX_THLD_WINDOW	64

[Severity: Medium]
The commit message says the window "is one DMA transfer wide". The TX
DMA burst size, though, is set per platform in bcmgenet_dma_init():

	bcmgenet_tdma_writel(priv, priv->dma_max_burst_length,
			     DMA_SCB_BURST_SIZE);

bcm2711_plat_data uses .dma_max_burst_length = 0x08. The v2, v3, v4, v5
and bcm7712 platform data use DMA_MAX_BURST_LENGTH (0x10).

Suppose the window grows with the programmed burst. On the 0x10 burst
parts, a frame could end 64 bytes or more past tx_thld_len and still
fall inside one transfer. Would bcmgenet_xmit() then skip the split for
it? That would leave the Tx DMA stall and the bcmgenet_init_dma()
failure in place on those cores.

The measurements in the message come from one v5. Once "net: bcmgenet:
allow the MTU to be changed" raises max_mtu on every non-v1 core, this
case becomes reachable.

Should the window be derived from dma_max_burst_length, or has 64 been
checked on a core with a 0x10 burst?

[ ... ]

> @@ -2689,6 +2761,17 @@ static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
>  		       ENET_THLD_MAX_LEN / ENET_THLD_UNIT);
>  }
>  
> +/* The transmit threshold in register units. Keep the reset value while the
> + * MTU has no use for more, the hardware never reaches it below ETH_DATA_LEN.
> + */
> +static unsigned int bcmgenet_tx_pkt_rdy_thld(unsigned int mtu)
> +{
> +	if (mtu <= ETH_DATA_LEN)
> +		return ENET_THLD_DEFAULT;
> +
> +	return ENET_THLD_MAX;
> +}
> +
>  /* A buffer has to hold everything the threshold lets the hardware deliver */
>  static unsigned int bcmgenet_rx_buf_len(unsigned int mtu)
>  {
> @@ -2699,8 +2782,10 @@ static unsigned int bcmgenet_rx_buf_len(unsigned int mtu)
>  /* Program the MTU dependent registers. Call with the MAC disabled. */
>  static void bcmgenet_set_mtu_regs(struct bcmgenet_priv *priv, unsigned int mtu)
>  {
> +	u32 tx_thld = bcmgenet_tx_pkt_rdy_thld(mtu);
>  	u32 thld = bcmgenet_pkt_rdy_thld(mtu);
>  
> +	priv->tx_thld_len = tx_thld * ENET_THLD_UNIT;
>  	bcmgenet_umac_writel(priv, ENET_MAC_FRAME_LEN(mtu), UMAC_MAX_FRAME_LEN);
>  
>  	/* GENET v1 maps other registers at these offsets */
> @@ -2708,6 +2793,8 @@ static void bcmgenet_set_mtu_regs(struct bcmgenet_priv *priv, unsigned int mtu)
>  		return;
>  
>  	bcmgenet_rbuf_writel(priv, thld, RBUF_PKT_RDY_THLD);
> +	bcmgenet_writel(tx_thld, priv->base + priv->hw_params->tbuf_offset +
> +			TBUF_PKT_RDY_THLD);
>  }

[Severity: Medium]
Does ENET_THLD_MAX (3840 bytes) fit in the TBUF of every GENET version
that gets it here? bcmgenet_tx_pkt_rdy_thld() returns the same value
for all versions. bcmgenet_set_mtu_regs() then writes it on v2 and
later, with no clamp per version.

init_umac() programs the TBUF size control only on v3 and later:

	if (!GENET_IS_V1(priv) && !GENET_IS_V2(priv))
		bcmgenet_rbuf_writel(priv, 1, RBUF_TBUF_SIZE_CTRL);

So v2 has different TBUF sizing but gets the same threshold. The
measurements in the commit message are from a v5 only.

Suppose a core's TBUF cannot hold 3840 bytes. Could a frame longer than
that TBUF then never reach the threshold and stall the transmitter?

"net: bcmgenet: allow the MTU to be changed" later sets
dev->max_mtu = ENET_MAX_MTU for every non-v1 core. That would make this
setting reachable on v2 through v4.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-nb-genet-mtu-nn-v2-v7-0-43a5d90b3256%40tipi-net.de