Re: [PATCH net-next 2/7] net: bcmgenet: allow a continuation descriptor without the alignment pad

[netdev-bot+sashiko@kernel.org]

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] Possible commit message mismatch in "net: bcmgenet: allow a
  continuation descriptor without the alignment pad".
- [Low] The RX buffer layout comment above GENET_RSB_PAD
  (bcmgenet.c:54-58 at this commit, 67-72 at the end of the series) is…

--

net: bcmgenet: allow a continuation descriptor without the alignment pad

This patch changes the RX length check in bcmgenet_desc_rx(). Only the
first (DMA_SOP) descriptor of a frame now has to carry the 2-byte
alignment pad after the status block. Continuation descriptors only need
room for the 64-byte struct status_64. The patch prepares for a larger
MTU.

> A frame longer than the packet ready threshold arrives in several
> descriptors, each with its own status block. Only the first one also
> carries the two alignment bytes. The length check assumes the pad is always
> there, so a continuation holding a single byte looks two bytes too short
> and the whole frame is dropped.

[Severity: Low]
Is this description accurate for the tree at this commit?

A continuation holding a single byte has len = 64 + 1 = 65. That is one
byte below the old GENET_RSB_PAD minimum of 66, not two.

Also, a continuation descriptor that passes the new min_len check still
reaches the unchanged check just below it in bcmgenet_desc_rx():

	if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
		netif_err(priv, rx_status, dev,
			  "dropping fragmented packet!\n");
		BCMGENET_STATS64_INC(stats, fragmented_errors);
		...
		goto next;
	}

So at this commit, the only change for such a descriptor is the counter
and log message. It goes to fragmented_errors instead of length_errors.

The behaviour the message describes, where a length error drops the whole
frame through bcmgenet_discard_frags(), only exists once the later patch
in the series is applied: "net: bcmgenet: reassemble jumbo frames from
status block fragments".

Could the message say that this bound is meant for the reassembly path
added later in the series? Could it also correct the one versus two byte
arithmetic?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index a82579879f4b5..d89ae65997604 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2365,8 +2366,12 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  			  __func__, p_index, ring->c_index,
>  			  ring->read_ptr, dma_length_status);
>  
> +		/* Only the first descriptor carries the alignment pad */
> +		min_len = dma_flag & DMA_SOP ? GENET_RSB_PAD
> +					     : sizeof(struct status_64);

[Severity: Low]
This isn't a bug, but the RX buffer layout comment above GENET_RSB_PAD
still says every buffer has the pad:

/* Page pool RX buffer layout:
 * RSB(64) + pad(2) | frame data | skb_shared_info
 * The HW writes the 64B RSB + 2B alignment padding before the frame.
 */
#define GENET_RSB_PAD		(sizeof(struct status_64) + 2)

That contradicts the rule added here. The later reassembly patch in the
series depends on this rule: bcmgenet_add_frag() reads continuation data
from offset + sizeof(struct status_64), not offset + GENET_RSB_PAD.
However, no patch in the series updates the layout comment.

Should the layout comment say that only the SOP buffer carries the 2-byte
pad?

> +
>  		/* Reject lengths that would underflow the SKB build path. */
> -		if (unlikely(len > RX_BUF_LENGTH || len < GENET_RSB_PAD)) {
> +		if (unlikely(len > RX_BUF_LENGTH || len < min_len)) {

[ ... ]

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