Re: [PATCH net-next v4 3/8] 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] Commit message accuracy / dead code at this point in the series
  (baseline verdict: discard, resolved later in the series).

--

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

This changes the RX length check in bcmgenet_desc_rx() so that only a
descriptor with DMA_SOP has to cover the 2 byte alignment pad. A
continuation descriptor now only has to cover the 64 byte status block.
The commit message says this prepares for a larger MTU.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 86c8f8f8fe151..9f9c3fde87250 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2372,8 +2374,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);
> +
>  		/* 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)) {
>  			netif_err(priv, rx_status, dev,
>  				  "invalid packet length %d\n", len);
>  			BCMGENET_STATS64_INC(stats, length_errors);

[Severity: Low]
At this commit, does the relaxed bound change which frames are accepted?
Right after the length check, bcmgenet_desc_rx() still has this
unchanged check:

    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;
    }

At this point in the series the driver has no frag_head or
bcmgenet_add_frag() reassembly. So every descriptor without DMA_SOP is
still dropped, whatever the MTU.

The only visible change is in the accounting. A non-SOP descriptor with
len 64 or 65 now increments fragmented_errors and logs "dropping
fragmented packet!". Before this patch it incremented length_errors and
logged "invalid packet length".

The commit message says:

  "The MTU cannot produce a frame past the threshold yet, so nothing
  hits this today. It is preparation for the larger MTU."

Would it be more accurate to say this depends on the reassembly patch
rather than on the MTU change? A larger MTU alone would still hit the
fragmented check above.

The new bound also accepts len == 64 for a continuation, which is a
descriptor with no payload. The commit message only argues for the
single byte (len == 65) case.

Both points are handled later in the series. "net: bcmgenet: reassemble
jumbo frames from status block fragments" adds the reassembly path and
rewrites this logic. The non-SOP minimum becomes
sizeof(struct status_64) + 1, and a SOP descriptor without EOP gets
min_len += ETH_HLEN.

Would it make sense to fold this change into that patch, or to reword
the commit message so the dependency is clear?

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