From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 737A7A00BE; Wed, 20 Apr 2022 11:16:25 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1345C4068E; Wed, 20 Apr 2022 11:16:25 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E144C40687 for ; Wed, 20 Apr 2022 11:16:22 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 9A28E89; Wed, 20 Apr 2022 12:16:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 9A28E89 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1650446181; bh=4ApFJhieCDSpRbalO3qPKmLtY4CnzYRthFPF8p7no34=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=euz4AzvXyuwSe3ORVQJkSqhcCre8x0OQGYW5A7i1hR3rRtWN7Lpf57eJdnlHeJfMP 5yihNp9OnbT6I1o0COrlO4Q//q9/4AHC16bZIFcc8bS6bKTB/DQW3lQnAWsIshv3f5 MJ5Ox1adunOhGpsjIcr0IeruWp3bpei9b2b+EBpk= Message-ID: <61bb171e-ecc7-c250-19af-d98b78fa60e2@oktetlabs.ru> Date: Wed, 20 Apr 2022 12:16:21 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v3 1/5] ethdev: add telemetry command for module EEPROM Content-Language: en-US To: Robin Zhang , dev@dpdk.org Cc: qiming.yang@intel.com, qi.z.zhang@intel.com, stevex.yang@intel.com, thomas@monjalon.net, bruce.richardson@intel.com, david.marchand@redhat.com References: <20220215101853.919735-1-robinx.zhang@intel.com> <20220420070017.119739-1-robinx.zhang@intel.com> <20220420070017.119739-2-robinx.zhang@intel.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220420070017.119739-2-robinx.zhang@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 4/20/22 10:00, Robin Zhang wrote: > Add a new telemetry command /ethdev/module_eeprom to dump the module > EEPROM of each port. The format of module EEPROM information follows > the SFF(Small Form Factor) Committee specifications. > > Current the format support SFP(Small Formfactor Pluggable)/SFP+/ > QSFP+(Quad Small Formfactor Pluggable)/QSFP28 with specs SFF-8079/ > SFF-8472/SFF-8024/SFF-8636. > > Signed-off-by: Robin Zhang > --- > lib/ethdev/ethdev_sff_telemetry.c | 135 ++++++++++++++++++++++++++++++ > lib/ethdev/ethdev_sff_telemetry.h | 42 ++++++++++ > lib/ethdev/meson.build | 5 ++ > lib/ethdev/rte_ethdev.c | 3 + > 4 files changed, 185 insertions(+) > create mode 100644 lib/ethdev/ethdev_sff_telemetry.c > create mode 100644 lib/ethdev/ethdev_sff_telemetry.h > > diff --git a/lib/ethdev/ethdev_sff_telemetry.c b/lib/ethdev/ethdev_sff_telemetry.c > new file mode 100644 > index 0000000000..60dc19c206 > --- /dev/null > +++ b/lib/ethdev/ethdev_sff_telemetry.c > @@ -0,0 +1,135 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2018 Cavium, Inc I'm wondering why the patch comes from intel.com, but has Cavium copyright dated by 2018. > + */ > + > +#include > +#include > +#include "ethdev_sff_telemetry.h" > + > +static void > +sff_port_module_eeprom_display(uint16_t port_id, sff_item *items) > +{ > + struct rte_eth_dev_module_info minfo; > + struct rte_dev_eeprom_info einfo; > + int ret; > + > + ret = rte_eth_dev_get_module_info(port_id, &minfo); > + if (ret != 0) { > + switch (ret) { > + case -ENODEV: > + fprintf(stderr, "port index %d invalid\n", port_id); Why is it go to stderr directly? Shouldn't DPDK logging be used? > + break; > + case -ENOTSUP: > + fprintf(stderr, "operation not supported by device\n"); > + break; > + case -EIO: > + fprintf(stderr, "device is removed\n"); > + break; > + default: > + fprintf(stderr, "Unable to get module EEPROM: %d\n", > + ret); > + break; > + } > + return; > + } > + > + einfo.offset = 0; > + einfo.length = minfo.eeprom_len; > + einfo.data = calloc(1, minfo.eeprom_len); > + if (!einfo.data) { Compare vs NULL > + fprintf(stderr, > + "Allocation of port %u eeprom data failed\n", > + port_id); > + return; > + } > + > + ret = rte_eth_dev_get_module_eeprom(port_id, &einfo); > + if (ret != 0) { > + switch (ret) { > + case -ENODEV: > + fprintf(stderr, "port index %d invalid\n", port_id); > + break; > + case -ENOTSUP: > + fprintf(stderr, "operation not supported by device\n"); > + break; > + case -EIO: > + fprintf(stderr, "device is removed\n"); > + break; > + default: > + fprintf(stderr, "Unable to get module EEPROM: %d\n", > + ret); > + break; > + } It looks like a duplicate of above switch. > + free(einfo.data); > + return; > + } > + > + switch (minfo.type) { > + case RTE_ETH_MODULE_SFF_8079: > + sff_8079_show_all(einfo.data, items); I guess the build is broken after the patch since the function is not defined. It is added in follow up patches. I think corresponding cases should be added in follow up patches which add corresponding spec support. > + break; > + case RTE_ETH_MODULE_SFF_8472: > + sff_8079_show_all(einfo.data, items); > + sff_8472_show_all(einfo.data, items); same here > + break; > + case RTE_ETH_MODULE_SFF_8436: > + case RTE_ETH_MODULE_SFF_8636: > + sff_8636_show_all(einfo.data, einfo.length, items); same here > + break; > + default: Don't we need some logging to help user to understand that eeprom type is not supported? > + break; > + } > + printf("Finish -- Port: %d MODULE EEPROM length: %d bytes\n", port_id, einfo.length); > + free(einfo.data); > +} > + > +void > +add_item_string(sff_item *items, const char *name_str, const char *value_str) > +{ > + /* append different values for same keys */ > + if (sff_item_count > 0 && > + (strcmp(items[sff_item_count - 1].name, name_str) == 0)) { > + strcat(items[sff_item_count - 1].value, "; "); > + strcat(items[sff_item_count - 1].value, value_str); > + return; > + } > + > + sprintf(items[sff_item_count].name, "%s", name_str); > + sprintf(items[sff_item_count].value, "%s", value_str); strcat() and sprintf() are terribly unsafe and should be avoided. Use strlcat() and snprintf() instead. > + sff_item_count++; > +} > + > +int > +eth_dev_handle_port_module_eeprom(const char *cmd __rte_unused, const char *params, > + struct rte_tel_data *d) > +{ > + char *end_param; > + int port_id, i; > + sff_item *items; > + sff_item_count = 0; > + > + if (params == NULL || strlen(params) == 0 || !isdigit(*params)) > + return -1; > + > + port_id = strtoul(params, &end_param, 0); Strictly speaking we need to care about about overflow here, since if input string is a long long string of digits, end_param will point to \0, but errno will be set to ERANGE. 'man strtoul' recommends to reset errno to 0 and check it after the call. > + if (*end_param != '\0') > + RTE_ETHDEV_LOG(NOTICE, > + "Extra parameters passed to ethdev telemetry command, ignoring"); > + > + items = (sff_item *)malloc(SFF_ITEM_SIZE * SFF_ITEM_MAX_COUNT); Since we allocate an array here with specified element size and number of elements, calloc() should be used. Type cast from 'void *' is not required in C. > + if (items == NULL) { > + printf("Error allocating memory of items\n"); Why is RTE_ETHDEV_LOG used above, but not here? Why above error logging uses fprintf(stderr, ...), but error logs are sent to stdout here? I think that RTE_ETHDEV_LOG should be used everywhere. > + free(items); What's the point to free items, if it is known to be NULL? > + return -1; > + } > + > + sff_port_module_eeprom_display(port_id, items); > + > + rte_tel_data_start_dict(d); > + for (i = 0; i < sff_item_count; i++) > + rte_tel_data_add_dict_string(d, items[i].name, items[i].value); > + > + free(items); > + return 0; > +} > + > diff --git a/lib/ethdev/ethdev_sff_telemetry.h b/lib/ethdev/ethdev_sff_telemetry.h > new file mode 100644 > index 0000000000..fd032407c8 > --- /dev/null > +++ b/lib/ethdev/ethdev_sff_telemetry.h > @@ -0,0 +1,42 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2018 Cavium, Inc > + */ > + > +#ifndef ETHDEV_SFF_H_ > +#define ETHDEV_SFF_H_ Header guard names should follow file name, i.e. ETHDEV_SFF_TELEMETRY_H_. (It is still open question to have some prefix, leading understand etc since various approaches are present in DPDK headers). > + > +#include It looks like the include is not actually used in the header. > +#include > + > +#define ARRAY_SIZE(arr) RTE_DIM(arr) > + > +#define SFF_ITEM_NAME_SIZE 64 > +#define SFF_ITEM_VALUE_SIZE 256 > +#define SFF_ITEM_MAX_COUNT 256 > +#define TMP_STRING_SIZE 64 This one is really bad define since it does not explain its indented usage. Is it for value? Or is it to compose name? So, it is hard to say if specified value is sufficient and taking into account that you don't check overflow anyway. > + > +typedef struct sff_module_info_item { > + char name[SFF_ITEM_NAME_SIZE]; /* The item name. */ > + char value[SFF_ITEM_VALUE_SIZE]; /* The item value. */ > +} sff_item; Typically DPDK does not use typedefs for structures and use 'struct sff_item' as is. > + > +#define SFF_ITEM_SIZE sizeof(sff_item) I see no value in introduction of the define. As far as I can see it is used only once and sizeof(struct sff_item) would look better there. > + > +uint16_t sff_item_count; > + > +/* SFF-8079 Optics diagnostics */ > +void sff_8079_show_all(const uint8_t *id, sff_item *items); What is 'id' above? Is it some kind identifier? As far as I can see struct rte_dev_eeprom_info.data is passed there. If so, why is it called 'id' here? Don't we need to pass length to check inside that we never have out-of-bounds access (e.g. if something is buggy and provided information is not terminated properly). > + > +/* SFF-8472 Optics diagnostics */ > +void sff_8472_show_all(const uint8_t *id, sff_item *items); > + > +/* SFF-8636 Optics diagnostics */ > +void sff_8636_show_all(const uint8_t *id, uint32_t eeprom_len, sff_item *items); > + > +int eth_dev_handle_port_module_eeprom(const char *cmd __rte_unused, > + const char *params, > + struct rte_tel_data *d); > + > +void add_item_string(sff_item *items, const char *name_str, const char *value_str); > + > +#endif /* ETHDEV_SFF_H_ */ > diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build > index a094585bf7..88ceeb12b9 100644 > --- a/lib/ethdev/meson.build > +++ b/lib/ethdev/meson.build > @@ -11,6 +11,11 @@ sources = files( > 'rte_flow.c', > 'rte_mtr.c', > 'rte_tm.c', > + 'ethdev_sff_telemetry.c', > + 'sff_common.c', > + 'sff_8079.c', > + 'sff_8472.c', > + 'sff_8636.c', Is it a mail client glitch or alignment above really differs: TABs vs spaces? > ) > > headers = files( [snip]