there are three minor comments, with these fixed,
Acked-by: Chengwen Feng <fengcheng...@huawei.com>
On 2024/9/14 9:49, Chaoyong He wrote:
> From: James Hershaw <james.hers...@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.hers...@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong...@corigine.com>
> ---
> app/test-pmd/cmdline.c | 55 +++++++++++++++++++++
> app/test-pmd/config.c | 19 +++++++
> app/test-pmd/testpmd.h | 1 +
> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 7 +++
> 4 files changed, 82 insertions(+)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 51c72b0826..9357abb036 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -523,6 +523,10 @@ static void cmd_help_long_parsed(void *parsed_result,
> " user is acknowledging and taking responsibility for"
> " this risk.\n\n"
>
> + "set port (port_id) led (on|off)\n"
> + " set a controllable LED associated with a certain\n"
No need to split up two line by \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"
> @@ -7609,6 +7613,56 @@ static cmdline_parse_inst_t cmd_seteeprom = {
> },
> };
>
> +/* *** 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 state;
> +};
> +
> +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 (strcmp(res->led, "led") != 0)
> + return;
No need to add such judgement.
because static cmdline_parse_token_string_t cmd_dev_led =
TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, led, "led");
already limit this parameter must be "led"
> +
> + if (strcmp(res->state, "on") == 0)
> + set_dev_led(res->port_id, true);
> + else
> + set_dev_led(res->port_id, false);
> +}
> +
> +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_state =
> + TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, state, "on#off");
> +
> +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_state,
> + NULL,
> + },
> +};
> +
> /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
> struct cmd_set_qmap_result {
> cmdline_fixed_string_t set;
> @@ -13348,6 +13402,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..ab9b99ef0c 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 %u\n", port_id);
> + 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
> %u\n", port_id);
It's better add reason by rte_strerror(-ret)
> +}
> +
> 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 f9dd380ea0..1c416686c5 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -1375,6 +1375,13 @@ Note that this is a high-risk command and its misuse
> may result in unexpected
> behaviour from the NIC. By inserting "confirm" into the command, the user is
> acknowledging and taking responsibility for this risk.
>
> +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
> ~~~~~~~~~~~~
>
