From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id BBE8C4598A; Sat, 14 Sep 2024 09:50:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9100F4065D; Sat, 14 Sep 2024 09:50:31 +0200 (CEST) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id B06D54026A for ; Sat, 14 Sep 2024 09:50:29 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.88.105]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4X5Nd151sDz69Tj; Sat, 14 Sep 2024 15:50:13 +0800 (CST) Received: from dggpeml500024.china.huawei.com (unknown [7.185.36.10]) by mail.maildlp.com (Postfix) with ESMTPS id 0DD3C140137; Sat, 14 Sep 2024 15:50:23 +0800 (CST) Received: from [10.67.121.161] (10.67.121.161) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Sat, 14 Sep 2024 15:50:22 +0800 Message-ID: <2b761946-456b-442f-ac2a-7948ef0eb06a@huawei.com> Date: Sat, 14 Sep 2024 15:50:22 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/2] app/testpmd: add support for setting device EEPROM To: Chaoyong He , CC: , James Hershaw References: <20240912065459.2858959-1-chaoyong.he@corigine.com> <20240914014913.2886626-1-chaoyong.he@corigine.com> <20240914014913.2886626-2-chaoyong.he@corigine.com> Content-Language: en-US From: fengchengwen In-Reply-To: <20240914014913.2886626-2-chaoyong.he@corigine.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.161] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To dggpeml500024.china.huawei.com (7.185.36.10) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2024/9/14 9:49, Chaoyong He wrote: > From: James Hershaw > > 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 eeprom magic \ > value offset > > - 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. > - is a decimal > - is a hex-as-string with no leading "0x" > - is a decimal (this field is optional and defaults to 0 if > not specified.) > > Signed-off-by: James Hershaw > Reviewed-by: Chaoyong He > --- > 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 eeprom magic " > + "value 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 eeprom magic \ > + value 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 > ~~~~~~~~~~~~ >