Re: [PATCH v9 13/14] eal/pci: Add rte_eal_dev_attach/detach() functions

[Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>]

On 2015/02/19 22:30, Tetsuya Mukawa wrote:
> On 2015/02/19 21:10, Thomas Monjalon wrote:
>> 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?
> I try to do same behavior when rte_eal_init() is called.

Sorry for lack of explanation.

"vdevargs" of rte_eal_dev_attach_vdev() will be same format of "--vdev"
option.
And when rte_eal_init() is called, such a "--vdev" option value will be
stored in devargs_list.
So I try to same thing here.

> If only hotplug doesn't do this, someone may be confused when they try
> to realize devargs_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.
> Yes, I will.
> I also add comment about it.
>
>>> +		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?
> This function pointer will be implemented in PMDs.
> For example, in pcap PMD, rte_pmd_pcap_devuninit() is the function.
>
> static struct rte_driver pmd_pcap_drv = {
>         .name = "eth_pcap",
>         .type = PMD_VDEV,
>         .init = rte_pmd_pcap_devinit,
>         .uninit = rte_pmd_pcap_devuninit,
> };
>
> "port_id" isn't needed in PMD.
>
>> [...]
>>> --- 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?
> To include rte_ethdev.h in this code, rte_mbuf.h is also needed.
>
>> [...]
>>> --- 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.
>>
> Sure, I will.
>
> Thanks,
> Tetsuya
>