From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH v9 13/14] eal/pci: Add
	rte_eal_dev_attach/detach() functions
Date: Thu, 19 Feb 2015 13:10:16 +0100
Message-ID: <10399957.vsF1Tt8fsr@xps13>
References: <1424060073-23484-12-git-send-email-mukawa@igel.co.jp>
 <1424314187-25177-1-git-send-email-mukawa@igel.co.jp>
 <1424314187-25177-14-git-send-email-mukawa@igel.co.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev-VfR2kkLFssw@public.gmane.org
To: Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <1424314187-25177-14-git-send-email-mukawa-AlSX/UN32fvPDbFq/vQRIQ@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>

2015-02-19 11:49, Tetsuya Mukawa:
> +/* attach the new virtual device, then store port_id of the device */
> +static int
> +rte_eal_dev_attach_vdev(const char *vdevargs, uint8_t *port_id)
> +{
> +	char *args;
> +	uint8_t new_port_id;
> +	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
> +
> +	if ((vdevargs == NULL) || (port_id == NULL))
> +		goto err0;
> +
> +	args = strdup(vdevargs);
> +	if (args == NULL)
> +		goto err0;
> +
> +	/* save current port status */
> +	if (rte_eth_dev_save(devs, sizeof(devs)))
> +		goto err1;
> +	/* add the vdevargs to devargs_list */
> +	if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, args))
> +		goto err1;

Could you explain why you store devargs in a list?

> +	/* parse vdevargs, then retrieve device name */
> +	get_vdev_name(args);
> +	/* walk around dev_driver_list to find the driver of the device,
> +	 * then invoke probe function o the driver */
> +	if (rte_eal_vdev_find_and_init(args))

TODO: get port_id from init.

> +		goto err2;
> +	/* get port_id enabled by above procedures */
> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> +		goto err2;

[...]
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -47,6 +47,7 @@ extern "C" {
>  #endif
>  
>  #include <sys/queue.h>
> +#include <rte_pci.h>
>  
>  /** Double linked list of device drivers. */
>  TAILQ_HEAD(rte_driver_list, rte_driver);
> @@ -57,6 +58,11 @@ TAILQ_HEAD(rte_driver_list, rte_driver);
>  typedef int (rte_dev_init_t)(const char *name, const char *args);
>  
>  /**
> + * Uninitilization function called for each device driver once.
> + */
> +typedef int (rte_dev_uninit_t)(const char *name);

Why using name as parameter and not port_id?

[...]
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -45,6 +45,7 @@ CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
>  CFLAGS += -I$(RTE_SDK)/lib/librte_ring
>  CFLAGS += -I$(RTE_SDK)/lib/librte_mempool
>  CFLAGS += -I$(RTE_SDK)/lib/librte_malloc
> +CFLAGS += -I$(RTE_SDK)/lib/librte_mbuf

Why do you need mbuf?

[...]
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -21,6 +21,8 @@ DPDK_2.0 {
>  	rte_eal_alarm_cancel;
>  	rte_eal_alarm_set;
>  	rte_eal_dev_init;
> +	rte_eal_dev_attach;
> +	rte_eal_dev_detach;

Please keep alphabetical order.