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

[Bhargava Chenna Marreddy]

On Tue, Feb 24, 2026 at 5:42 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> 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.

Thanks Jakub, good catch.
The missing sentinel was unintentional and could lead to an OOB read.
I'll fix this in v2

Thanks,
Bhargava Marreddy

>
> > +
> > +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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature