DPDK patches and discussions
 help / color / mirror / Atom feed
From: David Liu <dliu@iol.unh.edu>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dpdk-dev <dev@dpdk.org>, Lincoln Lavoie <lylavoie@iol.unh.edu>,
	tmonjalon@nvidia.com
Subject: Re: [dpdk-dev] [PATCH V1] testpmd: add eeprom/module eeprom display
Date: Fri, 28 Aug 2020 09:46:40 -0400	[thread overview]
Message-ID: <CAAuqQpTX0mpog=s+bTX9vZz0xYpkvhumuz-7qM9s2-TfoZs-Yg@mail.gmail.com> (raw)
In-Reply-To: <9f48e895-c90d-7cce-7431-ba1a85944162@intel.com>

z

On Wed, Aug 26, 2020 at 6:46 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> 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.
>
> Sure, I can use that syntax above and update the documentation.


> > +       .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?
>
> Thank you for the catch. I will include a negative length error checking,

> > +
> > +       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);
> >
>
>

  reply	other threads:[~2020-08-28 13:47 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-26 19:00 David Liu
2020-08-26 22:46 ` Ferruh Yigit
2020-08-28 13:46   ` David Liu [this message]
2020-09-01 18:56 David Liu
2020-09-01 19:07 David Liu
2020-09-02 10:00 ` Ferruh Yigit
2020-09-03 16:40   ` David Liu
2020-09-03 18:40     ` Ferruh Yigit
2020-09-10  6:00 David Liu
2020-09-10 11:47 ` Ferruh Yigit
2020-09-10 18:48   ` David Liu
2020-09-10 20:00 ` David Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAuqQpTX0mpog=s+bTX9vZz0xYpkvhumuz-7qM9s2-TfoZs-Yg@mail.gmail.com' \
    --to=dliu@iol.unh.edu \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=lylavoie@iol.unh.edu \
    --cc=tmonjalon@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).