[PATCH 0/2] add two commands for testpmd application

[PATCH 0/2] add two commands for testpmd application

[Chaoyong He]

This patch series aims to add two commands for the testpmd application:
- set port <port-id> eeprom magic <magic> value <value>
  offset <offset>
- set port <port-id> led <on/off>

James Hershaw (2):
  app/testpmd: add support for setting device EEPROM
  app/testpmd: add set dev led on/off command

 app/test-pmd/cmdline.c                      | 167 ++++++++++++++++++++
 app/test-pmd/config.c                       |  85 ++++++++++
 app/test-pmd/testpmd.h                      |   3 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  20 +++
 4 files changed, 275 insertions(+)

-- 
2.39.1

[PATCH 1/2] app/testpmd: add support for setting device EEPROM

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

There is currently no means to test the .set_eeprom function callback of
a given PMD in drivers/net/. This patch adds functionality to allow a
user to set device eeprom from the testpmd cmdline.

usage:
  testpmd> set port <port-id> eeprom magic <magic> value <value>
  offset <offset>

- <magic> is a decimal
- <value> is a hex-as-string with no leading "0x"
- <offset> is a decimal (this field is optional and defaults to 0 if
  not specified.)

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 app/test-pmd/cmdline.c                      | 106 ++++++++++++++++++++
 app/test-pmd/config.c                       |  66 ++++++++++++
 app/test-pmd/testpmd.h                      |   2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  11 ++
 4 files changed, 185 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 358319c20a..ebc5a9a361 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -514,6 +514,10 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set eth-peer (port_id) (peer_addr)\n"
 			"    set the peer address for certain port.\n\n"
 
+			"set port (port_id) eeprom magic (magic_num)\n"
+			" value (value) offset (offset)/n"
+			"    Set the device eeprom for certain port.\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"
@@ -7488,6 +7492,107 @@ static cmdline_parse_inst_t cmd_set_fwd_eth_peer = {
 	},
 };
 
+/* *** SET PORT EEPROM *** */
+struct cmd_seteeprom_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t port;
+	uint16_t portnum;
+	cmdline_fixed_string_t eeprom;
+	cmdline_fixed_string_t magic_str;
+	uint32_t magic;
+	cmdline_fixed_string_t value;
+	cmdline_multi_string_t token_str;
+};
+
+static void cmd_seteeprom_parsed(void *parsed_result,
+		__rte_unused struct cmdline *cl,
+		__rte_unused void *data)
+{
+	char *token;
+	char *token_str;
+	uint32_t length;
+	uint32_t offset = 0;
+	struct cmd_seteeprom_result *res = parsed_result;
+
+	token_str = res->token_str;
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+
+	/* Parse Hex string to Byte data */
+	if (strlen(token) % 2 != 0) {
+		fprintf(stderr, "Bad Argument: %s\n", token);
+		return;
+	}
+
+	length = strlen(token)/2;
+	uint8_t value[length];
+	for (int count = 0; count < (int)(length); count++) {
+		if (sscanf(token, "%2hhx", &value[count]) != 1) {
+			fprintf(stderr, "Bad Argument: %s\n", token);
+			return;
+		}
+		token += 2;
+	}
+
+	/* Second token: offset string */
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+	if (token != NULL) {
+		if (strcmp(token, "offset") == 0) {
+			/* Third token: offset */
+			char *end;
+			token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+			if (token == NULL) {
+				fprintf(stderr, "No offset specified\n");
+				return;
+			}
+			offset = strtoul(token, &end, 10);
+
+			if (offset == 0 && strcmp(end, "") != 0) {
+				fprintf(stderr, "Bad Argument: %s\n", token);
+				return;
+			}
+		} else {
+			fprintf(stderr, "Bad Argument: %s\n", token);
+			return;
+		}
+	}
+
+	port_eeprom_set(res->portnum, res->magic, offset, length, value);
+}
+
+static cmdline_parse_token_string_t cmd_seteeprom_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, set, "set");
+static cmdline_parse_token_string_t cmd_seteeprom_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, port, "port");
+static cmdline_parse_token_num_t cmd_seteeprom_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, portnum, RTE_UINT16);
+static cmdline_parse_token_string_t cmd_seteeprom_eeprom =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, eeprom, "eeprom");
+static cmdline_parse_token_string_t cmd_seteeprom_magic_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, magic_str, "magic");
+static cmdline_parse_token_num_t cmd_seteeprom_magic =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, magic, RTE_UINT32);
+static cmdline_parse_token_string_t cmd_seteeprom_value =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, value, "value");
+static cmdline_parse_token_string_t cmd_seteeprom_token_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, token_str, TOKEN_STRING_MULTI);
+
+static cmdline_parse_inst_t cmd_seteeprom = {
+	.f = cmd_seteeprom_parsed,
+	.data = NULL,
+	.help_str = "set port <port_id> eeprom magic <magic_num> value <value> offset <offset>",
+	.tokens = {
+		(void *)&cmd_seteeprom_set,
+		(void *)&cmd_seteeprom_port,
+		(void *)&cmd_seteeprom_portnum,
+		(void *)&cmd_seteeprom_eeprom,
+		(void *)&cmd_seteeprom_magic_str,
+		(void *)&cmd_seteeprom_magic,
+		(void *)&cmd_seteeprom_value,
+		(void *)&cmd_seteeprom_token_str,
+		NULL,
+	},
+};
+
 /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
 struct cmd_set_qmap_result {
 	cmdline_fixed_string_t set;
@@ -13153,6 +13258,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	&cmd_showport,
 	&cmd_showqueue,
 	&cmd_showeeprom,
+	&cmd_seteeprom,
 	&cmd_showportall,
 	&cmd_representor_info,
 	&cmd_showdevice,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 6f0beafa27..fb1a1485e2 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1063,6 +1063,72 @@ port_eeprom_display(portid_t port_id)
 	free(einfo.data);
 }
 
+void
+port_eeprom_set(portid_t port_id,
+		uint32_t magic,
+		uint32_t offset,
+		uint32_t length,
+		uint8_t *value)
+{
+	int ret;
+	int len_eeprom;
+	struct rte_dev_eeprom_info einfo;
+
+	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
+		print_valid_ports();
+		return;
+	}
+
+	len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
+	if (len_eeprom < 0) {
+		switch (len_eeprom) {
+		case -ENODEV:
+			fprintf(stderr, "port index %d invalid\n", port_id);
+			break;
+		case -ENOTSUP:
+			fprintf(stderr, "operation not supported by device\n");
+			break;
+		case -EIO:
+			fprintf(stderr, "device is removed\n");
+			break;
+		default:
+			fprintf(stderr, "Unable to get EEPROM: %d\n",
+				len_eeprom);
+			break;
+		}
+		return;
+	}
+
+	einfo.data = value;
+	einfo.magic = magic;
+	einfo.length = length;
+	einfo.offset = offset;
+
+	if (einfo.offset + einfo.length > (uint32_t)(len_eeprom)) {
+		fprintf(stderr, "offset and length exceed capabilities of EEPROM length: %d\n",
+			len_eeprom);
+		return;
+	}
+
+	ret = rte_eth_dev_set_eeprom(port_id, &einfo);
+	if (ret != 0) {
+		switch (ret) {
+		case -ENODEV:
+			fprintf(stderr, "port index %d invalid\n", port_id);
+			break;
+		case -ENOTSUP:
+			fprintf(stderr, "operation not supported by device\n");
+			break;
+		case -EIO:
+			fprintf(stderr, "device is removed\n");
+			break;
+		default:
+			fprintf(stderr, "Unable to get EEPROM: %d\n", ret);
+			break;
+		}
+	}
+}
+
 void
 port_module_eeprom_display(portid_t port_id)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 9facd7f281..1292d1a0a7 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -927,6 +927,8 @@ void device_infos_display(const char *identifier);
 void port_infos_display(portid_t port_id);
 void port_summary_display(portid_t port_id);
 void port_eeprom_display(portid_t port_id);
+void port_eeprom_set(portid_t port_id, uint32_t magic, uint32_t offset,
+		     uint32_t length, uint8_t *value);
 void port_module_eeprom_display(portid_t port_id);
 void port_summary_header_display(void);
 void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f00ab07605..3aa214ef9f 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1359,6 +1359,17 @@ Set the forwarding peer address for certain port::
 
 This is equivalent to the ``--eth-peer`` command-line option.
 
+set eeprom
+~~~~~~~~~~
+
+Write a value to the device eeprom of a port at a specific offset::
+
+   testpmd> set port (port_id) eeprom magic (magic_num) value (value) offset (offset)
+
+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-uta
 ~~~~~~~~~~~~
 
-- 
2.39.1

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

[Chaoyong He]

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 |  9 +++
 4 files changed, 90 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;
+};
+
+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;
+		}
+
+		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);
+
+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);
+		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);
+}
+
 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..1a360d9bab 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1359,6 +1359,7 @@ Set the forwarding peer address for certain port::
 
 This is equivalent to the ``--eth-peer`` command-line option.
 
+<<<<<<< HEAD
 set eeprom
 ~~~~~~~~~~
 
@@ -1369,6 +1370,14 @@ Write a value to the device eeprom of a port at a specific offset::
 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)
+>>>>>>> 38280ecd32 (app/testpmd: add set dev led on/off command)
 
 set port-uta
 ~~~~~~~~~~~~
-- 
2.39.1

[PATCH v2 0/2] add two commands for testpmd application

[Chaoyong He]

This patch series aims to add two commands for the testpmd application:
- set port <port-id> eeprom magic <magic> value <value>
  offset <offset>
- set port <port-id> led <on/off>

---
v2:
* Solve the conflict in document file.
---

James Hershaw (2):
  app/testpmd: add support for setting device EEPROM
  app/testpmd: add set dev led on/off command

 app/test-pmd/cmdline.c                      | 167 ++++++++++++++++++++
 app/test-pmd/config.c                       |  85 ++++++++++
 app/test-pmd/testpmd.h                      |   3 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  18 +++
 4 files changed, 273 insertions(+)

-- 
2.39.1

[PATCH v2 1/2] app/testpmd: add support for setting device EEPROM

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

There is currently no means to test the .set_eeprom function callback of
a given PMD in drivers/net/. This patch adds functionality to allow a
user to set device eeprom from the testpmd cmdline.

usage:
  testpmd> set port <port-id> eeprom magic <magic> value <value>
  offset <offset>

