Re: [PATCH v3 2/2] app/testpmd: add set dev led on/off command

[fengchengwen <fengchengwen@huawei.com>]

there are three minor comments, with these fixed,
Acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2024/9/14 9:49, 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                      | 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
>  ~~~~~~~~~~~~
>