Re: [RFC PATCH v4] ethtool: add FEC bins histogramm report

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 7 Aug 2025 08:59:24 -0700 Vadim Fedorenko wrote:
> +		/* add FEC bins information */
> +		len += (nla_total_size(0) +  /* _A_FEC_HIST */
> +			nla_total_size(4) +  /* _A_FEC_HIST_BIN_LOW */
> +			nla_total_size(4) +  /* _A_FEC_HIST_BIN_HI */
> +			/* _A_FEC_HIST_BIN_VAL + per-lane values */
> +			nla_total_size_64bit(sizeof(u64) *
> +					     (1 + ETHTOOL_MAX_LANES))) *

That's the calculation for basic stats because they are reported as a
raw array. Each nla_put() should correspond to a nla_total_size().
This patch nla_put()s things individually.

> +			ETHTOOL_FEC_HIST_MAX;
> +	}
>  
>  	return len;
>  }
>  
> +static int fec_put_hist(struct sk_buff *skb, const struct ethtool_fec_hist *hist)
> +{
> +	const struct ethtool_fec_hist_range *ranges = hist->ranges;
> +	const struct ethtool_fec_hist_value *values = hist->values;
> +	struct nlattr *nest;
> +	int i, j;
> +
> +	if (!ranges)
> +		return 0;
> +
> +	for (i = 0; i < ETHTOOL_FEC_HIST_MAX; i++) {
> +		if (i && !ranges[i].low && !ranges[i].high)
> +			break;
> +
> +		if (WARN_ON_ONCE(values[i].bin_value == ETHTOOL_STAT_NOT_SET))
> +			break;
> +
> +		nest = nla_nest_start(skb, ETHTOOL_A_FEC_STAT_HIST);
> +		if (!nest)
> +			return -EMSGSIZE;
> +
> +		if (nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_LOW,
> +				ranges[i].low) ||
> +		    nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_HIGH,
> +				ranges[i].high) ||
> +		    nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL,
> +				 values[i].bin_value))
> +			goto err_cancel_hist;
> +		for (j = 0; j < ETHTOOL_MAX_LANES; j++) {
> +			if (values[i].bin_value_per_lane[j] == ETHTOOL_STAT_NOT_SET)
> +				break;
> +			nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE,
> +				     values[i].bin_value_per_lane[j]);

TBH I'm a bit unsure if this is really worth breaking out into
individual nla_puts(). We generally recommend that, but here it's
an array of simple ints.. maybe we're better of with a binary / C
array of u64. Like the existing FEC stats but without also folding
the total value into index 0.