- <magic> is a decimal
- <value> is a hex-as-string with no leading "0x"
- <offset> is a decimal (this field is optional and defaults to 0 if
  not specified.)

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 app/test-pmd/cmdline.c                      | 106 ++++++++++++++++++++
 app/test-pmd/config.c                       |  66 ++++++++++++
 app/test-pmd/testpmd.h                      |   2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  11 ++
 4 files changed, 185 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 358319c20a..ebc5a9a361 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -514,6 +514,10 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set eth-peer (port_id) (peer_addr)\n"
 			"    set the peer address for certain port.\n\n"
 
+			"set port (port_id) eeprom magic (magic_num)\n"
+			" value (value) offset (offset)/n"
+			"    Set the device eeprom for certain port.\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"
@@ -7488,6 +7492,107 @@ static cmdline_parse_inst_t cmd_set_fwd_eth_peer = {
 	},
 };
 
+/* *** SET PORT EEPROM *** */
+struct cmd_seteeprom_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t port;
+	uint16_t portnum;
+	cmdline_fixed_string_t eeprom;
+	cmdline_fixed_string_t magic_str;
+	uint32_t magic;
+	cmdline_fixed_string_t value;
+	cmdline_multi_string_t token_str;
+};
+
+static void cmd_seteeprom_parsed(void *parsed_result,
+		__rte_unused struct cmdline *cl,
+		__rte_unused void *data)
+{
+	char *token;
+	char *token_str;
+	uint32_t length;
+	uint32_t offset = 0;
+	struct cmd_seteeprom_result *res = parsed_result;
+
+	token_str = res->token_str;
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+
+	/* Parse Hex string to Byte data */
+	if (strlen(token) % 2 != 0) {
+		fprintf(stderr, "Bad Argument: %s\n", token);
+		return;
+	}
+
+	length = strlen(token)/2;
+	uint8_t value[length];
+	for (int count = 0; count < (int)(length); count++) {
+		if (sscanf(token, "%2hhx", &value[count]) != 1) {
+			fprintf(stderr, "Bad Argument: %s\n", token);
+			return;
+		}
+		token += 2;
+	}
+
+	/* Second token: offset string */
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+	if (token != NULL) {
+		if (strcmp(token, "offset") == 0) {
+			/* Third token: offset */
+			char *end;
+			token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+			if (token == NULL) {
+				fprintf(stderr, "No offset specified\n");
+				return;
+			}
+			offset = strtoul(token, &end, 10);
+
+			if (offset == 0 && strcmp(end, "") != 0) {
+				fprintf(stderr, "Bad Argument: %s\n", token);
+				return;
+			}
+		} else {
+			fprintf(stderr, "Bad Argument: %s\n", token);
+			return;
+		}
+	}
+
+	port_eeprom_set(res->portnum, res->magic, offset, length, value);
+}
+
+static cmdline_parse_token_string_t cmd_seteeprom_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, set, "set");
+static cmdline_parse_token_string_t cmd_seteeprom_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, port, "port");
+static cmdline_parse_token_num_t cmd_seteeprom_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, portnum, RTE_UINT16);
+static cmdline_parse_token_string_t cmd_seteeprom_eeprom =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, eeprom, "eeprom");
+static cmdline_parse_token_string_t cmd_seteeprom_magic_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, magic_str, "magic");
+static cmdline_parse_token_num_t cmd_seteeprom_magic =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, magic, RTE_UINT32);
+static cmdline_parse_token_string_t cmd_seteeprom_value =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, value, "value");
+static cmdline_parse_token_string_t cmd_seteeprom_token_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, token_str, TOKEN_STRING_MULTI);
+
+static cmdline_parse_inst_t cmd_seteeprom = {
+	.f = cmd_seteeprom_parsed,
+	.data = NULL,
+	.help_str = "set port <port_id> eeprom magic <magic_num> value <value> offset <offset>",
+	.tokens = {
+		(void *)&cmd_seteeprom_set,
+		(void *)&cmd_seteeprom_port,
+		(void *)&cmd_seteeprom_portnum,
+		(void *)&cmd_seteeprom_eeprom,
+		(void *)&cmd_seteeprom_magic_str,
+		(void *)&cmd_seteeprom_magic,
+		(void *)&cmd_seteeprom_value,
+		(void *)&cmd_seteeprom_token_str,
+		NULL,
+	},
+};
+
 /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
 struct cmd_set_qmap_result {
 	cmdline_fixed_string_t set;
@@ -13153,6 +13258,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	&cmd_showport,
 	&cmd_showqueue,
 	&cmd_showeeprom,
+	&cmd_seteeprom,
 	&cmd_showportall,
 	&cmd_representor_info,
 	&cmd_showdevice,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 6f0beafa27..fb1a1485e2 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1063,6 +1063,72 @@ port_eeprom_display(portid_t port_id)
 	free(einfo.data);
 }
 
+void
+port_eeprom_set(portid_t port_id,
+		uint32_t magic,
+		uint32_t offset,
+		uint32_t length,
+		uint8_t *value)
+{
+	int ret;
+	int len_eeprom;
+	struct rte_dev_eeprom_info einfo;
+
+	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
+		print_valid_ports();
+		return;
+	}
+
+	len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
+	if (len_eeprom < 0) {
+		switch (len_eeprom) {
+		case -ENODEV:
+			fprintf(stderr, "port index %d invalid\n", port_id);
+			break;
+		case -ENOTSUP:
+			fprintf(stderr, "operation not supported by device\n");
+			break;
+		case -EIO:
+			fprintf(stderr, "device is removed\n");
+			break;
+		default:
+			fprintf(stderr, "Unable to get EEPROM: %d\n",
+				len_eeprom);
+			break;
+		}
+		return;
+	}
+
+	einfo.data = value;
+	einfo.magic = magic;
+	einfo.length = length;
+	einfo.offset = offset;
+
+	if (einfo.offset + einfo.length > (uint32_t)(len_eeprom)) {
+		fprintf(stderr, "offset and length exceed capabilities of EEPROM length: %d\n",
+			len_eeprom);
+		return;
+	}
+
+	ret = rte_eth_dev_set_eeprom(port_id, &einfo);
+	if (ret != 0) {
+		switch (ret) {
+		case -ENODEV:
+			fprintf(stderr, "port index %d invalid\n", port_id);
+			break;
+		case -ENOTSUP:
+			fprintf(stderr, "operation not supported by device\n");
+			break;
+		case -EIO:
+			fprintf(stderr, "device is removed\n");
+			break;
+		default:
+			fprintf(stderr, "Unable to get EEPROM: %d\n", ret);
+			break;
+		}
+	}
+}
+
 void
 port_module_eeprom_display(portid_t port_id)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 9facd7f281..1292d1a0a7 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -927,6 +927,8 @@ void device_infos_display(const char *identifier);
 void port_infos_display(portid_t port_id);
 void port_summary_display(portid_t port_id);
 void port_eeprom_display(portid_t port_id);
+void port_eeprom_set(portid_t port_id, uint32_t magic, uint32_t offset,
+		     uint32_t length, uint8_t *value);
 void port_module_eeprom_display(portid_t port_id);
 void port_summary_header_display(void);
 void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f00ab07605..3aa214ef9f 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1359,6 +1359,17 @@ Set the forwarding peer address for certain port::
 
 This is equivalent to the ``--eth-peer`` command-line option.
 
+set eeprom
+~~~~~~~~~~
+
+Write a value to the device eeprom of a port at a specific offset::
+
+   testpmd> set port (port_id) eeprom magic (magic_num) value (value) offset (offset)
+
+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-uta
 ~~~~~~~~~~~~
 
-- 
2.39.1

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

[Chaoyong He]

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;
+};
+
+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;
+		}
+
+		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);
+
+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);
+		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);
+}
+
 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
 ~~~~~~~~~~~~
 
-- 
2.39.1

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

[fengchengwen]

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
>  ~~~~~~~~~~~~
>  

Re: [PATCH v2 1/2] app/testpmd: add support for setting device EEPROM

[fengchengwen]

On 2024/9/12 14:54, Chaoyong He wrote:
> From: James Hershaw <james.hershaw@corigine.com>
> 
> There is currently no means to test the .set_eeprom function callback of
> a given PMD in drivers/net/. This patch adds functionality to allow a
> user to set device eeprom from the testpmd cmdline.
> 
> usage:
>   testpmd> set port <port-id> eeprom magic <magic> value <value>
>   offset <offset>

this should be a high risk command (it will modify the eeprom of NIC), if use mis-use may lead to serious problem.

if add it, I suggest add acknowledgement from user.

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

[Chaoyong He]

> 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.

RE: [PATCH v2 1/2] app/testpmd: add support for setting device EEPROM

[Chaoyong He]

> On 2024/9/12 14:54, Chaoyong He wrote:
> > From: James Hershaw <james.hershaw@corigine.com>
> >
> > There is currently no means to test the .set_eeprom function callback
> > of a given PMD in drivers/net/. This patch adds functionality to allow
> > a user to set device eeprom from the testpmd cmdline.
> >
> > usage:
> >   testpmd> set port <port-id> eeprom magic <magic> value <value>
> >   offset <offset>
> 
> this should be a high risk command (it will modify the eeprom of NIC), if use
> mis-use may lead to serious problem.
> 
> if add it, I suggest add acknowledgement from user.

Yeah, we'll send a new version patch to see if it can make it better.
Thanks for your review.

[PATCH v3 0/2] add two commands for testpmd application

[Chaoyong He]

This patch series aims to add two commands for the testpmd application:
- testpmd> set port <port-id> eeprom <confirm> magic <magic> \
          value <value> offset <offset>
- testpmd> set port <port-id> led <on/off>

---
v3:
* Add acknowledgement for the set eeprom command.
* Modify logic following the request from reviewer.
v2:
* Solve the conflict in document file.
---

James Hershaw (2):
  app/testpmd: add support for setting device EEPROM
  app/testpmd: add set dev led on/off command

 app/test-pmd/cmdline.c                      | 177 ++++++++++++++++++++
 app/test-pmd/config.c                       |  85 ++++++++++
 app/test-pmd/testpmd.h                      |   3 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  23 +++
 4 files changed, 288 insertions(+)

-- 
2.39.1

[PATCH v3 1/2] app/testpmd: add support for setting device EEPROM

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

There is currently no means to test the .set_eeprom function callback of
a given PMD in drivers/net/. This patch adds functionality to allow a
user to set device eeprom from the testpmd cmdline.

Usage:
  testpmd> set port <port-id> eeprom <confirm> magic <magic> \
  		value <value> offset <offset>

- <confirm> is a fixed string that is required from the user to
  acknowledge the risk involved in using this command and accepting the
  reposibility for that.
