From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh jain Subject: Re: [PATCH v6 15/17] eal: add hotplug operations for pci and vdev Date: Fri, 15 Jul 2016 15:22:34 +0530 Message-ID: <5788B262.3010301@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-16-git-send-email-shreyansh.jain@nxp.com> <20160714185021.493910ea@jvn> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Cc: , , To: Jan Viktorin Return-path: Received: from NAM02-SN1-obe.outbound.protection.outlook.com (mail-sn1nam02on0080.outbound.protection.outlook.com [104.47.36.80]) by dpdk.org (Postfix) with ESMTP id B024E3977 for ; Fri, 15 Jul 2016 11:52:02 +0200 (CEST) In-Reply-To: <20160714185021.493910ea@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:20 PM, Jan Viktorin wrote: > On Tue, 12 Jul 2016 11:31:20 +0530 > Shreyansh Jain wrote: > >> Hotplug which deals with resources should come from the layer that already >> handles them, i.e. EAL. >> >> For both attach and detach operations, 'name' is used to select the bus >> that will handle the request. >> >> Signed-off-by: David Marchand >> Signed-off-by: Shreyansh Jain >> --- >> lib/librte_eal/bsdapp/eal/rte_eal_version.map | 2 ++ >> lib/librte_eal/common/eal_common_dev.c | 47 +++++++++++++++++++++++++ >> lib/librte_eal/common/include/rte_dev.h | 25 +++++++++++++ >> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 2 ++ >> 4 files changed, 76 insertions(+) >> >> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map >> index 1852c4a..6f9324f 100644 >> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map >> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map >> @@ -159,5 +159,7 @@ DPDK_16.07 { >> rte_keepalive_mark_sleep; >> rte_keepalive_register_relay_callback; >> rte_thread_setname; >> + rte_eal_dev_attach; >> + rte_eal_dev_detach; >> >> } DPDK_16.04; >> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c >> index a8a4146..14c6cf1 100644 >> --- a/lib/librte_eal/common/eal_common_dev.c >> +++ b/lib/librte_eal/common/eal_common_dev.c >> @@ -150,3 +150,50 @@ rte_eal_vdev_uninit(const char *name) >> RTE_LOG(ERR, EAL, "no driver found for %s\n", name); >> return -EINVAL; >> } >> + >> +int rte_eal_dev_attach(const char *name, const char *devargs) >> +{ >> + struct rte_pci_addr addr; >> + int ret = -1; >> + >> + if (name == NULL || devargs == NULL) { >> + RTE_LOG(ERR, EAL, "Invalid device arguments provided\n"); >> + return ret; >> + } >> + >> + if (eal_parse_pci_DomBDF(name, &addr) == 0) { >> + if (rte_eal_pci_probe_one(&addr) < 0) >> + goto err; >> + >> + } else { >> + if (rte_eal_vdev_init(name, devargs)) >> + goto err; >> + } >> + >> + return 0; >> + >> +err: >> + RTE_LOG(ERR, EAL, "Driver cannot attach the device\n"); > > I think this log can be more specific. We have the name of the device. > It might be confusing to the user when the attach fails and there is > no simple way how to determine which one. Agree. I will update with 'name'. > >> + return ret; >> +} >> + >> +int rte_eal_dev_detach(const char *name) >> +{ >> + struct rte_pci_addr addr; >> + >> + if (name == NULL) >> + goto err; >> + >> + if (eal_parse_pci_DomBDF(name, &addr) == 0) { >> + if (rte_eal_pci_detach(&addr) < 0) >> + goto err; >> + } else { >> + if (rte_eal_vdev_uninit(name)) >> + goto err; >> + } >> + return 0; >> + >> +err: >> + RTE_LOG(ERR, EAL, "Driver cannot detach the device\n"); > > Same as for attach. OK. > >> + return -1; >> +} >> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h >> index 994650b..2f0579c 100644 >> --- a/lib/librte_eal/common/include/rte_dev.h >> +++ b/lib/librte_eal/common/include/rte_dev.h >> @@ -178,6 +178,31 @@ int rte_eal_vdev_init(const char *name, const char *args); >> */ >> int rte_eal_vdev_uninit(const char *name); >> >> +/** >> + * Attach a resource to a registered driver. > > From my POV, the term "resource" is not good here. We have > rte_pci_resource but we refer to a device here. The function is called > rte_eal_DEV_attach. No idea why to call it a resource. Your point of 'resource' for 'rte_eal_*dev*_attach' makes sense. Ideally I wouldn't have given much heed to this - for me 'resource' is just a device representation within EAL. But, now that you have highlighted, name<=>comment is certainly mismatch. I will change. > >> + * >> + * @param name >> + * The resource name, that refers to a pci resource or some private >> + * way of designating a resource for vdev drivers. Based on this >> + * resource name, eal will identify a driver capable of handling >> + * this resource and pass this resource to the driver probing >> + * function. >> + * @param devargs >> + * Device arguments to be passed to the driver. >> + * @return >> + * 0 on success, negative on error. >> + */ >> +int rte_eal_dev_attach(const char *name, const char *devargs); >> + >> +/** >> + * Detach a resource from its driver. > > Same here for resource. I will change to "Detach a device from its driver". > > Jan > >> + * >> + * @param name >> + * Same description as for rte_eal_dev_attach(). >> + * Here, eal will call the driver detaching function. >> + */ >> +int rte_eal_dev_detach(const char *name); >> + >> #define DRIVER_EXPORT_NAME_ARRAY(n, idx) n##idx[] >> >> #define DRIVER_EXPORT_NAME(name, idx) \ >> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> index a617b9e..db866b8 100644 >> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> @@ -163,5 +163,7 @@ DPDK_16.07 { >> rte_keepalive_mark_sleep; >> rte_keepalive_register_relay_callback; >> rte_thread_setname; >> + rte_eal_dev_attach; >> + rte_eal_dev_detach; >> >> } DPDK_16.04; >