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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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.