- <magic> is a decimal
- <value> is a hex-as-string with no leading "0x"
- <offset> is a decimal (this field is optional and defaults to 0 if
  not specified.)

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 app/test-pmd/cmdline.c                      | 122 ++++++++++++++++++++
 app/test-pmd/config.c                       |  66 +++++++++++
 app/test-pmd/testpmd.h                      |   2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  16 +++
 4 files changed, 206 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 358319c20a..51c72b0826 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -514,6 +514,15 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set eth-peer (port_id) (peer_addr)\n"
 			"    set the peer address for certain port.\n\n"
 
+			"set port (port_id) eeprom (confirm) magic (magic_num)"
+			" value (value) offset (offset)\n"
+			"    Set the device eeprom for certain port.\nNote:\n"
+			"    This is a high-risk command and its misuse may"
+			" result in unexpected behaviour from the NIC.\n"
+			"    By inserting \"confirm\" into the command, the"
+			" user is acknowledging and taking responsibility for"
+			" this risk.\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"
@@ -7488,6 +7497,118 @@ static cmdline_parse_inst_t cmd_set_fwd_eth_peer = {
 	},
 };
 
+/* *** SET PORT EEPROM *** */
+struct cmd_seteeprom_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t port;
+	uint16_t portnum;
+	cmdline_fixed_string_t eeprom;
+	cmdline_fixed_string_t confirm_str;
+	cmdline_fixed_string_t magic_str;
+	uint32_t magic;
+	cmdline_fixed_string_t value;
+	cmdline_multi_string_t token_str;
+};
+
+static void
+cmd_seteeprom_parsed(void *parsed_result,
+		__rte_unused struct cmdline *cl,
+		__rte_unused void *data)
+{
+	char *token;
+	char *token_str;
+	uint32_t length;
+	uint32_t offset = 0;
+	struct cmd_seteeprom_result *res = parsed_result;
+
+	token_str = res->token_str;
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+
+	/* Parse Hex string to Byte data */
+	if (strlen(token) % 2 != 0) {
+		fprintf(stderr, "Bad Argument: %s\n", token);
+		return;
+	}
+
+	length = strlen(token)/2;
+	uint8_t value[length];
+	for (int count = 0; count < (int)(length); count++) {
+		if (sscanf(token, "%2hhx", &value[count]) != 1) {
+			fprintf(stderr, "Bad Argument: %s\n", token);
+			return;
+		}
+		token += 2;
+	}
+
+	/* Second token: offset string */
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+	if (token != NULL) {
+		if (strcmp(token, "offset") == 0) {
+			/* Third token: offset */
+			char *end;
+			token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+			if (token == NULL) {
+				fprintf(stderr, "No offset specified\n");
+				return;
+			}
+			offset = strtoul(token, &end, 10);
+
+			if (offset == 0 && strcmp(end, "") != 0) {
+				fprintf(stderr, "Bad Argument: %s\n", token);
+				return;
+			}
+		} else {
+			fprintf(stderr, "Bad Argument: %s\n", token);
+			return;
+		}
+	}
+
+	port_eeprom_set(res->portnum, res->magic, offset, length, value);
+}
+
+static cmdline_parse_token_string_t cmd_seteeprom_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, set, "set");
+static cmdline_parse_token_string_t cmd_seteeprom_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, port, "port");
+static cmdline_parse_token_num_t cmd_seteeprom_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, portnum, RTE_UINT16);
+static cmdline_parse_token_string_t cmd_seteeprom_eeprom =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, eeprom, "eeprom");
+static cmdline_parse_token_string_t cmd_seteeprom_confirm_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, confirm_str, "confirm");
+static cmdline_parse_token_string_t cmd_seteeprom_magic_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, magic_str, "magic");
+static cmdline_parse_token_num_t cmd_seteeprom_magic =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, magic, RTE_UINT32);
+static cmdline_parse_token_string_t cmd_seteeprom_value =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, value, "value");
+static cmdline_parse_token_string_t cmd_seteeprom_token_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, token_str, TOKEN_STRING_MULTI);
+
+static cmdline_parse_inst_t cmd_seteeprom = {
+	.f = cmd_seteeprom_parsed,
+	.data = NULL,
+	.help_str = "set port <port_id> eeprom <confirm> magic <magic_num> "
+		"value <value> offset <offset>: Set eeprom value for port_id.\n"
+		"Note:\n"
+		"This is a high-risk command and its misuse may result in "
+		"unexpected behaviour from the NIC.\n"
+		"By inserting \"confirm\" into the command, the user is "
+		"acknowledging and taking responsibility for this risk.",
+	.tokens = {
+		(void *)&cmd_seteeprom_set,
+		(void *)&cmd_seteeprom_port,
+		(void *)&cmd_seteeprom_portnum,
+		(void *)&cmd_seteeprom_eeprom,
+		(void *)&cmd_seteeprom_confirm_str,
+		(void *)&cmd_seteeprom_magic_str,
+		(void *)&cmd_seteeprom_magic,
+		(void *)&cmd_seteeprom_value,
+		(void *)&cmd_seteeprom_token_str,
+		NULL,
+	},
+};
+
 /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
 struct cmd_set_qmap_result {
 	cmdline_fixed_string_t set;
@@ -13153,6 +13274,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	&cmd_showport,
 	&cmd_showqueue,
 	&cmd_showeeprom,
+	&cmd_seteeprom,
 	&cmd_showportall,
 	&cmd_representor_info,
 	&cmd_showdevice,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 6f0beafa27..fb1a1485e2 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1063,6 +1063,72 @@ port_eeprom_display(portid_t port_id)
 	free(einfo.data);
 }
 
+void
+port_eeprom_set(portid_t port_id,
+		uint32_t magic,
+		uint32_t offset,
+		uint32_t length,
+		uint8_t *value)
+{
+	int ret;
+	int len_eeprom;
+	struct rte_dev_eeprom_info einfo;
+
+	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
+		print_valid_ports();
+		return;
+	}
+
+	len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
+	if (len_eeprom < 0) {
+		switch (len_eeprom) {
+		case -ENODEV:
+			fprintf(stderr, "port index %d invalid\n", port_id);
+			break;
+		case -ENOTSUP:
+			fprintf(stderr, "operation not supported by device\n");
+			break;
+		case -EIO:
+			fprintf(stderr, "device is removed\n");
+			break;
+		default:
+			fprintf(stderr, "Unable to get EEPROM: %d\n",
+				len_eeprom);
+			break;
+		}
+		return;
+	}
+
+	einfo.data = value;
+	einfo.magic = magic;
+	einfo.length = length;
+	einfo.offset = offset;
+
+	if (einfo.offset + einfo.length > (uint32_t)(len_eeprom)) {
+		fprintf(stderr, "offset and length exceed capabilities of EEPROM length: %d\n",
+			len_eeprom);
+		return;
+	}
+
+	ret = rte_eth_dev_set_eeprom(port_id, &einfo);
+	if (ret != 0) {
+		switch (ret) {
+		case -ENODEV:
+			fprintf(stderr, "port index %d invalid\n", port_id);
+			break;
+		case -ENOTSUP:
+			fprintf(stderr, "operation not supported by device\n");
+			break;
+		case -EIO:
+			fprintf(stderr, "device is removed\n");
+			break;
+		default:
+			fprintf(stderr, "Unable to get EEPROM: %d\n", ret);
+			break;
+		}
+	}
+}
+
 void
 port_module_eeprom_display(portid_t port_id)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 9facd7f281..1292d1a0a7 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -927,6 +927,8 @@ void device_infos_display(const char *identifier);
 void port_infos_display(portid_t port_id);
 void port_summary_display(portid_t port_id);
 void port_eeprom_display(portid_t port_id);
+void port_eeprom_set(portid_t port_id, uint32_t magic, uint32_t offset,
+		     uint32_t length, uint8_t *value);
 void port_module_eeprom_display(portid_t port_id);
 void port_summary_header_display(void);
 void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f00ab07605..f9dd380ea0 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1359,6 +1359,22 @@ Set the forwarding peer address for certain port::
 
 This is equivalent to the ``--eth-peer`` command-line option.
 
+set eeprom
+~~~~~~~~~~
+
+Write a value to the device eeprom of a port at a specific offset::
+
+   testpmd> set port <port-id> eeprom <confirm> magic <magic> \
+            value <value> offset <offset>
+
+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.
+
+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-uta
 ~~~~~~~~~~~~
 
-- 
2.39.1

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

[Chaoyong He]

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"
+			"    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;
+
+	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);
+}
+
 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
 ~~~~~~~~~~~~
 
-- 
2.39.1

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

[fengchengwen]

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
>  ~~~~~~~~~~~~
>  

Re: [PATCH v3 1/2] app/testpmd: add support for setting device EEPROM

[fengchengwen]

