From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id D66B91BBE for ; Wed, 8 Mar 2017 12:07:25 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP; 08 Mar 2017 03:07:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,262,1486454400"; d="scan'208";a="233656535" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.254.176.34]) ([10.254.176.34]) by fmsmga004.fm.intel.com with ESMTP; 08 Mar 2017 03:07:22 -0800 To: Wei Zhao , dev@dpdk.org References: <1488516984-34702-1-git-send-email-wei.zhao1@intel.com> <1488516984-34702-2-git-send-email-wei.zhao1@intel.com> Cc: Wenzhuo Lu From: Ferruh Yigit Message-ID: <3a152048-f129-33fd-d31e-4220c505fe92@intel.com> Date: Wed, 8 Mar 2017 11:07:21 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <1488516984-34702-2-git-send-email-wei.zhao1@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 1/3] app/testpmd: add port reset command into testpmd 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: , X-List-Received-Date: Wed, 08 Mar 2017 11:07:26 -0000 On 3/3/2017 4:56 AM, Wei Zhao wrote: > Add vf port reset command into testpmd project, it is the interface for > user to reset vf port. I think it is better to change the order of this patch, first implement new API in ethdev, later this patch implement new API in testpmd. > > Signed-off-by: Wei Zhao > Signed-off-by: Wenzhuo Lu > --- > app/test-pmd/cmdline.c | 17 ++++++++++--- > app/test-pmd/testpmd.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ > app/test-pmd/testpmd.h | 1 + > 3 files changed, 81 insertions(+), 4 deletions(-) > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c > index 43fc636..59db672 100644 > --- a/app/test-pmd/cmdline.c > +++ b/app/test-pmd/cmdline.c > @@ -596,6 +596,9 @@ static void cmd_help_long_parsed(void *parsed_result, > "port close (port_id|all)\n" > " Close all ports or port_id.\n\n" > > + "port reset (port_id|all)\n" > + " Reset all ports or port_id.\n\n" It is not clear what reset does to the port. This is only for VF right? Adding reset here hides that it is for VF. <...> > @@ -601,6 +602,7 @@ init_config(void) > if (init_fwd_streams() < 0) > rte_exit(EXIT_FAILURE, "FAIL from init_fwd_streams()\n"); > > + This may be unintentional. <...> > @@ -1350,6 +1363,10 @@ start_port(portid_t pid) > return -1; > } > } > + > + /* register reset interrupt callback */ > + rte_eth_dev_callback_register(pi, RTE_ETH_EVENT_INTR_RESET, > + reset_event_callback, NULL); So each port started will register a callback to handle reset events, 1- isn't this overkill for the usecases that does not need this reset? 2- should there be an unregister event? 3- This issue can be fixed in testpmd, but for user application, is this the suggested way? > if (port->need_reconfig_queues > 0) { > port->need_reconfig_queues = 0; > /* setup tx queues */ > @@ -1559,6 +1576,56 @@ close_port(portid_t pid) > } > > void > +reset_port(portid_t pid) > +{ > + portid_t pi; > + struct rte_port *port; > + > + if (port_id_is_invalid(pid, ENABLED_WARN)) > + return; > + > + printf("Closing ports...\n"); > + > + FOREACH_PORT(pi, ports) { Since we already know the port_id (pid), why iterating through all ports? > + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > + continue; > + > + if (port_is_forwarding(pi) != 0 && test_done == 0) { > + printf("Please remove port %d from forwarding " > + "configuration.\n", pi); > + continue; > + } > + > + if (port_is_bonding_slave(pi)) { > + printf("Please remove port %d from " > + "bonded device.\n", pi); > + continue; > + } > + > + if (!reset_ports[pi]) { > + printf("vf must get reset port %d info from " > + "pf before reset.\n", pi); > + continue; > + } Can there be a timing issue here? Is it possible that reset occurred already and we are in the middle of the callback function when this check done? <...>