Re: [PATCH net-next v8 10/10] bng_en: add support for ethtool -S stats display

[Simon Horman]

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

Simon says: This is somewhat more subjective than the other two AI
generated reviews I released for this patch-set. But I think it does
at least warrant discussion.

---
bng_en: add support for ethtool -S stats display

This patch implements the legacy ethtool statistics interface to expose
hardware counters through ethtool -S, including per-queue ring stats,
TPA stats, port level stats, and per-priority stats.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> index 2ae13f18e2d7..1a0eb987ef6f 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c

[ ... ]

> @@ -42,6 +307,219 @@ static void bnge_get_drvinfo(struct net_device *dev,
>  	strscpy(info->bus_info, pci_name(bd->pdev), sizeof(info->bus_info));
>  }
>
> +static int bnge_get_sset_count(struct net_device *dev, int sset)
> +{
> +	struct bnge_net *bn = netdev_priv(dev);
> +
> +	switch (sset) {
> +	case ETH_SS_STATS:
> +		return bnge_get_num_stats(bn);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

[ ... ]

> +static void bnge_get_ethtool_stats(struct net_device *dev,
> +				   struct ethtool_stats *stats, u64 *buf)
> +{
> +	struct bnge_net *bn = netdev_priv(dev);
> +	struct bnge_dev *bd = bn->bd;
> +	u32 tpa_stats;
> +	u32 i, j = 0;
> +
> +	if (!bn->bnapi) {
> +		j += bnge_get_num_ring_stats(bd);
> +		goto skip_ring_stats;
> +	}
> +
> +	tpa_stats = bnge_get_num_tpa_ring_stats(bd);
> +	for (i = 0; i < bd->nq_nr_rings; i++) {
> +		struct bnge_napi *bnapi = bn->bnapi[i];
> +		struct bnge_nq_ring_info *nqr;
> +		u64 *sw_stats;
> +		int k;
> +
> +		nqr = &bnapi->nq_ring;
> +		sw_stats = nqr->stats.sw_stats;
> +
> +		if (is_rx_ring(bd, i)) {
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, rx_ucast_pkts);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, rx_mcast_pkts);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, rx_bcast_pkts);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, rx_ucast_bytes);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, rx_mcast_bytes);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, rx_bcast_bytes);
> +		}
> +		if (is_tx_ring(bd, i)) {
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, tx_ucast_pkts);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, tx_mcast_pkts);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, tx_bcast_pkts);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, tx_ucast_bytes);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, tx_mcast_bytes);
> +			buf[j++] = BNGE_GET_RING_STATS64(sw_stats, tx_bcast_bytes);
> +		}

This isn't a bug, but Documentation/networking/statistics.rst flags a
potential concern about interface duplication. The patch adds per-queue
ring statistics (rxqN_ucast_packets, rxqN_mcast_packets,
rxqN_bcast_packets, rxqN_ucast_bytes, rxqN_mcast_bytes, rxqN_bcast_bytes,
and tx equivalents) to the ethtool -S interface.

However, this driver already implements the standard per-queue statistics
API via netdev_stat_ops (bnge_get_queue_stats_rx and
bnge_get_queue_stats_tx in bnge_netdev.c). That interface exposes
per-queue packets and bytes through netlink, which is the designated
mechanism for per-queue counters.

According to Documentation/networking/statistics.rst, per-queue stats
should use the standard netdev_stat_ops interface rather than ethtool -S.
The documentation states that drivers should avoid adding new counters to
ethtool -S for values that have a standard uAPI.

While the ethtool -S stats provide a ucast/mcast/bcast breakdown not
directly available from the standard per-queue API (which aggregates to
total packets and bytes), this still represents duplication of the
fundamental per-queue packet and byte counters. Could these per-queue
stats be omitted from ethtool -S, keeping only the truly hardware-specific
stats like TPA, port-level, and priority stats that don't have a standard
interface?

[ ... ]

> @@ -262,6 +740,9 @@ static const struct ethtool_ops bnge_ethtool_ops = {
>  	.nway_reset		= bnge_nway_reset,
>  	.get_pauseparam		= bnge_get_pauseparam,
>  	.set_pauseparam		= bnge_set_pauseparam,
> +	.get_sset_count		= bnge_get_sset_count,
> +	.get_strings		= bnge_get_strings,
> +	.get_ethtool_stats	= bnge_get_ethtool_stats,
>  	.get_eth_phy_stats	= bnge_get_eth_phy_stats,
>  	.get_eth_mac_stats	= bnge_get_eth_mac_stats,
>  	.get_eth_ctrl_stats	= bnge_get_eth_ctrl_stats,