On 2024/9/14 9:49, Chaoyong He wrote:
> From: James Hershaw <james.hershaw@corigine.com>
> 
> There is currently no means to test the .set_eeprom function callback of
> a given PMD in drivers/net/. This patch adds functionality to allow a
> user to set device eeprom from the testpmd cmdline.
> 
> Usage:
>   testpmd> set port <port-id> eeprom <confirm> magic <magic> \
>   		value <value> offset <offset>
> 
> - <confirm> is a fixed string that is required from the user to
>   acknowledge the risk involved in using this command and accepting the
>   reposibility for that.
> - <magic> is a decimal
> - <value> is a hex-as-string with no leading "0x"
> - <offset> is a decimal (this field is optional and defaults to 0 if
>   not specified.)
> 
> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> ---
>  app/test-pmd/cmdline.c                      | 122 ++++++++++++++++++++
>  app/test-pmd/config.c                       |  66 +++++++++++
>  app/test-pmd/testpmd.h                      |   2 +
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  16 +++
>  4 files changed, 206 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 358319c20a..51c72b0826 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -514,6 +514,15 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"set eth-peer (port_id) (peer_addr)\n"
>  			"    set the peer address for certain port.\n\n"
>  
> +			"set port (port_id) eeprom (confirm) magic (magic_num)"
> +			" value (value) offset (offset)\n"
> +			"    Set the device eeprom for certain port.\nNote:\n"
> +			"    This is a high-risk command and its misuse may"
> +			" result in unexpected behaviour from the NIC.\n"
> +			"    By inserting \"confirm\" into the command, the"
> +			" user is acknowledging and taking responsibility for"
> +			" this risk.\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"
> @@ -7488,6 +7497,118 @@ static cmdline_parse_inst_t cmd_set_fwd_eth_peer = {
>  	},
>  };
>  
> +/* *** SET PORT EEPROM *** */
> +struct cmd_seteeprom_result {
> +	cmdline_fixed_string_t set;
> +	cmdline_fixed_string_t port;
> +	uint16_t portnum;
> +	cmdline_fixed_string_t eeprom;
> +	cmdline_fixed_string_t confirm_str;
> +	cmdline_fixed_string_t magic_str;
> +	uint32_t magic;
> +	cmdline_fixed_string_t value;
> +	cmdline_multi_string_t token_str;
> +};
> +
> +static void
> +cmd_seteeprom_parsed(void *parsed_result,
> +		__rte_unused struct cmdline *cl,
> +		__rte_unused void *data)
> +{
> +	char *token;
> +	char *token_str;
> +	uint32_t length;
> +	uint32_t offset = 0;
> +	struct cmd_seteeprom_result *res = parsed_result;

the local variables should placed by inverted triangle, the long one in front.

> +
> +	token_str = res->token_str;
> +	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> +
> +	/* Parse Hex string to Byte data */
> +	if (strlen(token) % 2 != 0) {
> +		fprintf(stderr, "Bad Argument: %s\n", token);

Suggest detail error info

> +		return;
> +	}
> +
> +	length = strlen(token)/2;
> +	uint8_t value[length];
> +	for (int count = 0; count < (int)(length); count++) {
> +		if (sscanf(token, "%2hhx", &value[count]) != 1) {
> +			fprintf(stderr, "Bad Argument: %s\n", token);

token could add, so it may confuse user.

> +			return;
> +		}
> +		token += 2;
> +	}
> +
> +	/* Second token: offset string */
> +	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> +	if (token != NULL) {
> +		if (strcmp(token, "offset") == 0) {
> +			/* Third token: offset */
> +			char *end;
> +			token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> +			if (token == NULL) {
> +				fprintf(stderr, "No offset specified\n");
> +				return;
> +			}
> +			offset = strtoul(token, &end, 10);
> +
> +			if (offset == 0 && strcmp(end, "") != 0) {
> +				fprintf(stderr, "Bad Argument: %s\n", token);
> +				return;
> +			}
> +		} else {
> +			fprintf(stderr, "Bad Argument: %s\n", token);
> +			return;
> +		}
> +	}
> +
> +	port_eeprom_set(res->portnum, res->magic, offset, length, value);
> +}
> +
> +static cmdline_parse_token_string_t cmd_seteeprom_set =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, set, "set");
> +static cmdline_parse_token_string_t cmd_seteeprom_port =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, port, "port");
> +static cmdline_parse_token_num_t cmd_seteeprom_portnum =
> +	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, portnum, RTE_UINT16);
> +static cmdline_parse_token_string_t cmd_seteeprom_eeprom =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, eeprom, "eeprom");
> +static cmdline_parse_token_string_t cmd_seteeprom_confirm_str =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, confirm_str, "confirm");
> +static cmdline_parse_token_string_t cmd_seteeprom_magic_str =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, magic_str, "magic");
> +static cmdline_parse_token_num_t cmd_seteeprom_magic =
> +	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, magic, RTE_UINT32);
> +static cmdline_parse_token_string_t cmd_seteeprom_value =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, value, "value");
> +static cmdline_parse_token_string_t cmd_seteeprom_token_str =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, token_str, TOKEN_STRING_MULTI);
> +
> +static cmdline_parse_inst_t cmd_seteeprom = {
> +	.f = cmd_seteeprom_parsed,
> +	.data = NULL,
> +	.help_str = "set port <port_id> eeprom <confirm> magic <magic_num> "
> +		"value <value> offset <offset>: Set eeprom value for port_id.\n"
> +		"Note:\n"
> +		"This is a high-risk command and its misuse may result in "
> +		"unexpected behaviour from the NIC.\n"
> +		"By inserting \"confirm\" into the command, the user is "
> +		"acknowledging and taking responsibility for this risk.",
> +	.tokens = {
> +		(void *)&cmd_seteeprom_set,
> +		(void *)&cmd_seteeprom_port,
> +		(void *)&cmd_seteeprom_portnum,
> +		(void *)&cmd_seteeprom_eeprom,
> +		(void *)&cmd_seteeprom_confirm_str,
> +		(void *)&cmd_seteeprom_magic_str,
> +		(void *)&cmd_seteeprom_magic,
> +		(void *)&cmd_seteeprom_value,
> +		(void *)&cmd_seteeprom_token_str,
> +		NULL,
> +	},
> +};
> +
>  /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
>  struct cmd_set_qmap_result {
>  	cmdline_fixed_string_t set;
> @@ -13153,6 +13274,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
>  	&cmd_showport,
>  	&cmd_showqueue,
>  	&cmd_showeeprom,
> +	&cmd_seteeprom,
>  	&cmd_showportall,
>  	&cmd_representor_info,
>  	&cmd_showdevice,
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 6f0beafa27..fb1a1485e2 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1063,6 +1063,72 @@ port_eeprom_display(portid_t port_id)
>  	free(einfo.data);
>  }
>  
> +void
> +port_eeprom_set(portid_t port_id,
> +		uint32_t magic,
> +		uint32_t offset,
> +		uint32_t length,
> +		uint8_t *value)
> +{
> +	int ret;
> +	int len_eeprom;
> +	struct rte_dev_eeprom_info einfo;

plese placed by inverted triangle

> +
> +	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
> +		print_valid_ports();
> +		return;
> +	}
> +
> +	len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
> +	if (len_eeprom < 0) {
> +		switch (len_eeprom) {
> +		case -ENODEV:
> +			fprintf(stderr, "port index %d invalid\n", port_id);
> +			break;
> +		case -ENOTSUP:
> +			fprintf(stderr, "operation not supported by device\n");
> +			break;
> +		case -EIO:
> +			fprintf(stderr, "device is removed\n");
> +			break;
> +		default:
> +			fprintf(stderr, "Unable to get EEPROM: %d\n",
> +				len_eeprom);
> +			break;
> +		}

this could simplied by rte_strerror(-ret)

> +		return;
> +	}
> +
> +	einfo.data = value;
> +	einfo.magic = magic;
> +	einfo.length = length;
> +	einfo.offset = offset;
> +
> +	if (einfo.offset + einfo.length > (uint32_t)(len_eeprom)) {
> +		fprintf(stderr, "offset and length exceed capabilities of EEPROM length: %d\n",
> +			len_eeprom);
> +		return;
> +	}
> +
> +	ret = rte_eth_dev_set_eeprom(port_id, &einfo);
> +	if (ret != 0) {
> +		switch (ret) {
> +		case -ENODEV:
> +			fprintf(stderr, "port index %d invalid\n", port_id);
> +			break;
> +		case -ENOTSUP:
> +			fprintf(stderr, "operation not supported by device\n");
> +			break;
> +		case -EIO:
> +			fprintf(stderr, "device is removed\n");
> +			break;
> +		default:
> +			fprintf(stderr, "Unable to get EEPROM: %d\n", ret);

here should set not get, but as I said before, both suggest simplied by rte_strerror(-ret)

> +			break;
> +		}
> +	}
> +}
> +
>  void
>  port_module_eeprom_display(portid_t port_id)
>  {
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 9facd7f281..1292d1a0a7 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -927,6 +927,8 @@ void device_infos_display(const char *identifier);
>  void port_infos_display(portid_t port_id);
>  void port_summary_display(portid_t port_id);
>  void port_eeprom_display(portid_t port_id);
> +void port_eeprom_set(portid_t port_id, uint32_t magic, uint32_t offset,
> +		     uint32_t length, uint8_t *value);
>  void port_module_eeprom_display(portid_t port_id);
>  void port_summary_header_display(void);
>  void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index f00ab07605..f9dd380ea0 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -1359,6 +1359,22 @@ Set the forwarding peer address for certain port::
>  
>  This is equivalent to the ``--eth-peer`` command-line option.
>  
> +set eeprom
> +~~~~~~~~~~
> +
> +Write a value to the device eeprom of a port at a specific offset::
> +
> +   testpmd> set port <port-id> eeprom <confirm> magic <magic> \
> +            value <value> offset <offset>
> +
> +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.
> +
> +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.

please use ".. note::"

> +
>  set port-uta
>  ~~~~~~~~~~~~
>  

[PATCH v4 0/2] add two commands for testpmd application

[Chaoyong He]

This patch series aims to add two commands for the testpmd application:
- testpmd> set port <port-id> eeprom <confirm> magic <magic> \
          value <value> offset <offset>
- testpmd> set port <port-id> led <on/off>

---
v4:
* Make the log more readable by using 'rte_strerror()'.
* Make the modification of document following the rules.
v3:
* Add acknowledgement for the set eeprom command.
* Modify logic following the request from reviewer.
v2:
* Solve the conflict in document file.
---

James Hershaw (2):
  app/testpmd: add support for setting device EEPROM
  app/testpmd: add set dev led on/off command

 app/test-pmd/cmdline.c                      | 175 ++++++++++++++++++++
 app/test-pmd/config.c                       |  59 +++++++
 app/test-pmd/testpmd.h                      |   3 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  25 +++
 4 files changed, 262 insertions(+)

-- 
2.39.1

[PATCH v4 1/2] app/testpmd: add support for setting device EEPROM

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

There is currently no means to test the .set_eeprom function callback of
a given PMD in drivers/net/. This patch adds functionality to allow a
user to set device eeprom from the testpmd cmdline.

Usage:
  testpmd> set port <port-id> eeprom <confirm> magic <magic> \
  		value <value> offset <offset>

- <confirm> is a fixed string that is required from the user to
  acknowledge the risk involved in using this command and accepting the
  reposibility for that.
- <magic> is a decimal
- <value> is a hex-as-string with no leading "0x"
- <offset> is a decimal (this field is optional and defaults to 0 if
  not specified.)

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 app/test-pmd/cmdline.c                      | 123 ++++++++++++++++++++
 app/test-pmd/config.c                       |  39 +++++++
 app/test-pmd/testpmd.h                      |   2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  18 +++
 4 files changed, 182 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 358319c20a..a227d3cdc5 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -514,6 +514,15 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set eth-peer (port_id) (peer_addr)\n"
 			"    set the peer address for certain port.\n\n"
 
+			"set port (port_id) eeprom (confirm) magic (magic_num)"
+			" value (value) offset (offset)\n"
+			"    Set the device eeprom for certain port.\nNote:\n"
+			"    This is a high-risk command and its misuse may"
+			" result in unexpected behaviour from the NIC.\n"
+			"    By inserting \"confirm\" into the command, the"
+			" user is acknowledging and taking responsibility for"
+			" this risk.\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"
@@ -7488,6 +7497,119 @@ static cmdline_parse_inst_t cmd_set_fwd_eth_peer = {
 	},
 };
 
