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

[Bhargava Chenna Marreddy]

On Tue, Mar 3, 2026 at 8:30 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
> 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/

Thanks, Jakub.

The code was functionally safe, but I agree the inconsistency in the
array patterns was not ideal.

I will address this in the next spin by explicitly defining all speed
mask array sizes as [__BNGE_LINK_SPEED_END]. This ensures a uniform
structure and removes the need for manual sentinel entries.

Ex:
For example:
    static const u16 bnge_nrz_speeds2_masks[__BNGE_LINK_SPEED_END] = {
            [BNGE_LINK_SPEED_100GB_IDX] = BNGE_LINK_SPEEDS2_MSK_100GB,
    };

Thanks,
Bhargava Marreddy

The code was functionally safe, but I agree the inconsistency in the
array patterns was not ideal. I will address this in the next spin by
explicitly defining all speed mask array sizes as
[__BNGE_LINK_SPEED_END]. This ensures a uniform structure and removes
the need for manual sentinel entries.
>
> [ ... ]
> --
> pw-bot: cr

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