| 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
| ||
|
Message-ID: <2fe84499-b768-49b5-bf0c-3264f8598f9e@linux.dev>
Date: Sat, 4 Oct 2025 13:33:47 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: ALOK TIWARI <alok.a.tiwari@...cle.com>, mkubecek@...e.cz
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [External] : [PATCH ethtool-next] netlink: fec: add errors
histogram statistics
On 04/10/2025 10:53, ALOK TIWARI wrote:
>
>
> On 10/4/2025 4:22 AM, Vadim Fedorenko wrote:
>> Linux 6.18 has FEC errors histogram statistics API added. Add support
>> for extra attributes in ethtool.
>>
>> # ethtool -I --show-fec eni8np1
>> FEC parameters for eni8np1:
>> Supported/Configured FEC encodings: None
>> Active FEC encoding: None
>> Statistics:
>> corrected_blocks: 123
>> uncorrectable_blocks: 4
>> fec_bit_err_0: 445 [ per_lane: 125, 120, 100, 100 ]
>> fec_bit_err_1_to_3: 12
>> fec_bit_err_4_to_7: 2
>>
>> # ethtool -j -I --show-fec eni8np1
>> [ {
>> "ifname": "eni8np1",
>> "config": [ "None" ],
>> "active": [ "None" ],
>> "statistics": {
>> "corrected_blocks": {
>> "total": 123
>> },
>> "uncorrectable_blocks": {
>> "total": 4
>> },
>> "hist": [ {
>> "bin_low": 0,
>> "bin_high": 0,
>> "total": 445,
>> "lanes": [ 125,120,100,100 ]
>> },{
>> "bin_low": 1,
>> "bin_high": 3,
>> "total": 12
>> },{
>> "bin_low": 4,
>> "bin_high": 7,
>> "total": 2
>> } ]
>> }
>> } ]
>>
>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>> ---
>> netlink/fec.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 72 insertions(+)
>>
>> diff --git a/netlink/fec.c b/netlink/fec.c
>> index ed100d7..32f7ca7 100644
>> --- a/netlink/fec.c
>> +++ b/netlink/fec.c
>> @@ -44,6 +44,64 @@ fec_mode_walk(unsigned int idx, const char *name,
>> bool val, void *data)
>> print_string(PRINT_ANY, NULL, " %s", name);
>> }
>> +static void fec_show_hist_bin(const struct nlattr *hist)
>> +{
>> + const struct nlattr *tb[ETHTOOL_A_FEC_HIST_MAX + 1] = {};
>> + DECLARE_ATTR_TB_INFO(tb);
>> + unsigned int i, lanes, bin_high, bin_low;
>> + uint64_t val, *vals;
>> + int ret;
>> +
>> + ret = mnl_attr_parse_nested(hist, attr_cb, &tb_info);
>> + if (ret < 0)
>> + return;
>> +
>> + if (!tb[ETHTOOL_A_FEC_HIST_BIN_LOW] || !
>> tb[ETHTOOL_A_FEC_HIST_BIN_HIGH])
>> + return;
>> +
>> + bin_high = mnl_attr_get_u32(tb[ETHTOOL_A_FEC_HIST_BIN_HIGH]);
>> + bin_low = mnl_attr_get_u32(tb[ETHTOOL_A_FEC_HIST_BIN_LOW]);
>> + /* Bin value is uint, so it may be u32 or u64 depeding on the
>> value */
>
> typo depeding -> depending
yep, missed it..
>
>> + if (mnl_attr_validate(tb[ETHTOOL_A_FEC_HIST_BIN_VAL],
>> MNL_TYPE_U32) < 0)
>> + val = mnl_attr_get_u64(tb[ETHTOOL_A_FEC_HIST_BIN_VAL]);
>> + else
>> + val = mnl_attr_get_u32(tb[ETHTOOL_A_FEC_HIST_BIN_VAL]);
>> +
>> + if (is_json_context()) {
>> + print_u64(PRINT_JSON, "bin_low", NULL, bin_low);
>> + print_u64(PRINT_JSON, "bin_high", NULL, bin_high);
>> + print_u64(PRINT_JSON, "total", NULL, val);
>> + } else {
>> + printf(" fec_bit_err_%d", bin_low);
>> + if (bin_low != bin_high)
>> + printf("_to_%d", bin_high);
>> + printf(": %" PRIu64, val);
>> + }
>> + if (!tb[ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE]) {
>> + if (!is_json_context())
>> + print_nl();
>> + return;
>> + }
>> +
>> + vals =
>> mnl_attr_get_payload(tb[ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE]);
>> + lanes =
>> mnl_attr_get_payload_len(tb[ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE]) / 8;
>
> 8 -> sizeof(uint64_t)
There is no history of using sizeof(uint64_t) in code base, but the
codearound uses "/ 8" and "% 8" for u64 attributes.
>> + if (is_json_context())
>> + open_json_array("lanes", "");
>> + else
>> + printf(" [ per_lane:");
>> + for (i = 0; i < lanes; i++) {
>> + if (is_json_context())
>> + print_u64(PRINT_JSON, NULL, NULL, *vals++);
>> + else
>> + printf("%s %" PRIu64, i ? "," : "", *vals++);
>
> "" -> " "
That will break formatting. Empty string is intended here.
>
>> + }
>> +
>> + if (is_json_context())
>> + close_json_array("");
>> + else
>> + printf(" ]\n");
>> +}
>> +
>> static int fec_show_stats(const struct nlattr *nest)
>> {
>> const struct nlattr *tb[ETHTOOL_A_FEC_STAT_MAX + 1] = {};
>> @@ -108,6 +166,20 @@ static int fec_show_stats(const struct nlattr *nest)
>> close_json_object();
>> }
>> +
>> + if (tb[ETHTOOL_A_FEC_STAT_HIST]) {
>> + const struct nlattr *attr;
>> +
>> + open_json_array("hist", "");
>> + mnl_attr_for_each_nested(attr, nest) {
>
> "mnl_attr_for_each_nested(attr, nest) {" or
> "mnl_attr_for_each_nested(attr, tb[ETHTOOL_A_FEC_STAT_HIST]) {" ? please
> check it
tb[ETHTOOL_A_FEC_STAT_HIST] will not have nested attributes of type
ETHTOOL_A_FEC_STAT_HIST, the correct way is the one in the patch
>> + if (mnl_attr_get_type(attr) == ETHTOOL_A_FEC_STAT_HIST) {
>> + open_json_object(NULL);
>> + fec_show_hist_bin(attr);
>> + close_json_object();
>> + }
>> + }
>> + close_json_array("");
>> + }
>> close_json_object();
>> return 0;
>
> Thanks,
> Alok
>
Powered by blists - more mailing lists