[dpdk-dev,v6,10/13] eal/pci: Cleanup pci driver initialization code

  - Add rte_eal_pci_close_one_dirver()
  The function is used for closing the specified driver and device.
- Add pci_invoke_all_drivers()
  The function is based on pci_probe_all_drivers. But it can not only
  probe but also close drivers.
- Add pci_close_all_drivers()
  The function tries to find a driver for the specified device, and
  then close the driver.
- Add rte_eal_pci_probe_one() and rte_eal_pci_close_one()
  The functions are used for probe and close a device.
  First the function tries to find a device that has the specfied
  PCI address. Then, probe or close the device.

v5:
- Remove RTE_EAL_INVOKE_TYPE_UNKNOWN, because it's unused.
v4:
- Fix paramerter checking.
- Fix indent of 'if' statement.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_common_pci.c  | 90 +++++++++++++++++++++++++++++----
 lib/librte_eal/common/eal_private.h     | 24 +++++++++
 lib/librte_eal/common/include/rte_pci.h | 33 ++++++++++++
 lib/librte_eal/linuxapp/eal/eal_pci.c   | 69 +++++++++++++++++++++++++
 4 files changed, 206 insertions(+), 10 deletions(-)
  

On 2/1/2015 12:02 PM, Tetsuya Mukawa wrote:
> - Add rte_eal_pci_close_one_dirver()
>   The function is used for closing the specified driver and device.
> - Add pci_invoke_all_drivers()
>   The function is based on pci_probe_all_drivers. But it can not only
>   probe but also close drivers.
> - Add pci_close_all_drivers()
>   The function tries to find a driver for the specified device, and
>   then close the driver.
> - Add rte_eal_pci_probe_one() and rte_eal_pci_close_one()
>   The functions are used for probe and close a device.
>   First the function tries to find a device that has the specfied
>   PCI address. Then, probe or close the device.
>
> v5:
> - Remove RTE_EAL_INVOKE_TYPE_UNKNOWN, because it's unused.
> v4:
> - Fix paramerter checking.
> - Fix indent of 'if' statement.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/common/eal_common_pci.c  | 90 +++++++++++++++++++++++++++++----
>  lib/librte_eal/common/eal_private.h     | 24 +++++++++
>  lib/librte_eal/common/include/rte_pci.h | 33 ++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci.c   | 69 +++++++++++++++++++++++++
>  4 files changed, 206 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
> index a89f5c3..7c9b8c5 100644
> --- a/lib/librte_eal/common/eal_common_pci.c
> +++ b/lib/librte_eal/common/eal_common_pci.c
> @@ -99,19 +99,27 @@ static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
>  	return NULL;
>  }
>  
> -/*
> - * If vendor/device ID match, call the devinit() function of all
> - * registered driver for the given device. Return -1 if initialization
> - * failed, return 1 if no driver is found for this device.
> - */
>  static int
> -pci_probe_all_drivers(struct rte_pci_device *dev)
> +pci_invoke_all_drivers(struct rte_pci_device *dev,
> +		enum rte_eal_invoke_type type)
>  {
>  	struct rte_pci_driver *dr = NULL;
> -	int rc;
> +	int rc = 0;
> +
> +	if ((dev == NULL) || (type >= RTE_EAL_INVOKE_TYPE_MAX))
> +		return -1;
>  
>  	TAILQ_FOREACH(dr, &pci_driver_list, next) {
> -		rc = rte_eal_pci_probe_one_driver(dr, dev);
> +		switch (type) {
> +		case RTE_EAL_INVOKE_TYPE_PROBE:
> +			rc = rte_eal_pci_probe_one_driver(dr, dev);
> +			break;
> +		case RTE_EAL_INVOKE_TYPE_CLOSE:
> +			rc = rte_eal_pci_close_one_driver(dr, dev);
> +			break;
> +		default:
> +			return -1;
> +		}
>  		if (rc < 0)
>  			/* negative value is an error */
>  			return -1;
> @@ -123,6 +131,66 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
>  	return 1;
>  }
>  
> +#ifdef ENABLE_HOTPLUG
> +static int
> +rte_eal_pci_invoke_one(struct rte_pci_addr *addr,
> +		enum rte_eal_invoke_type type)
> +{
> +	struct rte_pci_device *dev = NULL;
> +	int ret = 0;
> +
> +	if ((addr == NULL) || (type >= RTE_EAL_INVOKE_TYPE_MAX))
> +		return -1;
> +
> +	TAILQ_FOREACH(dev, &pci_device_list, next) {
> +		if (eal_compare_pci_addr(&dev->addr, addr))
> +			continue;
> +
> +		ret = pci_invoke_all_drivers(dev, type);
> +		if (ret < 0)
> +			goto invoke_err_return;
> +
> +		if (type == RTE_EAL_INVOKE_TYPE_CLOSE)
> +			goto remove_dev;
> +
> +		return 0;
> +	}
> +
> +	return -1;
> +
> +invoke_err_return:
> +	RTE_LOG(WARNING, EAL, "Requested device " PCI_PRI_FMT
> +			" cannot be used\n", dev->addr.domain, dev->addr.bus,
> +			dev->addr.devid, dev->addr.function);
> +	return -1;
> +
> +remove_dev:
> +	TAILQ_REMOVE(&pci_device_list, dev, next);
> +	return 0;
> +}
> +
> +
> +/*
> + * Find the pci device specified by pci address, then invoke probe function of
> + * the driver of the devive.
> + */
> +int
> +rte_eal_pci_probe_one(struct rte_pci_addr *addr)
> +{
> +	return rte_eal_pci_invoke_one(addr, RTE_EAL_INVOKE_TYPE_PROBE);
> +}
> +
> +/*
> + * Find the pci device specified by pci address, then invoke close function of
> + * the driver of the devive.
> + */
> +int
> +rte_eal_pci_close_one(struct rte_pci_addr *addr)
> +{
> +	return rte_eal_pci_invoke_one(addr, RTE_EAL_INVOKE_TYPE_CLOSE);
> +}
> +#endif /* ENABLE_HOTPLUG */
> +
>  /*
>   * Scan the content of the PCI bus, and call the devinit() function for
>   * all registered drivers that have a matching entry in its id_table
> @@ -148,10 +216,12 @@ rte_eal_pci_probe(void)
>  
>  		/* probe all or only whitelisted devices */
>  		if (probe_all)
> -			ret = pci_probe_all_drivers(dev);
> +			ret = pci_invoke_all_drivers(dev,
> +					RTE_EAL_INVOKE_TYPE_PROBE);
>  		else if (devargs != NULL &&
>  			devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
> -			ret = pci_probe_all_drivers(dev);
> +			ret = pci_invoke_all_drivers(dev,
> +					RTE_EAL_INVOKE_TYPE_PROBE);
>  		if (ret < 0)
>  			rte_exit(EXIT_FAILURE, "Requested device " PCI_PRI_FMT
>  				 " cannot be used\n", dev->addr.domain, dev->addr.bus,
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index 159cd66..1a362ab 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -154,6 +154,15 @@ struct rte_pci_driver;
>  struct rte_pci_device;
>  
>  /**
> + * The invoke type.
> + */
> +enum rte_eal_invoke_type {
> +	RTE_EAL_INVOKE_TYPE_PROBE,  /**< invoke probe function */
> +	RTE_EAL_INVOKE_TYPE_CLOSE,  /**< invoke close function */
> +	RTE_EAL_INVOKE_TYPE_MAX     /**< max value of this enum */
> +};
> +
> +/**
>   * Mmap memory for single PCI device
>   *
>   * This function is private to EAL.
> @@ -165,6 +174,21 @@ int rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr,
>  		struct rte_pci_device *dev);
>  
>  /**
> + * Munmap memory for single PCI device
> + *
> + * This function is private to EAL.
> + *
> + * @param	dr
> + *  The pointer to the pci driver structure
> + * @param	dev
> + *  The pointer to the pci device structure
> + * @return
> + *   0 on success, negative on error
> + */
> +int rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
> +		struct rte_pci_device *dev);
> +
> +/**
>   * Init tail queues for non-EAL library structures. This is to allow
>   * the rings, mempools, etc. lists to be shared among multiple processes
>   *
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index 87ca4cf..a111066 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -82,6 +82,7 @@ extern "C" {
>  #include <inttypes.h>
>  
>  #include <rte_interrupts.h>
> +#include <rte_dev_hotplug.h>
>  
>  TAILQ_HEAD(pci_device_list, rte_pci_device); /**< PCI devices in D-linked Q. */
>  TAILQ_HEAD(pci_driver_list, rte_pci_driver); /**< PCI drivers in D-linked Q. */
> @@ -323,6 +324,38 @@ eal_compare_pci_addr(struct rte_pci_addr *addr, struct rte_pci_addr *addr2)
>   */
>  int rte_eal_pci_probe(void);
>  
> +#ifdef ENABLE_HOTPLUG
> +/**
> + * Probe the single PCI device.
> + *
> + * Scan the content of the PCI bus, and find the pci device specified by pci
> + * address, then call the probe() function for registered driver that has a
> + * matching entry in its id_table for discovered device.
> + *
> + * @param addr
> + *	The PCI Bus-Device-Function address to probe or close.
> + * @return
> + *   - 0 on success.
> + *   - Negative on error.
> + */
> +int rte_eal_pci_probe_one(struct rte_pci_addr *addr);
> +
> +/**
> + * Close the single PCI device.
> + *
> + * Scan the content of the PCI bus, and find the pci device specified by pci
> + * address, then call the close() function for registered driver that has a
> + * matching entry in its id_table for discovered device.
> + *
> + * @param addr
> + *	The PCI Bus-Device-Function address to probe or close.
> + * @return
> + *   - 0 on success.
> + *   - Negative on error.
> + */
> +int rte_eal_pci_close_one(struct rte_pci_addr *addr);
> +#endif /* ENABLE_HOTPLUG */
> +
>  /**
>   * Dump the content of the PCI bus.
>   *
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index c3b7917..831422e 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -682,6 +682,75 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
>  	return 1;
>  }
>  
> +#ifdef ENABLE_HOTPLUG
> +/*
> + * If vendor/device ID match, call the devuninit() function of the
> + * driver.
> + */
> +int
> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
> +		struct rte_pci_device *dev)
> +{
> +	struct rte_pci_id *id_table;
> +
> +	if ((dr == NULL) || (dev == NULL))
> +		return -EINVAL;
> +
> +	for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
> +
> +		/* check if device's identifiers match the driver's ones */
> +		if (id_table->vendor_id != dev->id.vendor_id &&
> +		    id_table->vendor_id != PCI_ANY_ID)
> +			continue;
> +		if (id_table->device_id != dev->id.device_id &&
> +		    id_table->device_id != PCI_ANY_ID)
> +			continue;
> +		if (id_table->subsystem_vendor_id !=
> +		    dev->id.subsystem_vendor_id &&
> +		    id_table->subsystem_vendor_id != PCI_ANY_ID)
> +			continue;
> +		if (id_table->subsystem_device_id !=
> +		    dev->id.subsystem_device_id &&
> +		    id_table->subsystem_device_id != PCI_ANY_ID)
> +			continue;
> +
> +		struct rte_pci_addr *loc = &dev->addr;
> +
> +		RTE_LOG(DEBUG, EAL,
> +				"PCI device "PCI_PRI_FMT" on NUMA socket %i\n",
> +				loc->domain, loc->bus, loc->devid,
> +				loc->function, dev->numa_node);
> +
> +		RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n",
> +				dev->id.vendor_id, dev->id.device_id,
> +				dr->name);
> +
> +		/* call the driver devuninit() function */
> +		if (dr->devuninit && (dr->devuninit(dr, dev) < 0))
> +			return -1;	/* negative value is an error */
> +
> +		/* clear driver structure */
> +		dev->driver = NULL;
> +
> +		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> +			/* unmap resources for devices that use igb_uio */
> +			pci_unmap_device(dev);

Hi, Tetsuya

I have one question,  as the code shows, in pci_unmap_device(), will
check pt_driver.

But assume that, we are now try to detach a vfio device, after print out
a error message of unsupported, the does this port workable?

I think this port will unworkable, am I right?

But actually, we should keep it workable.

My suggestion is to add a check in  rte_eth_dev_check_detachable() for
pci_device port.


Thanks
Michael

> +
> +		return 0;
> +	}
> +	/* return positive value if driver is not found */
> +	return 1;
> +}
> +#else /* ENABLE_HOTPLUG */
> +int
> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr __rte_unused,
> +		struct rte_pci_device *dev __rte_unused)
> +{
> +	RTE_LOG(ERR, EAL, "Hotplug support isn't enabled\n");
> +	return -1;
> +}
> +#endif /* ENABLE_HOTPLUG */
> +
>  /* Init the PCI EAL subsystem */
>  int
>  rte_eal_pci_init(void)
  

