From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh jain Subject: Re: [PATCH v6 16/17] ethdev: convert to eal hotplug Date: Fri, 15 Jul 2016 16:06:25 +0530 Message-ID: <5788BCA9.5010205@nxp.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Cc: , , To: Jan Viktorin Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0075.outbound.protection.outlook.com [104.47.41.75]) by dpdk.org (Postfix) with ESMTP id D072947D0 for ; Fri, 15 Jul 2016 12:35:53 +0200 (CEST) In-Reply-To: <20160714185148.738f1fa3@jvn> 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 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? > >> + 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. > >> } >> + *port_id = eth_dev_last_created_port; >> + ret = 0; >> >> - return 0; >> err: >> - RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); >> + free(name); >> + free(args); >> return ret; >> } >> >> @@ -590,7 +464,6 @@ err: >> int >> rte_eth_dev_detach(uint8_t port_id, char *name) >> { >> - struct rte_pci_addr addr; >> int ret = -1; >> >> if (name == NULL) { >> @@ -598,33 +471,19 @@ rte_eth_dev_detach(uint8_t port_id, char *name) >> goto err; >> } >> >> - /* check whether the driver supports detach feature, or not */ >> + /* FIXME: move this to eal, once device flags are relocated there */ >> if (rte_eth_dev_is_detachable(port_id)) >> goto err; >> >> - if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) { >> - ret = rte_eth_dev_get_addr_by_port(port_id, &addr); >> - if (ret < 0) >> - goto err; >> - >> - ret = rte_eth_dev_detach_pdev(port_id, &addr); >> - if (ret < 0) >> - goto err; >> - >> - snprintf(name, RTE_ETH_NAME_MAX_LEN, >> - "%04x:%02x:%02x.%d", >> - addr.domain, addr.bus, >> - addr.devid, addr.function); >> - } else { >> - ret = rte_eth_dev_detach_vdev(port_id, name); >> - if (ret < 0) >> - goto err; >> - } >> + snprintf(name, sizeof(rte_eth_devices[port_id].data->name), >> + "%s", rte_eth_devices[port_id].data->name); >> + ret = rte_eal_dev_detach(name); >> + if (ret < 0) >> + goto err; >> >> return 0; >> >> err: >> - RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n"); > > I'd be more specific about the failing device, we have its name. Agree. I will add 'name' to this. >> return ret; >> } >> >