Re: [dpdk-dev] [PATCH 07/15] eal: Make vdev init path generic for both virtual and physcial devices

[Thomas Monjalon <thomas.monjalon@6wind.com>]

I have some comments inline.

2014-04-15 14:06, Neil Horman:
> Currently, physical device pmds use a separate initalization path
> (rte_pmd_init_all) while virtual devices use a constructor registration and
> rte_eal_dev_init.  Theres no reason to have them be separate.  This patch
> removes the vdev specific nomenclature from the vdev init path and makes it
> more generic for use with all pmds.  This is the first step in converting
> the physical device pmds to using the same constructor based registration
> path that the virtual devices use
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

> -	if (rte_eal_vdev_init() < 0)
> +	if (rte_eal_dev_init() < 0)
>  		rte_panic("Cannot init virtual devices\n");

You should update the panic log here.

> +/** Global list of virtual device drivers. */
> +static struct rte_driver_list dev_driver_list =
> +	TAILQ_HEAD_INITIALIZER(dev_driver_list);

Same comment about "virtual device".

> +	/* No need to register drivers that are embeded in DPDK
> +	 * (pmd_pcap, pmd_ring, ...). The initialization function have
> +	 * the ((constructor)) attribute so they will register at
> +	 * startup. */

Should we keep this comment?

> +#ifndef _RTE_VDEV_H_
> +#define _RTE_VDEV_H_

Should be _RTE_DEV_H_

> +/**
> + * @file
> + *
> + * RTE Virtual Devices Interface
> + *
> + * This file manages the list of the virtual device drivers.
> + */

Not only virtual.

> +/** Double linked list of virtual device drivers. */
[...]
> + * Initialization function called for each virtual device probing.
[...]
> +/**
> + * A structure describing a virtual device driver.
> + */
[...]
> + * Register a virtual device driver.
[...]
> + * Unregister a virtual device driver.

You probably understood the idea ;)

> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> -	if (rte_eal_vdev_init() < 0)
> +	if (rte_eal_dev_init() < 0)
>  		rte_panic("Cannot init virtual devices\n");

Still "virtual" typo

Except typos, it seems a good step.
I think we could abstract more things in order to have even simpler API and 
simpler command line. But we'll see it in another step.

Thanks
-- 
Thomas