Re: [dpdk-dev] [PATCH v2 7/7] pci: Clarify interfaces for dynamic attach/detach of drivers

[Shreyansh Jain <shreyansh.jain@nxp.com>]

On Thursday 24 November 2016 01:37 AM, Ben Walker wrote:
> There are now two functions - rte_eal_pci_attach_driver and
> rte_eal_pci_detach_driver - that dynamically attempt to attach
> and detach drivers from PCI devices. These only control
> whether a registered PCI driver is loaded or not - they are
> independent of whether the PCI device exists on the system.
>
> Signed-off-by: Ben Walker <benjamin.walker@intel.com>
> ---
>  lib/librte_eal/common/eal_common_dev.c  |   4 +-
>  lib/librte_eal/common/eal_common_pci.c  | 109 +++++++++-----------------------
>  lib/librte_eal/common/include/rte_pci.h |  22 ++++---
>  3 files changed, 43 insertions(+), 92 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index 4f3b493..1c6834e 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -114,7 +114,7 @@ int rte_eal_dev_attach(const char *name, const char *devargs)
>  	}
>
>  	if (eal_parse_pci_DomBDF(name, &addr) == 0) {

IMO, ideally all the PCI specific code should be within eal_common_pci. 
For example, the above function (eal_common_pci_DomBDF) shouldn't be 
part of the generic fn rte_eal_dev_attach. Somehow this should be 
hidden/closed with PCI specific files.

In the eal_common_dev, rte_eal_dev_attach and rte_eal_dev_detach are 
directly referring to PCI as devices.

isn't it?

> -		if (rte_eal_pci_probe_one(&addr) < 0)
> +		if (rte_eal_pci_attach_driver(&addr) < 0)
>  			goto err;
>
>  	} else {
> @@ -139,7 +139,7 @@ int rte_eal_dev_detach(const char *name)
>  	}
>
>  	if (eal_parse_pci_DomBDF(name, &addr) == 0) {
> -		if (rte_eal_pci_detach(&addr) < 0)
> +		if (rte_eal_pci_detach_driver(&addr) < 0)
>  			goto err;
>  	} else {
>  		if (rte_eal_vdev_uninit(name))
> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
> index d50a534..67c6ce6 100644
> --- a/lib/librte_eal/common/eal_common_pci.c
> +++ b/lib/librte_eal/common/eal_common_pci.c
> @@ -228,59 +228,36 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
>  	return 1;
>  }
>
> -/*
> - * If vendor/device ID match, call the remove() function of the
> - * driver.
> - */
>  static int
> -rte_eal_pci_detach_dev(struct rte_pci_driver *dr,
> -		struct rte_pci_device *dev)
> +rte_eal_pci_remove_driver(struct rte_pci_device *dev)
>  {
> -	const struct rte_pci_id *id_table;
> +	struct rte_pci_driver *driver;
> +	struct rte_pci_addr *loc;
>
> -	if ((dr == NULL) || (dev == NULL))
> -		return -EINVAL;
> +	loc = &dev->addr;
> +	driver = dev->driver;
>
> -	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;
> +	if (driver == NULL)
> +		return 0;
>
> -		RTE_LOG(DEBUG, EAL, "PCI device "PCI_PRI_FMT" on NUMA socket %i\n",
> -				loc->domain, loc->bus, loc->devid,
> -				loc->function, dev->device.numa_node);
> +	RTE_LOG(DEBUG, EAL, "PCI device "PCI_PRI_FMT" on NUMA socket %i\n",
> +			loc->domain, loc->bus, loc->devid,
> +			loc->function, dev->device.numa_node);
>
> -		RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
> -				dev->id.device_id, dr->driver.name);
> +	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
> +			dev->id.device_id, driver->driver.name);
>
> -		if (dr->remove && (dr->remove(dev) < 0))
> -			return -1;	/* negative value is an error */
> +	if (driver->remove && (driver->remove(dev) < 0))
> +		return -1;	/* negative value is an error */
>
> -		/* clear driver structure */
> -		dev->driver = NULL;
> +	/* clear driver structure */
> +	dev->driver = NULL;
>
> -		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> -			/* unmap resources for devices that use igb_uio */
> -			rte_eal_pci_unmap_device(dev);
> +	if (driver->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> +		/* unmap resources for devices that use igb_uio */
> +		rte_eal_pci_unmap_device(dev);
>
> -		return 0;
> -	}
> -
> -	/* return positive value if driver doesn't support this device */
> -	return 1;
> +	return 0;
>  }
>
>  /*
> @@ -315,38 +292,11 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
>  }
>
>  /*
> - * If vendor/device ID match, call the remove() 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_detach_all_drivers(struct rte_pci_device *dev)
> -{
> -	struct rte_pci_driver *dr = NULL;
> -	int rc = 0;
> -
> -	if (dev == NULL)
> -		return -1;
> -
> -	TAILQ_FOREACH(dr, &pci_driver_list, next) {
> -		rc = rte_eal_pci_detach_dev(dr, dev);
> -		if (rc < 0)
> -			/* negative value is an error */
> -			return -1;
> -		if (rc > 0)
> -			/* positive value means driver doesn't support it */
> -			continue;
> -		return 0;
> -	}
> -	return 1;
> -}
> -
> -/*
>   * Find the pci device specified by pci address, then invoke probe function of
>   * the driver of the devive.
>   */
>  int
> -rte_eal_pci_probe_one(const struct rte_pci_addr *addr)
> +rte_eal_pci_attach_driver(const struct rte_pci_addr *addr)
>  {
>  	struct rte_pci_device *dev = NULL;
>  	int ret = 0;
> @@ -382,10 +332,9 @@ rte_eal_pci_probe_one(const struct rte_pci_addr *addr)
>   * Detach device specified by its pci address.
>   */
>  int
> -rte_eal_pci_detach(const struct rte_pci_addr *addr)
> +rte_eal_pci_detach_driver(const struct rte_pci_addr *addr)
>  {
>  	struct rte_pci_device *dev = NULL;
> -	int ret = 0;
>
>  	if (addr == NULL)
>  		return -1;
> @@ -394,17 +343,17 @@ rte_eal_pci_detach(const struct rte_pci_addr *addr)
>  		if (rte_eal_compare_pci_addr(&dev->addr, addr))
>  			continue;
>
> -		ret = pci_detach_all_drivers(dev);
> -		if (ret < 0)
> -			goto err_return;
> +		if (dev->driver == NULL) {
> +			/* The device at that address does not have a driver loaded */
> +			return 0;
> +		}
> +
> +		if (rte_eal_pci_remove_driver(dev) < 0)
> +			return -1;
>
> -		TAILQ_REMOVE(&pci_device_list, dev, next);
> -		free(dev);
>  		return 0;
>  	}
> -	return -1;
>
> -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);
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index 2154a54..e304853 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -459,11 +459,14 @@ void *pci_map_resource(void *requested_addr, int fd, off_t offset,
>  void pci_unmap_resource(void *requested_addr, size_t size);
>
>  /**
> - * Probe the single PCI device.
> + * Attempt to load the registered driver for the PCI device at addr.
>   *
> - * 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.
> + * Find the pci device specified by addr, then call the probe() function
> + * for each registered driver that has a matching entry in its id_table until
> + * the correct driver is found.
> + *
> + * If the PCI address is known, this is considerably more efficient than
> + * calling rte_eal_pci_probe.
>   *
>   * @param addr
>   *	The PCI Bus-Device-Function address to probe.
> @@ -471,14 +474,13 @@ void pci_unmap_resource(void *requested_addr, size_t size);
>   *   - 0 on success.
>   *   - Negative on error.
>   */
> -int rte_eal_pci_probe_one(const struct rte_pci_addr *addr);
> +int rte_eal_pci_attach_driver(const struct rte_pci_addr *addr);
>
>  /**
> - * Close the single PCI device.
> + * Unload the driver for the PCI device at addr.
>   *
> - * Scan the content of the PCI bus, and find the pci device specified by pci
> - * address, then call the remove() function for registered driver that has a
> - * matching entry in its id_table for discovered device.
> + * Find the pci device specified by addr, then call the remove() function
> + * for the currently loaded driver.
>   *
>   * @param addr
>   *	The PCI Bus-Device-Function address to close.
> @@ -486,7 +488,7 @@ int rte_eal_pci_probe_one(const struct rte_pci_addr *addr);
>   *   - 0 on success.
>   *   - Negative on error.
>   */
> -int rte_eal_pci_detach(const struct rte_pci_addr *addr);
> +int rte_eal_pci_detach_driver(const struct rte_pci_addr *addr);
>
>  /**
>   * Dump the content of the PCI bus.
>