|SUCCESS| net/mlx5: add hardware query for basic stats

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163164

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-17

Reviewing patch: net/mlx5: add hardware query for basic stats

Author: Rayane Boussanni <rboussanni at gmail.com>

Subject: [PATCH] net/mlx5: add hardware query for basic stats

This patch adds hardware counter querying for basic statistics when
MLX5_PMD_SOFT_COUNTERS is not defined, replacing the previous FIXME
comments with actual implementation.

> diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c
> index 5cd3e303cc..198f231559 100644
> --- a/drivers/net/mlx5/mlx5_stats.c
> +++ b/drivers/net/mlx5/mlx5_stats.c
> @@ -280,7 +280,46 @@ mlx5_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
>  		tmp.imissed = stats_ctrl->imissed;
>  	}
>  #ifndef MLX5_PMD_SOFT_COUNTERS
> -	/* FIXME: retrieve and add hardware counters. */
> +	{
> +		uint64_t counters[MLX5_MAX_XSTATS];
> +		struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl;
> +		bool bond_master = (priv->master && priv->pf_bond >= 0);
> +
> +		ret = mlx5_os_read_dev_counters(dev, bond_master, counters);
> +		if (ret) {
> +			DRV_LOG(WARNING, "port %u unable to read device counters",
> +				dev->data->port_id);
> +			return ret;
> +		}
> +		for (i = 0; i != xstats_ctrl->mlx5_stats_n; ++i) {

Does the variable 'i' overflow when xstats_ctrl->mlx5_stats_n exceeds
MLX5_MAX_XSTATS? The counters array is sized to MLX5_MAX_XSTATS, but
there is no bounds checking before accessing counters[i] and
xstats_ctrl->base[i].

> +			const char *name = xstats_ctrl->info[i].dpdk_name;
> +			uint64_t val = (counters[i] - xstats_ctrl->base[i]);
> +
> +			if (!strcmp(name, "rx_unicast_packets") ||
> +			    !strcmp(name, "rx_multicast_packets") ||
> +			    !strcmp(name, "rx_broadcast_packets"))
> +				tmp.ipackets += val;
> +			else if (!strcmp(name, "rx_unicast_bytes") ||
> +				 !strcmp(name, "rx_multicast_bytes") ||
> +				 !strcmp(name, "rx_broadcast_bytes"))
> +				tmp.ibytes += val;
> +			else if (!strcmp(name, "tx_unicast_packets") ||
> +				 !strcmp(name, "tx_multicast_packets") ||
> +				 !strcmp(name, "tx_broadcast_packets"))
> +				tmp.opackets += val;
> +			else if (!strcmp(name, "tx_unicast_bytes") ||
> +				 !strcmp(name, "tx_multicast_bytes") ||
> +				 !strcmp(name, "tx_broadcast_bytes"))
> +				tmp.obytes += val;
> +			else if (!strcmp(name, "rx_wqe_errors") ||
> +				 !strcmp(name, "rx_phy_crc_errors") ||
> +				 !strcmp(name, "rx_phy_in_range_len_errors") ||
> +				 !strcmp(name, "rx_phy_symbol_errors"))
> +				tmp.ierrors += val;
> +			else if (!strcmp(name, "tx_phy_errors"))
> +				tmp.oerrors += val;

Can xstats_ctrl->info[i].dpdk_name be NULL? If so, does this code
properly handle that case before calling strcmp?

[ ... ]

> @@ -319,7 +358,21 @@ mlx5_stats_reset(struct rte_eth_dev *dev)
>  	mlx5_os_read_dev_stat(priv, "out_of_buffer", &stats_ctrl->imissed_base);
>  	stats_ctrl->imissed = 0;
>  #ifndef MLX5_PMD_SOFT_COUNTERS
> -	/* FIXME: reset hardware counters. */
> +	{
> +		uint64_t counters[MLX5_MAX_XSTATS];
> +		struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl;
> +		bool bond_master = (priv->master && priv->pf_bond >= 0);
> +		int ret;
> +
> +		ret = mlx5_os_read_dev_counters(dev, bond_master, counters);
> +		if (ret) {
> +			DRV_LOG(WARNING, "port %u unable to read device counters",
> +				dev->data->port_id);
> +			return ret;
> +		}
> +		for (i = 0; i != xstats_ctrl->mlx5_stats_n; ++i)
> +			xstats_ctrl->base[i] = counters[i];

Same question as above: does this loop have proper bounds checking to
ensure mlx5_stats_n does not exceed MLX5_MAX_XSTATS before accessing
counters[i]?

> +	}
>  #endif
>  
>  	return 0;