Re: [net-next,3/7] bng_en: add ethtool link settings, get_link, and nway_reset

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
bng_en: add ethtool link settings, get_link, and nway_reset

This commit adds get/set_link_ksettings, get_link, and nway_reset
support. It also adds reporting for supported, advertised, and
link-partner speeds across NRZ, PAM4, and PAM4-112 signaling modes,
and enables lane count reporting.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_link.c b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> index bff04dfadaca..f037229db7b4 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_link.c

[ ... ]

> +enum bnge_link_speed_indices {
> +	BNGE_LINK_SPEED_UNKNOWN = 0,
> +	BNGE_LINK_SPEED_50GB_IDX,
> +	BNGE_LINK_SPEED_100GB_IDX,
> +	BNGE_LINK_SPEED_200GB_IDX,
> +	BNGE_LINK_SPEED_400GB_IDX,
> +	BNGE_LINK_SPEED_800GB_IDX,
> +	__BNGE_LINK_SPEED_END
> +};

[ ... ]

> +static const u16 bnge_nrz_speed_masks[] = {
> +	[BNGE_LINK_SPEED_100GB_IDX] = BNGE_LINK_SPEED_MSK_100GB,
> +	[__BNGE_LINK_SPEED_END - 1] = 0 /* make any legal speed a valid index */
> +};
> +
> +static const u16 bnge_pam4_speed_masks[] = {
> +	[BNGE_LINK_SPEED_50GB_IDX] = BNGE_LINK_PAM4_SPEED_MSK_50GB,
> +	[BNGE_LINK_SPEED_100GB_IDX] = BNGE_LINK_PAM4_SPEED_MSK_100GB,
> +	[BNGE_LINK_SPEED_200GB_IDX] = BNGE_LINK_PAM4_SPEED_MSK_200GB,
> +	[__BNGE_LINK_SPEED_END - 1] = 0 /* make any legal speed a valid index */
> +};
> +
> +static const u16 bnge_nrz_speeds2_masks[] = {
> +	[BNGE_LINK_SPEED_100GB_IDX] = BNGE_LINK_SPEEDS2_MSK_100GB,
> +	[__BNGE_LINK_SPEED_END - 1] = 0 /* make any legal speed a valid index */
> +};
> +
> +static const u16 bnge_pam4_speeds2_masks[] = {
> +	[BNGE_LINK_SPEED_50GB_IDX] = BNGE_LINK_SPEEDS2_MSK_50GB_PAM4,
> +	[BNGE_LINK_SPEED_100GB_IDX] = BNGE_LINK_SPEEDS2_MSK_100GB_PAM4,
> +	[BNGE_LINK_SPEED_200GB_IDX] = BNGE_LINK_SPEEDS2_MSK_200GB_PAM4,
> +	[BNGE_LINK_SPEED_400GB_IDX] = BNGE_LINK_SPEEDS2_MSK_400GB_PAM4,
> +};

Is the missing sentinel entry here intentional? The bnge_pam4_speeds2_masks
array has its highest initializer at BNGE_LINK_SPEED_400GB_IDX (4), making
it 5 elements long (indices 0-4). However, bnge_set_ethtool_speeds loops
'for (i = 1; i < __BNGE_LINK_SPEED_END; i++)' which reaches i = 5
(BNGE_LINK_SPEED_800GB_IDX). When BNGE_PHY_FL_SPEEDS2 is set, sp_pam4_msks
points to bnge_pam4_speeds2_masks, causing sp_pam4_msks[5] to read past
the array end. All other speed mask arrays include
'[__BNGE_LINK_SPEED_END - 1] = 0' to prevent this.

> +
> +static const u16 bnge_pam4_112_speeds2_masks[] = {
> +	[BNGE_LINK_SPEED_100GB_IDX] = BNGE_LINK_SPEEDS2_MSK_100GB_PAM4_112,
> +	[BNGE_LINK_SPEED_200GB_IDX] = BNGE_LINK_SPEEDS2_MSK_200GB_PAM4_112,
> +	[BNGE_LINK_SPEED_400GB_IDX] = BNGE_LINK_SPEEDS2_MSK_400GB_PAM4_112,
> +	[BNGE_LINK_SPEED_800GB_IDX] = BNGE_LINK_SPEEDS2_MSK_800GB_PAM4_112,
> +};
-- 
pw-bot: cr