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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch rewrote the comment above the TX checksum fallback in
  bcmgenet_xmit() to say: "The MAC holds a frame to insert its checksum,…
- [Low] This patch updated the comment on bcmgenet_pkt_rdy_thld() to say
  the threshold covers the frame "up to the register maximum".

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but adaptive RX
  coalescing gets per-poll counts where it expects cumulative counters.

--

net: bcmgenet: reassemble jumbo frames from status block fragments

The hardware can split a frame across several RX descriptors and write a
status block at the start of each one. With this commit,
bcmgenet_desc_rx() strips those status blocks and joins the pieces into
one skb. The commit also raises max_mtu to the limit of the 14-bit
UMAC_MAX_FRAME_LEN, and TX frames longer than the threshold now fall back
to a software checksum.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 9f28006957d4d..323839adebbae 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2200,8 +2197,8 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>  		}
>  	}
>  
> -	/* The MAC only inserts a checksum into a frame it holds in full, and
> -	 * silently drops a longer one, so fall back to software.
> +	/* 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) {

[Severity: Low]
This isn't a bug, but does the new comment in bcmgenet_xmit() describe the
limit the code checks?

The condition under the comment compares skb->len with
priv->tx_thld_len. That is the programmable packet ready threshold that
bcmgenet_tx_pkt_rdy_thld() picks from the MTU, somewhere between
ENET_THLD_DEFAULT and ENET_THLD_MAX units. It is not a fixed FIFO size.
The commit message also says "anything longer than the threshold falls
back to software".

The new wording also drops "so fall back to software". The comment no
longer says that these frames go through skb_checksum_help() rather than
being dropped.

Could the comment name the threshold and mention the software fallback?

[ ... ]

> @@ -2552,7 +2637,8 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  	}
>  
>  	ring->dim.bytes = bytes_processed;
> -	ring->dim.packets = rxpktprocessed;
> +	/* A long frame spans several descriptors but is one packet */
> +	ring->dim.packets = rxframes;
>  
>  	return rxpktprocessed;
>  }

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch, but this
hunk touches the DIM accounting. Should ring->dim.packets and
ring->dim.bytes be cumulative counters?

Each poll overwrites them with that poll's counts. bcmgenet_rx_poll() then
passes them to DIM as a sample:

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_rx_poll() {
    ...
	if (ring->dim.use_dim) {
		dim_update_sample(ring->dim.event_ctr, ring->dim.packets,
				  ring->dim.bytes, &dim_sample);
		net_dim(&ring->dim.dim, &dim_sample);
	}
    ...
}

dim_calc_stats() treats both values as counters that only go up and takes
a wrap-aware difference:

lib/dim/dim.c:dim_calc_stats() {
    ...
	u32 npkts = BIT_GAP(BITS_PER_TYPE(u32), end->pkt_ctr, start->pkt_ctr);
	u32 nbytes = BIT_GAP(BITS_PER_TYPE(u32), end->byte_ctr,
			     start->byte_ctr);
    ...
}

Two polls of the same size give a difference of zero, even under steady
traffic. A smaller poll after a larger one wraps to about 4 billion.

With adaptive-rx turned on through ethtool -C, would the ppms and bpms
values that net_dim_stats_compare() uses to pick coalescing profiles be
meaningless?

The baseline assigned rxpktprocessed per poll in the same way.

[ ... ]

> @@ -2696,7 +2784,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 for
bcmgenet_pkt_rdy_thld()?

The function clamps to ENET_THLD_MAX_LEN / ENET_THLD_UNIT, and
ENET_THLD_MAX_LEN takes the smaller of the register limit and the page
limit:

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

The comment above ENET_THLD_PAGE_LEN says "on 4K pages the page bounds
the threshold before the register does". On 64-bit with 4K pages,
ENET_THLD_PAGE_LEN works out to 3584, below the register limit of 3840.
That value is also where jumbo frames get split across descriptors.

Could the comment refer to the page or ENET_THLD_MAX_LEN limit rather than
the register maximum?

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