+/* *** SET PORT EEPROM *** */
+struct cmd_seteeprom_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t port;
+	uint16_t portnum;
+	cmdline_fixed_string_t eeprom;
+	cmdline_fixed_string_t confirm_str;
+	cmdline_fixed_string_t magic_str;
+	uint32_t magic;
+	cmdline_fixed_string_t value;
+	cmdline_multi_string_t token_str;
+};
+
+static void cmd_seteeprom_parsed(void *parsed_result,
+		__rte_unused struct cmdline *cl,
+		__rte_unused void *data)
+{
+	struct cmd_seteeprom_result *res = parsed_result;
+	uint32_t offset = 0;
+	uint32_t length;
+	char *token_str;
+	char *token;
+
+	token_str = res->token_str;
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+
+	/* Parse Hex string to Byte data */
+	if (strlen(token) % 2 != 0) {
+		fprintf(stderr, "Bad Argument: %s\nHex string must be in multiples of 2 Bytes\n",
+			token);
+		return;
+	}
+
+	length = strlen(token) / 2;
+	uint8_t value[length];
+	for (int count = 0; count < (int)(length); count++) {
+		if (sscanf(token, "%2hhx", &value[count]) != 1) {
+			fprintf(stderr, "Bad Argument: %s\nValue must be a hex string\n",
+				token - (count + 1));
+			return;
+		}
+		token += 2;
+	}
+
+	/* Second token: offset string */
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+	if (token != NULL) {
+		if (strcmp(token, "offset") == 0) {
+			/* Third token: offset */
+			token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+			if (token == NULL) {
+				fprintf(stderr, "No offset specified\n");
+				return;
+			}
+
+			char *end;
+			offset = strtoul(token, &end, 10);
+			if (offset == 0 && strcmp(end, "") != 0) {
+				fprintf(stderr, "Bad Argument: %s\n", token);
+				return;
+			}
+		} else {
+			fprintf(stderr, "Bad Argument: %s\n", token);
+			return;
+		}
+	}
+
+	port_eeprom_set(res->portnum, res->magic, offset, length, value);
+}
+
+static cmdline_parse_token_string_t cmd_seteeprom_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, set, "set");
+static cmdline_parse_token_string_t cmd_seteeprom_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, port, "port");
+static cmdline_parse_token_num_t cmd_seteeprom_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, portnum, RTE_UINT16);
+static cmdline_parse_token_string_t cmd_seteeprom_eeprom =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, eeprom, "eeprom");
+static cmdline_parse_token_string_t cmd_seteeprom_confirm_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, confirm_str, "confirm");
+static cmdline_parse_token_string_t cmd_seteeprom_magic_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, magic_str, "magic");
+static cmdline_parse_token_num_t cmd_seteeprom_magic =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, magic, RTE_UINT32);
+static cmdline_parse_token_string_t cmd_seteeprom_value =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, value, "value");
+static cmdline_parse_token_string_t cmd_seteeprom_token_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, token_str, TOKEN_STRING_MULTI);
+
+static cmdline_parse_inst_t cmd_seteeprom = {
+	.f = cmd_seteeprom_parsed,
+	.data = NULL,
+	.help_str = "set port <port_id> eeprom <confirm> magic <magic_num> "
+		"value <value> offset <offset>: Set eeprom value for port_id.\n"
+		"Note:\n"
+		"This is a high-risk command and its misuse may result in "
+		"unexpected behaviour from the NIC.\n"
+		"By inserting \"confirm\" into the command, the user is "
+		"acknowledging and taking responsibility for this risk.",
+	.tokens = {
+		(void *)&cmd_seteeprom_set,
+		(void *)&cmd_seteeprom_port,
+		(void *)&cmd_seteeprom_portnum,
+		(void *)&cmd_seteeprom_eeprom,
+		(void *)&cmd_seteeprom_confirm_str,
+		(void *)&cmd_seteeprom_magic_str,
+		(void *)&cmd_seteeprom_magic,
+		(void *)&cmd_seteeprom_value,
+		(void *)&cmd_seteeprom_token_str,
+		NULL,
+	},
+};
+
 /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
 struct cmd_set_qmap_result {
 	cmdline_fixed_string_t set;
@@ -13153,6 +13275,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	&cmd_showport,
 	&cmd_showqueue,
 	&cmd_showeeprom,
+	&cmd_seteeprom,
 	&cmd_showportall,
 	&cmd_representor_info,
 	&cmd_showdevice,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 6f0beafa27..73549b2b57 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1063,6 +1063,45 @@ port_eeprom_display(portid_t port_id)
 	free(einfo.data);
 }
 
+void
+port_eeprom_set(portid_t port_id,
+		uint32_t magic,
+		uint32_t offset,
+		uint32_t length,
+		uint8_t *value)
+{
+	struct rte_dev_eeprom_info einfo;
+	int len_eeprom;
+	int ret;
+
+	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
+		print_valid_ports();
+		return;
+	}
+
+	len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
+	if (len_eeprom < 0) {
+		fprintf(stderr, "Unable to get EEPROM length: %s\n",
+			rte_strerror(-len_eeprom));
+		return;
+	}
+
+	einfo.data = value;
+	einfo.magic = magic;
+	einfo.length = length;
+	einfo.offset = offset;
+
+	if (einfo.offset + einfo.length > (uint32_t)(len_eeprom)) {
+		fprintf(stderr, "offset and length exceed capabilities of EEPROM length: %d\n",
+			len_eeprom);
+		return;
+	}
+
+	ret = rte_eth_dev_set_eeprom(port_id, &einfo);
+	if (ret != 0)
+		fprintf(stderr, "Unable to set EEPROM: %s\n", rte_strerror(-ret));
+}
+
 void
 port_module_eeprom_display(portid_t port_id)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 9facd7f281..1292d1a0a7 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -927,6 +927,8 @@ void device_infos_display(const char *identifier);
 void port_infos_display(portid_t port_id);
 void port_summary_display(portid_t port_id);
 void port_eeprom_display(portid_t port_id);
+void port_eeprom_set(portid_t port_id, uint32_t magic, uint32_t offset,
+		     uint32_t length, uint8_t *value);
 void port_module_eeprom_display(portid_t port_id);
 void port_summary_header_display(void);
 void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f00ab07605..5c33d610ea 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1359,6 +1359,24 @@ Set the forwarding peer address for certain port::
 
 This is equivalent to the ``--eth-peer`` command-line option.
 
+set eeprom
+~~~~~~~~~~
+
+Write a value to the device eeprom of a port at a specific offset::
+
+   testpmd> set port (port_id) eeprom (confirm) magic (magic_num) value (value) \
+            offset (offset)
+
+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.
+
+.. note::
+
+   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-uta
 ~~~~~~~~~~~~
 
-- 
2.39.1

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

[Chaoyong He]

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                      | 52 +++++++++++++++++++++
 app/test-pmd/config.c                       | 20 ++++++++
 app/test-pmd/testpmd.h                      |  1 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
 4 files changed, 80 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index a227d3cdc5..a117a4c2fb 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"
+			" 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"
@@ -7610,6 +7614,53 @@ 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->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;
@@ -13349,6 +13400,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 73549b2b57..5c4599e903 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -5358,6 +5358,26 @@ 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: %s\n",
+			port_id, 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 5c33d610ea..4ea745e66c 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1377,6 +1377,13 @@ to 0.
    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
 ~~~~~~~~~~~~
 
-- 
2.39.1

Re: [PATCH v4 1/2] app/testpmd: add support for setting device EEPROM

[Ferruh Yigit]

On 9/18/2024 3:38 AM, Chaoyong He wrote:
> From: James Hershaw <james.hershaw@corigine.com>
> 
> There is currently no means to test the .set_eeprom function callback of
> a given PMD in drivers/net/. This patch adds functionality to allow a
> user to set device eeprom from the testpmd cmdline.
> 
> Usage:
>   testpmd> set port <port-id> eeprom <confirm> magic <magic> \
>   		value <value> offset <offset>
> 
> - <confirm> is a fixed string that is required from the user to
>   acknowledge the risk involved in using this command and accepting the
>   reposibility for that.
>

+1 have a warning for user, but 'confirm' may not be clear enough, what
about something around "accept_risk"?

s/reposibility/responsibility/


> - <magic> is a decimal
> - <value> is a hex-as-string with no leading "0x"
> - <offset> is a decimal (this field is optional and defaults to 0 if
>   not specified.)
> 
> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> ---
>  app/test-pmd/cmdline.c                      | 123 ++++++++++++++++++++
>  app/test-pmd/config.c                       |  39 +++++++
>  app/test-pmd/testpmd.h                      |   2 +
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  18 +++
>  4 files changed, 182 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 358319c20a..a227d3cdc5 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -514,6 +514,15 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"set eth-peer (port_id) (peer_addr)\n"
>  			"    set the peer address for certain port.\n\n"
>  
> +			"set port (port_id) eeprom (confirm) magic (magic_num)"
> +			" value (value) offset (offset)\n"
> +			"    Set the device eeprom for certain port.\nNote:\n"
> +			"    This is a high-risk command and its misuse may"
> +			" result in unexpected behaviour from the NIC.\n"
> +			"    By inserting \"confirm\" into the command, the"
> +			" user is acknowledging and taking responsibility for"
> +			" this risk.\n\n"
> +
>

There is another eeprom command above [1], lets put this one below it.
Also can you please move the function implementation in similar order.


