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 124A64598A; Sat, 14 Sep 2024 09:27:16 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F285A4065D; Sat, 14 Sep 2024 09:27:15 +0200 (CEST) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mails.dpdk.org (Postfix) with ESMTP id 62FB94026A for ; Sat, 14 Sep 2024 09:27:14 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.88.214]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4X5N6G5mQpz20mvC; Sat, 14 Sep 2024 15:27:02 +0800 (CST) Received: from dggpeml500024.china.huawei.com (unknown [7.185.36.10]) by mail.maildlp.com (Postfix) with ESMTPS id E43F11A016C; Sat, 14 Sep 2024 15:27:11 +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:27:11 +0800 Message-ID: Date: Sat, 14 Sep 2024 15:27:11 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 2/2] app/testpmd: add set dev led on/off command To: Chaoyong He , CC: , James Hershaw References: <20240912065459.2858959-1-chaoyong.he@corigine.com> <20240914014913.2886626-1-chaoyong.he@corigine.com> <20240914014913.2886626-3-chaoyong.he@corigine.com> Content-Language: en-US From: fengchengwen In-Reply-To: <20240914014913.2886626-3-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 there are three minor comments, with these fixed, Acked-by: Chengwen Feng On 2024/9/14 9:49, Chaoyong He wrote: > From: James Hershaw > > Add command to change the state of a controllable LED on an ethdev port > to on/off. This is for the purpose of identifying which physical port is > associated with an ethdev. > > Usage: > testpmd> set port led > > Signed-off-by: James Hershaw > Reviewed-by: Chaoyong He > --- > app/test-pmd/cmdline.c | 55 +++++++++++++++++++++ > app/test-pmd/config.c | 19 +++++++ > app/test-pmd/testpmd.h | 1 + > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 7 +++ > 4 files changed, 82 insertions(+) > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c > index 51c72b0826..9357abb036 100644 > --- a/app/test-pmd/cmdline.c > +++ b/app/test-pmd/cmdline.c > @@ -523,6 +523,10 @@ static void cmd_help_long_parsed(void *parsed_result, > " user is acknowledging and taking responsibility for" > " this risk.\n\n" > > + "set port (port_id) led (on|off)\n" > + " set a controllable LED associated with a certain\n" No need to split up two line by \n > + " port on or off.\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" > @@ -7609,6 +7613,56 @@ static cmdline_parse_inst_t cmd_seteeprom = { > }, > }; > > +/* *** SET THE LED FOR CERTAIN PORT TO ON/OFF *** */ > +struct cmd_dev_led_result { > + cmdline_fixed_string_t set; > + cmdline_fixed_string_t port; > + portid_t port_id; > + cmdline_fixed_string_t led; > + cmdline_fixed_string_t state; > +}; > + > +static void > +cmd_set_dev_led_parsed(void *parsed_result, > + __rte_unused struct cmdline *cl, > + __rte_unused void *data) > +{ > + struct cmd_dev_led_result *res = parsed_result; > + > + if (strcmp(res->led, "led") != 0) > + return; No need to add such judgement. because static cmdline_parse_token_string_t cmd_dev_led = TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, led, "led"); already limit this parameter must be "led" > + > + if (strcmp(res->state, "on") == 0) > + set_dev_led(res->port_id, true); > + else > + set_dev_led(res->port_id, false); > +} > + > +static cmdline_parse_token_string_t cmd_dev_led_set = > + TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, set, "set"); > +static cmdline_parse_token_string_t cmd_dev_led_port = > + TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, port, "port"); > +static cmdline_parse_token_num_t cmd_dev_led_port_id = > + TOKEN_NUM_INITIALIZER(struct cmd_dev_led_result, port_id, RTE_UINT16); > +static cmdline_parse_token_string_t cmd_dev_led = > + TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, led, "led"); > +static cmdline_parse_token_string_t cmd_dev_state = > + TOKEN_STRING_INITIALIZER(struct cmd_dev_led_result, state, "on#off"); > + > +static cmdline_parse_inst_t cmd_set_dev_led = { > + .f = cmd_set_dev_led_parsed, > + .data = NULL, > + .help_str = "set port led ", > + .tokens = { > + (void *)&cmd_dev_led_set, > + (void *)&cmd_dev_led_port, > + (void *)&cmd_dev_led_port_id, > + (void *)&cmd_dev_led, > + (void *)&cmd_dev_state, > + NULL, > + }, > +}; > + > /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */ > struct cmd_set_qmap_result { > cmdline_fixed_string_t set; > @@ -13348,6 +13402,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = { > &cmd_stop, > &cmd_mac_addr, > &cmd_set_fwd_eth_peer, > + &cmd_set_dev_led, > &cmd_set_qmap, > &cmd_set_xstats_hide_zero, > &cmd_set_record_core_cycles, > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index fb1a1485e2..ab9b99ef0c 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -5385,6 +5385,25 @@ set_fwd_eth_peer(portid_t port_id, char *peer_addr) > peer_eth_addrs[port_id] = new_peer_addr; > } > > +void > +set_dev_led(portid_t port_id, bool active) > +{ > + int ret; > + > + if (!rte_eth_dev_is_valid_port(port_id)) { > + fprintf(stderr, "Error: Invalid port number %u\n", port_id); > + return; > + } > + > + if (active) > + ret = rte_eth_led_on(port_id); > + else > + ret = rte_eth_led_off(port_id); > + > + if (ret < 0) > + fprintf(stderr, "Error: Unable to change LED state for port %u\n", port_id); It's better add reason by rte_strerror(-ret) > +} > + > int > set_fwd_lcores_list(unsigned int *lcorelist, unsigned int nb_lc) > { > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h > index 1292d1a0a7..aa1f0e9dd1 100644 > --- a/app/test-pmd/testpmd.h > +++ b/app/test-pmd/testpmd.h > @@ -944,6 +944,7 @@ int init_fwd_streams(void); > void update_fwd_ports(portid_t new_pid); > > void set_fwd_eth_peer(portid_t port_id, char *peer_addr); > +void set_dev_led(portid_t port_id, bool active); > > void port_mtu_set(portid_t port_id, uint16_t mtu); > int port_action_handle_create(portid_t port_id, uint32_t id, bool indirect_list, > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index f9dd380ea0..1c416686c5 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -1375,6 +1375,13 @@ 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. > > +set port led > +~~~~~~~~~~~~ > + > +Set a controllable LED associated with a certain port on or off:: > + > + testpmd> set port (port_id) led (on|off) > + > set port-uta > ~~~~~~~~~~~~ >