From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so1.wedos.net (wes1-so1.wedos.net [46.28.106.15]) by dpdk.org (Postfix) with ESMTP id 941E92BBD for ; Mon, 11 Jul 2016 16:08:55 +0200 (CEST) Received: from pcviktorin.fit.vutbr.cz (pcviktorin.fit.vutbr.cz [147.229.13.147]) by wes1-so1.wedos.net (Postfix) with ESMTPSA id 3rp6Qb1Z6qz59H; Mon, 11 Jul 2016 16:08:55 +0200 (CEST) Date: Mon, 11 Jul 2016 16:08:15 +0200 From: Jan Viktorin To: Shreyansh jain Cc: , , David Marchand Message-ID: <20160711160815.4d43e184@pcviktorin.fit.vutbr.cz> In-Reply-To: <57839F4C.40507@nxp.com> References: <20160708190945.24225-1-viktorin@rehivetech.com> <20160708190945.24225-2-viktorin@rehivetech.com> <57839F4C.40507@nxp.com> Organization: RehiveTech MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1 01/15] eal: extract vdev infra X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Jul 2016 14:08:55 -0000 On Mon, 11 Jul 2016 18:59:48 +0530 Shreyansh jain wrote: > Hi Jan, > > Some comments. > > On Saturday 09 July 2016 12:39 AM, Jan Viktorin wrote: > > Move all PMD_VDEV-specific code into a separate module and header > > file to not polute the generic code anymore. There is now a list > > of virtual devices available. > > > > The rte_vdev_driver integrates the original rte_driver inside > > (C inheritance). The rte_driver will be however change in the > > future to serve as a common base for all other types of drivers. > > > > The existing PMDs (PMD_VDEV) are to be modified later (there is > > no change for them at the moment). > > > > There is however a inconsistency. The functions rte_eal_vdev_init > > and rte_eal_vdev_uninit are still placed in the rte_dev.h (instead > > of the rte_vdev.h). > > > > Signed-off-by: Jan Viktorin > > --- [...] > > + > > +/* unregister a driver */ > > +void > > +rte_eal_vdrv_unregister(struct rte_vdev_driver *driver) > > +{ > > + TAILQ_REMOVE(&vdev_driver_list, driver, next); > > +} > > + > > +int > > +rte_eal_vdev_init(const char *name, const char *args) > > +{ > > + struct rte_vdev_driver *driver; > > + > > + if (name == NULL) > > + return -EINVAL; > > + > > + TAILQ_FOREACH(driver, &vdev_driver_list, next) { > > + if (driver->driver.type != PMD_VDEV) > > + continue; > > Now that two separate lists for vdev and pdev exist, we don't need this check anymore. > In fact, PMD_VDEV might not even exist. Solved already in the next 2 patches. > > > + > > + /* > > + * search a driver prefix in virtual device name. > > + * For example, if the driver is pcap PMD, driver->name > > + * will be "eth_pcap", but "name" will be "eth_pcapN". > > + * So use strncmp to compare. > > + */ > > + if (!strncmp(driver->driver.name, name, strlen(driver->driver.name))) > > + return driver->driver.init(name, args); > > + } > > + > > + RTE_LOG(ERR, EAL, "no driver found for %s\n", name); > > + return -EINVAL; > > +} > > + > > +int > > +rte_eal_vdev_uninit(const char *name) > > +{ > > + struct rte_vdev_driver *driver; > > + > > + if (name == NULL) > > + return -EINVAL; > > + > > + TAILQ_FOREACH(driver, &vdev_driver_list, next) { > > + if (driver->driver.type != PMD_VDEV) > > + continue; > > Same as above, redundant check. Solved already in the next 2 patches. > > > + > > + /* > > + * search a driver prefix in virtual device name. > > + * For example, if the driver is pcap PMD, driver->name > > + * will be "eth_pcap", but "name" will be "eth_pcapN". > > + * So use strncmp to compare. > > + */ > > + if (!strncmp(driver->driver.name, name, strlen(driver->driver.name))) > > + return driver->driver.uninit(name); > > + } > > + > > + RTE_LOG(ERR, EAL, "no driver found for %s\n", name); > > + return -EINVAL; > > +} > > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h > > index b1c0520..2aeb752 100644 > > --- a/lib/librte_eal/common/include/rte_dev.h > > +++ b/lib/librte_eal/common/include/rte_dev.h > > @@ -210,6 +210,7 @@ static void devinitfn_ ##d(void)\ > > rte_eal_driver_register(&d);\ > > } > > > > + > > Probably a stray newline. Will fix. > > > #ifdef __cplusplus > > } > > #endif > > diff --git a/lib/librte_eal/common/include/rte_vdev.h b/lib/librte_eal/common/include/rte_vdev.h > > new file mode 100644 > > index 0000000..523bd92 > > --- /dev/null > > +++ b/lib/librte_eal/common/include/rte_vdev.h > > @@ -0,0 +1,83 @@ [...] > > +/** > > + * Unregister a virtual device driver. > > + * > > + * @param driver > > + * A pointer to a rte_vdev_driver structure describing the driver > > + * to be unregistered. > > + */ > > +void rte_eal_vdrv_unregister(struct rte_vdev_driver *driver); > > + > > +#define RTE_EAL_VDRV_REGISTER(d)\ > > In the recent commits, I noticed that macros have taken the (name, driver) format. > PMD_REGISTER_DRIVER() (now redundant), DRIVER_REGISTER_PCI_TABLE() ... etc > It might be better to stick to the same format. Yes, I will change this when rebasing. Thanks Jan > > > +RTE_INIT(vdrvinitfn_ ##d);\ > > +static void vdrvinitfn_ ##d(void)\ > > +{\ > > + rte_eal_vdrv_register(&d);\ > > +} > > + > > +#ifdef __cplusplus > > +} > > +#endif > > + > > +#endif > > diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile > > index 30b30f3..9553e97 100644 > > --- a/lib/librte_eal/linuxapp/eal/Makefile > > +++ b/lib/librte_eal/linuxapp/eal/Makefile > > @@ -85,6 +85,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_timer.c > > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_memzone.c > > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_log.c > > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_launch.c > > +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_vdev.c > > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_pci.c > > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_pci_uio.c > > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_memory.c > > >