Re: [PATCH net-next v2 6/7] net: bcmgenet: pad transmit frames out of the packet ready window

[Nicolai Buchwitz <nb@tipi-net.de>]

On 7.10.2026 00:35, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 2
> 
> 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] skb_put_padto() in bcmgenet_xmit() appends up to 63 zero 
> bytes
>   after the existing end of the frame.
> - [Medium] The two new error exits in bcmgenet_xmit()
>   (skb_checksum_help() failure and skb_put_padto() failure) jump…
> - [Low] bcmgenet_xmit() gains a software checksum fallback that this
>   commit's message never mentions.
> - [Low] Series ordering leaves one intermediate commit broken.
> 
> --
> 
> net: bcmgenet: pad transmit frames out of the packet ready window
> 
> This commit pads transmit frames that end just past the TBUF packet 
> ready
> threshold out to 64 bytes past it, to avoid a Tx DMA stall seen on CM4. 
> It
> also lowers the transmit threshold for the MTUs where the longest frame 
> the
> MAC accepts would land in that window. It adds a software checksum 
> fallback
> for CHECKSUM_PARTIAL frames longer than the threshold.
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index 3e2ebd9a2cc50..e8f86374c7cd8 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> 
> [ ... ]
> 
>> @@ -2173,6 +2178,31 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
>> *skb, struct net_device *dev)
>>  		goto out;
>>  	}
>> 
>> +	/* The MAC only inserts a checksum into a frame it holds in full, 
>> and
>> +	 * silently drops a longer one, so fall back to software.
>> +	 */
>> +	if (unlikely(skb->len > priv->tx_thld_len) &&
>> +	    skb->ip_summed == CHECKSUM_PARTIAL) {
>> +		if (skb_checksum_help(skb)) {
> 
> [Severity: Low]
> The commit message covers the padding and the threshold change. Should 
> it
> also mention this software checksum fallback?
> 
> In bcmgenet_xmit(), any CHECKSUM_PARTIAL skb longer than 
> priv->tx_thld_len
> now goes through skb_checksum_help() instead of hardware offload. If 
> that
> call fails, the skb is dropped and counted in ring->stats64.dropped.
> 
> bcmgenet_tx_pkt_rdy_thld() also lowers the threshold from 0xf0 (3840 
> bytes)
> to 0xe0 (3584 bytes) for MTU 3809 and up. So at those MTUs the CPU now
> checksums every full-size frame, which the hardware handled before. In 
> this
> tree that covers MTU 3809..3820, and only on kernels with pages larger 
> than
> 4K.
> 
> The changelog doesn't explain the hardware behaviour claimed in the 
> comment
> ("silently drops a longer one") or the new drop path either.
> 
> The last patch in the series, "net: bcmgenet: reassemble jumbo frames 
> from
> status block fragments", does say "anything longer than the threshold 
> falls
> back to software", so the series does disclose it. Could this commit
> describe it too, since this is where the code is added?

Yes

> 
>> +			BCMGENET_STATS64_INC((&ring->stats64), dropped);
>> +			dev_kfree_skb_any(skb);
>> +			ret = NETDEV_TX_OK;
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	/* 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)) {
>> +		if (skb_put_padto(skb, priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
> 
> [Severity: Medium]
> skb_put_padto() appends up to 63 zero bytes after the current end of 
> the
> frame. Can that break protocols that keep their metadata at the tail?
> 
> DSA tail taggers write the egress tag as the last bytes of the frame 
> with
> skb_put(). Examples are ksz9477_xmit(), ksz_common_xmit() and 
> lan937x_xmit()
> in net/dsa/tag_ksz.c, and trailer_xmit() in net/dsa/tag_trailer.c:
> 
> 	trailer = skb_put(skb, 4);
> 	trailer[0] = 0x80;
> 	trailer[1] = dsa_xmit_port_mask(skb, dev);
> 
> The DSA core pads short frames itself before tagging, in 
> dsa_user_xmit(),
> so that conduit padding never ends up after a tail tag:
> 
> 	if (dev->needed_tailroom)
> 		eth_skb_pad(skb);
> 
> With GENET as the conduit, the switch would read zeros where it expects 
> the
> port mask/override or the 0x80 marker. The real tag bytes would then go 
> out
> on the wire.
> 
> PRP has the same problem. prp_fill_rct() puts the RCT last, and
> skb_get_PRP_rct() looks for it at skb_tail_pointer(skb) - HSR_HLEN. 
> With
> zeros appended, the receiver treats the frame as a standard frame and 
> loses
> duplicate discard.
> 
> bcmgenet_tx_pkt_rdy_thld() lowers the threshold to 3584 for MTU 3809 
> and up,
> so 3585..3647 byte frames get padded as well. At the end of the series 
> (max
> MTU 16347), a conduit with a jumbo MTU hits this for any 3841..3903 
> byte
> frame. A remote peer can steer frame sizes into that range, for example
> with ICMP echo sizes.
> 
> The commit message accepts this and says the alternatives are dropping 
> the
> frame or leaving the transmitter stalled. Where the tail is known to 
> carry
> data, for example netdev_uses_dsa() with a tail tagger, would dropping 
> the
> frame be safer than sending it with a corrupted tag?

Dropping loses the frame too. Keeping the padding and will add a note.

> 
>> +			BCMGENET_STATS64_INC((&ring->stats64), dropped);
>> +			ret = NETDEV_TX_OK;
>> +			goto out;
> 
> [Severity: Medium]
> Both new error exits, after skb_checksum_help() and after 
> skb_put_padto(),
> jump to out. That label comes after the doorbell write at the end of
> bcmgenet_xmit():
> 
> 	if (!netdev_xmit_more() || netif_xmit_stopped(txq))
> 		/* Packets are ready, update producer index */
> 		bcmgenet_tdma_ring_writel(priv, ring->index,
> 					  ring->prod_index, TDMA_PROD_INDEX);
> out:
> 	spin_unlock(&ring->lock);
> 
> Suppose an earlier skb in the same batch was queued with 
> netdev_xmit_more()
> true. ring->prod_index was advanced and netdev_tx_sent_queue() ran, but
> TDMA_PROD_INDEX was not written.
> 
> The last skb of the batch might then fail here, for example on a 
> GFP_ATOMIC
> failure in __skb_pad() or pskb_expand_head(). In that case NETDEV_TX_OK 
> is
> returned without prod_index being published, and dev_hard_start_xmit()
> treats that as the end of the batch.
> 
> Would the descriptors already queued then stay unsent until a later 
> xmit
> rings the doorbell? The queue isn't stopped, so the TX watchdog 
> wouldn't
> step in.

Yes, same as the existing exits. Will fix them...

> 
> The existing exits for bcmgenet_add_tsb() failure and DMA mapping 
> failure
> behave the same way. This patch adds two more paths that do it.
> 
>> +		}
>> +	}
>> +
>> +	nr_frags = skb_shinfo(skb)->nr_frags;
>> +
> 
> [ ... ]
> 
>> @@ -2680,8 +2729,7 @@ 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(ENET_THLD_MAX,
>> -			priv->base + priv->hw_params->tbuf_offset +
>> +	bcmgenet_writel(tx_thld, priv->base + priv->hw_params->tbuf_offset +
>>  			TBUF_PKT_RDY_THLD);
>>  }
> 
> [Severity: Low]
> This one is about the order of the series. Does it leave an 
> intermediate
> commit broken?
> 
> "net: bcmgenet: derive the receive buffer length from the MTU" programs
> TBUF_PKT_RDY_THLD to ENET_THLD_MAX and defers the fix ("A later patch 
> lowers
> it for the few MTUs that need the room").
> 
> "net: bcmgenet: allow the MTU to be changed" then raises dev->max_mtu 
> to
> ENET_MAX_MTU, which is 3820 on kernels with pages larger than 4K.
> 
> vlan_dev_change_mtu() lets an 802.1Q device use the full parent MTU. So 
> at
> that commit, a stacked VLAN at MTU 3820 produces 3820 + 14 + 8 = 3842 
> byte
> frames.
> 
> Frames of that size fall in the 3841..3886 window this patch describes 
> for a
> 3840 threshold. And according to the new comment in bcmgenet_xmit(), 
> the MAC
> silently drops a CHECKSUM_PARTIAL frame longer than 3840 bytes.
> 
> Could this patch, and the threshold logic, be ordered before the 
> max_mtu
> increase? That way a bisect landing on "allow the MTU to be changed" 
> would
> not hit the stalled transmitter.

Yes