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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4d3a0a08-bda4-483f-a120-b1f905ec0761@nvidia.com>
Date: Thu, 18 Sep 2025 17:25:40 +0300
From: Carolina Jubran <cjubran@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>,
 Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Andrew Lunn <andrew@...n.ch>, Michael Chan <michael.chan@...adcom.com>,
 Pavan Chebbi <pavan.chebbi@...adcom.com>, Tariq Toukan <tariqt@...dia.com>,
 Gal Pressman <gal@...dia.com>, intel-wired-lan@...ts.osuosl.org,
 Donald Hunter <donald.hunter@...il.com>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
 Yael Chemla <ychemla@...dia.com>, Dragos Tatulea <dtatulea@...dia.com>
Subject: Re: [PATCH net-next v3 3/4] net/mlx5e: Add logic to read RS-FEC
 histogram bin ranges from PPHCR


On 18/09/2025 3:46, Jakub Kicinski wrote:
> On Tue, 16 Sep 2025 19:12:56 +0000 Vadim Fedorenko wrote:
>> From: Carolina Jubran <cjubran@...dia.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 :\


You are right. will change.

>> 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?

My intention was to keep mlx5 capped at 16 even if ethtool raises its max.
We’ll only increase this once we know the device should expose more than 16.
Since our HW has internal modes that can report more than 16 bins, this 
ensures
we don’t accidentally expose them if ethtool increases its max.

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

Ack! Thanks! Carolina