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 24AB8A04FD; Wed, 4 May 2022 12:16:38 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 047314069F; Wed, 4 May 2022 12:16:38 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E681C40694 for ; Wed, 4 May 2022 12:16:35 +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 3FC8482; Wed, 4 May 2022 13:16:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 3FC8482 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1651659395; bh=QMNtg/tS4sg8jx74ipN1zqO5nKmWbWZngphzvEGLpOQ=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=MtMWjBetoxXMUr8VmNUBFp/ViBZIBUIkkKYHA5iRoeAroYqWKZm/P74T6zSg75VoC PzvESml/4JLBTTkB3uglJJYqg+/NKmnPKYF01Ry41Z5Gx8qvIm7LuDMI2fQoccstbR DMQ/vi05KriH74TawKdk5uFZGaYQ+pTIkm2eVEz8= Message-ID: <9356864d-d698-e384-aa31-7bd30c9e1030@oktetlabs.ru> Date: Wed, 4 May 2022 13:16:34 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH v5 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> <20220426024349.1081666-1-robinx.zhang@intel.com> <20220426024349.1081666-2-robinx.zhang@intel.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220426024349.1081666-2-robinx.zhang@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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/26/22 05:43, 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. Above sounds misleading in the patch since support for some specs is added in subsequent patches. > > Signed-off-by: Robin Zhang There is a warning if I build just after the patch: [2/146] Compiling C object lib/librte_ethdev.a.p/ethdev_ethdev_sff_telemetry.c.o ../../src/dpdk/lib/ethdev/ethdev_sff_telemetry.c: In function ‘sff_port_module_eeprom_display’: ../../src/dpdk/lib/ethdev/ethdev_sff_telemetry.c:14:67: warning: unused parameter ‘items’ [-Wunused-parameter] 14 | sff_port_module_eeprom_display(uint16_t port_id, struct sff_item *items) | ~~~~~~~~~~~~~~~~~^~~~~ [snip] > diff --git a/lib/ethdev/ethdev_sff_telemetry.c b/lib/ethdev/ethdev_sff_telemetry.c > new file mode 100644 > index 0000000000..968b640b17 > --- /dev/null > +++ b/lib/ethdev/ethdev_sff_telemetry.c > @@ -0,0 +1,131 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2022 Intel Corporation > + */ > + > +#include > + > +#include > +#include > +#include "ethdev_sff_telemetry.h" > + > +static uint16_t sff_item_count; Do we really need the static variable? It raises too many questions about threadsafity and typically says that API is defined bad. You pass 'items' anywya. What's the problem to pass the count as well? > + > +static void > +sff_port_module_eeprom_display(uint16_t port_id, struct 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: > + RTE_ETHDEV_LOG(ERR, "port index %d invalid\n", port_id); > + break; > + case -ENOTSUP: > + RTE_ETHDEV_LOG(ERR, "operation not supported by device\n"); > + break; > + case -EIO: > + RTE_ETHDEV_LOG(ERR, "device is removed\n"); > + break; > + default: > + RTE_ETHDEV_LOG(ERR, "Unable to get port %d EEPROM module info\n", ret); Above log message sounds like 'ret' contains port number. > + break; > + } > + return; > + } Close curly bracket is incorrectly aligned above. > + > + einfo.offset = 0; > + einfo.length = minfo.eeprom_len; > + einfo.data = calloc(1, minfo.eeprom_len); > + if (einfo.data == NULL) { > + RTE_ETHDEV_LOG(ERR, "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: > + RTE_ETHDEV_LOG(ERR, "port index %d invalid\n", port_id); > + break; > + case -ENOTSUP: > + RTE_ETHDEV_LOG(ERR, "operation not supported by device\n"); > + break; > + case -EIO: > + RTE_ETHDEV_LOG(ERR, "device is removed\n"); > + break; > + default: > + RTE_ETHDEV_LOG(ERR, "Unable to get port %d module EEPROM\n", ret); same here > + break; > + } > + free(einfo.data); > + return; > + } > + > + switch (minfo.type) { > + /* parsing module EEPROM data base on different module type */ > + default: > + RTE_ETHDEV_LOG(NOTICE, "Unsupported module type: %u\n", minfo.type); > + break; > + } > + > + free(einfo.data); > +} > + > +void > +add_item_string(struct 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)) { Can we have the same key in on of previous item? > + strlcat(items[sff_item_count - 1].value, "; ", SFF_ITEM_VALUE_SIZE); > + strlcat(items[sff_item_count - 1].value, value_str, SFF_ITEM_VALUE_SIZE); > + return; > + } > + > + snprintf(items[sff_item_count].name, SFF_ITEM_NAME_SIZE, "%s", name_str); > + snprintf(items[sff_item_count].value, SFF_ITEM_VALUE_SIZE, "%s", value_str); If you just want to copy string, strlcpy() should be used. > + 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; > + struct sff_item *items; > + sff_item_count = 0; > + > + if (params == NULL || strlen(params) == 0 || !isdigit(*params)) > + return -1; > + > + errno = 0; > + port_id = strtoul(params, &end_param, 0); > + > + if (errno != 0) { > + RTE_ETHDEV_LOG(ERR, "Invalid argument\n"); > + return -1; > + } > + > + if (*end_param != '\0') > + RTE_ETHDEV_LOG(NOTICE, > + "Extra parameters passed to ethdev telemetry command, ignoring"); missing \n, also it would be useful to log these extra parameters. > + > + items = calloc(1, sizeof(struct sff_item) * SFF_ITEM_MAX_COUNT); Since you use calloc, it should be: items = calloc(SFF_ITEM_MAX_COUNT, sizeof(struct sff_item)); > + if (items == NULL) { > + RTE_ETHDEV_LOG(ERR, "Error allocating memory of items\n"); > + return -1; > + } > + > + sff_port_module_eeprom_display(port_id, items); Function name sounds like it prints out the information to terminal etc, but as I understand it just fill in items array. > + > + 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); What I really don't understand is why do we need intermediate storage at all? Can we add to telemetry dictionary from the very beginning? The only argument to use intermediate storage is "append to the same key", but since we check the last element only, I guess we can handle it easily inside the code. > + > + 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..5788bd6e60 > --- /dev/null > +++ b/lib/ethdev/ethdev_sff_telemetry.h > @@ -0,0 +1,37 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2022 Intel Corporation > + */ > + > +#ifndef _ETHDEV_SFF_TELEMETRY_H_ > +#define _ETHDEV_SFF_TELEMETRY_H_ > + > +#include > + > +#define ARRAY_SIZE(arr) RTE_DIM(arr) What's the point to define it? You can use RTE_DIM() directly instead. > + > +#define SFF_ITEM_NAME_SIZE 64 > +#define SFF_ITEM_VALUE_SIZE 256 > +#define SFF_ITEM_MAX_COUNT 256 > +#define SFF_ITEM_VAL_COMPOSE_SIZE 64 Are above defines come from spec? If yes, please, add comments with references to spec. Or is it current implementation limitations? > + > +struct sff_item { > + char name[SFF_ITEM_NAME_SIZE]; /* The item name. */ > + char value[SFF_ITEM_VALUE_SIZE]; /* The item value. */ > +}; > + > +/* SFF-8079 Optics diagnostics */ > +void sff_8079_show_all(const uint8_t *data, struct sff_item *items); > + > +/* SFF-8472 Optics diagnostics */ > +void sff_8472_show_all(const uint8_t *data, struct sff_item *items); > + > +/* SFF-8636 Optics diagnostics */ > +void sff_8636_show_all(const uint8_t *data, uint32_t eeprom_len, struct 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(struct sff_item *items, const char *name_str, const char *value_str); The function name sounds too generic. Please, add sff_ prefix. May be just sff_add_item()? > + > +#endif /* _ETHDEV_SFF_TELEMETRY_H_ */ > diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build > index a094585bf7..49c77acb3f 100644 > --- a/lib/ethdev/meson.build > +++ b/lib/ethdev/meson.build > @@ -11,6 +11,7 @@ sources = files( > 'rte_flow.c', > 'rte_mtr.c', > 'rte_tm.c', > + 'ethdev_sff_telemetry.c', > ) > > headers = files( > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > index 29a3d80466..2b87df1b32 100644 > --- a/lib/ethdev/rte_ethdev.c > +++ b/lib/ethdev/rte_ethdev.c > @@ -39,6 +39,7 @@ > #include "ethdev_driver.h" > #include "ethdev_profile.h" > #include "ethdev_private.h" > +#include "ethdev_sff_telemetry.h" > > struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS]; > > @@ -5876,4 +5877,6 @@ RTE_INIT(ethdev_init_telemetry) > "Returns the link status for a port. Parameters: int port_id"); > rte_telemetry_register_cmd("/ethdev/info", eth_dev_handle_port_info, > "Returns the device info for a port. Parameters: int port_id"); > + rte_telemetry_register_cmd("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom, > + "Returns module EEPROM info with SFF specs. Parameters: int port_id"); > }