[dpdk-dev,v8,12/14] eal/pci: Add rte_eal_dev_attach/detach() functions

Message ID 1424060073-23484-13-git-send-email-mukawa@igel.co.jp (mailing list archive)
State Superseded, archived
Headers

Commit Message

Tetsuya Mukawa Feb. 16, 2015, 4:14 a.m. UTC
These functions are used for attaching or detaching a port.
When rte_eal_dev_attach() is called, the function tries to realize the
device name as pci address. If this is done successfully,
rte_eal_dev_attach() will attach physical device port. If not, attaches
virtual devive port.
When rte_eal_dev_detach() is called, the function gets the device type
of this port to know whether the port is come from physical or virtual.
And then specific detaching function will be called.

v8:
- Add missing symbol in version map.
  (Thanks to Qiu, Michael and Iremonger, Bernard)
v7:
- Fix typo of warning messages.
  (Thanks to Qiu, Michael)
v5:
- Change function names like below.
  rte_eal_dev_find_and_invoke() to rte_eal_vdev_find_and_invoke().
  rte_eal_dev_invoke() to rte_eal_vdev_invoke().
- Add code to handle a return value of rte_eal_devargs_remove().
- Fix pci address format in rte_eal_dev_detach().
v4:
- Fix comment.
- Add error checking.
- Fix indent of 'if' statement.
- Change function name.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_common_dev.c          | 276 ++++++++++++++++++++++++
 lib/librte_eal/common/eal_private.h             |  11 +
 lib/librte_eal/common/include/rte_dev.h         |  33 +++
 lib/librte_eal/linuxapp/eal/Makefile            |   1 +
 lib/librte_eal/linuxapp/eal/eal_pci.c           |   6 +-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |   2 +
 6 files changed, 326 insertions(+), 3 deletions(-)
  

Comments

Iremonger, Bernard Feb. 16, 2015, 4:30 p.m. UTC | #1
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Monday, February 16, 2015 4:15 AM
> To: dev@dpdk.org
> Cc: Qiu, Michael; Iremonger, Bernard; Tetsuya Mukawa
> Subject: [PATCH v8 12/14] eal/pci: Add rte_eal_dev_attach/detach() functions
> 
> These functions are used for attaching or detaching a port.
> When rte_eal_dev_attach() is called, the function tries to realize the device name as pci address. If
> this is done successfully,
> rte_eal_dev_attach() will attach physical device port. If not, attaches virtual devive port.
> When rte_eal_dev_detach() is called, the function gets the device type of this port to know whether
> the port is come from physical or virtual.
> And then specific detaching function will be called.
> 
> v8:
> - Add missing symbol in version map.
>   (Thanks to Qiu, Michael and Iremonger, Bernard)
> v7:
> - Fix typo of warning messages.
>   (Thanks to Qiu, Michael)
> v5:
> - Change function names like below.
>   rte_eal_dev_find_and_invoke() to rte_eal_vdev_find_and_invoke().
>   rte_eal_dev_invoke() to rte_eal_vdev_invoke().
> - Add code to handle a return value of rte_eal_devargs_remove().
> - Fix pci address format in rte_eal_dev_detach().
> v4:
> - Fix comment.
> - Add error checking.
> - Fix indent of 'if' statement.
> - Change function name.
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
  
Thomas Monjalon Feb. 17, 2015, 1:48 a.m. UTC | #2
2015-02-16 13:14, Tetsuya Mukawa:
> These functions are used for attaching or detaching a port.
> When rte_eal_dev_attach() is called, the function tries to realize the
> device name as pci address. If this is done successfully,
> rte_eal_dev_attach() will attach physical device port. If not, attaches
> virtual devive port.
> When rte_eal_dev_detach() is called, the function gets the device type
> of this port to know whether the port is come from physical or virtual.
> And then specific detaching function will be called.
> 
> v8:
> - Add missing symbol in version map.
>   (Thanks to Qiu, Michael and Iremonger, Bernard)
> v7:
> - Fix typo of warning messages.
>   (Thanks to Qiu, Michael)
> v5:
> - Change function names like below.
>   rte_eal_dev_find_and_invoke() to rte_eal_vdev_find_and_invoke().
>   rte_eal_dev_invoke() to rte_eal_vdev_invoke().
> - Add code to handle a return value of rte_eal_devargs_remove().
> - Fix pci address format in rte_eal_dev_detach().
> v4:
> - Fix comment.
> - Add error checking.
> - Fix indent of 'if' statement.
> - Change function name.
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/common/eal_common_dev.c          | 276 ++++++++++++++++++++++++
>  lib/librte_eal/common/eal_private.h             |  11 +
>  lib/librte_eal/common/include/rte_dev.h         |  33 +++
>  lib/librte_eal/linuxapp/eal/Makefile            |   1 +
>  lib/librte_eal/linuxapp/eal/eal_pci.c           |   6 +-
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |   2 +
>  6 files changed, 326 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index eae5656..3d169a4 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -32,10 +32,13 @@
>   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> +#include <stdio.h>
> +#include <limits.h>
>  #include <string.h>
>  #include <inttypes.h>
>  #include <sys/queue.h>
>  
> +#include <rte_ethdev.h>
>  #include <rte_dev.h>
>  #include <rte_devargs.h>
>  #include <rte_debug.h>
> @@ -107,3 +110,276 @@ rte_eal_dev_init(void)
>  	}
>  	return 0;
>  }
> +
> +/* So far, DPDK hotplug function only supports linux */

This comment should be in the configuration.

