From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1C6A0A0350; Tue, 30 Jun 2020 08:20:09 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 800241B9BF; Tue, 30 Jun 2020 08:20:08 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 82F032C01 for ; Tue, 30 Jun 2020 08:20:06 +0200 (CEST) IronPort-SDR: CQd9kcnaTWhweno48Isgte4KyzzJW7Q458e2BnqK5O0pBMbVyRMf0Y2Ly3Je2go+eVWXeeJLNA X3fqhi2CXPUg== X-IronPort-AV: E=McAfee;i="6000,8403,9666"; a="146146228" X-IronPort-AV: E=Sophos;i="5.75,296,1589266800"; d="scan'208";a="146146228" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jun 2020 23:20:04 -0700 IronPort-SDR: xVzNchFsL2HODeZNVlwNvR80edqvMIR8yRIJ9/ROswZ8rBePMeUnm0IrdbZ/ob8p1fn0Uydaqg ukH/RCDbQo9w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,296,1589266800"; d="scan'208";a="281117873" Received: from jguo15x-mobl.ccr.corp.intel.com (HELO [10.67.68.180]) ([10.67.68.180]) by orsmga006.jf.intel.com with ESMTP; 29 Jun 2020 23:20:02 -0700 To: "Di, ChenxuX" , Kevin Traynor , "dev@dpdk.org" Cc: "Xing, Beilei" , "Yang, Qiming" References: <20200611060142.75465-1-chenxux.di@intel.com> <20200611060142.75465-4-chenxux.di@intel.com> From: Jeff Guo Message-ID: <87a24434-7b7d-d99c-46c9-765512ca7a13@intel.com> Date: Tue, 30 Jun 2020 14:20:01 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH 3/5] app/testpmd: re-implement commands by using private API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" hi, chenxu On 6/17/2020 6:12 PM, Di, ChenxuX wrote: > >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Wednesday, June 17, 2020 4:12 AM >> To: Di, ChenxuX ; dev@dpdk.org >> Cc: Xing, Beilei ; Yang, Qiming >> Subject: Re: [dpdk-dev] [PATCH 3/5] app/testpmd: re-implement commands by >> using private API >> >> On 11/06/2020 07:01, Chenxu Di wrote: >>> The legacy filter API will be superseded. This patch use private api >>> to change the implementation of commands global_config >>> gre-key-len and show port fdir >>> >>> Signed-off-by: Chenxu Di >>> --- >>> app/test-pmd/cmdline.c | 23 ++++++++----- app/test-pmd/config.c | >>> 74 +++++++++++++++++++++++++++++++++++------- >>> 2 files changed, 78 insertions(+), 19 deletions(-) >>> >>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index >>> 996a49876..01d793e89 100644 >>> --- a/app/test-pmd/cmdline.c >>> +++ b/app/test-pmd/cmdline.c >>> @@ -9280,15 +9280,22 @@ cmd_global_config_parsed(void *parsed_result, >>> { >>> struct cmd_global_config_result *res = parsed_result; >>> struct rte_eth_global_cfg conf; >>> - int ret; >>> + int ret = -ENOTSUP; >>> >>> - memset(&conf, 0, sizeof(conf)); >>> - conf.cfg_type = RTE_ETH_GLOBAL_CFG_TYPE_GRE_KEY_LEN; >>> - conf.cfg.gre_key_len = res->len; >>> - ret = rte_eth_dev_filter_ctrl(res->port_id, RTE_ETH_FILTER_NONE, >>> - RTE_ETH_FILTER_SET, &conf); >>> - if (ret != 0) >>> - printf("Global config error\n"); >>> +#ifdef RTE_LIBRTE_I40E_PMD >>> + if (ret == -ENOTSUP) you init ret to be default value and then check if ret to be default value, it that need? >>> + ret = rte_pmd_i40e_set_gre_key_len(res->port_id, res->len); >> #endif >>> + if (ret == -ENOTSUP) { >>> + memset(&conf, 0, sizeof(conf)); >>> + conf.cfg_type = RTE_ETH_GLOBAL_CFG_TYPE_GRE_KEY_LEN; >>> + conf.cfg.gre_key_len = res->len; >>> + ret = rte_eth_dev_filter_ctrl(res->port_id, >> RTE_ETH_FILTER_NONE, >>> + RTE_ETH_FILTER_SET, &conf); >>> + } >>> + >>> + if (ret < 0) >>> + printf("Global config error: (%s)\n", strerror(-ret)); >>> } >>> >>> cmdline_parse_token_string_t cmd_global_config_cmd = diff --git >>> a/app/test-pmd/config.c b/app/test-pmd/config.c index >>> 016bcb09c..f519246c7 100644 >>> --- a/app/test-pmd/config.c >>> +++ b/app/test-pmd/config.c >>> @@ -3729,30 +3729,82 @@ print_fdir_flow_type(uint32_t flow_types_mask) >>> printf("\n"); >>> } >>> >>> +static int >>> +get_fdir_info(portid_t port_id, struct rte_eth_fdir_info *fdir_info) >>> +{ >>> + int ret; >> this looks like it won't compile if the RTE_LIBRTE_I40E_PMD is undefined, you'll >> need to initialize ret >> > Yes, got it, I will fix it in next version patch. > >>> +#ifdef RTE_LIBRTE_I40E_PMD >>> + ret = rte_pmd_i40e_get_fdir_info(port_id, fdir_info); #endif #ifdef >>> +RTE_LIBRTE_IXGBE_PMD >>> + if (ret == -ENOTSUP) Suggest not init ret to be -ENOTSUP, so this checking of if (ret == -ENOTSUP) is no need. >>> + ret = rte_pmd_ixgbe_get_fdir_info(port_id, fdir_info); #endif >>> + if (ret == -ENOTSUP) { >>> + ret = rte_eth_dev_filter_supported(port_id, >>> + RTE_ETH_FILTER_FDIR); >>> + if (ret < 0) { >>> + printf("\n FDIR is not supported on port %-2d\n", >>> + port_id); >>> + return ret; >>> + } >>> + rte_eth_dev_filter_ctrl(port_id, RTE_ETH_FILTER_FDIR, >>> + RTE_ETH_FILTER_INFO, fdir_info); >>> + } >>> + >>> + if (ret < 0) >>> + printf("Get fdir infos error: (%s)\n", strerror(-ret)); >>> + >>> + return ret; >>> +} >>> + >>> +static int >>> +get_fdir_stat(portid_t port_id, struct rte_eth_fdir_stats *fdir_stat) >>> +{ >>> + int ret; >> Same comment as above >> > OK > >>> +#ifdef RTE_LIBRTE_I40E_PMD >>> + ret = rte_pmd_i40e_get_fdir_stats(port_id, fdir_stat); #endif #ifdef >>> +RTE_LIBRTE_IXGBE_PMD >>> + if (ret == -ENOTSUP) >>> + ret = rte_pmd_ixgbe_get_fdir_stats(port_id, fdir_stat); #endif >>> + if (ret == -ENOTSUP) { >>> + ret = rte_eth_dev_filter_supported(port_id, >>> + RTE_ETH_FILTER_FDIR); >>> + if (ret < 0) { >>> + printf("\n FDIR is not supported on port %-2d\n", >>> + port_id); >>> + return ret; >>> + } >>> + rte_eth_dev_filter_ctrl(port_id, RTE_ETH_FILTER_FDIR, >>> + RTE_ETH_FILTER_STATS, fdir_stat); >>> + } >>> + >>> + if (ret < 0) if (ret) should be enough. >>> + printf("Get fdir infos error: (%s)\n", strerror(-ret)); >>> + >>> + return ret; >>> +} >>> + >>> void >>> fdir_get_infos(portid_t port_id) >>> { >>> struct rte_eth_fdir_stats fdir_stat; >>> struct rte_eth_fdir_info fdir_info; >>> - int ret; >>> >>> static const char *fdir_stats_border = "########################"; >>> >>> if (port_id_is_invalid(port_id, ENABLED_WARN)) >>> return; >>> - ret = rte_eth_dev_filter_supported(port_id, RTE_ETH_FILTER_FDIR); >>> - if (ret < 0) { >>> - printf("\n FDIR is not supported on port %-2d\n", >>> - port_id); >>> - return; >>> - } >>> >>> memset(&fdir_info, 0, sizeof(fdir_info)); >>> - rte_eth_dev_filter_ctrl(port_id, RTE_ETH_FILTER_FDIR, >>> - RTE_ETH_FILTER_INFO, &fdir_info); >>> + if (get_fdir_info(port_id, &fdir_info)) >>> + return; Suggest print error log out of the caller but not in the calling, it should be better. >>> memset(&fdir_stat, 0, sizeof(fdir_stat)); >>> - rte_eth_dev_filter_ctrl(port_id, RTE_ETH_FILTER_FDIR, >>> - RTE_ETH_FILTER_STATS, &fdir_stat); >>> + if (get_fdir_stat(port_id, &fdir_stat)) >>> + return; >>> + >>> printf("\n %s FDIR infos for port %-2d %s\n", >>> fdir_stats_border, port_id, fdir_stats_border); >>> printf(" MODE: "); >>>