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

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

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 <robinx.zhang@intel.com>
> ---
>   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 <rte_ethdev.h>
> +#include <rte_common.h>
> +#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 <rte_ethdev.h>

It looks like the include is not actually used in the
header.

> +#include <rte_telemetry.h>
> +
> +#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]