> +#ifdef ENABLE_HOTPLUG
> +static void
> +rte_eal_vdev_invoke(struct rte_driver *driver,
> +		struct rte_devargs *devargs, enum rte_eal_invoke_type type)
> +{
> +	if ((driver == NULL) || (devargs == NULL))
> +		return;
> +
> +	switch (type) {
> +	case RTE_EAL_INVOKE_TYPE_PROBE:
> +		driver->init(devargs->virtual.drv_name, devargs->args);
> +		break;
> +	case RTE_EAL_INVOKE_TYPE_CLOSE:
> +		driver->uninit(devargs->virtual.drv_name, devargs->args);
> +		break;
> +	default:
> +		break;
> +	}
> +}

It would be clearer to directly call init and uninit instead of using this
"invoke" method.

> +
> +static int
> +rte_eal_vdev_find_and_invoke(const char *name, int type)
> +{
> +	struct rte_devargs *devargs;
> +	struct rte_driver *driver;
> +
> +	if (name == NULL)
> +		return -EINVAL;
> +
> +	/* call the init function for each virtual device */

This comment is wrong.

> +	TAILQ_FOREACH(devargs, &devargs_list, next) {
> +
> +		if (devargs->type != RTE_DEVTYPE_VIRTUAL)
> +			continue;
> +
> +		if (strncmp(name, devargs->virtual.drv_name, strlen(name)))
> +			continue;
> +
> +		TAILQ_FOREACH(driver, &dev_driver_list, next) {
> +			if (driver->type != PMD_VDEV)
> +				continue;
> +
> +			/* search a driver prefix in virtual device name */
> +			if (!strncmp(driver->name, devargs->virtual.drv_name,
> +			    strlen(driver->name))) {

Why not use strcmp?

> +				rte_eal_vdev_invoke(driver, devargs, type);
> +				break;
> +			}
> +		}
> +
> +		if (driver == NULL) {
> +			RTE_LOG(WARNING, EAL, "no driver found for %s\n",
> +				  devargs->virtual.drv_name);
> +		}
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +/* attach the new physical device, then store port_id of the device */
> +static int
> +rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
> +{
> +	uint8_t new_port_id;
> +	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
> +
> +	if ((addr == NULL) || (port_id == NULL))
> +		goto err;
> +
> +	/* save current port status */
> +	if (rte_eth_dev_save(devs, sizeof(devs)))
> +		goto err;
> +	/* re-construct pci_device_list */
> +	if (rte_eal_pci_scan())
> +		goto err;
> +	/* invoke probe func of the driver can handle the new device */
> +	if (rte_eal_pci_probe_one(addr))
> +		goto err;

You should get the port_id from the previous function instead of searching it.

> +	/* get port_id enabled by above procedures */
> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> +		goto err;
> +
> +	*port_id = new_port_id;
> +	return 0;
> +err:
> +	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
> +	return -1;
> +}

[...]

> +/* 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;
> +	/* 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_invoke(args, RTE_EAL_INVOKE_TYPE_PROBE))
> +		goto err2;

Again, you should get port_id from the attach procedure.

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

[...]

>  /**
> + * Uninitilization function called for each device driver once.
> + */
> +typedef int (rte_dev_uninit_t)(const char *name, const char *args);

Why do you need args for uninit?
  
Tetsuya Mukawa Feb. 17, 2015, 8:51 a.m. UTC | #3
On 2015/02/17 10:48, Thomas Monjalon wrote:
> 2015-02-16 13:14, Tetsuya Mukawa:
>> These functions are used for attaching or detaching a port.
>> When rte_eal_dev_attach() is called, the function tries to realize the
>> device name as pci address. If this is done successfully,
>> rte_eal_dev_attach() will attach physical device port. If not, attaches
>> virtual devive port.
>> When rte_eal_dev_detach() is called, the function gets the device type
>> of this port to know whether the port is come from physical or virtual.
>> And then specific detaching function will be called.
>>
>> v8:
>> - Add missing symbol in version map.
>>   (Thanks to Qiu, Michael and Iremonger, Bernard)
>> v7:
>> - Fix typo of warning messages.
>>   (Thanks to Qiu, Michael)
>> v5:
>> - Change function names like below.
>>   rte_eal_dev_find_and_invoke() to rte_eal_vdev_find_and_invoke().
>>   rte_eal_dev_invoke() to rte_eal_vdev_invoke().
>> - Add code to handle a return value of rte_eal_devargs_remove().
>> - Fix pci address format in rte_eal_dev_detach().
>> v4:
>> - Fix comment.
>> - Add error checking.
>> - Fix indent of 'if' statement.
>> - Change function name.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>>  lib/librte_eal/common/eal_common_dev.c          | 276 ++++++++++++++++++++++++
>>  lib/librte_eal/common/eal_private.h             |  11 +
>>  lib/librte_eal/common/include/rte_dev.h         |  33 +++
>>  lib/librte_eal/linuxapp/eal/Makefile            |   1 +
>>  lib/librte_eal/linuxapp/eal/eal_pci.c           |   6 +-
>>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |   2 +
>>  6 files changed, 326 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
>> index eae5656..3d169a4 100644
>> --- a/lib/librte_eal/common/eal_common_dev.c
>> +++ b/lib/librte_eal/common/eal_common_dev.c
>> @@ -32,10 +32,13 @@
>>   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>   */
>>  
>> +#include <stdio.h>
>> +#include <limits.h>
>>  #include <string.h>
>>  #include <inttypes.h>
>>  #include <sys/queue.h>
>>  
>> +#include <rte_ethdev.h>
>>  #include <rte_dev.h>
>>  #include <rte_devargs.h>
>>  #include <rte_debug.h>
>> @@ -107,3 +110,276 @@ rte_eal_dev_init(void)
>>  	}
>>  	return 0;
>>  }
>> +
>> +/* So far, DPDK hotplug function only supports linux */
> This comment should be in the configuration.

