[dpdk-dev] [PATCH v1 01/15] eal: extract vdev infra

[Jan Viktorin]

On Mon, 11 Jul 2016 18:59:48 +0530
Shreyansh jain <shreyansh.jain at nxp.com> 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 <viktorin at rehivetech.com>
> > ---

[...]

> > +
> > +/* 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
> >   
>