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

[David Liu <dliu@iol.unh.edu>]

On Thu, Sep 10, 2020 at 7:47 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 9/10/2020 7:00 AM, David Liu wrote:
> > 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.
>
>
> Hi David,
>
> When sending a new version, can you please increase the verstion tag in the
> title, V1 -> V2 -> ... -> vN
>
> Also sending new patch as reply to previos one keep all versions in same
> email
> thread and helps reviewers also people who later checks from archives for
> a feature.
>
> For both above you can find more details in the contribution guide, please
> check
> https://doc.dpdk.org/guides/contributing/patches.html#sending-patches
>
>
> And there are a few errors below that this patch shouldn't be compiling
> successfully, you can verify the build error from lab reports :)
>

Thank you so much, I will change to that format.


> <...>
>
> >  /* *** SHOW QUEUE INFO *** */
> >  struct cmd_showqueue_result {
> >       cmdline_fixed_string_t show;
> > @@ -19325,6 +19373,8 @@ 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_showmoduleeeprom,
>
> This shouldn't compile because 'cmd_showmoduleeeprom' no more exists ...
>
> <...>
>
> > +
> > +void
> > +port_module_eeprom_displao(portid_t port_id)
> > +{
>
> There is a typo in the function name.
>
> > +     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",
> len_eeprom);
>
> I guess this is copy/paste error 'len_eeprom' is not defined in this
> function.
> There is one more occurance below.
>
>
I will send out a new patch with all the fixes and that can be compiled
hopefully.

<...>
>