From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Viktorin Subject: Re: [PATCH v6 16/17] ethdev: convert to eal hotplug Date: Fri, 15 Jul 2016 13:27:59 +0200 Message-ID: <20160715132759.292d0103@pcviktorin.fit.vutbr.cz> References: <1466510566-9240-1-git-send-email-shreyansh.jain@nxp.com> <1468303282-2806-1-git-send-email-shreyansh.jain@nxp.com> <1468303282-2806-17-git-send-email-shreyansh.jain@nxp.com> <20160714185148.738f1fa3@jvn> <5788BCA9.5010205@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: , , To: Shreyansh jain Return-path: Received: from wes1-so1.wedos.net (wes1-so1.wedos.net [46.28.106.15]) by dpdk.org (Postfix) with ESMTP id 288E85587 for ; Fri, 15 Jul 2016 13:28:36 +0200 (CEST) In-Reply-To: <5788BCA9.5010205@nxp.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, 15 Jul 2016 16:06:25 +0530 Shreyansh jain wrote: > On Thursday 14 July 2016 10:21 PM, Jan Viktorin wrote: > > On Tue, 12 Jul 2016 11:31:21 +0530 > > Shreyansh Jain wrote: > > > >> Remove bus logic from ethdev hotplug by using eal for this. > >> > >> Current api is preserved: > >> - the last port that has been created is tracked to return it to the > >> application when attaching, > >> - the internal device name is reused when detaching. > >> > >> We can not get rid of ethdev hotplug yet since we still need some mechanism > >> to inform applications of port creation/removal to substitute for ethdev > >> hotplug api. > >> > >> dev_type field in struct rte_eth_dev and rte_eth_dev_allocate are kept as > >> is, but this information is not needed anymore and is removed in the following > >> commit. > >> > >> Signed-off-by: David Marchand > >> Signed-off-by: Shreyansh Jain > >> --- > >> lib/librte_ether/rte_ethdev.c | 207 +++++++----------------------------------- > >> 1 file changed, 33 insertions(+), 174 deletions(-) > >> > >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > >> index a667012..8d14fd7 100644 > >> --- a/lib/librte_ether/rte_ethdev.c > >> +++ b/lib/librte_ether/rte_ethdev.c > >> @@ -72,6 +72,7 @@ > >> static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data"; > >> struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS]; > >> static struct rte_eth_dev_data *rte_eth_dev_data; > >> +static uint8_t eth_dev_last_created_port; > >> static uint8_t nb_ports; > >> > > > > [...] > > > >> - > >> /* attach the new device, then store port_id of the device */ > >> int > >> rte_eth_dev_attach(const char *devargs, uint8_t *port_id) > >> { > >> - struct rte_pci_addr addr; > >> int ret = -1; > >> + int current = eth_dev_last_created_port; > >> + char *name = NULL; > >> + char *args = NULL; > >> > >> if ((devargs == NULL) || (port_id == NULL)) { > >> ret = -EINVAL; > >> goto err; > >> } > >> > >> - if (eal_parse_pci_DomBDF(devargs, &addr) == 0) { > >> - ret = rte_eth_dev_attach_pdev(&addr, port_id); > >> - if (ret < 0) > >> - goto err; > >> - } else { > >> - ret = rte_eth_dev_attach_vdev(devargs, port_id); > >> - if (ret < 0) > >> - goto err; > >> + /* parse devargs, then retrieve device name and args */ > >> + if (rte_eal_parse_devargs_str(devargs, &name, &args)) > >> + goto err; > >> + > >> + ret = rte_eal_dev_attach(name, args); > >> + if (ret < 0) > >> + goto err; > >> + > >> + /* no point looking at eth_dev_last_created_port if no port exists */ > > > > I am not sure about this comment. What is "no point"? > > > > Isn't this also a potential bug? (Like the one below.) How could it > > happen there is no port after a successful attach? > > Yes, even searching through code path I couldn't find a positive case where control would reach here without nb_ports>0. > Though, i am not sure if some rough application attempts to mix-up calls - and that, in my opinion, is not worth checking. > Should I remove it? At least a loud log might be helpful. If there is really no path to reach this point, I'd put RTE_VERIFY here. > > > > >> + if (!nb_ports) { > >> + ret = -1; > >> + goto err; > >> + } > >> + /* if nothing happened, there is a bug here, since some driver told us > >> + * it did attach a device, but did not create a port */ > >> + if (current == eth_dev_last_created_port) { > >> + ret = -1; > >> + goto err; > > > > Should we log this? Or call some kind panic? > > I will place a log because applications should have a chance to ignore a device which cannot be attached for whatever reason. OK, we just should shout loudly if it means a bug... [...] -- Jan Viktorin E-mail: Viktorin@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic