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

[Chaoyong He <chaoyong.he@corigine.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/
> 

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.