Re: [dpdk-dev] [PATCH] [PATCH V2] testpmd: add eeprom/module eeprom display

[Phil Yang <Phil.Yang@arm.com>]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of David Liu
> Sent: Wednesday, September 16, 2020 1:28 AM
> To: ferruh.yigit@intel.com
> Cc: dev@dpdk.org; lylavoie@iol.unh.edu; thomas@monjalon.net; David Liu
> <dliu@iol.unh.edu>
> Subject: [dpdk-dev] [PATCH] [PATCH V2] testpmd: add eeprom/module
> eeprom display
> 
> Add module EEPROM/EEPROM dump command
>    "show port <port_id> (module_eeprom|eeprom)"
> Commands will dump the content of the EEPROM/module
> EEPROM for the selected port.


Verified with some popular NICs, it can dump the same info as the Linux ethtool does.
This is supported in examples/ethtool as well.

Minor nitpick comments inlined.

Thanks,
Phil

> 
> Signed-off-by: David Liu <dliu@iol.unh.edu>
> ---
>  app/test-pmd/cmdline.c                      |  49 ++++++++
>  app/test-pmd/config.c                       | 118 ++++++++++++++++++++
>  app/test-pmd/testpmd.h                      |   2 +
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |   7 ++
>  4 files changed, 176 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0a6ed85f3..8d38de197 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -166,6 +166,9 @@ static void cmd_help_long_parsed(void
> *parsed_result,
>  			"show port
> (info|stats|summary|xstats|fdir|stat_qmap|dcb_tc|cap) (port_id|all)\n"
>  			"    Display information for port_id, or all.\n\n"
> 
> +			"show port port_id (module_eeprom|eeprom)\n"
> +			"    Display the module EEPROM or EEPROM
> information for port_id.\n\n"
> +
>  			"show port X rss reta (size) (mask0,mask1,...)\n"
>  			"    Display the rss redirection table entry indicated"
>  			" by masks on port X. size is used to indicate the"
> @@ -7682,6 +7685,51 @@ cmdline_parse_inst_t cmd_showdevice = {
>  		NULL,
>  	},
>  };
> +
> +/* *** SHOW MODULE EEPROM/EEPROM port INFO *** */
> +struct cmd_showeeprom_result {
> +	cmdline_fixed_string_t show;
> +	cmdline_fixed_string_t port;
> +	uint16_t portnum;
> +	cmdline_fixed_string_t type;
> +};
> +
> +static void cmd_showeeprom_parsed(void *parsed_result,
> +		__rte_unused struct cmdline *cl,
> +		__rte_unused void *data)
> +{
> +	struct cmd_showeeprom_result *res = parsed_result;
> +
> +	if (!strcmp(res->type, "eeprom"))
> +		port_eeprom_display(res->portnum);
> +	else if (!strcmp(res->type, "module_eeprom"))
> +		port_module_eeprom_display(res->portnum);
> +	else
> +		printf("Unknown argument\n");
> +}
> +
> +cmdline_parse_token_string_t cmd_showeeprom_show =
> +	TOKEN_STRING_INITIALIZER(struct cmd_showeeprom_result, show,
> "show");
> +cmdline_parse_token_string_t cmd_showeeprom_port =
> +	TOKEN_STRING_INITIALIZER(struct cmd_showeeprom_result, port,
> "port");
> +cmdline_parse_token_num_t cmd_showeeprom_portnum =
> +	TOKEN_NUM_INITIALIZER(struct cmd_showeeprom_result,
> portnum, UINT16);
> +cmdline_parse_token_string_t cmd_showeeprom_type =
> +	TOKEN_STRING_INITIALIZER(struct cmd_showeeprom_result, type,
> "module_eeprom#eeprom");
> +
> +cmdline_parse_inst_t cmd_showeeprom = {
> +	.f = cmd_showeeprom_parsed,
> +	.data = NULL,
> +	.help_str = "show port <port_id> module_eeprom|eeprom",
> +	.tokens = {
> +		(void *)&cmd_showeeprom_show,
> +		(void *)&cmd_showeeprom_port,
> +		(void *)&cmd_showeeprom_portnum,
> +		(void *)&cmd_showeeprom_type,
> +		NULL,
> +	},
> +};
> +
>  /* *** SHOW QUEUE INFO *** */
>  struct cmd_showqueue_result {
>  	cmdline_fixed_string_t show;
> @@ -19400,6 +19448,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_load_from_file,
>  	(cmdline_parse_inst_t *)&cmd_showport,
>  	(cmdline_parse_inst_t *)&cmd_showqueue,
> +	(cmdline_parse_inst_t *)&cmd_showeeprom,
>  	(cmdline_parse_inst_t *)&cmd_showportall,
>  	(cmdline_parse_inst_t *)&cmd_showdevice,
>  	(cmdline_parse_inst_t *)&cmd_showcfg,
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 30bee3324..6203d27e7 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -50,6 +50,8 @@
>  #endif
>  #include <rte_gro.h>
> 

Remove the blank line above the rte_hexdump.h include statement.

> +#include <rte_hexdump.h>
> +
>  #include "testpmd.h"
> 
>  #define ETHDEV_FWVERS_LEN 32
> @@ -783,6 +785,122 @@ port_summary_display(portid_t port_id)
>  		(unsigned int) link.link_speed);
>  }
> 
> +void
> +port_eeprom_display(portid_t port_id)
> +{
> +	struct rte_dev_eeprom_info einfo;
> +	int ret;
> +	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
> +		print_valid_ports();
> +		return;
> +	}
> +
> +	int len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
> +	if (len_eeprom < 0) {
> +		switch (len_eeprom) {
> +		case -ENODEV:
> +			printf("port index %d invalid\n", port_id);
> +			break;
> +		case -ENOTSUP:
> +			printf("operation not supported by device\n");
> +			break;
> +		case -EIO:
> +			printf("device is removed\n");
> +			break;
> +		default:
> +			printf("Unable to get EEPROM: %d\n", len_eeprom);
> +			break;
> +		}
> +		return;
> +	}
> +
> +	char buf[len_eeprom];