[1]
"show port port_id (module_eeprom|eeprom)\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"
> @@ -7488,6 +7497,119 @@ static cmdline_parse_inst_t cmd_set_fwd_eth_peer = {
>  	},
>  };
>  
> +/* *** SET PORT EEPROM *** */
> +struct cmd_seteeprom_result {
> +	cmdline_fixed_string_t set;
> +	cmdline_fixed_string_t port;
> +	uint16_t portnum;
> +	cmdline_fixed_string_t eeprom;
> +	cmdline_fixed_string_t confirm_str;
> +	cmdline_fixed_string_t magic_str;
> +	uint32_t magic;
> +	cmdline_fixed_string_t value;
> +	cmdline_multi_string_t token_str;
> +};
> +
> +static void cmd_seteeprom_parsed(void *parsed_result,
> +		__rte_unused struct cmdline *cl,
> +		__rte_unused void *data)
> +{
> +	struct cmd_seteeprom_result *res = parsed_result;
> +	uint32_t offset = 0;
> +	uint32_t length;
> +	char *token_str;
> +	char *token;
> +
> +	token_str = res->token_str;
> +	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> +
> +	/* Parse Hex string to Byte data */
> +	if (strlen(token) % 2 != 0) {
> +		fprintf(stderr, "Bad Argument: %s\nHex string must be in multiples of 2 Bytes\n",
> +			token);
> +		return;
> +	}
> +
> +	length = strlen(token) / 2;
> +	uint8_t value[length];
>

VLA can cause stack overflow, what about dynamic memory allocation.


> +	for (int count = 0; count < (int)(length); count++) {
> +		if (sscanf(token, "%2hhx", &value[count]) != 1) {
> +			fprintf(stderr, "Bad Argument: %s\nValue must be a hex string\n",
> +				token - (count + 1));
> +			return;
> +		}
> +		token += 2;
> +	}
> +
> +	/* Second token: offset string */
> +	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> +	if (token != NULL) {
> +		if (strcmp(token, "offset") == 0) {
> +			/* Third token: offset */
> +			token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> +			if (token == NULL) {
> +				fprintf(stderr, "No offset specified\n");
> +				return;
> +			}
>

Why not move 'offset' before value, so can use it as res->offset?


> +
> +			char *end;
> +			offset = strtoul(token, &end, 10);
> +			if (offset == 0 && strcmp(end, "") != 0) {
> +				fprintf(stderr, "Bad Argument: %s\n", token);
> +				return;
> +			}
> +		} else {
> +			fprintf(stderr, "Bad Argument: %s\n", token);
> +			return;
> +		}
> +	}
> +
> +	port_eeprom_set(res->portnum, res->magic, offset, length, value);
> +}
> +
> +static cmdline_parse_token_string_t cmd_seteeprom_set =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, set, "set");
> +static cmdline_parse_token_string_t cmd_seteeprom_port =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, port, "port");
> +static cmdline_parse_token_num_t cmd_seteeprom_portnum =
> +	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, portnum, RTE_UINT16);
> +static cmdline_parse_token_string_t cmd_seteeprom_eeprom =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, eeprom, "eeprom");
> +static cmdline_parse_token_string_t cmd_seteeprom_confirm_str =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, confirm_str, "confirm");
> +static cmdline_parse_token_string_t cmd_seteeprom_magic_str =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, magic_str, "magic");
> +static cmdline_parse_token_num_t cmd_seteeprom_magic =
> +	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, magic, RTE_UINT32);
> +static cmdline_parse_token_string_t cmd_seteeprom_value =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, value, "value");
> +static cmdline_parse_token_string_t cmd_seteeprom_token_str =
> +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, token_str, TOKEN_STRING_MULTI);
> +
> +static cmdline_parse_inst_t cmd_seteeprom = {
> +	.f = cmd_seteeprom_parsed,
> +	.data = NULL,
> +	.help_str = "set port <port_id> eeprom <confirm> magic <magic_num> "
> +		"value <value> offset <offset>: Set eeprom value for port_id.\n"
> +		"Note:\n"
> +		"This is a high-risk command and its misuse may result in "
> +		"unexpected behaviour from the NIC.\n"
> +		"By inserting \"confirm\" into the command, the user is "
> +		"acknowledging and taking responsibility for this risk.",
> +	.tokens = {
> +		(void *)&cmd_seteeprom_set,
> +		(void *)&cmd_seteeprom_port,
> +		(void *)&cmd_seteeprom_portnum,
> +		(void *)&cmd_seteeprom_eeprom,
> +		(void *)&cmd_seteeprom_confirm_str,
> +		(void *)&cmd_seteeprom_magic_str,
> +		(void *)&cmd_seteeprom_magic,
> +		(void *)&cmd_seteeprom_value,
> +		(void *)&cmd_seteeprom_token_str,
> +		NULL,
> +	},
> +};
> +
>  /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
>  struct cmd_set_qmap_result {
>  	cmdline_fixed_string_t set;
> @@ -13153,6 +13275,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
>  	&cmd_showport,
>  	&cmd_showqueue,
>  	&cmd_showeeprom,
> +	&cmd_seteeprom,
>  	&cmd_showportall,
>  	&cmd_representor_info,
>  	&cmd_showdevice,
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 6f0beafa27..73549b2b57 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1063,6 +1063,45 @@ port_eeprom_display(portid_t port_id)
>  	free(einfo.data);
>  }
>  
> +void
> +port_eeprom_set(portid_t port_id,
> +		uint32_t magic,
> +		uint32_t offset,
> +		uint32_t length,
> +		uint8_t *value)
> +{
> +	struct rte_dev_eeprom_info einfo;
> +	int len_eeprom;
> +	int ret;
> +
> +	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
> +		print_valid_ports();
> +		return;
> +	}
> +
> +	len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
> +	if (len_eeprom < 0) {
> +		fprintf(stderr, "Unable to get EEPROM length: %s\n",
> +			rte_strerror(-len_eeprom));
> +		return;
> +	}
> +
> +	einfo.data = value;
> +	einfo.magic = magic;
> +	einfo.length = length;
> +	einfo.offset = offset;
> +
> +	if (einfo.offset + einfo.length > (uint32_t)(len_eeprom)) {
> +		fprintf(stderr, "offset and length exceed capabilities of EEPROM length: %d\n",
> +			len_eeprom);
> +		return;
> +	}
> +
> +	ret = rte_eth_dev_set_eeprom(port_id, &einfo);
> +	if (ret != 0)
> +		fprintf(stderr, "Unable to set EEPROM: %s\n", rte_strerror(-ret));
> +}
> +
>  void
>  port_module_eeprom_display(portid_t port_id)
>  {
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 9facd7f281..1292d1a0a7 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -927,6 +927,8 @@ void device_infos_display(const char *identifier);
>  void port_infos_display(portid_t port_id);
>  void port_summary_display(portid_t port_id);
>  void port_eeprom_display(portid_t port_id);
> +void port_eeprom_set(portid_t port_id, uint32_t magic, uint32_t offset,
> +		     uint32_t length, uint8_t *value);
>  void port_module_eeprom_display(portid_t port_id);
>  void port_summary_header_display(void);
>  void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index f00ab07605..5c33d610ea 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -1359,6 +1359,24 @@ Set the forwarding peer address for certain port::
>  
>  This is equivalent to the ``--eth-peer`` command-line option.
>  
> +set eeprom
> +~~~~~~~~~~
> +
> +Write a value to the device eeprom of a port at a specific offset::
> +
> +   testpmd> set port (port_id) eeprom (confirm) magic (magic_num) value (value) \
> +            offset (offset)
> +
> +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.
> +
> +.. note::
> +
> +   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.
> +
> 

Similarly, can you please move this around the eeprom display command.

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

[Ferruh Yigit]

On 9/18/2024 3:38 AM, 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                      | 52 +++++++++++++++++++++
>  app/test-pmd/config.c                       | 20 ++++++++
>  app/test-pmd/testpmd.h                      |  1 +
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
>  4 files changed, 80 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index a227d3cdc5..a117a4c2fb 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"
> +			" port on or off.\n\n"
> +
>

new command seems added in between some mac related commands (excluding
new eeprom cmd, assuming it will be moved), as we don't have anything
related to the led before maybe it can go last.
Same comment for all updates in this patch, can you please double check
the order, and put updates in middle of other grouping.

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

[Chaoyong He]

> On 9/18/2024 3:38 AM, 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                      | 52 +++++++++++++++++++++
> >  app/test-pmd/config.c                       | 20 ++++++++
> >  app/test-pmd/testpmd.h                      |  1 +
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
> >  4 files changed, 80 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > a227d3cdc5..a117a4c2fb 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"
> > +			" port on or off.\n\n"
> > +
> >
> 
> new command seems added in between some mac related commands
> (excluding new eeprom cmd, assuming it will be moved), as we don't have
> anything related to the led before maybe it can go last.
> Same comment for all updates in this patch, can you please double check the
> order, and put updates in middle of other grouping.

Okay, we will send a new version patch.

RE: [PATCH v4 1/2] app/testpmd: add support for setting device EEPROM

[Chaoyong He]

> On 9/18/2024 3:38 AM, Chaoyong He wrote:
> > From: James Hershaw <james.hershaw@corigine.com>
> >
> > There is currently no means to test the .set_eeprom function callback
> > of a given PMD in drivers/net/. This patch adds functionality to allow
> > a user to set device eeprom from the testpmd cmdline.
> >
> > Usage:
> >   testpmd> set port <port-id> eeprom <confirm> magic <magic> \
> >   		value <value> offset <offset>
> >
> > - <confirm> is a fixed string that is required from the user to
> >   acknowledge the risk involved in using this command and accepting the
> >   reposibility for that.
> >
> 
> +1 have a warning for user, but 'confirm' may not be clear enough, what
> about something around "accept_risk"?
> 
> s/reposibility/responsibility/
> 

Make sense, will send a new version patch.

> 
> > - <magic> is a decimal
> > - <value> is a hex-as-string with no leading "0x"
> > - <offset> is a decimal (this field is optional and defaults to 0 if
> >   not specified.)
> >
> > Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > ---
> >  app/test-pmd/cmdline.c                      | 123 ++++++++++++++++++++
> >  app/test-pmd/config.c                       |  39 +++++++
> >  app/test-pmd/testpmd.h                      |   2 +
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  18 +++
> >  4 files changed, 182 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 358319c20a..a227d3cdc5 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -514,6 +514,15 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> >  			"set eth-peer (port_id) (peer_addr)\n"
> >  			"    set the peer address for certain port.\n\n"
> >
> > +			"set port (port_id) eeprom (confirm) magic
> (magic_num)"
> > +			" value (value) offset (offset)\n"
> > +			"    Set the device eeprom for certain port.\nNote:\n"
> > +			"    This is a high-risk command and its misuse may"
> > +			" result in unexpected behaviour from the NIC.\n"
> > +			"    By inserting \"confirm\" into the command, the"
> > +			" user is acknowledging and taking responsibility for"
> > +			" this risk.\n\n"
> > +
> >
> 
> There is another eeprom command above [1], lets put this one below it.
> Also can you please move the function implementation in similar order.
> 

Okay.

> 
> [1]
> "show port port_id (module_eeprom|eeprom)\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"
> > @@ -7488,6 +7497,119 @@ static cmdline_parse_inst_t
> cmd_set_fwd_eth_peer = {
> >  	},
> >  };
> >
> > +/* *** SET PORT EEPROM *** */
> > +struct cmd_seteeprom_result {
> > +	cmdline_fixed_string_t set;
> > +	cmdline_fixed_string_t port;
> > +	uint16_t portnum;
> > +	cmdline_fixed_string_t eeprom;
> > +	cmdline_fixed_string_t confirm_str;
> > +	cmdline_fixed_string_t magic_str;
> > +	uint32_t magic;
> > +	cmdline_fixed_string_t value;
> > +	cmdline_multi_string_t token_str;
> > +};
> > +
> > +static void cmd_seteeprom_parsed(void *parsed_result,
> > +		__rte_unused struct cmdline *cl,
> > +		__rte_unused void *data)
> > +{
> > +	struct cmd_seteeprom_result *res = parsed_result;
> > +	uint32_t offset = 0;
> > +	uint32_t length;
> > +	char *token_str;
> > +	char *token;
> > +
> > +	token_str = res->token_str;
> > +	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> > +
> > +	/* Parse Hex string to Byte data */
> > +	if (strlen(token) % 2 != 0) {
> > +		fprintf(stderr, "Bad Argument: %s\nHex string must be in
> multiples of 2 Bytes\n",
> > +			token);
> > +		return;
> > +	}
> > +
> > +	length = strlen(token) / 2;
> > +	uint8_t value[length];
> >
> 
> VLA can cause stack overflow, what about dynamic memory allocation.

Okay.

> 
> 
> > +	for (int count = 0; count < (int)(length); count++) {
> > +		if (sscanf(token, "%2hhx", &value[count]) != 1) {
> > +			fprintf(stderr, "Bad Argument: %s\nValue must be a
> hex string\n",
> > +				token - (count + 1));
> > +			return;
> > +		}
> > +		token += 2;
> > +	}
> > +
> > +	/* Second token: offset string */
> > +	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> > +	if (token != NULL) {
> > +		if (strcmp(token, "offset") == 0) {
> > +			/* Third token: offset */
> > +			token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
> > +			if (token == NULL) {
> > +				fprintf(stderr, "No offset specified\n");
> > +				return;
> > +			}
> >
> 
> Why not move 'offset' before value, so can use it as res->offset?
> 

The 'offset' is the last argument provided because it is an optional argument, if the user doesn't specify an offset, 0 is used.
If we were to move the 'offset' to before the value then it would have to be specified every time.

> 
> > +
> > +			char *end;
> > +			offset = strtoul(token, &end, 10);
> > +			if (offset == 0 && strcmp(end, "") != 0) {
> > +				fprintf(stderr, "Bad Argument: %s\n", token);
> > +				return;
> > +			}
> > +		} else {
> > +			fprintf(stderr, "Bad Argument: %s\n", token);
> > +			return;
> > +		}
> > +	}
> > +
> > +	port_eeprom_set(res->portnum, res->magic, offset, length, value); }
> > +
> > +static cmdline_parse_token_string_t cmd_seteeprom_set =
> > +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, set,
> "set");
> > +static cmdline_parse_token_string_t cmd_seteeprom_port =
> > +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, port,
> "port");
> > +static cmdline_parse_token_num_t cmd_seteeprom_portnum =
> > +	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, portnum,
> > +RTE_UINT16); static cmdline_parse_token_string_t
> cmd_seteeprom_eeprom =
> > +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, eeprom,
> > +"eeprom"); static cmdline_parse_token_string_t
> cmd_seteeprom_confirm_str =
> > +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result,
> confirm_str,
> > +"confirm"); static cmdline_parse_token_string_t cmd_seteeprom_magic_str
> =
> > +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, magic_str,
> > +"magic"); static cmdline_parse_token_num_t cmd_seteeprom_magic =
> > +	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, magic,
> > +RTE_UINT32); static cmdline_parse_token_string_t cmd_seteeprom_value
> =
> > +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, value,
> > +"value"); static cmdline_parse_token_string_t cmd_seteeprom_token_str =
> > +	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, token_str,
> > +TOKEN_STRING_MULTI);
> > +
> > +static cmdline_parse_inst_t cmd_seteeprom = {
> > +	.f = cmd_seteeprom_parsed,
> > +	.data = NULL,
> > +	.help_str = "set port <port_id> eeprom <confirm> magic <magic_num>
> "
> > +		"value <value> offset <offset>: Set eeprom value for
> port_id.\n"
> > +		"Note:\n"
> > +		"This is a high-risk command and its misuse may result in "
> > +		"unexpected behaviour from the NIC.\n"
> > +		"By inserting \"confirm\" into the command, the user is "
> > +		"acknowledging and taking responsibility for this risk.",
> > +	.tokens = {
> > +		(void *)&cmd_seteeprom_set,
> > +		(void *)&cmd_seteeprom_port,
> > +		(void *)&cmd_seteeprom_portnum,
> > +		(void *)&cmd_seteeprom_eeprom,
> > +		(void *)&cmd_seteeprom_confirm_str,
> > +		(void *)&cmd_seteeprom_magic_str,
> > +		(void *)&cmd_seteeprom_magic,
> > +		(void *)&cmd_seteeprom_value,
> > +		(void *)&cmd_seteeprom_token_str,
> > +		NULL,
> > +	},
> > +};
> > +
> >  /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */  struct
> > cmd_set_qmap_result {
> >  	cmdline_fixed_string_t set;
> > @@ -13153,6 +13275,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
> >  	&cmd_showport,
> >  	&cmd_showqueue,
> >  	&cmd_showeeprom,
> > +	&cmd_seteeprom,
> >  	&cmd_showportall,
> >  	&cmd_representor_info,
> >  	&cmd_showdevice,
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 6f0beafa27..73549b2b57 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -1063,6 +1063,45 @@ port_eeprom_display(portid_t port_id)
> >  	free(einfo.data);
> >  }
> >
> > +void
> > +port_eeprom_set(portid_t port_id,
> > +		uint32_t magic,
> > +		uint32_t offset,
> > +		uint32_t length,
> > +		uint8_t *value)
> > +{
> > +	struct rte_dev_eeprom_info einfo;
> > +	int len_eeprom;
> > +	int ret;
> > +
> > +	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
> > +		print_valid_ports();
> > +		return;
> > +	}
> > +
> > +	len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
> > +	if (len_eeprom < 0) {
> > +		fprintf(stderr, "Unable to get EEPROM length: %s\n",
> > +			rte_strerror(-len_eeprom));
> > +		return;
> > +	}
> > +
> > +	einfo.data = value;
> > +	einfo.magic = magic;
> > +	einfo.length = length;
> > +	einfo.offset = offset;
> > +
> > +	if (einfo.offset + einfo.length > (uint32_t)(len_eeprom)) {
> > +		fprintf(stderr, "offset and length exceed capabilities of
> EEPROM length: %d\n",
> > +			len_eeprom);
> > +		return;
> > +	}
> > +
> > +	ret = rte_eth_dev_set_eeprom(port_id, &einfo);
> > +	if (ret != 0)
> > +		fprintf(stderr, "Unable to set EEPROM: %s\n", rte_strerror(-
> ret));
> > +}
> > +
> >  void
> >  port_module_eeprom_display(portid_t port_id)  { diff --git
> > a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> > 9facd7f281..1292d1a0a7 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -927,6 +927,8 @@ void device_infos_display(const char *identifier);
> > void port_infos_display(portid_t port_id);  void
> > port_summary_display(portid_t port_id);  void
> > port_eeprom_display(portid_t port_id);
> > +void port_eeprom_set(portid_t port_id, uint32_t magic, uint32_t offset,
> > +		     uint32_t length, uint8_t *value);
> >  void port_module_eeprom_display(portid_t port_id);  void
> > port_summary_header_display(void);
> >  void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
> > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index f00ab07605..5c33d610ea 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -1359,6 +1359,24 @@ Set the forwarding peer address for certain
> port::
> >
> >  This is equivalent to the ``--eth-peer`` command-line option.
> >
> > +set eeprom
> > +~~~~~~~~~~
> > +
> > +Write a value to the device eeprom of a port at a specific offset::
> > +
> > +   testpmd> set port (port_id) eeprom (confirm) magic (magic_num) value
> (value) \
> > +            offset (offset)
> > +
> > +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.
> > +
> > +.. note::
> > +
> > +   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.
> > +
> >
> 
> Similarly, can you please move this around the eeprom display command.

Okay.

[PATCH v5 0/2] add two commands for testpmd application

[Chaoyong He]

This patch series aims to add two commands for the testpmd application:
- testpmd> set port <port-id> eeprom <accept_risk> magic <magic> \
          value <value> offset <offset>
- testpmd> set port <port-id> led <on/off>

---
v5:
* Use 'accept_risk' rather than 'confirm' to make it clear enough.
* Use dynamic memory alloc rather than VLA.
* Place the logic in the right place following the reviewer's request.
v4:
* Make the log more readable by using 'rte_strerror()'.
* Make the modification of document following the rules.
v3:
* Add acknowledgement for the set eeprom command.
* Modify logic following the request from reviewer.
v2:
* Solve the conflict in document file.
---

James Hershaw (2):
  app/testpmd: add support for setting device EEPROM
  app/testpmd: add set dev led on/off command

 app/test-pmd/cmdline.c                      | 179 ++++++++++++++++++++
 app/test-pmd/config.c                       |  59 +++++++
 app/test-pmd/testpmd.h                      |   3 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  25 +++
 4 files changed, 266 insertions(+)

-- 
2.39.1

[PATCH v5 1/2] app/testpmd: add support for setting device EEPROM

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

There is currently no means to test the .set_eeprom function callback of
a given PMD in drivers/net/. This patch adds functionality to allow a
user to set device eeprom from the testpmd cmdline.

Usage:
  testpmd> set port <port-id> eeprom <accept_risk> magic <magic> \
  		value <value> offset <offset>

- <accept_risk> is a fixed string that is required from the user to
  acknowledge the risk involved in using this command and accepting the
  responsibility for that.
- <magic> is a decimal.
- <value> is a hex-as-string with no leading "0x".
- <offset> is a decimal (this field is optional and defaults to 0 if
  not specified.)

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 app/test-pmd/cmdline.c                      | 127 ++++++++++++++++++++
 app/test-pmd/config.c                       |  39 ++++++
 app/test-pmd/testpmd.h                      |   2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  18 +++
 4 files changed, 186 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 1fc1cce5fe..c917c4e83b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -170,6 +170,15 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"show port port_id (module_eeprom|eeprom)\n"
 			"    Display the module EEPROM or EEPROM information for port_id.\n\n"
 
+			"set port (port_id) eeprom (accept_risk) magic (magic_num)"
+			" value (value) offset (offset)\n"
+			"    Set the device eeprom for certain port.\nNote:\n"
+			"    This is a high-risk command and its misuse may"
+			" result in unexpected behaviour from the NIC.\n"
+			"    By inserting \"accept_risk\" into the command, the"
+			" user is acknowledging and taking responsibility for"
+			" this risk.\n\n"
+
 			"show port X rss reta (size) (mask0,mask1,...)\n"
 			"    Display the rss redirection table entry indicated"
 			" by masks on port X. size is used to indicate the"
@@ -7462,6 +7471,123 @@ static cmdline_parse_inst_t cmd_showeeprom = {
 	},
 };
 
