|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

Thank you for the patch submission. I have reviewed the changes against the DPDK coding standards and guidelines. Below are my findings:

---

Commit: patchwork-163164 (series.patch)
Author: Rayane Boussanni <rboussanni at gmail.com>
Subject: [PATCH] net/mlx5: add hardware query for basic stats

This patch modifies mlx5_stats_get() and mlx5_stats_reset() to read
hardware counters when MLX5_PMD_SOFT_COUNTERS is not defined, rather
than returning zeroed values.

> 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);

Does this code handle the case where priv->master is true but pf_bond is
less than zero? Should this be checking both conditions independently, or
does the bitwise AND ensure correctness?

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

Does this error path leave stats in an inconsistent state? The tmp
structure has been partially filled with imissed values but then error
returns without updating the output stats parameter. Should the function
either complete the stats update or clear the tmp structure before
returning an error?

> +		for (i = 0; i != xstats_ctrl->mlx5_stats_n; ++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;

The coding style guide requires that operands to logical operators be
enclosed in parentheses for multi-line conditions. Do these strcmp()
conditions need grouping parentheses? Example:
	if ((!strcmp(name, "rx_unicast_packets")) ||
	    (!strcmp(name, "rx_multicast_packets")) ||
	    (!strcmp(name, "rx_broadcast_packets")))

This applies to all the compound conditions in this loop.

> +			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;
> +		}
> +	}
>  #endif
>  	*stats = tmp;
>  	return 0;

[ ... ]

> @@ -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];
> +	}

Does this error path have the same consistency issue as mlx5_stats_get?
If mlx5_os_read_dev_counters() fails, the stats_ctrl->imissed_base has
been updated but the base[] array is not updated. Is this partial reset
acceptable, or should the function restore imissed_base on error?

>  #endif
>  
>  	return 0;