On 9/18/2024 3:38 AM, Chaoyong He wrote:
> From: James Hershaw <james.hers...@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.hers...@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong...@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.
