From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH 07/15] eal: Make vdev init path generic for
	both virtual and physcial devices
Date: Fri, 18 Apr 2014 14:02:17 +0200
Message-ID: <2223050.MHJTNgySuI@xps13>
References: <1397585169-14537-1-git-send-email-nhorman@tuxdriver.com>
 <1397585169-14537-8-git-send-email-nhorman@tuxdriver.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev-VfR2kkLFssw@public.gmane.org
To: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <1397585169-14537-8-git-send-email-nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev-VfR2kkLFssw@public.gmane.org>
List-Help: <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=subscribe>
Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org
Sender: "dev" <dev-bounces-VfR2kkLFssw@public.gmane.org>

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-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>

> -	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