+/* *** SET PORT EEPROM *** */
+struct cmd_seteeprom_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t port;
+	uint16_t portnum;
+	cmdline_fixed_string_t eeprom;
+	cmdline_fixed_string_t confirm_str;
+	cmdline_fixed_string_t magic_str;
+	uint32_t magic;
+	cmdline_fixed_string_t value;
+	cmdline_multi_string_t token_str;
+};
+
+static void cmd_seteeprom_parsed(void *parsed_result,
+		__rte_unused struct cmdline *cl,
+		__rte_unused void *data)
+{
+	struct cmd_seteeprom_result *res = parsed_result;
+	uint32_t offset = 0;
+	uint32_t length;
+	uint8_t *value;
+	char *token_str;
+	char *token;
+
+	token_str = res->token_str;
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+
+	/* Parse Hex string to Byte data */
+	if (strlen(token) % 2 != 0) {
+		fprintf(stderr, "Bad Argument: %s\nHex string must be in multiples of 2 Bytes\n",
+			token);
+		return;
+	}
+
+	length = strlen(token) / 2;
+	value = calloc(length, sizeof(uint8_t));
+	for (int count = 0; count < (int)(length); count++) {
+		if (sscanf(token, "%2hhx", &value[count]) != 1) {
+			fprintf(stderr, "Bad Argument: %s\nValue must be a hex string\n",
+				token - (count + 1));
+			goto out;
+		}
+		token += 2;
+	}
+
+	/* Second token: offset string */
+	token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+	if (token != NULL) {
+		if (strcmp(token, "offset") == 0) {
+			/* Third token: offset */
+			token = strtok_r(token_str, " \f\n\r\t\v", &token_str);
+			if (token == NULL) {
+				fprintf(stderr, "No offset specified\n");
+				goto out;
+			}
+
+			char *end;
+			offset = strtoul(token, &end, 10);
+			if (offset == 0 && strcmp(end, "") != 0) {
+				fprintf(stderr, "Bad Argument: %s\n", token);
+				goto out;
+			}
+		} else {
+			fprintf(stderr, "Bad Argument: %s\n", token);
+			goto out;
+		}
+	}
+
+	port_eeprom_set(res->portnum, res->magic, offset, length, value);
+
+out:
+	free(value);
+}
+
+static cmdline_parse_token_string_t cmd_seteeprom_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, set, "set");
+static cmdline_parse_token_string_t cmd_seteeprom_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, port, "port");
+static cmdline_parse_token_num_t cmd_seteeprom_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, portnum, RTE_UINT16);
+static cmdline_parse_token_string_t cmd_seteeprom_eeprom =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, eeprom, "eeprom");
+static cmdline_parse_token_string_t cmd_seteeprom_confirm_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, confirm_str, "accept_risk");
+static cmdline_parse_token_string_t cmd_seteeprom_magic_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, magic_str, "magic");
+static cmdline_parse_token_num_t cmd_seteeprom_magic =
+	TOKEN_NUM_INITIALIZER(struct cmd_seteeprom_result, magic, RTE_UINT32);
+static cmdline_parse_token_string_t cmd_seteeprom_value =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, value, "value");
+static cmdline_parse_token_string_t cmd_seteeprom_token_str =
+	TOKEN_STRING_INITIALIZER(struct cmd_seteeprom_result, token_str, TOKEN_STRING_MULTI);
+
+static cmdline_parse_inst_t cmd_seteeprom = {
+	.f = cmd_seteeprom_parsed,
+	.data = NULL,
+	.help_str = "set port <port_id> eeprom <accept_risk> magic <magic_num> "
+		"value <value> offset <offset>: Set eeprom value for port_id.\n"
+		"Note:\n"
+		"This is a high-risk command and its misuse may result in "
+		"unexpected behaviour from the NIC.\n"
+		"By inserting \"accept_risk\" into the command, the user is "
+		"acknowledging and taking responsibility for this risk.",
+	.tokens = {
+		(void *)&cmd_seteeprom_set,
+		(void *)&cmd_seteeprom_port,
+		(void *)&cmd_seteeprom_portnum,
+		(void *)&cmd_seteeprom_eeprom,
+		(void *)&cmd_seteeprom_confirm_str,
+		(void *)&cmd_seteeprom_magic_str,
+		(void *)&cmd_seteeprom_magic,
+		(void *)&cmd_seteeprom_value,
+		(void *)&cmd_seteeprom_token_str,
+		NULL,
+	},
+};
+
 /* *** SHOW QUEUE INFO *** */
 struct cmd_showqueue_result {
 	cmdline_fixed_string_t show;
@@ -13397,6 +13523,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	&cmd_showport,
 	&cmd_showqueue,
 	&cmd_showeeprom,
+	&cmd_seteeprom,
 	&cmd_showportall,
 	&cmd_representor_info,
 	&cmd_showdevice,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1d23feec2a..37f074a69b 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1066,6 +1066,45 @@ port_eeprom_display(portid_t port_id)
 	free(einfo.data);
 }
 
