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 <[email protected]>
---
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 <errno.h>
+
+#include <rte_ethdev.h>
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 <rte_common.h>
+#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]
