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.
