Re: [PATCH net-next 3/7] net: bcmgenet: rename ENET_MAX_MTU_SIZE to ENET_MAX_FRAME_LEN

[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] In drivers/net/ethernet/broadcom/genet/bcmgenet.h, the patch adds
  the comment "Longest frame the MAC must accept for the default MTU"…

--

net: bcmgenet: rename ENET_MAX_MTU_SIZE to ENET_MAX_FRAME_LEN

This commit renames ENET_MAX_MTU_SIZE to ENET_MAX_FRAME_LEN, because the
value is a frame length that gets programmed into the hardware and not an
MTU. It also replaces the comment above the definition. The commit says
there is no functional change.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 86f2aed20dbe8..501dd12566936 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -28,12 +28,11 @@
>  /* which ring is descriptor based */
>  #define DESC_INDEX				16
>  
> -/* Body(1500) + EH_SIZE(14) + VLANTAG(4) + BRCMTAG(6) + FCS(4) = 1528.
> - * 1536 is multiple of 256 bytes
> - */
>  #define ENET_BRCM_TAG_LEN	6
>  #define ENET_PAD		8
> -#define ENET_MAX_MTU_SIZE	(ETH_DATA_LEN + ETH_HLEN + VLAN_HLEN + \
> +
> +/* Longest frame the MAC must accept for the default MTU */
> +#define ENET_MAX_FRAME_LEN	(ETH_DATA_LEN + ETH_HLEN + VLAN_HLEN + \
>  				 ENET_BRCM_TAG_LEN + ETH_FCS_LEN + ENET_PAD)

[Severity: Low]
This isn't a bug, but does the new comment match the value of the macro?

ENET_MAX_FRAME_LEN works out to:

  1500 + 14 + 4 + 6 + 4 + 8 = 1536

ENET_PAD (8 bytes) is not part of an Ethernet frame. The longest frame for
the default MTU is 1528 bytes, which the removed comment spelled out.

The removed comment also explained that the value was rounded up to 1536
because 1536 is a multiple of 256. Without it, nothing in the driver says
why ENET_PAD exists. The new comment presents the padded value as a frame
length.

The same wording is still there at the end of the series. In "net: bcmgenet:
allow the MTU to be changed", the macro becomes:

  /* Longest frame the MAC must accept for a given MTU */
  #define ENET_FRAME_OVERHEAD (ETH_HLEN + VLAN_HLEN + ENET_BRCM_TAG_LEN + \
                               ETH_FCS_LEN + ENET_PAD)
  #define ENET_MAX_FRAME_LEN(mtu) ((mtu) + ENET_FRAME_OVERHEAD)

With an arbitrary MTU there is no rounding to a 256-byte multiple, so
ENET_PAD is just 8 bytes of slack with no stated reason.

Could the comment either keep the 1528/1536 explanation, or say why ENET_PAD
is still counted as frame overhead?

>  #define DMA_MAX_BURST_LENGTH    0x10
>

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