Re: [PATCH net-next v7 9/9] net: bcmgenet: reassemble jumbo frames from status block fragments

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 3

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] This patch raises dev->max_mtu to ENET_MAX_MTU = GENMASK(13,0)
  - ENET_FRAME_OVERHEAD = 16347 on every GENET revision except v1…
- [Low] The comment this patch rewrote above bcmgenet_pkt_rdy_thld() says
  the receive threshold "covers the alignment bytes and the frame up to…
- [Low] In bcmgenet_desc_rx() the length check (`if (unlikely(len >
  priv->rx_buf_len || len < min_len))`) runs before the new…
- [Low] net: bcmgenet: frag_drop not cleared on EOP in bcmgenet_desc_rx()

--

net: bcmgenet: reassemble jumbo frames from status block fragments

When a frame is longer than the packet ready threshold, the hardware
splits it across several RX descriptors. This patch reassembles those
frames and strips the per-descriptor status blocks. It also raises
ENET_MAX_MTU to the 14 bit UMAC_MAX_FRAME_LEN limit, which allows an MTU
of up to 16347.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 30ba77931c4f0..eb6975459917b 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -84,11 +84,8 @@
>  				      ENET_THLD_MAX * ENET_THLD_UNIT, \
>  				      ENET_THLD_PAGE_LEN)
>  
> -/* Largest MTU that fits one descriptor, with room for a VLAN tag so a VLAN
> - * interface can use the parent MTU.
> - */
> -#define ENET_MAX_MTU		(ENET_THLD_MAX_LEN - GENET_RBUF_ALIGN - \
> -				 ETH_HLEN - VLAN_HLEN)
> +/* UMAC_MAX_FRAME_LEN is 14 bits wide and counts the FCS */
> +#define ENET_MAX_MTU		(GENMASK(13, 0) - ENET_FRAME_OVERHEAD)

[Severity: Medium]
Has the wider TX descriptor length field been confirmed on GENET v2, v3
and v4, or only on v5?

bcmgenet_probe() applies the new maximum to every revision except v1:

	if (!GENET_IS_V1(priv))
		dev->max_mtu = ENET_MAX_MTU;

Before this change, max_mtu was tied to the single-descriptor RX
threshold (about 3564 with 4K pages). So no TX buffer passed to
bcmgenet_xmit() could reach 4096 bytes, even with the 64 byte TSB added.

With an MTU near 16K, a linear head can now be 4096 bytes or more, for
example with SG off, from a packet socket, or after skb_linearize(). A
single page frag can be that large too. bcmgenet_xmit() writes the
length without clamping or splitting it:

		len_stat = (size << DMA_BUFLENGTH_SHIFT) |
			   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);

bcmgenet.h still documents the field as 12 bits for both directions:

/* Tx/Rx Dma Descriptor common bits*/
#define DMA_BUFLENGTH_MASK		0x0fff

The commit message says the TX field is wider, but the only evidence
given is a UMAC loopback test on v5. The RTL confirmation from Broadcom
that it cites covers how RX splits long frames. It doesn't cover the
width of the TX length field.

If the TDMA on v2 to v4 decodes only 12 length bits, would TX buffers of
4096 bytes or more be sent truncated modulo 4096?

[ ... ]

> @@ -2518,24 +2563,42 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,

[ ... ]

>  		/* Reject lengths that would underflow the SKB build path. */
>  		if (unlikely(len > priv->rx_buf_len || len < min_len)) {
>  			netif_err(priv, rx_status, dev,
>  				  "invalid packet length %d\n", len);
>  			BCMGENET_STATS64_INC(stats, length_errors);
> +			bcmgenet_discard_frags(ring, dma_flag & DMA_EOP);
>  			page_pool_put_full_page(ring->page_pool, rx_page,
>  						true);
>  			goto next;
>  		}

[Severity: Low]
Should this branch clear ring->frag_drop when the descriptor carries
DMA_EOP?

bcmgenet_discard_frags() sets frag_drop to !eop. The rest of an
aborted frame is then dropped without being counted again, and
counting resumes once that frame has ended. This branch can only set
frag_drop to true. So when the last continuation of a dropped frame
lands here with DMA_EOP set, frag_drop stays true even though the
frame is over.

The same thing happens in two other cases. A lone orphan descriptor
with DMA_EOP and no SOP sets frag_drop to true here. A failure in
bcmgenet_rx_refill() calls bcmgenet_discard_frags(ring, false) before
the status word has been read, so it cannot know whether that
descriptor ended the frame.

Suppose the next frame on the ring has also lost its SOP descriptor.
Its continuations reach this branch with frag_drop already set, so
they are dropped without incrementing fragmented_errors. That lost
frame then never shows up in the error counters.

This is only a counter inaccuracy on already abnormal paths. Would
setting ring->frag_drop to !(dma_flag & DMA_EOP) here, after the
count, keep this branch consistent with bcmgenet_discard_frags()?

[Severity: Low]
Should this length check in bcmgenet_desc_rx() come after the new SOP and
frag_drop handling? Because it runs first, two cases skip the reassembly
accounting.

The first case is a frame that has already been dropped (frag_drop set,
frag_head NULL). Each later non-SOP descriptor with an out of range
length still logs "invalid packet length" and increments length_errors.

The comment on bcmgenet_discard_frags() says "do not count it again until
the next SOP". Continuations that carry DMA error flags, on the other
hand, are absorbed silently by the frag_drop branch.

The second case is a SOP descriptor with an invalid length that arrives
while ring->frag_head holds an incomplete frame.
bcmgenet_discard_frags() frees the pending frame, but fragmented_errors
is not incremented. A SOP with a valid length in the same situation does
increment it:

		if (dma_flag & DMA_SOP) {
			if (ring->frag_head) {
				BCMGENET_STATS64_INC(stats, fragmented_errors);
				bcmgenet_discard_frags(ring, false);
			}

Is the lost incomplete frame meant to go uncounted when the next SOP has
a bad length?

[ ... ]

> @@ -2768,7 +2854,8 @@ static void bcmgenet_link_intr_enable(struct bcmgenet_priv *priv)
>  }
>  
>  /* Receive threshold in register units. Covers the alignment bytes and the
> - * frame, but not the status block, which the hardware adds on top.
> + * frame up to the register maximum, but not the status block, which the
> + * hardware adds on top. A longer frame arrives in several descriptors.
>   */
>  static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
>  {

[Severity: Low]
This isn't a bug, but is "up to the register maximum" accurate? The clamp
in bcmgenet_pkt_rdy_thld() uses ENET_THLD_MAX_LEN:

#define ENET_THLD_MAX_LEN	min_t(unsigned int, \
				      ENET_THLD_MAX * ENET_THLD_UNIT, \
				      ENET_THLD_PAGE_LEN)

With 4K pages, ENET_THLD_PAGE_LEN (3584) is below the register maximum
(0xf0 * 16 = 3840), so the page sets the limit.

The comment above ENET_THLD_PAGE_LEN says the same: "on 4K pages the page
bounds the threshold before the register does". The two comments now
contradict each other.

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