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

[Nicolai Buchwitz]

On 5.10.2026 17:04, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (1) · Medium: 2 · Low: 0
>
> 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).
>
> - [High] bcmgenet: skb_trim() panics on non-linear jumbo frame SKBs
> - [Medium] This patch adds a TX checksum fallback in bcmgenet_xmit().
> - [Medium] The patch raises dev->max_mtu from the old ENET_MAX_MTU 
> (3564
>   on 4K pages, 3820 otherwise) to ENET_MAX_JUMBO_MTU = GENMASK(13,0) -…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   bcmgenet_timeout() updates ring->stats64 without holding ring->lock.
>
> --
>
> net: bcmgenet: reassemble jumbo frames from status block fragments
>
> This commit strips the per-descriptor status blocks from RX frames 
> longer
> than the packet ready threshold and reassembles them into one skb. It 
> also
> adds a software checksum fallback in bcmgenet_xmit() for 
> CHECKSUM_PARTIAL
> frames longer than tx_thld_len, and raises max_mtu to 
> ENET_MAX_JUMBO_MTU
> (16347).
>
> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> > b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > index 0cd56ec1c5ccf..62edbe51fe076 100644
> > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>
> [ ... ]
>
> > @@ -2176,6 +2173,19 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
> > *skb, struct net_device *dev)
> >  		goto out;
> >  	}
> >
> > +	/* The MAC holds a frame to insert its checksum, but only as much as
> > +	 * its FIFO takes. Longer frames are dropped silently.
> > +	 */
> > +	if (unlikely(skb->len > priv->tx_thld_len) &&
> > +	    skb->ip_summed == CHECKSUM_PARTIAL) {
> > +		if (skb_checksum_help(skb)) {
> > +			BCMGENET_STATS64_INC((&ring->stats64), dropped);
> > +			dev_kfree_skb_any(skb);
> > +			ret = NETDEV_TX_OK;
> > +			goto out;
> > +		}
> > +	}
> > +
>
> [Severity: Medium]
> The new comment says the MAC silently drops CHECKSUM_PARTIAL frames 
> longer
> than tx_thld_len. Can that already happen before this patch?
>
> An earlier patch in this series, "net: bcmgenet: pad transmit frames 
> out
> of the packet ready window", added bcmgenet_tx_pkt_rdy_thld():
>
> 	while (thld > ENET_THLD_DEFAULT &&
> 	       ENET_MAX_FRAME_LEN(mtu) - ETH_FCS_LEN > thld * ENET_THLD_UNIT 
> &&
> 	       thld * ENET_THLD_UNIT + ENET_TX_SAFE_MARGIN > mtu + ETH_HLEN)
> 		thld -= ENET_THLD_BURST / ENET_THLD_UNIT;
>
> On 16K and 64K page kernels ENET_THLD_MAX_LEN is 3840, which makes the 
> old
> ENET_MAX_MTU 3820. For MTUs from 3809 to 3820:
>
>     mtu + 32 > 3840
>     3840 + 64 > mtu + 14
>
> So the loop lowers the threshold to 0xe0 and tx_thld_len becomes 3584.
> Untagged frames can still be up to mtu + 14 bytes long.
>
> In that earlier commit, CHECKSUM_PARTIAL frames longer than 3584 bytes 
> go
> out with DMA_TX_DO_CSUM and have no software fallback. Going by the 
> comment
> above, the MAC loses them.
>
> Could this fallback move into, or ahead of, the patch that first lets
> tx_thld_len drop below the largest frame? Bisects and backports that 
> stop
> at that patch would then not see silent TX loss on large-page kernels.
>
> The commit message only mentions the fallback in passing:
>
>     The MAC only checksums a frame it holds in full, so anything longer 
> than
>     the threshold falls back to software.
>
> Could it also say that this fixes TX for the earlier patch?
>
> >  	/* Keep the frame out of the window just past the threshold */
> >  	if (unlikely(skb->len > priv->tx_thld_len &&
> >  		     skb->len < priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
>
> [ ... ]
>
> > @@ -4272,7 +4358,7 @@ static int bcmgenet_probe(struct platform_device 
> > *pdev)
> >  	/* v1 cannot program the thresholds, so it stays at the default MTU 
> > */
> >  	priv->rx_buf_len = bcmgenet_rx_buf_len(dev->mtu);
> >  	if (!GENET_IS_V1(priv))
> > -		dev->max_mtu = ENET_MAX_MTU;
> > +		dev->max_mtu = ENET_MAX_JUMBO_MTU;
>
> [Severity: Medium]
> Now that max_mtu is 16347, can the TX side describe frames this large?
> This patch leaves bcmgenet_xmit() unchanged. It still puts each skb 
> head or
> frag in one descriptor and shifts the size in without a mask:
>
> 	len_stat = (size << DMA_BUFLENGTH_SHIFT) |
> 		   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
>
> The header defines that field as 12 bits:
>
> /* Tx/Rx Dma Descriptor common bits*/
> #define DMA_BUFLENGTH_MASK		0x0fff
> #define DMA_BUFLENGTH_SHIFT		16
>
> Buffers over 4095 bytes are common at jumbo MTUs. Near MTU 9000, a UDP 
> or
> ICMP datagram gets a single linear head from __ip_append_data(). A frag
> from an order-3 page_frag page in TCP or soft-GSO can be almost as 
> large as
> the MSS.
>
> For a 9014 byte head the field would hold 822, and the upper bits would
> spill above the field.
>
> If the hardware field really is 12 bits, would these frames go out
> truncated or corrupt while BQL and bytes_sent count the full length?

It is not 12 bits. On a CM4 at MTU 9000 I sent raw single descriptor 
frames
of 1498 to 8998 bytes (20 each). All frames arrived at full length with 
every
payload byte intact. With a 12 bit field 4098 would have gone out as 2 
bytes.

DMA_BUFLENGTH_MASK masks the length out of the RX status word, it is not 
the
hardware field width, so max_mtu needs no cap.

> [...]