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 69200A054F; Wed, 25 May 2022 11:24:11 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0A533400EF; Wed, 25 May 2022 11:24:11 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 225EC400D6 for ; Wed, 25 May 2022 11:24:09 +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 (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 71BDE93; Wed, 25 May 2022 12:24:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 71BDE93 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1653470648; bh=vYx2+/GdOLwo/0gtUiaaTqjsiW9fS/W5AuK5PeUSp4c=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=SsgAnlfx5RXInxDrOE7M2cOGwD3YbPkzoar8uTKEyoMQt6ayEBOB9XsyC7ddP96ty 7363OHPmCAL8FLrjw7s2ny7oeNN1aA7iB4U2J2UVW5B0sZe8vGY0B51Ke9mqQ7TLzU ePjmRcuZFG3aHYR4O3NrCCDIFMJ2L6eG53f/+7UY= Message-ID: <8484a4d5-598b-b17b-fc90-9c76ab044400@oktetlabs.ru> Date: Wed, 25 May 2022 12:24:07 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH v8 1/5] ethdev: add telemetry command for module EEPROM Content-Language: en-US To: Robin Zhang , dev@dpdk.org Cc: thomas@monjalon.net, kevinx.liu@intel.com References: <20220524062442.194809-1-robinx.zhang@intel.com> <20220525031446.72578-1-robinx.zhang@intel.com> <20220525031446.72578-2-robinx.zhang@intel.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220525031446.72578-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 5/25/22 06:14, 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. Please, add SFF to devtools/words-case.txt > > Signed-off-by: Robin Zhang > --- > lib/ethdev/ethdev_sff_telemetry.c | 138 ++++++++++++++++++++++++++++++ > lib/ethdev/ethdev_sff_telemetry.h | 27 ++++++ I think we should be consistent with naming. Other patches name added files as sff_*.[ch]. I see no strong reason to have ethdev_ prefix here. sff_ prefix should be sufficient. > lib/ethdev/meson.build | 1 + > lib/ethdev/rte_ethdev.c | 3 + > 4 files changed, 169 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..f756b9643f > --- /dev/null > +++ b/lib/ethdev/ethdev_sff_telemetry.c > @@ -0,0 +1,138 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2022 Intel Corporation > + */ > + > +#include > + > +#include Other C files in ethdev use double-quotes to include headers provided by the library itself. Also it should go after headers provided by other DPDK libraries. > +#include > +#include "ethdev_sff_telemetry.h" > +#include "telemetry_data.h" Why are double quotes used for the include? It is a header from other DPDK library similar to rte_common.h. > + > +static void > +sff_port_module_eeprom_parse(uint16_t port_id, struct rte_tel_data *d) > +{ > + struct rte_eth_dev_module_info minfo; > + struct rte_dev_eeprom_info einfo; > + int ret; > + > + if (d == NULL) { > + RTE_ETHDEV_LOG(ERR, "Dict invalid\n"); > + return; > + } > + > + 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); The majority of ethdev logs start from capital letter. Please, be consistent. > + break; > + case -ENOTSUP: > + RTE_ETHDEV_LOG(ERR, "operation not supported by device\n"); same here > + break; > + case -EIO: > + RTE_ETHDEV_LOG(ERR, "device is removed\n"); same here > + break; > + default: > + RTE_ETHDEV_LOG(ERR, "Unable to get port module info, %d\n", ret); > + break; > + } > + return; > + } > + > + 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); Please, be consistent in logging: eeprom -> EEPROM as below > + errno = 0; > + port_id = strtoul(params, &end_param, 0); > + > + if (errno != 0) { > + RTE_ETHDEV_LOG(ERR, "Invalid argument\n"); Please, log the invalid argument (params). > + return -1; > + } > + > + if (*end_param != '\0') > + RTE_ETHDEV_LOG(NOTICE, > + "Extra parameters passed to ethdev telemetry command, ignoring\n"); I think it would be very useful to log these extra parameters. > + > + rte_tel_data_start_dict(d); > + > + sff_port_module_eeprom_parse(port_id, d); > + > + return 0; > +} [snip]