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