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

[Jakub Kicinski]

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
to the bnge driver. It implements speed reporting across NRZ, PAM4, and
PAM4-112 signaling modes and enables lane count reporting.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge.h b/drivers/net/ethernet/broadcom/bnge/bnge.h
> index 83ee4749cc70..e9b83c0c64b6 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge.h
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge.h

[ ... ]

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

[ ... ]

> @@ -458,3 +503,755 @@ void bnge_report_link(struct bnge_dev *bd)
>  		netdev_err(bd->netdev, "NIC Link is Down\n");
>  	}
>  }
> +
> +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,
> +	[__BNGE_LINK_SPEED_END - 1] = 0 /* make any legal speed a valid index */
> +};
> +
> +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,
> +};

The bnge_pam4_112_speeds2_masks array is missing the sentinel entry
that all other speed mask arrays have. Jakub Kicinski's AI review in v1
identified a potential out-of-bounds array access in this array.

The v2 changelog claims to have fixed this by 'adding the missing sentinel',
but only bnge_pam4_speeds2_masks[] was fixed. All other speed mask arrays
include '[__BNGE_LINK_SPEED_END - 1] = 0' to ensure any legal speed is a
valid index, but bnge_pam4_112_speeds2_masks[] still lacks this.

While the array currently has an entry at the highest accessed index
([5] = 800GB), the missing sentinel makes it inconsistent with the other
arrays and may cause issues if the loop or access patterns change.

Looking at bnge_set_ethtool_speeds():

drivers/net/ethernet/broadcom/bnge/bnge_link.c:bnge_set_ethtool_speeds() {
	for (i = 1; i < __BNGE_LINK_SPEED_END; i++) {
		for (m = 1; m < __BNGE_MEDIA_END; m++) {
			...
			bnge_update_speed(&delta_pam4_112, m == media,
					  adv_pam4_112, sp_pam4_112_msks[i],
					  et_mask,
					  bnge_link_modes[i][BNGE_SIG_MODE_PAM4_112][m]);
		}
	}
}

Should this array follow the same pattern as the other speed mask arrays?

Reference: https://lore.kernel.org/netdev/20260224001201.146384-2-kuba@xxxxxxxxxx/

[ ... ]
-- 
pw-bot: cr