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 18B04A0548; Wed, 21 Apr 2021 10:05:58 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DABA84195D; Wed, 21 Apr 2021 10:05:57 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id 1862940140 for ; Wed, 21 Apr 2021 10:05:55 +0200 (CEST) IronPort-SDR: RPoH0egVbmGzCoMtiTThmM/eKY1ka+kt9hKvUZ6r7OsfuLoSZWPvk3N+nWanXmOIIr6ZZMecjb U5A1LvYizMyw== X-IronPort-AV: E=McAfee;i="6200,9189,9960"; a="216294307" X-IronPort-AV: E=Sophos;i="5.82,238,1613462400"; d="scan'208";a="216294307" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Apr 2021 01:05:55 -0700 IronPort-SDR: 97nR9ctiQFe+XULViaw04vA/pihUAXdn8QyfKOTe/Re6+o8oKKJzTDGUhfupswHOgcdF52i8iI 7OiAQB7odcLQ== X-IronPort-AV: E=Sophos;i="5.82,238,1613462400"; d="scan'208";a="455238597" Received: from yvankina-mobl.ger.corp.intel.com (HELO [10.213.206.218]) ([10.213.206.218]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Apr 2021 01:05:53 -0700 From: Ferruh Yigit To: Huisong Li , Kevin Traynor Cc: xiaoyun.li@intel.com, "Min Hu (Connor)" , dev@dpdk.org References: <1618469214-35316-1-git-send-email-humin29@huawei.com> <08a3555c-cc5f-5ba0-3d8f-cf5270703ade@huawei.com> X-User: ferruhy Message-ID: <29c696ff-f737-12eb-4d9a-2c84ab24c1b5@intel.com> Date: Wed, 21 Apr 2021 09:05:50 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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 4/21/2021 8:56 AM, Ferruh Yigit wrote: > On 4/21/2021 7:18 AM, Huisong Li wrote: >> Hi Kevin, >> >> Thank you for your review. >> >> This patchset has been applied to dpdk-next-net/main. I will send a new patch >> to fix it. >> >> Your suggestion is better.  I intend to use the following format: >> >> ********************* Flow control info for port 0 ********************* >> FC mode: >>      Rx pause: off >>      Tx pause: off >> Autoneg: on >> Pause time: 0x680 >> High waterline: 0x80 >> Low waterline: 0x40 >> Send XON: on >> Forward MAC control frames: off >> *********************************** End ******************************** >> > > I missed the outstanding comments from Kevin, I will drop the patch from > next-net, we can proceed with next version. > v2 dropped from tree, patchwork status updated. Please send v3. >> >> 在 2021/4/19 23:30, Kevin Traynor 写道: >>> 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 >>>>   ~~~~~~~~~~~~~~~ >>>> >>> . >