Re: [net-next,v3,6/8] bng_en: add initial support for ethtool stats display

[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 initial support for ethtool stats display
>
> This commit implements ethtool -S callbacks for per-ring RX/TX, TPA,
> and port-level MAC/extended statistics, including string names, counts,
> and DMA-backed memory allocation for port stats via HWRM wrappers.
>
> > diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> > --- a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> > +++ b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> > @@ -31,6 +31,331 @@ static int bnge_nway_reset(struct net_device *dev)
> >       return rc;
> >  }
> >
> > +static const char * const bnge_ring_rx_stats_str[] = {
> > +     "rx_ucast_packets",
> > +     "rx_mcast_packets",
> > +     "rx_bcast_packets",
> > +     "rx_discards",
> > +     "rx_errors",
> > +     "rx_ucast_bytes",
> > +     "rx_mcast_bytes",
> > +     "rx_bcast_bytes",
> > +};
> > +
> > +static const char * const bnge_ring_tx_stats_str[] = {
> > +     "tx_ucast_packets",
> > +     "tx_mcast_packets",
> > +     "tx_bcast_packets",
> > +     "tx_errors",
> > +     "tx_discards",
>
> Jakub Kicinski raised this concern during v3 review: standard stats like
> rx_errors and tx_errors should not be duplicated in ethtool -S output.
> While this patch adds structured callbacks (.get_eth_mac_stats,
> .get_rmon_stats, etc.) for standard stats, rx_errors and tx_errors still
> appear in the bnge_ring_rx_stats_str[] and bnge_ring_tx_stats_str[]
> arrays above.
>
> These arrays are used by bnge_get_strings() which feeds ethtool -S,
> meaning the standard stats will still appear in ethtool -S output.
>
> Should rx_errors and tx_errors be removed from these arrays to comply
> with the kernel policy against duplicating standard stats?

Thanks, Jakub.

I see the issue. These aren't actually duplicates of the standard
per-interface stats.
They are unique per-ring hardware counters, but the strings happen to match.

To fix the collision, I'll prefix the entire ring stat arrays with
rxq_ and txq_.

Example of the new output:
  - Per-ring stats (via ethtool -S):
      # ethtool -S eno12399np0 | grep rxq_errors
        [0]: rxq_errors: 0
         [1]: rxq_errors: 0
         ...
        [n-1]: rxq_errors: 0

  - Standard interface stats (via .ndo_get_stats64 / sysfs):
      # cat /sys/class/net/eno12399np0/statistics/rx_errors
         0

Does this approach look okay for v4?

Thanks,
Bhargava Marreddy.

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