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

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

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