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 A5FC0A0524; Mon, 19 Apr 2021 17:30:26 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8F4194130F; Mon, 19 Apr 2021 17:30:26 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 1ABD2412E6 for ; Mon, 19 Apr 2021 17:30:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1618846224; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xB7Mbwo/XOrmw02qv+sOC7w6Jh/7ZW302nt4AGx2vzU=; b=erhxh+NzytQAxmuXZrRCAPx0b3rBGHZ2VlEx1T6yKvpzqGFsh0+9tKZSdEBplpO0Hrj0M1 AigPyg1hTDuHgMVrXosuBvQ7vPGu2QKctT1rFva01rGIuEqLok0iqdh8zVJWNuaaCQRmvN qjx/MW/DMHVdrn3zqGi9PTjuY9ZWaxY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-288-W6UeeVahMUe9RCIiJCdfFg-1; Mon, 19 Apr 2021 11:30:17 -0400 X-MC-Unique: W6UeeVahMUe9RCIiJCdfFg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7FA86107ACED; Mon, 19 Apr 2021 15:30:16 +0000 (UTC) Received: from [10.36.112.162] (ovpn-112-162.ams2.redhat.com [10.36.112.162]) by smtp.corp.redhat.com (Postfix) with ESMTP id 16804107D5CB; Mon, 19 Apr 2021 15:30:14 +0000 (UTC) To: "Min Hu (Connor)" , dev@dpdk.org Cc: ferruh.yigit@intel.com, xiaoyun.li@intel.com References: <1618469214-35316-1-git-send-email-humin29@huawei.com> From: Kevin Traynor Message-ID: Date: Mon, 19 Apr 2021 16:30:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <1618469214-35316-1-git-send-email-humin29@huawei.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ktraynor@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] app/testpmd: support the query of link flow ctrl info 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 Sender: "dev" On 15/04/2021 07:46, Min Hu (Connor) wrote: > From: Huisong Li > > This patch supports the query of the link flow control parameter > on a port. > > The command format is as follows: > show port flow_ctrl > > Signed-off-by: Huisong Li > Signed-off-by: Min Hu (Connor) Hi Connor, Just a few minor comments, thanks, Kevin. > --- > app/test-pmd/cmdline.c | 83 +++++++++++++++++++++++++++++ > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 7 +++ > 2 files changed, 90 insertions(+) > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c > index 5bf1497..9fa85c4 100644 > --- a/app/test-pmd/cmdline.c > +++ b/app/test-pmd/cmdline.c > @@ -258,6 +258,9 @@ static void cmd_help_long_parsed(void *parsed_result, > > "show port (port_id) fec_mode" > " Show fec mode of a port.\n\n" > + > + "show port flow_ctrl" > + " Show flow control info of a port.\n\n" > ); > } > > @@ -6863,6 +6866,85 @@ cmdline_parse_inst_t cmd_set_allmulti_mode_one = { > }, > }; > > +/* *** GET CURRENT ETHERNET LINK FLOW CONTROL *** */ > +struct cmd_link_flow_ctrl_show { > + cmdline_fixed_string_t show; > + cmdline_fixed_string_t port; > + portid_t port_id; > + cmdline_fixed_string_t flow_ctrl; > +}; > + > +cmdline_parse_token_string_t cmd_lfc_show_show = > + TOKEN_STRING_INITIALIZER(struct cmd_link_flow_ctrl_show, > + show, "show"); > +cmdline_parse_token_string_t cmd_lfc_show_port = > + TOKEN_STRING_INITIALIZER(struct cmd_link_flow_ctrl_show, > + port, "port"); > +cmdline_parse_token_num_t cmd_lfc_show_portid = > + TOKEN_NUM_INITIALIZER(struct cmd_link_flow_ctrl_show, > + port_id, RTE_UINT16); > +cmdline_parse_token_string_t cmd_lfc_show_flow_ctrl = > + TOKEN_STRING_INITIALIZER(struct cmd_link_flow_ctrl_show, > + flow_ctrl, "flow_ctrl"); > + > +static void > +cmd_link_flow_ctrl_show_parsed(void *parsed_result, > + __rte_unused struct cmdline *cl, > + __rte_unused void *data) > +{ > + struct cmd_link_flow_ctrl_show *res = parsed_result; > + static const char *info_border = "*********************"; > + struct rte_eth_fc_conf fc_conf; > + bool rx_fc_en = false; > + bool tx_fc_en = false; > + int ret; > + > + if (!rte_eth_dev_is_valid_port(res->port_id)) { This ^ is already checked in rte_eth_dev_flow_ctrl_get() below. That said, it's not doing any harm here and you would need to add check for -ENODEV below if you wanted to tell the user that the port is invalid so either way is ok IMHO. > + printf("Invalid port id %u\n", res->port_id); > + return; > + } > + > + ret = rte_eth_dev_flow_ctrl_get(res->port_id, &fc_conf); > + if (ret != 0) { > + printf("Failed to get current flow ctrl information: err = %d\n", > + ret); > + return; > + } > + > + if ((fc_conf.mode == RTE_FC_RX_PAUSE) || (fc_conf.mode == RTE_FC_FULL)) You can remove unnecessary brackets, i.e. if (fc_conf.mode == RTE_FC_RX_PAUSE || fc_conf.mode == RTE_FC_FULL) > + rx_fc_en = true; > + if ((fc_conf.mode == RTE_FC_TX_PAUSE) || (fc_conf.mode == RTE_FC_FULL)) same comment on brackets > + tx_fc_en = true; > + > + printf("\n%s Flow control infos for port %-2d %s\n", s/infos/info/ > + info_border, res->port_id, info_border); > + printf("FC mode:\n"); It's better not to introduce a new acronym (even if it's a bit obvious) > + printf(" Rx: %s\n", rx_fc_en ? "On" : "Off"); > + printf(" Tx: %s\n", tx_fc_en ? "On" : "Off"); > + printf("FC autoneg status: %s\n", fc_conf.autoneg != 0 ? "On" : "Off"); > + printf("pause_time: 0x%x\n", fc_conf.pause_time); > + printf("high_water: 0x%x\n", fc_conf.high_water); > + printf("low_water: 0x%x\n", fc_conf.low_water); > + printf("Send Xon: %s\n", fc_conf.send_xon ? "On" : "Off"); > + printf("mac ctrl frame fwd: %s\n", > + fc_conf.mac_ctrl_frame_fwd ? "On" : "Off"); There's a mix of struct member names and descriptions here. I think it's better to stick to one or the other. i.e. currently testpmd> show port 0 flow_ctrl ********************* Flow control infos for port 0 ********************* FC mode: Rx: Off Tx: Off FC autoneg status: On pause_time: 0x680 high_water: 0x80 low_water: 0x40 Send Xon: On Forward MAC control frames: Off Suggestion: ********************* Flow control info for port 0 ********************* Rx mode: off Tx mode: off Autoneg: on Pause time: 0x680 High water: 0x80 Low water: 0x40 Send XON: on Forward MAC control frames: off *********************************** End ******************************** As ever, display is subjective, so please take as suggestion, you/others may have different view. > + printf("\n%s************** End ***********%s\n", > + info_border, info_border); > +} > + > +cmdline_parse_inst_t cmd_link_flow_control_show = { > + .f = cmd_link_flow_ctrl_show_parsed, > + .data = NULL, > + .help_str = "show port flow_ctrl", > + .tokens = { > + (void *)&cmd_lfc_show_show, > + (void *)&cmd_lfc_show_port, > + (void *)&cmd_lfc_show_portid, > + (void *)&cmd_lfc_show_flow_ctrl, > + NULL, > + }, > +}; > + > /* *** SETUP ETHERNET LINK FLOW CONTROL *** */ > struct cmd_link_flow_ctrl_set_result { > cmdline_fixed_string_t set; > @@ -17001,6 +17083,7 @@ cmdline_parse_ctx_t main_ctx[] = { > (cmdline_parse_inst_t *)&cmd_link_flow_control_set_xon, > (cmdline_parse_inst_t *)&cmd_link_flow_control_set_macfwd, > (cmdline_parse_inst_t *)&cmd_link_flow_control_set_autoneg, > + (cmdline_parse_inst_t *)&cmd_link_flow_control_show, > (cmdline_parse_inst_t *)&cmd_priority_flow_control_set, > (cmdline_parse_inst_t *)&cmd_config_dcb, > (cmdline_parse_inst_t *)&cmd_read_reg, > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index e3bfed5..3fca011 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -1526,6 +1526,13 @@ Where: > > * ``autoneg``: Change the auto-negotiation parameter. > > +show flow ctrl > +~~~~~~~~~~~~~~ > + > +show the link flow control parameter on a port:: > + > + testpmd> show port flow_ctrl > + > set pfc_ctrl rx > ~~~~~~~~~~~~~~~ > >