+void
+port_eeprom_set(portid_t port_id,
+		uint32_t magic,
+		uint32_t offset,
+		uint32_t length,
+		uint8_t *value)
+{
+	struct rte_dev_eeprom_info einfo;
+	int len_eeprom;
+	int ret;
+
+	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
+		print_valid_ports();
+		return;
+	}
+
+	len_eeprom = rte_eth_dev_get_eeprom_length(port_id);
+	if (len_eeprom < 0) {
+		fprintf(stderr, "Unable to get EEPROM length: %s\n",
+			rte_strerror(-len_eeprom));
+		return;
+	}
+
+	einfo.data = value;
+	einfo.magic = magic;
+	einfo.length = length;
+	einfo.offset = offset;
+
+	if (einfo.offset + einfo.length > (uint32_t)(len_eeprom)) {
+		fprintf(stderr, "offset and length exceed capabilities of EEPROM length: %d\n",
+			len_eeprom);
+		return;
+	}
+
+	ret = rte_eth_dev_set_eeprom(port_id, &einfo);
+	if (ret != 0)
+		fprintf(stderr, "Unable to set EEPROM: %s\n", rte_strerror(-ret));
+}
+
 void
 port_module_eeprom_display(portid_t port_id)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index f9ab88d667..56ab7ef1db 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -927,6 +927,8 @@ void device_infos_display(const char *identifier);
 void port_infos_display(portid_t port_id);
 void port_summary_display(portid_t port_id);
 void port_eeprom_display(portid_t port_id);
+void port_eeprom_set(portid_t port_id, uint32_t magic, uint32_t offset,
+		     uint32_t length, uint8_t *value);
 void port_module_eeprom_display(portid_t port_id);
 void port_summary_header_display(void);
 void rx_queue_infos_display(portid_t port_idi, uint16_t queue_id);
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f00ab07605..eb19e747b5 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -223,6 +223,24 @@ Display the EEPROM information of a port::
 
    testpmd> show port (port_id) (module_eeprom|eeprom)
 
+set eeprom
+~~~~~~~~~~
+
+Write a value to the device eeprom of a port at a specific offset::
+
+   testpmd> set port (port_id) eeprom (accept_risk) magic (magic_num) value (value) \
+            offset (offset)
+
+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.
+
+.. note::
+
+   This is a high-risk command and its misuse may result in unexpected
+   behaviour from the NIC. By inserting "accept_risk" into the command, the user
+   is acknowledging and taking responsibility for this risk.
+
 show port rss reta
 ~~~~~~~~~~~~~~~~~~
 
-- 
2.39.1

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

[Chaoyong He]

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                      | 52 +++++++++++++++++++++
 app/test-pmd/config.c                       | 20 ++++++++
 app/test-pmd/testpmd.h                      |  1 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
 4 files changed, 80 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index c917c4e83b..61c46d6394 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -674,6 +674,10 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"(max_thresh) (prob_inv)]\n"
 			"    Set congestion management configuration\n\n"
 
+			"set port (port_id) led (on|off)\n"
+			"    Set a controllable LED associated with a certain"
+			" port on or off.\n\n"
+
 			, list_pkt_forwarding_modes()
 		);
 	}
@@ -13512,6 +13516,53 @@ static cmdline_parse_inst_t cmd_config_tx_affinity_map = {
 	},
 };
 
+/* *** 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->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,
+	},
+};
+
 /* ******************************************************************************** */
 
 /* list of instructions */
@@ -13755,6 +13806,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	&cmd_show_port_cman_config,
 	&cmd_set_port_cman_config,
 	&cmd_config_tx_affinity_map,
+	&cmd_set_dev_led,
 	NULL,
 };
 
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 37f074a69b..88770b4dfc 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -5371,6 +5371,26 @@ 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: %s\n",
+			port_id, 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 56ab7ef1db..84517b0be7 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 eb19e747b5..0228a07701 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -1855,6 +1855,13 @@ during the flow rule creation::
 
 Otherwise the default index ``0`` is used.
 
+set port led
+~~~~~~~~~~~~
+
+Set a controllable LED associated with a certain port on or off::
+
+   testpmd> set port (port_id) led (on|off)
+
 Port Functions
 --------------
 
-- 
2.39.1

Re: [PATCH v5 0/2] add two commands for testpmd application

[Ferruh Yigit]

On 10/10/2024 7:42 AM, Chaoyong He wrote:
> This patch series aims to add two commands for the testpmd application:
> - testpmd> set port <port-id> eeprom <accept_risk> magic <magic> \
>           value <value> offset <offset>
> - testpmd> set port <port-id> led <on/off>
> 
> ---
> v5:
> * Use 'accept_risk' rather than 'confirm' to make it clear enough.
> * Use dynamic memory alloc rather than VLA.
> * Place the logic in the right place following the reviewer's request.
> v4:
> * Make the log more readable by using 'rte_strerror()'.
> * Make the modification of document following the rules.
> v3:
> * Add acknowledgement for the set eeprom command.
> * Modify logic following the request from reviewer.
> v2:
> * Solve the conflict in document file.
> ---
> 
> James Hershaw (2):
>   app/testpmd: add support for setting device EEPROM
>   app/testpmd: add set dev led on/off command
>

For series,
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Series applied to dpdk-next-net/main, thanks.