On 2015/02/03 11:35, Qiu, Michael wrote:
> On 2/1/2015 12:02 PM, Tetsuya Mukawa wrote:
>> - Add rte_eal_pci_close_one_dirver()
>>   The function is used for closing the specified driver and device.
>> - Add pci_invoke_all_drivers()
>>   The function is based on pci_probe_all_drivers. But it can not only
>>   probe but also close drivers.
>> - Add pci_close_all_drivers()
>>   The function tries to find a driver for the specified device, and
>>   then close the driver.
>> - Add rte_eal_pci_probe_one() and rte_eal_pci_close_one()
>>   The functions are used for probe and close a device.
>>   First the function tries to find a device that has the specfied
>>   PCI address. Then, probe or close the device.
>>
>> v5:
>> - Remove RTE_EAL_INVOKE_TYPE_UNKNOWN, because it's unused.
>> v4:
>> - Fix paramerter checking.
>> - Fix indent of 'if' statement.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>>  lib/librte_eal/common/eal_common_pci.c  | 90 +++++++++++++++++++++++++++++----
>>  lib/librte_eal/common/eal_private.h     | 24 +++++++++
>>  lib/librte_eal/common/include/rte_pci.h | 33 ++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal_pci.c   | 69 +++++++++++++++++++++++++
>>  4 files changed, 206 insertions(+), 10 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
>> index a89f5c3..7c9b8c5 100644
>> --- a/lib/librte_eal/common/eal_common_pci.c
>> +++ b/lib/librte_eal/common/eal_common_pci.c
>> @@ -99,19 +99,27 @@ static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
>>  	return NULL;
>>  }
>>  
>> -/*
>> - * If vendor/device ID match, call the devinit() function of all
>> - * registered driver for the given device. Return -1 if initialization
>> - * failed, return 1 if no driver is found for this device.
>> - */
>>  static int
>> -pci_probe_all_drivers(struct rte_pci_device *dev)
>> +pci_invoke_all_drivers(struct rte_pci_device *dev,
>> +		enum rte_eal_invoke_type type)
>>  {
>>  	struct rte_pci_driver *dr = NULL;
>> -	int rc;
>> +	int rc = 0;
>> +
>> +	if ((dev == NULL) || (type >= RTE_EAL_INVOKE_TYPE_MAX))
>> +		return -1;
>>  
>>  	TAILQ_FOREACH(dr, &pci_driver_list, next) {
>> -		rc = rte_eal_pci_probe_one_driver(dr, dev);
>> +		switch (type) {
>> +		case RTE_EAL_INVOKE_TYPE_PROBE:
>> +			rc = rte_eal_pci_probe_one_driver(dr, dev);
>> +			break;
>> +		case RTE_EAL_INVOKE_TYPE_CLOSE:
>> +			rc = rte_eal_pci_close_one_driver(dr, dev);
>> +			break;
>> +		default:
>> +			return -1;
>> +		}
>>  		if (rc < 0)
>>  			/* negative value is an error */
>>  			return -1;
>> @@ -123,6 +131,66 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
>>  	return 1;
>>  }
>>  
>> +#ifdef ENABLE_HOTPLUG
>> +static int
>> +rte_eal_pci_invoke_one(struct rte_pci_addr *addr,
>> +		enum rte_eal_invoke_type type)
>> +{
>> +	struct rte_pci_device *dev = NULL;
>> +	int ret = 0;
>> +
>> +	if ((addr == NULL) || (type >= RTE_EAL_INVOKE_TYPE_MAX))
>> +		return -1;
>> +
>> +	TAILQ_FOREACH(dev, &pci_device_list, next) {
>> +		if (eal_compare_pci_addr(&dev->addr, addr))
>> +			continue;
>> +
>> +		ret = pci_invoke_all_drivers(dev, type);
>> +		if (ret < 0)
>> +			goto invoke_err_return;
>> +
>> +		if (type == RTE_EAL_INVOKE_TYPE_CLOSE)
>> +			goto remove_dev;
>> +
>> +		return 0;
>> +	}
>> +
>> +	return -1;
>> +
>> +invoke_err_return:
>> +	RTE_LOG(WARNING, EAL, "Requested device " PCI_PRI_FMT
>> +			" cannot be used\n", dev->addr.domain, dev->addr.bus,
>> +			dev->addr.devid, dev->addr.function);
>> +	return -1;
>> +
>> +remove_dev:
>> +	TAILQ_REMOVE(&pci_device_list, dev, next);
>> +	return 0;
>> +}
>> +
>> +
>> +/*
>> + * Find the pci device specified by pci address, then invoke probe function of
>> + * the driver of the devive.
>> + */
>> +int
>> +rte_eal_pci_probe_one(struct rte_pci_addr *addr)
>> +{
>> +	return rte_eal_pci_invoke_one(addr, RTE_EAL_INVOKE_TYPE_PROBE);
>> +}
>> +
>> +/*
>> + * Find the pci device specified by pci address, then invoke close function of
>> + * the driver of the devive.
>> + */
>> +int
>> +rte_eal_pci_close_one(struct rte_pci_addr *addr)
>> +{
>> +	return rte_eal_pci_invoke_one(addr, RTE_EAL_INVOKE_TYPE_CLOSE);
>> +}
>> +#endif /* ENABLE_HOTPLUG */
>> +
>>  /*
>>   * Scan the content of the PCI bus, and call the devinit() function for
>>   * all registered drivers that have a matching entry in its id_table
>> @@ -148,10 +216,12 @@ rte_eal_pci_probe(void)
>>  
>>  		/* probe all or only whitelisted devices */
>>  		if (probe_all)
>> -			ret = pci_probe_all_drivers(dev);
>> +			ret = pci_invoke_all_drivers(dev,
>> +					RTE_EAL_INVOKE_TYPE_PROBE);
>>  		else if (devargs != NULL &&
>>  			devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
>> -			ret = pci_probe_all_drivers(dev);
>> +			ret = pci_invoke_all_drivers(dev,
>> +					RTE_EAL_INVOKE_TYPE_PROBE);
>>  		if (ret < 0)
>>  			rte_exit(EXIT_FAILURE, "Requested device " PCI_PRI_FMT
>>  				 " cannot be used\n", dev->addr.domain, dev->addr.bus,
>> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
>> index 159cd66..1a362ab 100644
>> --- a/lib/librte_eal/common/eal_private.h
>> +++ b/lib/librte_eal/common/eal_private.h
>> @@ -154,6 +154,15 @@ struct rte_pci_driver;
>>  struct rte_pci_device;
>>  
>>  /**
>> + * The invoke type.
>> + */
>> +enum rte_eal_invoke_type {
>> +	RTE_EAL_INVOKE_TYPE_PROBE,  /**< invoke probe function */
>> +	RTE_EAL_INVOKE_TYPE_CLOSE,  /**< invoke close function */
>> +	RTE_EAL_INVOKE_TYPE_MAX     /**< max value of this enum */
>> +};
>> +
>> +/**
>>   * Mmap memory for single PCI device
>>   *
>>   * This function is private to EAL.
>> @@ -165,6 +174,21 @@ int rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr,
>>  		struct rte_pci_device *dev);
>>  
>>  /**
>> + * Munmap memory for single PCI device
>> + *
>> + * This function is private to EAL.
>> + *
>> + * @param	dr
>> + *  The pointer to the pci driver structure
>> + * @param	dev
>> + *  The pointer to the pci device structure
>> + * @return
>> + *   0 on success, negative on error
>> + */
>> +int rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
>> +		struct rte_pci_device *dev);
>> +
>> +/**
>>   * Init tail queues for non-EAL library structures. This is to allow
>>   * the rings, mempools, etc. lists to be shared among multiple processes
>>   *
>> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
>> index 87ca4cf..a111066 100644
>> --- a/lib/librte_eal/common/include/rte_pci.h
>> +++ b/lib/librte_eal/common/include/rte_pci.h
>> @@ -82,6 +82,7 @@ extern "C" {
>>  #include <inttypes.h>
>>  
>>  #include <rte_interrupts.h>
>> +#include <rte_dev_hotplug.h>
>>  
>>  TAILQ_HEAD(pci_device_list, rte_pci_device); /**< PCI devices in D-linked Q. */
>>  TAILQ_HEAD(pci_driver_list, rte_pci_driver); /**< PCI drivers in D-linked Q. */
>> @@ -323,6 +324,38 @@ eal_compare_pci_addr(struct rte_pci_addr *addr, struct rte_pci_addr *addr2)
>>   */
>>  int rte_eal_pci_probe(void);
>>  
>> +#ifdef ENABLE_HOTPLUG
>> +/**
>> + * Probe the single PCI device.
>> + *
>> + * Scan the content of the PCI bus, and find the pci device specified by pci
>> + * address, then call the probe() function for registered driver that has a
>> + * matching entry in its id_table for discovered device.
>> + *
>> + * @param addr
>> + *	The PCI Bus-Device-Function address to probe or close.
>> + * @return
>> + *   - 0 on success.
>> + *   - Negative on error.
>> + */
>> +int rte_eal_pci_probe_one(struct rte_pci_addr *addr);
>> +
>> +/**
>> + * Close the single PCI device.
>> + *
>> + * Scan the content of the PCI bus, and find the pci device specified by pci
>> + * address, then call the close() function for registered driver that has a
>> + * matching entry in its id_table for discovered device.
>> + *
>> + * @param addr
>> + *	The PCI Bus-Device-Function address to probe or close.
>> + * @return
>> + *   - 0 on success.
>> + *   - Negative on error.
>> + */
>> +int rte_eal_pci_close_one(struct rte_pci_addr *addr);
>> +#endif /* ENABLE_HOTPLUG */
>> +
>>  /**
>>   * Dump the content of the PCI bus.
>>   *
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
>> index c3b7917..831422e 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
>> @@ -682,6 +682,75 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
>>  	return 1;
>>  }
>>  
>> +#ifdef ENABLE_HOTPLUG
>> +/*
>> + * If vendor/device ID match, call the devuninit() function of the
>> + * driver.
>> + */
>> +int
>> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
>> +		struct rte_pci_device *dev)
>> +{
>> +	struct rte_pci_id *id_table;
>> +
>> +	if ((dr == NULL) || (dev == NULL))
>> +		return -EINVAL;
>> +
>> +	for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
>> +
>> +		/* check if device's identifiers match the driver's ones */
>> +		if (id_table->vendor_id != dev->id.vendor_id &&
>> +		    id_table->vendor_id != PCI_ANY_ID)
>> +			continue;
>> +		if (id_table->device_id != dev->id.device_id &&
>> +		    id_table->device_id != PCI_ANY_ID)
>> +			continue;
>> +		if (id_table->subsystem_vendor_id !=
>> +		    dev->id.subsystem_vendor_id &&
>> +		    id_table->subsystem_vendor_id != PCI_ANY_ID)
>> +			continue;
>> +		if (id_table->subsystem_device_id !=
>> +		    dev->id.subsystem_device_id &&
>> +		    id_table->subsystem_device_id != PCI_ANY_ID)
>> +			continue;
>> +
>> +		struct rte_pci_addr *loc = &dev->addr;
>> +
>> +		RTE_LOG(DEBUG, EAL,
>> +				"PCI device "PCI_PRI_FMT" on NUMA socket %i\n",
>> +				loc->domain, loc->bus, loc->devid,
>> +				loc->function, dev->numa_node);
>> +
>> +		RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n",
>> +				dev->id.vendor_id, dev->id.device_id,
>> +				dr->name);
>> +
>> +		/* call the driver devuninit() function */
>> +		if (dr->devuninit && (dr->devuninit(dr, dev) < 0))
>> +			return -1;	/* negative value is an error */
>> +
>> +		/* clear driver structure */
>> +		dev->driver = NULL;
>> +
>> +		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
>> +			/* unmap resources for devices that use igb_uio */
>> +			pci_unmap_device(dev);
> Hi, Tetsuya
>
> I have one question,  as the code shows, in pci_unmap_device(), will
> check pt_driver.
>
> But assume that, we are now try to detach a vfio device, after print out
> a error message of unsupported, the does this port workable?
>
> I think this port will unworkable, am I right?
>
> But actually, we should keep it workable.
>
> My suggestion is to add a check in  rte_eth_dev_check_detachable() for
> pci_device port.

Hi Michael,

I appreciate your comment.
In the function called "rte_eal_dev_detach_pdev()",
"rte_eth_dev_check_detachable()" has been already checked.
But in the future, someone may want to reuse
"rte_eal_pci_close_one_driver()".
So I will add the checking like your suggestion.

Thanks,
Tetsuya

>
> Thanks
> Michael
>
>> +
>> +		return 0;
>> +	}
>> +	/* return positive value if driver is not found */
>> +	return 1;
>> +}
>> +#else /* ENABLE_HOTPLUG */
>> +int
>> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr __rte_unused,
>> +		struct rte_pci_device *dev __rte_unused)
>> +{
>> +	RTE_LOG(ERR, EAL, "Hotplug support isn't enabled\n");
>> +	return -1;
>> +}
>> +#endif /* ENABLE_HOTPLUG */
>> +
>>  /* Init the PCI EAL subsystem */
>>  int
>>  rte_eal_pci_init(void)
  

