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 4364EA0548; Tue, 20 Apr 2021 14:11:54 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C90F341749; Tue, 20 Apr 2021 14:11:53 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mails.dpdk.org (Postfix) with ESMTP id B273841735 for ; Tue, 20 Apr 2021 14:11:51 +0200 (CEST) IronPort-SDR: 1HsrnIbr5QzMHUOLrUQ6Q8yJGPrhajRDX4yMCFdIO8K9TvuqPavjH7wubGx1NXJRsODHuxNaWm LOGjO0JqglhA== X-IronPort-AV: E=McAfee;i="6200,9189,9959"; a="175601013" X-IronPort-AV: E=Sophos;i="5.82,236,1613462400"; d="scan'208";a="175601013" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Apr 2021 05:11:50 -0700 IronPort-SDR: lYbo37LVkCQ7YYqyw0VNok4nK+dAPBdgAfTzH5rgqYJBcb+aR3RhJrmUfTKQrGaRa6MMvNTyI7 zs8nZfaKiAqA== X-IronPort-AV: E=Sophos;i="5.82,236,1613462400"; d="scan'208";a="420384584" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.207.27]) ([10.213.207.27]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Apr 2021 05:11:49 -0700 To: "Min Hu (Connor)" , dev@dpdk.org Cc: xiaoyun.li@intel.com References: <1618813303-32945-1-git-send-email-humin29@huawei.com> <1618813303-32945-2-git-send-email-humin29@huawei.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <7272e681-af7c-c700-1107-66ad1ae7d051@intel.com> Date: Tue, 20 Apr 2021 13:11:46 +0100 MIME-Version: 1.0 In-Reply-To: <1618813303-32945-2-git-send-email-humin29@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: add link speed check before port start 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/19/2021 7:21 AM, Min Hu (Connor) wrote: > From: Huisong Li > > Currently, to check whether the configured link_speeds is valid, we > have to run "port start". In addition, if the configuration fails, > "port->dev_conf.link_speeds" maintained in testpmd cannot be > restored. > > This patch adds the link_speeds check before port start by calling > dev_configure, and resolves these problems. > Hi Connor, I am not sure about this patch, overall design in many commands seems store the config changes and mark port or queue as needs reconfiguration ('cmd_reconfig_device_queue()'), and do the reconfigure during port_start. Your change is against this overall design. What is the downside of learning that speed is invalid in port start? To not able to restore the previous speed is not good, but user can set the desired speed again in case of failure. And do you think adding a check that if the requested speed is in the device reported speed capability help on early fail and restore? > Signed-off-by: Huisong Li > Signed-off-by: Min Hu (Connor) > --- > app/test-pmd/cmdline.c | 40 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 38 insertions(+), 2 deletions(-) > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c > index 5bf1497..09e30cf 100644 > --- a/app/test-pmd/cmdline.c > +++ b/app/test-pmd/cmdline.c > @@ -1537,8 +1537,12 @@ cmd_config_speed_all_parsed(void *parsed_result, > __rte_unused void *data) > { > struct cmd_config_speed_all *res = parsed_result; > + uint32_t old_link_speeds[RTE_MAX_ETHPORTS]; > + struct rte_port *port; > uint32_t link_speed; > portid_t pid; > + portid_t i; > + int ret; > > if (!all_ports_stopped()) { > printf("Please stop all ports first\n"); > @@ -1550,7 +1554,26 @@ cmd_config_speed_all_parsed(void *parsed_result, > return; > > RTE_ETH_FOREACH_DEV(pid) { > - ports[pid].dev_conf.link_speeds = link_speed; > + port = &ports[pid]; > + old_link_speeds[pid] = port->dev_conf.link_speeds; > + port->dev_conf.link_speeds = link_speed; > + ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq, > + &port->dev_conf); > + if (ret < 0) { > + printf("Failed to check link speeds for port %d, ret = %d.\n", > + pid, ret); > + goto roolback; > + } > + } > + > + cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); > + > + return; > + > +roolback: > + for (i = 0; i <= pid; i++) { > + port = &ports[i]; > + port->dev_conf.link_speeds = old_link_speeds[i]; > } > > cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); > @@ -1609,7 +1632,10 @@ cmd_config_speed_specific_parsed(void *parsed_result, > __rte_unused void *data) > { > struct cmd_config_speed_specific *res = parsed_result; > + uint32_t old_link_speeds; > + struct rte_port *port; > uint32_t link_speed; > + int ret; > > if (!all_ports_stopped()) { > printf("Please stop all ports first\n"); > @@ -1623,7 +1649,17 @@ cmd_config_speed_specific_parsed(void *parsed_result, > &link_speed) < 0) > return; > > - ports[res->id].dev_conf.link_speeds = link_speed; > + port = &ports[res->id]; > + old_link_speeds = port->dev_conf.link_speeds; > + port->dev_conf.link_speeds = link_speed; > + ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq, > + &port->dev_conf); > + if (ret < 0) { > + printf("Failed to check link speeds for port %d, ret = %d.\n", > + res->id, ret); > + port->dev_conf.link_speeds = old_link_speeds; > + return; > + } > > cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); > } >