From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v2 7/7] pci: Clarify interfaces for dynamic attach/detach of drivers Date: Sat, 3 Dec 2016 12:25:12 +0530 Message-ID: <4fb8991d-d026-5920-c951-466a9c14164a@nxp.com> References: <1479929804-19614-1-git-send-email-benjamin.walker@intel.com> <1479931644-78960-1-git-send-email-benjamin.walker@intel.com> <1479931644-78960-7-git-send-email-benjamin.walker@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: To: Ben Walker Return-path: Received: from NAM01-BN3-obe.outbound.protection.outlook.com (mail-bn3nam01on0072.outbound.protection.outlook.com [104.47.33.72]) by dpdk.org (Postfix) with ESMTP id A74C637B8 for ; Sat, 3 Dec 2016 07:52:28 +0100 (CET) In-Reply-To: <1479931644-78960-7-git-send-email-benjamin.walker@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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. >