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

[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: 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?

> +			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?

> +			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.

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.

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