Is GCC-11 happy about this array parameters? [-Warray-parameter]
I didn't check.



> +	einfo.offset = 0;
> +	einfo.length = len_eeprom;
> +	einfo.data = buf;
> +
> +	ret = rte_eth_dev_get_eeprom(port_id, &einfo);
> +	if (ret != 0) {
> +		switch (ret) {
> +		case -ENODEV:
> +			printf("port index %d invalid\n", port_id);
> +			break;
> +		case -ENOTSUP:
> +			printf("operation not supported by device\n");
> +			break;
> +		case -EIO:
> +			printf("device is removed\n");
> +			break;
> +		default:
> +			printf("Unable to get EEPROM: %d\n", ret);
> +			break;
> +		}
> +		return;
> +	}
> +	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
> +	printf("Finish -- Port: %d EEPROM length: %d bytes\n", port_id,
> len_eeprom);
> +}
> +
> +void
> +port_module_eeprom_display(portid_t port_id)
> +{
> +	struct rte_eth_dev_module_info minfo;
> +	struct rte_dev_eeprom_info einfo;
> +	int ret;
> +
> +	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
> +		print_valid_ports();
> +		return;
> +	}
> +
> +
> +	ret = rte_eth_dev_get_module_info(port_id, &minfo);
> +	if (ret != 0) {
> +		switch (ret) {
> +		case -ENODEV:
> +			printf("port index %d invalid\n", port_id);
> +			break;
> +		case -ENOTSUP:
> +			printf("operation not supported by device\n");
> +			break;
> +		case -EIO:
> +			printf("device is removed\n");
> +			break;
> +		default:
> +			printf("Unable to get module EEPROM: %d\n", ret);
> +			break;
> +		}
> +		return;
> +	}
> +
> +	char buf[minfo.eeprom_len];

Is GCC-11 happy about this array parameters? [-Warray-parameter]
I didn't check.



> +	einfo.offset = 0;
> +	einfo.length = minfo.eeprom_len;
> +	einfo.data = buf;
> +
> +	ret = rte_eth_dev_get_module_eeprom(port_id, &einfo);
> +	if (ret != 0) {
> +		switch (ret) {
> +		case -ENODEV:
> +			printf("port index %d invalid\n", port_id);
> +			break;
> +		case -ENOTSUP:
> +			printf("operation not supported by device\n");
> +			break;
> +		case -EIO:
> +			printf("device is removed\n");
> +			break;
> +		default:
> +			printf("Unable to get module EEPROM: %d\n", ret);
> +			break;
> +		}
> +		return;
> +	}
> +
> +	rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
> +	printf("Finish -- Port: %d MODULE EEPROM length: %d bytes\n",
> port_id, einfo.length);
> +}
> +
>  void
>  port_offload_cap_display(portid_t port_id)
>  {
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 25a12b14f..40225bc64 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -697,6 +697,8 @@ void nic_stats_mapping_display(portid_t port_id);
>  void device_infos_display(const char *identifier);
>  void port_infos_display(portid_t port_id);
>  void port_summary_display(portid_t port_id);
> +void port_eeprom_display(portid_t port_id);
> +void port_module_eeprom_display(portid_t port_id);
>  void port_summary_header_display(void);
>  void port_offload_cap_display(portid_t port_id);
>  void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 90cf252df..fba8428b7 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -218,6 +218,13 @@ For example:
>       nvgre
>       vxlan-gpe
> 
> +show port (module_eeprom|eeprom)
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Display the module EEPROM/EEPROM informatioon for port_id::
> +
> +   testpmd> show port (port_id) (module_eeprom|eeprom)
> +
>  show port rss reta
>  ~~~~~~~~~~~~~~~~~~
> 
> --
> 2.17.1