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

[netdev-bot+sashiko]

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] bcmgenet_xmit() pads every frame whose length falls in
  (tx_thld_len, tx_thld_len + 64) with up to 63 zero bytes via…
- [Medium] (Baseline: resolved later in the series.) At this commit,
  bcmgenet_tx_pkt_rdy_thld() lowers TBUF_PKT_RDY_THLD from 3840 to 3584…
- [Low] When skb_put_padto() fails, the new error path in bcmgenet_xmit()
  jumps to 'out' and skips the TDMA_PROD_INDEX doorbell write.

--

net: bcmgenet: pad transmit frames out of the packet ready window

This patch pads transmit frames that end just past the TX packet ready
threshold, so they land 64 bytes past it. The window causes the
transmitter to stop and Tx DMA to refuse to halt. For MTUs where a
maximum-length frame would fall inside that window, the patch lowers
the TX threshold instead.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 4634b6521ab8e..0cd56ec1c5ccf 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2171,6 +2176,18 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>  		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]
Can this padding corrupt frames that end in a trailer?

skb_put_padto() appends up to 63 zero bytes after whatever is already at
the end of the frame. It does not check what the frame carries. Some
encapsulations put data at the very end of the frame:

DSA tail taggers (tag_ksz, tag_trailer, tag_hellcreek, tag_xrs700x,
rtl8_4t): the switch would read the zeros as the tag. That is why
dsa_user_xmit() pads before the tail tagger runs:

net/dsa/user.c:dsa_user_xmit() {
    ...
	if (dev->needed_tailroom)
		eth_skb_pad(skb);
    ...
}

PRP: the redundancy control trailer would no longer be the last 6
bytes. The receiver would then treat the frame as non-PRP, and duplicate
discard would fail.

Software MACsec: macsec_decrypt() finds the ICV from the received
length:

	aead_request_set_ad(req, skb->len - icv_len);

so the trailing zeros would make authentication fail.

At this commit, on kernels with 16K or larger pages and an MTU of
3809..3820, the threshold drops to 3584. Frames of 3585..3647 bytes then
get padded.

Later in the series, "net: bcmgenet: reassemble jumbo frames from status
block fragments" raises max_mtu to ENET_MAX_JUMBO_MTU on all page sizes.
After that, any MTU of 3890 or more pads every frame of 3841..3903
bytes. MTUs of 3809..3889 pad frames of 3585..3647 bytes. Nothing later
in the series appears to fix this.

Should frames that carry a trailer be kept out of this window some other
way, rather than by appending bytes?

> +			BCMGENET_STATS64_INC((&ring->stats64), dropped);
> +			ret = NETDEV_TX_OK;
> +			goto out;

[Severity: Low]
If skb_put_padto() fails, this jumps to out. That skips the
TDMA_PROD_INDEX 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);

Suppose earlier skbs in the same xmit_more batch already advanced
ring->prod_index, and the dropped skb was the last one in the batch.
Could those descriptors sit unsubmitted until a later transmit
succeeds? The queue is not stopped, so the TX watchdog would not
notice.

skb_put_padto() can fail when the skb is short on tailroom or is
cloned, and the GFP_ATOMIC expansion fails. The existing
bcmgenet_add_tsb() and DMA mapping failure paths already behave this
way. This patch adds another way to reach that state.

> +		}
> +	}
> +
> +	nr_frags = skb_shinfo(skb)->nr_frags;
> +

[ ... ]

> @@ -2659,6 +2676,23 @@ static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
>  		       ENET_THLD_MAX_LEN / ENET_THLD_UNIT);
>  }
>  
> +/* Transmit threshold in register units. Frames landing in the window just
> + * past it are padded clear of it, so pick a threshold that leaves room for
> + * that padding inside the frame the MTU allows. Size the window against the
> + * longest frame the MAC has to accept, since the tag count is not bounded.
> + */
> +static unsigned int bcmgenet_tx_pkt_rdy_thld(unsigned int mtu)
> +{
> +	unsigned int thld = ENET_THLD_MAX;
> +
> +	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;

[Severity: Medium]
For an MTU of 3809..3820, this loop lowers TBUF_PKT_RDY_THLD from 3840
to 3584 bytes. At this commit that range is only reachable on kernels
with 16K or larger pages, where ENET_MAX_MTU is 3820.

bcmgenet_xmit() still sets DMA_TX_DO_CSUM for every CHECKSUM_PARTIAL
frame:

		if (!i) {
			len_stat |= DMA_SOP;
			if (skb->ip_summed == CHECKSUM_PARTIAL)
				len_stat |= DMA_TX_DO_CSUM;
		}

Full-size TCP/UDP frames of 3585..3834 bytes now exceed the transmit
threshold. Before this commit, no frame could.

The last patch in the series, "net: bcmgenet: reassemble jumbo frames
from status block fragments", says the MAC only holds threshold bytes
for checksum insertion and silently drops longer frames. It adds this
fallback:

	if (unlikely(skb->len > priv->tx_thld_len) &&
	    skb->ip_summed == CHECKSUM_PARTIAL) {
		if (skb_checksum_help(skb)) {

Between this patch and that one, are CHECKSUM_PARTIAL frames longer
than 3584 bytes dropped when NETIF_F_HW_CSUM is enabled, which is the
default?

The end of the series fixes this, but bisection can still land in
between. Could the skb_checksum_help() fallback move into this patch?

> +
> +	return thld;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee%40tipi-net.de