Sure I will.

>
>> +#ifdef ENABLE_HOTPLUG
>> +static void
>> +rte_eal_vdev_invoke(struct rte_driver *driver,
>> +		struct rte_devargs *devargs, enum rte_eal_invoke_type type)
>> +{
>> +	if ((driver == NULL) || (devargs == NULL))
>> +		return;
>> +
>> +	switch (type) {
>> +	case RTE_EAL_INVOKE_TYPE_PROBE:
>> +		driver->init(devargs->virtual.drv_name, devargs->args);
>> +		break;
>> +	case RTE_EAL_INVOKE_TYPE_CLOSE:
>> +		driver->uninit(devargs->virtual.drv_name, devargs->args);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
> It would be clearer to directly call init and uninit instead of using this
> "invoke" method.

Sure, I will change it.

>> +
>> +static int
>> +rte_eal_vdev_find_and_invoke(const char *name, int type)
>> +{
>> +	struct rte_devargs *devargs;
>> +	struct rte_driver *driver;
>> +
>> +	if (name == NULL)
>> +		return -EINVAL;
>> +
>> +	/* call the init function for each virtual device */
> This comment is wrong.

Thanks, I will fix it.

>> +	TAILQ_FOREACH(devargs, &devargs_list, next) {
>> +
>> +		if (devargs->type != RTE_DEVTYPE_VIRTUAL)
>> +			continue;
>> +
>> +		if (strncmp(name, devargs->virtual.drv_name, strlen(name)))
>> +			continue;
>> +
>> +		TAILQ_FOREACH(driver, &dev_driver_list, next) {
>> +			if (driver->type != PMD_VDEV)
>> +				continue;
>> +
>> +			/* search a driver prefix in virtual device name */
>> +			if (!strncmp(driver->name, devargs->virtual.drv_name,
>> +			    strlen(driver->name))) {
> Why not use strcmp?

I will replace it.

>> +				rte_eal_vdev_invoke(driver, devargs, type);
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (driver == NULL) {
>> +			RTE_LOG(WARNING, EAL, "no driver found for %s\n",
>> +				  devargs->virtual.drv_name);
>> +		}
>> +		return 0;
>> +	}
>> +	return 1;
>> +}
>> +
>> +/* attach the new physical device, then store port_id of the device */
>> +static int
>> +rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
>> +{
>> +	uint8_t new_port_id;
>> +	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
>> +
>> +	if ((addr == NULL) || (port_id == NULL))
>> +		goto err;
>> +
>> +	/* save current port status */
>> +	if (rte_eth_dev_save(devs, sizeof(devs)))
>> +		goto err;
>> +	/* re-construct pci_device_list */
>> +	if (rte_eal_pci_scan())
>> +		goto err;
>> +	/* invoke probe func of the driver can handle the new device */
>> +	if (rte_eal_pci_probe_one(addr))
>> +		goto err;
> You should get the port_id from the previous function instead of searching it.

I agree this will beautify this code, but actually to do like above
changes current DPDK code much more, and it will not be clear, and not
beautiful.

Could I explain it more.
Problem is initialization sequence of virtual device and physical device
are completely different.

(1) Attaching a physical device case
- To return port id, pci_invoke_all_drivers() needs to return port id.
- It means "devinit" of "struct rte_pci_driver" needs to return port_id.
- "devinit" will be rte_eth_dev_init(). But if the device is virtual,
this function is not implemented.

(2) Attaching virtual device case
- To return port id from rte_eal_pci_probe_one(),
pci_invoke_all_drivers() needs to return port id.
- It means "init" of "struct rte_driver" needs to return port_id.
- "init" will be implemented in PMD. But this function has different
usage in physical device and virtual device.
- Especially, In the case of physical device, "init" doesn't allocate
eth device, so cannot return port id.

As a result, to remove  rte_eth_dev_save() and
rte_eth_dev_get_changed_port(), below different functions needs to
return port id.
 - "devinit" of "struct rte_pci_driver".
 - "init" of "struct rte_driver".

That is why I implement like above.

>> +	/* get port_id enabled by above procedures */
>> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
>> +		goto err;
>> +
>> +	*port_id = new_port_id;
>> +	return 0;
>> +err:
>> +	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
>> +	return -1;
>> +}
> [...]
>
>> +/* 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;
>> +	/* 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_invoke(args, RTE_EAL_INVOKE_TYPE_PROBE))
>> +		goto err2;
> Again, you should get port_id from the attach procedure.
>
>> +	/* get port_id enabled by above procedures */
>> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
>> +		goto err2;
> [...]
>
>>  /**
>> + * Uninitilization function called for each device driver once.
>> + */
>> +typedef int (rte_dev_uninit_t)(const char *name, const char *args);
> Why do you need args for uninit?
>

I just added for the case that finalization code of PMD needs it.
But, probably "args" parameter can be removed.
  
Thomas Monjalon Feb. 17, 2015, 9:23 a.m. UTC | #4
2015-02-17 17:51, Tetsuya Mukawa:
> On 2015/02/17 10:48, Thomas Monjalon wrote:
> > 2015-02-16 13:14, Tetsuya Mukawa:
> >> +/* attach the new physical device, then store port_id of the device */
> >> +static int
> >> +rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
> >> +{
> >> +	uint8_t new_port_id;
> >> +	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
> >> +
> >> +	if ((addr == NULL) || (port_id == NULL))
> >> +		goto err;
> >> +
> >> +	/* save current port status */
> >> +	if (rte_eth_dev_save(devs, sizeof(devs)))
> >> +		goto err;
> >> +	/* re-construct pci_device_list */
> >> +	if (rte_eal_pci_scan())
> >> +		goto err;
> >> +	/* invoke probe func of the driver can handle the new device */
> >> +	if (rte_eal_pci_probe_one(addr))
> >> +		goto err;
> > You should get the port_id from the previous function instead of searching it.
> 
> I agree this will beautify this code, but actually to do like above
> changes current DPDK code much more, and it will not be clear, and not
> beautiful.
> 
> Could I explain it more.
> Problem is initialization sequence of virtual device and physical device
> are completely different.
> 
> (1) Attaching a physical device case
> - To return port id, pci_invoke_all_drivers() needs to return port id.
> - It means "devinit" of "struct rte_pci_driver" needs to return port_id.
> - "devinit" will be rte_eth_dev_init(). But if the device is virtual,
> this function is not implemented.
> 
> (2) Attaching virtual device case
> - To return port id from rte_eal_pci_probe_one(),
> pci_invoke_all_drivers() needs to return port id.
> - It means "init" of "struct rte_driver" needs to return port_id.
> - "init" will be implemented in PMD. But this function has different
> usage in physical device and virtual device.
> - Especially, In the case of physical device, "init" doesn't allocate
> eth device, so cannot return port id.
> 
> As a result, to remove  rte_eth_dev_save() and
> rte_eth_dev_get_changed_port(), below different functions needs to
> return port id.
>  - "devinit" of "struct rte_pci_driver".
>  - "init" of "struct rte_driver".

Yes, exactly,
I think you shouldn't hesitate to improve existing EAL code.
And I also think we should try to remove differences between virtual and
pci devices.

> That is why I implement like above.
> 
> >> +	/* get port_id enabled by above procedures */
> >> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> >> +		goto err;
> >> +
> >> +	*port_id = new_port_id;
> >> +	return 0;
> >> +err:
> >> +	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
> >> +	return -1;
> >> +}
> > [...]
> >
> >> +/* 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;
> >> +	/* 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_invoke(args, RTE_EAL_INVOKE_TYPE_PROBE))
> >> +		goto err2;
> > Again, you should get port_id from the attach procedure.
> >
> >> +	/* get port_id enabled by above procedures */
> >> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> >> +		goto err2;
> > [...]
> >
> >>  /**
> >> + * Uninitilization function called for each device driver once.
> >> + */
> >> +typedef int (rte_dev_uninit_t)(const char *name, const char *args);
> > Why do you need args for uninit?
> >
> 
> I just added for the case that finalization code of PMD needs it.
> But, probably "args" parameter can be removed.

Yes I think
  
Tetsuya Mukawa Feb. 17, 2015, 10:26 a.m. UTC | #5
On 2015/02/17 18:23, Thomas Monjalon wrote:
> 2015-02-17 17:51, Tetsuya Mukawa:
>> On 2015/02/17 10:48, Thomas Monjalon wrote:
>>> 2015-02-16 13:14, Tetsuya Mukawa:
>>>> +/* attach the new physical device, then store port_id of the device */
>>>> +static int
>>>> +rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
>>>> +{
>>>> +	uint8_t new_port_id;
>>>> +	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
>>>> +
>>>> +	if ((addr == NULL) || (port_id == NULL))
>>>> +		goto err;
>>>> +
>>>> +	/* save current port status */
>>>> +	if (rte_eth_dev_save(devs, sizeof(devs)))
>>>> +		goto err;
>>>> +	/* re-construct pci_device_list */
>>>> +	if (rte_eal_pci_scan())
>>>> +		goto err;
>>>> +	/* invoke probe func of the driver can handle the new device */
>>>> +	if (rte_eal_pci_probe_one(addr))
>>>> +		goto err;
>>> You should get the port_id from the previous function instead of searching it.
>> I agree this will beautify this code, but actually to do like above
>> changes current DPDK code much more, and it will not be clear, and not
>> beautiful.
>>
>> Could I explain it more.
>> Problem is initialization sequence of virtual device and physical device
>> are completely different.
>>
>> (1) Attaching a physical device case
>> - To return port id, pci_invoke_all_drivers() needs to return port id.
>> - It means "devinit" of "struct rte_pci_driver" needs to return port_id.
>> - "devinit" will be rte_eth_dev_init(). But if the device is virtual,
>> this function is not implemented.
>>
>> (2) Attaching virtual device case
>> - To return port id from rte_eal_pci_probe_one(),
>> pci_invoke_all_drivers() needs to return port id.
>> - It means "init" of "struct rte_driver" needs to return port_id.
>> - "init" will be implemented in PMD. But this function has different
>> usage in physical device and virtual device.
>> - Especially, In the case of physical device, "init" doesn't allocate
>> eth device, so cannot return port id.
>>
>> As a result, to remove  rte_eth_dev_save() and
>> rte_eth_dev_get_changed_port(), below different functions needs to
>> return port id.
>>  - "devinit" of "struct rte_pci_driver".
>>  - "init" of "struct rte_driver".
> Yes, exactly,
> I think you shouldn't hesitate to improve existing EAL code.
> And I also think we should try to remove differences between virtual and
> pci devices.

I strongly agree with it. But I haven't investigated how to remove it so
far.
To be honest, I want to submit hotplug patches to DPDK-2.0.
Is above functionality needed to merge the hotplug patches?
I guess I will not be able to remove it by 23rd.

Regards,
Tetsuya

>> That is why I implement like above.
>>
>>>> +	/* get port_id enabled by above procedures */
>>>> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
>>>> +		goto err;
>>>> +
>>>> +	*port_id = new_port_id;
>>>> +	return 0;
>>>> +err:
>>>> +	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
>>>> +	return -1;
>>>> +}
>>> [...]
>>>
>>>> +/* 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;
>>>> +	/* 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_invoke(args, RTE_EAL_INVOKE_TYPE_PROBE))
>>>> +		goto err2;
>>> Again, you should get port_id from the attach procedure.
>>>
>>>> +	/* get port_id enabled by above procedures */
>>>> +	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
>>>> +		goto err2;
>>> [...]
>>>
>>>>  /**
>>>> + * Uninitilization function called for each device driver once.
>>>> + */
>>>> +typedef int (rte_dev_uninit_t)(const char *name, const char *args);
>>> Why do you need args for uninit?
>>>
>> I just added for the case that finalization code of PMD needs it.
>> But, probably "args" parameter can be removed.
> Yes I think
>
>
  
Maxime Leroy Feb. 17, 2015, 4:15 p.m. UTC | #6
Hi Tetsuya,

On Tue, Feb 17, 2015 at 9:51 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
>
>
> >> +    /* get port_id enabled by above procedures */
> >> +    if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> >> +            goto err2;
> > [...]
> >
> >>  /**
> >> + * Uninitilization function called for each device driver once.
> >> + */
> >> +typedef int (rte_dev_uninit_t)(const char *name, const char *args);
> > Why do you need args for uninit?
> >
>
> I just added for the case that finalization code of PMD needs it.
> But, probably "args" parameter can be removed.
>
>

I think there are no needs to have any args in the uninit function:
1) You librte_pmd_null doesn't use it
2) You give exactly the same argument that was used by the init
function. A driver should have already stored these parameters in an
internal private structure at initialization. So it's not needed to
give me back for uninit method.

From my understanding devargs_list is only needed at the init to store
the arguments when we parse the command line. Then, at initialization,
rte_eal_dev_init  creates the devices from this list .

By removing args from uninit function, you doesn't need to add and
remove anymore devargs in devargs_list to (de)attach a new device.

What do you think ?

Maxime
  
Thomas Monjalon Feb. 18, 2015, 1:17 a.m. UTC | #7
2015-02-17 19:26, Tetsuya Mukawa:
> On 2015/02/17 18:23, Thomas Monjalon wrote:
> > 2015-02-17 17:51, Tetsuya Mukawa:
> >> On 2015/02/17 10:48, Thomas Monjalon wrote:
> >>> 2015-02-16 13:14, Tetsuya Mukawa:
> >>>> +/* attach the new physical device, then store port_id of the device */
> >>>> +static int
> >>>> +rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
> >>>> +{
> >>>> +	uint8_t new_port_id;
> >>>> +	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
> >>>> +
> >>>> +	if ((addr == NULL) || (port_id == NULL))
> >>>> +		goto err;
> >>>> +
> >>>> +	/* save current port status */
> >>>> +	if (rte_eth_dev_save(devs, sizeof(devs)))
> >>>> +		goto err;
> >>>> +	/* re-construct pci_device_list */
> >>>> +	if (rte_eal_pci_scan())
> >>>> +		goto err;
> >>>> +	/* invoke probe func of the driver can handle the new device */
> >>>> +	if (rte_eal_pci_probe_one(addr))
> >>>> +		goto err;
> >>> You should get the port_id from the previous function instead of searching it.
> >> I agree this will beautify this code, but actually to do like above
> >> changes current DPDK code much more, and it will not be clear, and not
> >> beautiful.
> >>
> >> Could I explain it more.
> >> Problem is initialization sequence of virtual device and physical device
> >> are completely different.
> >>
> >> (1) Attaching a physical device case
> >> - To return port id, pci_invoke_all_drivers() needs to return port id.
> >> - It means "devinit" of "struct rte_pci_driver" needs to return port_id.
> >> - "devinit" will be rte_eth_dev_init(). But if the device is virtual,
> >> this function is not implemented.
> >>
> >> (2) Attaching virtual device case
> >> - To return port id from rte_eal_pci_probe_one(),
> >> pci_invoke_all_drivers() needs to return port id.
> >> - It means "init" of "struct rte_driver" needs to return port_id.
> >> - "init" will be implemented in PMD. But this function has different
> >> usage in physical device and virtual device.
> >> - Especially, In the case of physical device, "init" doesn't allocate
> >> eth device, so cannot return port id.
> >>
> >> As a result, to remove  rte_eth_dev_save() and
> >> rte_eth_dev_get_changed_port(), below different functions needs to
> >> return port id.
> >>  - "devinit" of "struct rte_pci_driver".
> >>  - "init" of "struct rte_driver".
> > Yes, exactly,
> > I think you shouldn't hesitate to improve existing EAL code.
> > And I also think we should try to remove differences between virtual and
> > pci devices.
> 
> I strongly agree with it. But I haven't investigated how to remove it so
> far.
> To be honest, I want to submit hotplug patches to DPDK-2.0.
> Is above functionality needed to merge the hotplug patches?
> I guess I will not be able to remove it by 23rd.

Obviously, it would be better to have it in dpdk-2.0.0-rc1.
If not possible to fix it, would it be possible to work on other comments
and keep this cleanup for post-rc1 integration?
I feel this cleanup is important to get the right design but it wouldn't be
fair to block this (old) patchset for this reason.
  
Tetsuya Mukawa Feb. 18, 2015, 1:54 a.m. UTC | #8
On 2015/02/18 1:15, Maxime Leroy wrote:
> Hi Tetsuya,
>
> On Tue, Feb 17, 2015 at 9:51 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
>>
>>>> +    /* get port_id enabled by above procedures */
>>>> +    if (rte_eth_dev_get_changed_port(devs, &new_port_id))
>>>> +            goto err2;
>>> [...]
>>>
>>>>  /**
>>>> + * Uninitilization function called for each device driver once.
>>>> + */
>>>> +typedef int (rte_dev_uninit_t)(const char *name, const char *args);
>>> Why do you need args for uninit?
>>>
>> I just added for the case that finalization code of PMD needs it.
>> But, probably "args" parameter can be removed.
>>
>>
> I think there are no needs to have any args in the uninit function:
> 1) You librte_pmd_null doesn't use it
> 2) You give exactly the same argument that was used by the init
> function. A driver should have already stored these parameters in an
> internal private structure at initialization. So it's not needed to
> give me back for uninit method.
>
> From my understanding devargs_list is only needed at the init to store
> the arguments when we parse the command line. Then, at initialization,
> rte_eal_dev_init  creates the devices from this list .
>
> By removing args from uninit function, you doesn't need to add and
> remove anymore devargs in devargs_list to (de)attach a new device.
>
> What do you think ?

Hi Maxime,

Yes, I agree. We can remove the argument.
I will do it.

Regards,
Tetsuya

>
> Maxime
  
Tetsuya Mukawa Feb. 18, 2015, 1:55 a.m. UTC | #9
On 2015/02/18 10:17, Thomas Monjalon wrote:
> 2015-02-17 19:26, Tetsuya Mukawa:
>> On 2015/02/17 18:23, Thomas Monjalon wrote:
>>> 2015-02-17 17:51, Tetsuya Mukawa:
>>>> On 2015/02/17 10:48, Thomas Monjalon wrote:
>>>>> 2015-02-16 13:14, Tetsuya Mukawa:
>>>>>> +/* attach the new physical device, then store port_id of the device */
>>>>>> +static int
>>>>>> +rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
>>>>>> +{
>>>>>> +	uint8_t new_port_id;
>>>>>> +	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
>>>>>> +
>>>>>> +	if ((addr == NULL) || (port_id == NULL))
>>>>>> +		goto err;
>>>>>> +
>>>>>> +	/* save current port status */
>>>>>> +	if (rte_eth_dev_save(devs, sizeof(devs)))
>>>>>> +		goto err;
>>>>>> +	/* re-construct pci_device_list */
>>>>>> +	if (rte_eal_pci_scan())
>>>>>> +		goto err;
>>>>>> +	/* invoke probe func of the driver can handle the new device */
>>>>>> +	if (rte_eal_pci_probe_one(addr))
>>>>>> +		goto err;
>>>>> You should get the port_id from the previous function instead of searching it.
>>>> I agree this will beautify this code, but actually to do like above
>>>> changes current DPDK code much more, and it will not be clear, and not
>>>> beautiful.
>>>>
>>>> Could I explain it more.
>>>> Problem is initialization sequence of virtual device and physical device
>>>> are completely different.
>>>>
>>>> (1) Attaching a physical device case
>>>> - To return port id, pci_invoke_all_drivers() needs to return port id.
>>>> - It means "devinit" of "struct rte_pci_driver" needs to return port_id.
>>>> - "devinit" will be rte_eth_dev_init(). But if the device is virtual,
>>>> this function is not implemented.
>>>>
>>>> (2) Attaching virtual device case
>>>> - To return port id from rte_eal_pci_probe_one(),
>>>> pci_invoke_all_drivers() needs to return port id.
>>>> - It means "init" of "struct rte_driver" needs to return port_id.
>>>> - "init" will be implemented in PMD. But this function has different
>>>> usage in physical device and virtual device.
>>>> - Especially, In the case of physical device, "init" doesn't allocate
>>>> eth device, so cannot return port id.
>>>>
>>>> As a result, to remove  rte_eth_dev_save() and
>>>> rte_eth_dev_get_changed_port(), below different functions needs to
>>>> return port id.
>>>>  - "devinit" of "struct rte_pci_driver".
>>>>  - "init" of "struct rte_driver".
>>> Yes, exactly,
>>> I think you shouldn't hesitate to improve existing EAL code.
>>> And I also think we should try to remove differences between virtual and
>>> pci devices.
>> I strongly agree with it. But I haven't investigated how to remove it so
>> far.
>> To be honest, I want to submit hotplug patches to DPDK-2.0.
>> Is above functionality needed to merge the hotplug patches?
>> I guess I will not be able to remove it by 23rd.
> Obviously, it would be better to have it in dpdk-2.0.0-rc1.
> If not possible to fix it, would it be possible to work on other comments
> and keep this cleanup for post-rc1 integration?
> I feel this cleanup is important to get the right design but it wouldn't be
> fair to block this (old) patchset for this reason.
>

I appreciate for your suggestion. I will keep working on it for post-rc1.

Thanks,
Tetsuya
  

Patch

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index eae5656..3d169a4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -32,10 +32,13 @@ 
  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <stdio.h>
+#include <limits.h>
 #include <string.h>
 #include <inttypes.h>
 #include <sys/queue.h>
 
+#include <rte_ethdev.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
 #include <rte_debug.h>
@@ -107,3 +110,276 @@  rte_eal_dev_init(void)
 	}
 	return 0;
 }
