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

[fengchengwen <fengchengwen@huawei.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
>  ~~~~~~~~~~~~
>