On 2/3/2015 12:07 PM, Tetsuya Mukawa wrote:
> On 2015/02/03 11:35, Qiu, Michael wrote:
>> On 2/1/2015 12:02 PM, Tetsuya Mukawa wrote:
>>> - Add rte_eal_pci_close_one_dirver()
>>>   The function is used for closing the specified driver and device.
>>> - Add pci_invoke_all_drivers()

[...]
>>>  
>>> +#ifdef ENABLE_HOTPLUG
>>> +/*
>>> + * If vendor/device ID match, call the devuninit() function of the
>>> + * driver.
>>> + */
>>> +int
>>> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
>>> +		struct rte_pci_device *dev)
>>> +{
>>> +	struct rte_pci_id *id_table;
>>> +
>>> +	if ((dr == NULL) || (dev == NULL))
>>> +		return -EINVAL;
>>> +
>>> +	for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
>>> +
>>> +		/* check if device's identifiers match the driver's ones */
>>> +		if (id_table->vendor_id != dev->id.vendor_id &&
>>> +		    id_table->vendor_id != PCI_ANY_ID)
>>> +			continue;
>>> +		if (id_table->device_id != dev->id.device_id &&
>>> +		    id_table->device_id != PCI_ANY_ID)
>>> +			continue;
>>> +		if (id_table->subsystem_vendor_id !=
>>> +		    dev->id.subsystem_vendor_id &&
>>> +		    id_table->subsystem_vendor_id != PCI_ANY_ID)
>>> +			continue;
>>> +		if (id_table->subsystem_device_id !=
>>> +		    dev->id.subsystem_device_id &&
>>> +		    id_table->subsystem_device_id != PCI_ANY_ID)
>>> +			continue;
>>> +
>>> +		struct rte_pci_addr *loc = &dev->addr;
>>> +
>>> +		RTE_LOG(DEBUG, EAL,
>>> +				"PCI device "PCI_PRI_FMT" on NUMA socket %i\n",
>>> +				loc->domain, loc->bus, loc->devid,
>>> +				loc->function, dev->numa_node);
>>> +
>>> +		RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n",
>>> +				dev->id.vendor_id, dev->id.device_id,
>>> +				dr->name);
>>> +
>>> +		/* call the driver devuninit() function */
>>> +		if (dr->devuninit && (dr->devuninit(dr, dev) < 0))
>>> +			return -1;	/* negative value is an error */
>>> +
>>> +		/* clear driver structure */
>>> +		dev->driver = NULL;
>>> +
>>> +		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
>>> +			/* unmap resources for devices that use igb_uio */
>>> +			pci_unmap_device(dev);
>> Hi, Tetsuya
>>
>> I have one question,  as the code shows, in pci_unmap_device(), will
>> check pt_driver.
>>
>> But assume that, we are now try to detach a vfio device, after print out
>> a error message of unsupported, the does this port workable?
>>
>> I think this port will unworkable, am I right?
>>
>> But actually, we should keep it workable.
>>
>> My suggestion is to add a check in  rte_eth_dev_check_detachable() for
>> pci_device port.
> Hi Michael,
>
> I appreciate your comment.
> In the function called "rte_eal_dev_detach_pdev()",
> "rte_eth_dev_check_detachable()" has been already checked.

