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

[fengchengwen <fengchengwen@huawei.com>]

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