RE: [PATCH v2 2/2] app/testpmd: add set dev led on/off command

[Chaoyong He <chaoyong.he@corigine.com>]

> On 2024/9/12 14:54, Chaoyong He wrote:
> > From: James Hershaw <james.hershaw@corigine.com>
> >
> > Add command to change the state of a controllable LED on an ethdev
> > port to on/off. This is for the purpose of identifying which physical
> > port is associated with an ethdev.
> >
> > usage:
> >   testpmd> set port <port-id> led <on/off>
> >
> > Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > ---
> >  app/test-pmd/cmdline.c                      | 61 +++++++++++++++++++++
> >  app/test-pmd/config.c                       | 19 +++++++
> >  app/test-pmd/testpmd.h                      |  1 +
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
> >  4 files changed, 88 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > ebc5a9a361..3898c125c2 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -518,6 +518,10 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> >                       " value (value) offset (offset)/n"
> >                       "    Set the device eeprom for certain port.\n\n"
> >
> > +                     "set port (port_id) led (on|off)\n"
> > +                     "    set a controllable LED associated with a certain\n"
> > +                     "    port on or off.\n\n"
> > +
> >                       "set port (port_id) uta (mac_address|all) (on|off)\n"
> >                       "    Add/Remove a or all unicast hash filter(s)"
> >                       "from port X.\n\n"
> > @@ -7589,6 +7593,62 @@ static cmdline_parse_inst_t cmd_seteeprom = {
> >               (void *)&cmd_seteeprom_magic,
> >               (void *)&cmd_seteeprom_value,
> >               (void *)&cmd_seteeprom_token_str,
> > +                             NULL,
> > +     },
> > +};
> > +
> > +/* *** SET THE LED FOR CERTAIN PORT TO ON/OFF *** */ struct
> > +cmd_dev_led_result {
> > +     cmdline_fixed_string_t set;
> > +     cmdline_fixed_string_t port;
> > +     portid_t port_id;
> > +     cmdline_fixed_string_t led;
> > +     cmdline_fixed_string_t led_state;
> 
> led_state -> state, led_ is redundant.
> 
> > +};
> > +
> > +static void cmd_set_dev_led_parsed(void *parsed_result,
> > +                     __rte_unused struct cmdline *cl,
> > +                     __rte_unused void *data) {
> > +             struct cmd_dev_led_result *res = parsed_result;
> > +
> > +             if (test_done == 0) {
> > +                     fprintf(stderr, "Please stop forwarding first\n");
> > +                     return;
> > +             }
> 
> From my understanding, there is a priority:
> 1\ if led on, then both led are on
> 2\ if led off, then led was lighted by current link and active flow.
> 
> So there is no need to stop active flow when execute this command.
> 
> > +
> > +             if (strcmp(res->led, "led") != 0)
> > +                     return;
> > +
> > +             if (strcmp(res->led_state, "on") == 0)
> > +                     set_dev_led(res->port_id, true);
> > +             else if (strcmp(res->led_state, "off") == 0)
> > +                     set_dev_led(res->port_id, false);
> > +             else
> > +                     fprintf(stderr, "Invalid option for LED
> > +state\n"); }
> > +
> > +static cmdline_parse_token_string_t cmd_dev_led_set =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, set, "set");
> > +static cmdline_parse_token_string_t cmd_dev_led_port =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, port,
> > +"port"); static cmdline_parse_token_num_t cmd_dev_led_port_id =
> > +     TOKEN_NUM_INITIALIZER(struct cmd_dev_led_result, port_id,
> > +RTE_UINT16); static cmdline_parse_token_string_t cmd_dev_led =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, led, "led");
> > +static cmdline_parse_token_string_t cmd_dev_led_state =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, led_state,
> > +NULL);
> 
> Suggest use tool parse capa, use TOKEN_STRING_INITIALIZER(struct
> cmd_dev_led_result, led_state, "on|off");
> 
> then could only judge whether is on, like:
> 
>                 if (strcmp(res->led_state, "on") == 0)
>                         set_dev_led(res->port_id, true);
>                 else
>                         set_dev_led(res->port_id, false);
> 
> 
> > +
> > +static cmdline_parse_inst_t cmd_set_dev_led = {
> > +     .f = cmd_set_dev_led_parsed,
> > +     .data = NULL,
> > +     .help_str = "set port <port_id> led <on/off>",
> > +     .tokens = {
> > +             (void *)&cmd_dev_led_set,
> > +             (void *)&cmd_dev_led_port,
> > +             (void *)&cmd_dev_led_port_id,
> > +             (void *)&cmd_dev_led,
> > +             (void *)&cmd_dev_led_state,
> >               NULL,
> >       },
> >  };
> > @@ -13332,6 +13392,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
> >       &cmd_stop,
> >       &cmd_mac_addr,
> >       &cmd_set_fwd_eth_peer,
> > +     &cmd_set_dev_led,
> >       &cmd_set_qmap,
> >       &cmd_set_xstats_hide_zero,
> >       &cmd_set_record_core_cycles,
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > fb1a1485e2..9673f9f207 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -5385,6 +5385,25 @@ set_fwd_eth_peer(portid_t port_id, char
> *peer_addr)
> >       peer_eth_addrs[port_id] = new_peer_addr;  }
> >
> > +void
> > +set_dev_led(portid_t port_id, bool active) {
> > +     int ret;
> > +
> > +     if (!rte_eth_dev_is_valid_port(port_id)) {
> > +             fprintf(stderr, "Error: Invalid port number %i\n",
> > + port_id);
> 
> %i -> %u
> 
> > +             return;
> > +     }
> > +
> > +     if (active)
> > +             ret = rte_eth_led_on(port_id);
> > +     else
> > +             ret = rte_eth_led_off(port_id);
> > +
> > +     if (ret < 0)
> > +             fprintf(stderr, "Error: Unable to change LED state for
> > + port %i\n", port_id);
> 
> %i -> %u
> 
> > +}
> > +
> >  int
> >  set_fwd_lcores_list(unsigned int *lcorelist, unsigned int nb_lc)  {
> > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> > 1292d1a0a7..aa1f0e9dd1 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -944,6 +944,7 @@ int init_fwd_streams(void);  void
> > update_fwd_ports(portid_t new_pid);
> >
> >  void set_fwd_eth_peer(portid_t port_id, char *peer_addr);
> > +void set_dev_led(portid_t port_id, bool active);
> >
> >  void port_mtu_set(portid_t port_id, uint16_t mtu);  int
> > port_action_handle_create(portid_t port_id, uint32_t id, bool
> > indirect_list, diff --git
> > a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index 3aa214ef9f..c033b07aa7 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -1370,6 +1370,13 @@ Value should be given in the form of a hex-as-
> string, with no leading ``0x``.
> >  The offset field here is optional, if not specified then the offset
> > will default  to 0.
> >
> > +set port led
> > +~~~~~~~~~~~~
> > +
> > +Set a controllable LED associated with a certain port on or off::
> > +
> > +   testpmd> set port (port_id) led (on|off)
> > +
> >  set port-uta
> >  ~~~~~~~~~~~~
> >

Thanks for review, we'll send a new version patch and do as your advice.