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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/26/2020 8:00 PM, David Liu wrote:
> Commands will dump the content of the EEPROM/module EEPROM for the
> selected port.
> 
> Signed-off-by: David Liu <dliu@iol.unh.edu>
> ---
>  app/test-pmd/cmdline.c |  41 +++++++++++++++
>  app/test-pmd/config.c  | 112 +++++++++++++++++++++++++++++++++++++++++
>  app/test-pmd/testpmd.h |   2 +
>  3 files changed, 155 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index a037a55c6..84943a889 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -7594,6 +7594,46 @@ cmdline_parse_inst_t cmd_showdevice = {
>  		NULL,
>  	},
>  };
> +
> +/* ** SHOW EEPROM INFO *** */
> +struct cmd_showeeprom_result {
> +       cmdline_fixed_string_t show;
> +       cmdline_fixed_string_t type;
> +        uint16_t portnum;
> +};
> +
> +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);
> +}
> +
> +cmdline_parse_token_string_t cmd_showeeprom_show =
> +       TOKEN_STRING_INITIALIZER(struct cmd_showeeprom_result, show, "show");
> +cmdline_parse_token_string_t cmd_showeeprom_type =
> +       TOKEN_STRING_INITIALIZER(struct cmd_showeeprom_result, type, "eeprom#module_eeprom");
> +cmdline_parse_token_num_t cmd_showeeprom_portnum =
> +       TOKEN_NUM_INITIALIZER(struct cmd_showeeprom_result, portnum, UINT16);
> +
> +cmdline_parse_inst_t cmd_showeeprom = {
> +       .f = cmd_showeeprom_parsed,
> +       .data = NULL,
> +       .help_str = "show eeprom|module_eeprom <port_id>",

There is more common syntax like:
"show port X <port_id>|all"
or
"show port <port_id> Y"

Keeping command syntax consistent helps using testpmd. What do you think
selecting one of above two?

Also can you please update 'cmd_help_long_parsed()' (in same file) and
documentation (testpmd_funcs.rst) for new command.

> +       .tokens = {
> +               (void *)&cmd_showeeprom_show,
> +               (void *)&cmd_showeeprom_type,
> +               (void *)&cmd_showeeprom_portnum,
> +               NULL,
> +       },
> +};
> +
> +
>  /* *** SHOW QUEUE INFO *** */
>  struct cmd_showqueue_result {
>  	cmdline_fixed_string_t show;
> @@ -19325,6 +19365,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 8cf84ccd3..79fd81a65 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -49,6 +49,7 @@
>  #include <rte_pmd_bnxt.h>
>  #endif
>  #include <rte_gro.h>
> +#include <rte_hexdump.h>
>  
>  #include "testpmd.h"
>  
> @@ -710,6 +711,117 @@ 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); 
> +       char buf[len_eeprom];

If 'len_eeprom' can be negative what will be the 'buf' size?

> +
> +       if (len_eeprom > 0) {

To reduce the code indentation, you can eliminate this if block by returning on
error cases. (like as done in below 'port_module_eeprom_display()')

> +               printf("\nPort: %d\nModule EEPROM:\n", port_id);
> +
> +               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 Iis removed\n");

there is a typo before 'is' (and in same messages below)

> +                               break;
> +                       default:
> +                               printf("Unable to get module EEPROM: %d\n", ret);
> +                               break;
> +                       }
> +                       return;
> +               }
> +
> +               rte_hexdump(stdout, "hexdump", einfo.data, einfo.length);
> +               printf("Finish -- Total EEPROM length: %i bytes\n", len_eeprom);
> +
> +       } else if (len_eeprom == 0)
> +               printf("Port %d: Device does not have EEPROM\n", port_id);
> +       else if (len_eeprom == -ENOTSUP)
> +               printf("Port %d: Operation not supported\n", port_id);
> +       else
> +               printf("Port %d: Error getting EEPROM\n", port_id);
> +}
> +
> +void
> +port_module_eeprom_display(portid_t port_id)
> +{
> +       struct rte_eth_dev_module_info minfo;
> +       struct rte_dev_eeprom_info einfo;
> +       char buf[1024];

Size can exceed the '1024'

> +       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 Iis removed\n");
> +                       break;
> +               default:
> +                       printf("Unable to get module info: %d\n", ret);
> +                       break;
> +               }
> +               return;
> +       }
> +
> +       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 Iis removed\n");
> +                       break;
> +               default:
> +                       printf("Unable to get module EEPROM: %d\n", ret);
> +                       break;
> +               }
> +               return;
> +       }
> +
> +       printf("\nPort: %d\nEEPROM:\n", port_id);
> +       rte_hexdump(stdout, "hexdump", einfo.data, 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 7a7c73f79..e3a0d17e5 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -714,6 +714,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);
>