What I mean is check the pt_driver for pci_dev in
rte_eth_dev_check_detachable(), so that hotplug framework will not
affect vfio devices, just as I reply in another mail.

Current logic will affect vfio devices if try to detach( Not do the
really test, just the logic shows), am I right?

Thanks,
Michael
 
> But in the future, someone may want to reuse
> "rte_eal_pci_close_one_driver()".
> So I will add the checking like your suggestion.
>
> Thanks,
> Tetsuya
>
>> Thanks
>> Michael
>>
>>> +
>>> +		return 0;
>>> +	}
>>> +	/* return positive value if driver is not found */
>>> +	return 1;
>>> +}
>>> +#else /* ENABLE_HOTPLUG */
>>> +int
>>> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr __rte_unused,
>>> +		struct rte_pci_device *dev __rte_unused)
>>> +{
>>> +	RTE_LOG(ERR, EAL, "Hotplug support isn't enabled\n");
>>> +	return -1;
>>> +}
>>> +#endif /* ENABLE_HOTPLUG */
>>> +
>>>  /* Init the PCI EAL subsystem */
>>>  int
>>>  rte_eal_pci_init(void)
>
>
  

On 2015/02/03 14:05, Qiu, Michael wrote:
> On 2/3/2015 12:07 PM, Tetsuya Mukawa wrote:
>> On 2015/02/03 11:35, Qiu, Michael wrote:
>>> On 2/1/2015 12:02 PM, Tetsuya Mukawa wrote:
>>>> - Add rte_eal_pci_close_one_dirver()
>>>>   The function is used for closing the specified driver and device.
>>>> - Add pci_invoke_all_drivers()
> [...]
>>>>  
>>>> +#ifdef ENABLE_HOTPLUG
>>>> +/*
>>>> + * If vendor/device ID match, call the devuninit() function of the
>>>> + * driver.
>>>> + */
>>>> +int
>>>> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
>>>> +		struct rte_pci_device *dev)
>>>> +{
>>>> +	struct rte_pci_id *id_table;
>>>> +
>>>> +	if ((dr == NULL) || (dev == NULL))
>>>> +		return -EINVAL;
>>>> +
>>>> +	for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
>>>> +
>>>> +		/* check if device's identifiers match the driver's ones */
>>>> +		if (id_table->vendor_id != dev->id.vendor_id &&
>>>> +		    id_table->vendor_id != PCI_ANY_ID)
>>>> +			continue;
>>>> +		if (id_table->device_id != dev->id.device_id &&
>>>> +		    id_table->device_id != PCI_ANY_ID)
>>>> +			continue;
>>>> +		if (id_table->subsystem_vendor_id !=
>>>> +		    dev->id.subsystem_vendor_id &&
>>>> +		    id_table->subsystem_vendor_id != PCI_ANY_ID)
>>>> +			continue;
>>>> +		if (id_table->subsystem_device_id !=
>>>> +		    dev->id.subsystem_device_id &&
>>>> +		    id_table->subsystem_device_id != PCI_ANY_ID)
>>>> +			continue;
>>>> +
>>>> +		struct rte_pci_addr *loc = &dev->addr;
>>>> +
>>>> +		RTE_LOG(DEBUG, EAL,
>>>> +				"PCI device "PCI_PRI_FMT" on NUMA socket %i\n",
>>>> +				loc->domain, loc->bus, loc->devid,
>>>> +				loc->function, dev->numa_node);
>>>> +
>>>> +		RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n",
>>>> +				dev->id.vendor_id, dev->id.device_id,
>>>> +				dr->name);
>>>> +
>>>> +		/* call the driver devuninit() function */
>>>> +		if (dr->devuninit && (dr->devuninit(dr, dev) < 0))
>>>> +			return -1;	/* negative value is an error */
>>>> +
>>>> +		/* clear driver structure */
>>>> +		dev->driver = NULL;
>>>> +
>>>> +		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
>>>> +			/* unmap resources for devices that use igb_uio */
>>>> +			pci_unmap_device(dev);
>>> Hi, Tetsuya
>>>
>>> I have one question,  as the code shows, in pci_unmap_device(), will
>>> check pt_driver.
>>>
>>> But assume that, we are now try to detach a vfio device, after print out
>>> a error message of unsupported, the does this port workable?
>>>
>>> I think this port will unworkable, am I right?
>>>
>>> But actually, we should keep it workable.
>>>
>>> My suggestion is to add a check in  rte_eth_dev_check_detachable() for
>>> pci_device port.
>> Hi Michael,
>>
>> I appreciate your comment.
>> In the function called "rte_eal_dev_detach_pdev()",
>> "rte_eth_dev_check_detachable()" has been already checked.
> What I mean is check the pt_driver for pci_dev in
> rte_eth_dev_check_detachable(), so that hotplug framework will not
> affect vfio devices, just as I reply in another mail.
>
> Current logic will affect vfio devices if try to detach( Not do the
> really test, just the logic shows), am I right?

Thanks, I've got your point.
Yes, you are right. I will fix it.

Tetsuya

> Thanks,
> Michael
>  
>> But in the future, someone may want to reuse
>> "rte_eal_pci_close_one_driver()".
>> So I will add the checking like your suggestion.
>>
>> Thanks,
>> Tetsuya
>>
>>> Thanks
>>> Michael
>>>
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +	/* return positive value if driver is not found */
>>>> +	return 1;
>>>> +}
>>>> +#else /* ENABLE_HOTPLUG */
>>>> +int
>>>> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr __rte_unused,
>>>> +		struct rte_pci_device *dev __rte_unused)
>>>> +{
>>>> +	RTE_LOG(ERR, EAL, "Hotplug support isn't enabled\n");
>>>> +	return -1;
>>>> +}
>>>> +#endif /* ENABLE_HOTPLUG */
>>>> +
>>>>  /* Init the PCI EAL subsystem */
>>>>  int
>>>>  rte_eal_pci_init(void)
>>