+
+/* So far, DPDK hotplug function only supports linux */
+#ifdef ENABLE_HOTPLUG
+static void
+rte_eal_vdev_invoke(struct rte_driver *driver,
+		struct rte_devargs *devargs, enum rte_eal_invoke_type type)
+{
+	if ((driver == NULL) || (devargs == NULL))
+		return;
+
+	switch (type) {
+	case RTE_EAL_INVOKE_TYPE_PROBE:
+		driver->init(devargs->virtual.drv_name, devargs->args);
+		break;
+	case RTE_EAL_INVOKE_TYPE_CLOSE:
+		driver->uninit(devargs->virtual.drv_name, devargs->args);
+		break;
+	default:
+		break;
+	}
+}
+
+static int
+rte_eal_vdev_find_and_invoke(const char *name, int type)
+{
+	struct rte_devargs *devargs;
+	struct rte_driver *driver;
+
+	if (name == NULL)
+		return -EINVAL;
+
+	/* call the init function for each virtual device */
+	TAILQ_FOREACH(devargs, &devargs_list, next) {
+
+		if (devargs->type != RTE_DEVTYPE_VIRTUAL)
+			continue;
+
+		if (strncmp(name, devargs->virtual.drv_name, strlen(name)))
+			continue;
+
+		TAILQ_FOREACH(driver, &dev_driver_list, next) {
+			if (driver->type != PMD_VDEV)
+				continue;
+
+			/* search a driver prefix in virtual device name */
+			if (!strncmp(driver->name, devargs->virtual.drv_name,
+			    strlen(driver->name))) {
+				rte_eal_vdev_invoke(driver, devargs, type);
+				break;
+			}
+		}
+
+		if (driver == NULL) {
+			RTE_LOG(WARNING, EAL, "no driver found for %s\n",
+				  devargs->virtual.drv_name);
+		}
+		return 0;
+	}
+	return 1;
+}
+
+/* attach the new physical device, then store port_id of the device */
+static int
+rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
+{
+	uint8_t new_port_id;
+	struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
+
+	if ((addr == NULL) || (port_id == NULL))
+		goto err;
+
+	/* save current port status */
+	if (rte_eth_dev_save(devs, sizeof(devs)))
+		goto err;
+	/* re-construct pci_device_list */
+	if (rte_eal_pci_scan())
+		goto err;
+	/* invoke probe func of the driver can handle the new device */
+	if (rte_eal_pci_probe_one(addr))
+		goto err;
+	/* get port_id enabled by above procedures */
+	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
+		goto err;
+
+	*port_id = new_port_id;
+	return 0;
+err:
+	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
+	return -1;
+}
+
+/* detach the new physical device, then store pci_addr of the device */
+static int
+rte_eal_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
+{
+	struct rte_pci_addr freed_addr;
+	struct rte_pci_addr vp;
+
+	if (addr == NULL)
+		goto err;
+
+	/* check whether the driver supports detach feature, or not */
+	if (rte_eth_dev_check_detachable(port_id))
+		goto err;
+
+	/* get pci address by port id */
+	if (rte_eth_dev_get_addr_by_port(port_id, &freed_addr))
+		goto err;
+
+	/* Zerod pci addr means the port comes from virtual device */
+	vp.domain = vp.bus = vp.devid = vp.function = 0;
+	if (eal_compare_pci_addr(&vp, &freed_addr) == 0)
+		goto err;
+
+	/* invoke close func of the driver,
+	 * also remove the device from pci_device_list */
+	if (rte_eal_pci_close_one(&freed_addr))
+		goto err;
+
+	*addr = freed_addr;
+	return 0;
+err:
+	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
+	return -1;
+}
+
+static void
+get_vdev_name(char *vdevargs)
+{
+	char *sep;
+
+	if (vdevargs == NULL)
+		return;
+
+	/* set the first ',' to '\0' to split name and arguments */
+	sep = strchr(vdevargs, ',');
+	if (sep != NULL)
+		sep[0] = '\0';
+}
+
+/* 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;
+	/* 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_invoke(args, RTE_EAL_INVOKE_TYPE_PROBE))
+		goto err2;
+	/* get port_id enabled by above procedures */
+	if (rte_eth_dev_get_changed_port(devs, &new_port_id))
+		goto err2;
+
+	free(args);
+	*port_id = new_port_id;
+	return 0;
+err2:
+	rte_eal_devargs_remove(RTE_DEVTYPE_VIRTUAL, args);
+err1:
+	free(args);
+err0:
+	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
+	return -1;
+}
+
+/* detach the new virtual device, then store the name of the device */
+static int
+rte_eal_dev_detach_vdev(uint8_t port_id, char *vdevname)
+{
+	char name[RTE_ETH_NAME_MAX_LEN];
+
+	if (vdevname == NULL)
+		goto err;
+
+	/* check whether the driver supports detach feature, or not */
+	if (rte_eth_dev_check_detachable(port_id))
+		goto err;
+
+	/* get device name by port id */
+	if (rte_eth_dev_get_name_by_port(port_id, name))
+		goto err;
+	/* walk around dev_driver_list to find the driver of the device,
+	 * then invoke close function o the driver */
+	if (rte_eal_vdev_find_and_invoke(name, RTE_EAL_INVOKE_TYPE_CLOSE))
+		goto err;
+	/* remove the vdevname from devargs_list */
+	if (rte_eal_devargs_remove(RTE_DEVTYPE_VIRTUAL, name))
+		goto err;
+
+	strncpy(vdevname, name, sizeof(name));
+	return 0;
+err:
+	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
+	return -1;
+}
+
+/* attach the new device, then store port_id of the device */
+int
+rte_eal_dev_attach(const char *devargs, uint8_t *port_id)
+{
+	struct rte_pci_addr addr;
+
+	if ((devargs == NULL) || (port_id == NULL))
+		return -EINVAL;
+
+	if (eal_parse_pci_DomBDF(devargs, &addr) == 0)
+		return rte_eal_dev_attach_pdev(&addr, port_id);
+	else
+		return rte_eal_dev_attach_vdev(devargs, port_id);
+}
+
+/* detach the device, then store the name of the device */
+int
+rte_eal_dev_detach(uint8_t port_id, char *name)
+{
+	struct rte_pci_addr addr;
+	int ret;
+
+	if (name == NULL)
+		return -EINVAL;
+
+	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PHYSICAL) {
+		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
+		if (ret < 0)
+			return ret;
+
+		ret = rte_eal_dev_detach_pdev(port_id, &addr);
+		if (ret == 0)
+			snprintf(name, RTE_ETH_NAME_MAX_LEN,
+				"%04x:%02x:%02x.%d",
+				addr.domain, addr.bus,
+				addr.devid, addr.function);
+
+		return ret;
+	} else
+		return rte_eal_dev_detach_vdev(port_id, name);
+}
+#else /* ENABLE_HOTPLUG */
+int
+rte_eal_dev_attach(const char *devargs __rte_unused,
+			uint8_t *port_id __rte_unused)
+{
+	RTE_LOG(ERR, EAL, "Hotplug support isn't enabled\n");
+	return -1;
+}
+
+/* detach the device, then store the name of the device */
+int
+rte_eal_dev_detach(uint8_t port_id __rte_unused,
+			char *name __rte_unused)
+{
+	RTE_LOG(ERR, EAL, "Hotplug support isn't enabled\n");
+	return -1;
+}
+#endif /* ENABLE_HOTPLUG */
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 1a362ab..8168a7a 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -163,6 +163,17 @@  enum rte_eal_invoke_type {
 };
 
 /**
+ * Scan the content of the PCI bus, and the devices in the devices
+ * list
+ *
+ * This function is private to EAL.
+ *
+ * @return
+ *  0 on success, negative on error
+ */
+int rte_eal_pci_scan(void);
+
+/**
  * Mmap memory for single PCI device
  *
  * This function is private to EAL.
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index f7e3a10..e63dd1c 100644
--- 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, const char *args);
+
+/**
  * Driver type enumeration
  */
 enum pmd_type {
@@ -72,6 +78,7 @@  struct rte_driver {
 	enum pmd_type type;		   /**< PMD Driver type */
 	const char *name;                   /**< Driver name. */
 	rte_dev_init_t *init;              /**< Device init. function. */
+	rte_dev_uninit_t *uninit;          /**< Device uninit. function. */
 };
 
 /**
@@ -93,6 +100,32 @@  void rte_eal_driver_register(struct rte_driver *driver);
 void rte_eal_driver_unregister(struct rte_driver *driver);
 
 /**
+ * Attach a new device.
+ *
+ * @param devargs
+ *   A pointer to a strings array describing the new device
+ *   to be attached. The strings should be a pci address like
+ *   '0000:01:00.0' or virtual device name like 'eth_pcap0'.
+ * @param port_id
+ *  A pointer to a port identifier actually attached.
+ * @return
+ *  0 on success and port_id is filled, negative on error
+ */
+int rte_eal_dev_attach(const char *devargs, uint8_t *port_id);
+
+/**
+ * Detach a device.
+ *
+ * @param port_id
+ *   The port identifier of the device to detach.
+ * @param addr
+ *  A pointer to a device name actually detached.
+ * @return
+ *  0 on success and devname is filled, negative on error
+ */
+int rte_eal_dev_detach(uint8_t port_id, char *devname);
+
+/**
  * Initalize all the registered drivers in this process
  */
 int rte_eal_dev_init(void);
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index e117cec..b59b201 100644
--- 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
 CFLAGS += -I$(RTE_SDK)/lib/librte_ether
 CFLAGS += -I$(RTE_SDK)/lib/librte_ivshmem
 CFLAGS += -I$(RTE_SDK)/lib/librte_pmd_ring
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 0adb217..e2dd034 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -443,8 +443,8 @@  error:
  * Scan the content of the PCI bus, and the devices in the devices
  * list
  */
-static int
-pci_scan(void)
+int
+rte_eal_pci_scan(void)
 {
 	struct dirent *e;
 	DIR *dir;
@@ -776,7 +776,7 @@  rte_eal_pci_init(void)
 	if (internal_config.no_pci)
 		return 0;
 
-	if (pci_scan() < 0) {
+	if (rte_eal_pci_scan() < 0) {
 		RTE_LOG(ERR, EAL, "%s(): Cannot scan PCI bus\n", __func__);
 		return -1;
 	}
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index d36286e..ff46b3a 100644
--- 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;
 	rte_eal_devargs_add;
 	rte_eal_devargs_dump;
 	rte_eal_devargs_type_count;