Re: [Intel-wired-lan] [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC histogram bin ranges from PPHCR

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 16 Sep 2025 19:12:56 +0000 Vadim Fedorenko wrote:
> From: Carolina Jubran <cjubran@nvidia.com>
> 
> Introduce support for querying the Ports Phy Histogram Configuration
> Register (PPHCR) to retrieve RS-FEC histogram bin ranges. The ranges
> are stored in a static array and will be used to map histogram counters
> to error levels.
> 
> The actual RS-FEC histogram statistics are not yet reported in this
> commit and will be handled in a downstream patch.

> @@ -6246,8 +6246,17 @@ int mlx5e_priv_init(struct mlx5e_priv *priv,
>  	if (!priv->channel_stats)
>  		goto err_free_tx_rates;
>  
> +	priv->fec_ranges = kcalloc_node(ETHTOOL_FEC_HIST_MAX,
> +					sizeof(*priv->fec_ranges),
> +					GFP_KERNEL,
> +					node);

Why bother allocating his on the device node? We have no reason to
believe user will pin eth process that reads these stats to the node
where the device is :\

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index aae0022e8736..476689cb0c1f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -1490,8 +1490,63 @@ static void fec_set_corrected_bits_total(struct mlx5e_priv *priv,
>  				      phy_corrected_bits);
>  }
>  
> +#define MLX5E_FEC_RS_HIST_MAX 16
> +
> +static u8
> +fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv,
> +			     const struct ethtool_fec_hist_range **ranges)
> +{
> +	struct mlx5_core_dev *mdev = priv->mdev;
> +	u32 out[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
> +	u32 in[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
> +	int sz = MLX5_ST_SZ_BYTES(pphcr_reg);
> +	u8 active_hist_type, num_of_bins;
> +
> +	memset(priv->fec_ranges, 0,
> +	       ETHTOOL_FEC_HIST_MAX * sizeof(*priv->fec_ranges));
> +	MLX5_SET(pphcr_reg, in, local_port, 1);
> +	if (mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPHCR, 0, 0))
> +		return 0;
> +
> +	active_hist_type = MLX5_GET(pphcr_reg, out, active_hist_type);
> +	if (!active_hist_type)
> +		return 0;
> +
> +	num_of_bins = MLX5_GET(pphcr_reg, out, num_of_bins);
> +	if (WARN_ON_ONCE(num_of_bins > MLX5E_FEC_RS_HIST_MAX))

why does MLX5E_FEC_RS_HIST_MAX exist?
We care that bins_cnt <= ETHTOOL_FEC_HIST_MAX - 1
or is there something in the interface that hardcodes 16?

> +		return 0;

> @@ -2619,3 +2675,4 @@ unsigned int mlx5e_nic_stats_grps_num(struct mlx5e_priv *priv)
>  {
>  	return ARRAY_SIZE(mlx5e_nic_stats_grps);
>  }
> +

spurious change