Re: [PATCH v5 1/5] ethdev: add telemetry command for module EEPROM

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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 <robinx.zhang@intel.com>

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 <errno.h>
> +
> +#include <rte_ethdev.h>
> +#include <rte_common.h>
> +#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 <rte_telemetry.h>
> +
> +#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");
>   }