From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v11 07/13] pci: split match and probe Date: Wed, 15 Feb 2017 17:00:19 +0530 Message-ID: <4f1f05ef-7cea-9306-c37b-554a66cd15ac@nxp.com> References: <1484748329-5418-1-git-send-email-shreyansh.jain@nxp.com> <1484801117-779-1-git-send-email-thomas.monjalon@6wind.com> <1484801117-779-8-git-send-email-thomas.monjalon@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: dev To: Jan Blunck , Thomas Monjalon Return-path: Received: from NAM01-SN1-obe.outbound.protection.outlook.com (mail-sn1nam01on0069.outbound.protection.outlook.com [104.47.32.69]) by dpdk.org (Postfix) with ESMTP id 4A1F59E7 for ; Wed, 15 Feb 2017 12:25:35 +0100 (CET) In-Reply-To: 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 Wednesday 15 February 2017 04:15 PM, Jan Blunck wrote: > On Thu, Jan 19, 2017 at 5:45 AM, Thomas Monjalon > wrote: >> From: Shreyansh Jain >> >> Matching of PCI device address and driver ID table is being done at two >> discreet locations duplicating the code. (rte_eal_pci_probe_one_driver >> and rte_eal_pci_detach_dev). >> >> Splitting the matching function into a public fn rte_pci_match. >> >> Signed-off-by: Shreyansh Jain >> Reviewed-by: Ferruh Yigit >> --- >> lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 + >> lib/librte_eal/common/eal_common_pci.c | 189 +++++++++++++----------- >> lib/librte_eal/common/include/rte_pci.h | 15 ++ >> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 + >> 4 files changed, 121 insertions(+), 85 deletions(-) >> >> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map >> index a3d9fac..afa1643 100644 >> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map >> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map >> @@ -184,5 +184,6 @@ DPDK_17.02 { >> rte_bus_register; >> rte_bus_scan; >> rte_bus_unregister; >> + rte_pci_match; >> >> } DPDK_16.11; >> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c >> index 72547bd..4f155c6 100644 >> --- a/lib/librte_eal/common/eal_common_pci.c >> +++ b/lib/librte_eal/common/eal_common_pci.c >> @@ -152,129 +152,148 @@ pci_unmap_resource(void *requested_addr, size_t size) >> requested_addr); >> } >> >> -/* >> - * If vendor/device ID match, call the probe() function of the >> - * driver. >> - */ >> -static int >> -rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *dev) >> +int >> +rte_pci_match(const struct rte_pci_driver *pci_drv, >> + const struct rte_pci_device *pci_dev) >> { >> - int ret; >> + int match = 1; >> const struct rte_pci_id *id_table; >> >> - for (id_table = dr->id_table; id_table->vendor_id != 0; id_table++) { >> + if (!pci_drv || !pci_dev || !pci_drv->id_table) { >> + RTE_LOG(DEBUG, EAL, "Invalid PCI Driver object\n"); >> + return -1; >> + } >> >> + for (id_table = pci_drv->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 && >> + if (id_table->vendor_id != pci_dev->id.vendor_id && >> id_table->vendor_id != PCI_ANY_ID) >> continue; >> - if (id_table->device_id != dev->id.device_id && >> + if (id_table->device_id != pci_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) >> + if (id_table->subsystem_vendor_id != >> + pci_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) >> + if (id_table->subsystem_device_id != >> + pci_dev->id.subsystem_device_id && >> + id_table->subsystem_device_id != PCI_ANY_ID) >> continue; >> - if (id_table->class_id != dev->id.class_id && >> + if (id_table->class_id != pci_dev->id.class_id && >> id_table->class_id != RTE_CLASS_ANY_ID) >> continue; >> >> - struct rte_pci_addr *loc = &dev->addr; >> - >> - RTE_LOG(INFO, EAL, "PCI device "PCI_PRI_FMT" on NUMA socket %i\n", >> - loc->domain, loc->bus, loc->devid, loc->function, >> - dev->device.numa_node); >> - >> - /* no initialization when blacklisted, return without error */ >> - if (dev->device.devargs != NULL && >> - dev->device.devargs->type == >> - RTE_DEVTYPE_BLACKLISTED_PCI) { >> - RTE_LOG(INFO, EAL, " Device is blacklisted, not initializing\n"); >> - return 1; >> - } >> - >> - RTE_LOG(INFO, EAL, " probe driver: %x:%x %s\n", dev->id.vendor_id, >> - dev->id.device_id, dr->driver.name); >> - >> - if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) { >> - /* map resources for devices that use igb_uio */ >> - ret = rte_eal_pci_map_device(dev); >> - if (ret != 0) >> - return ret; >> - } >> - >> - /* reference driver structure */ >> - dev->driver = dr; >> - >> - /* call the driver probe() function */ >> - ret = dr->probe(dr, dev); >> - if (ret) { >> - dev->driver = NULL; >> - if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) >> - rte_eal_pci_unmap_device(dev); >> - } >> - >> - return ret; >> + match = 0; >> + break; >> } >> - /* return positive value if driver doesn't support this device */ >> - return 1; >> + >> + return match; >> } >> >> /* >> - * If vendor/device ID match, call the remove() function of the >> + * If vendor/device ID match, call the probe() function of the >> * driver. >> */ >> static int >> -rte_eal_pci_detach_dev(struct rte_pci_driver *dr, >> - struct rte_pci_device *dev) >> +rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, >> + struct rte_pci_device *dev) >> { >> - const struct rte_pci_id *id_table; >> + int ret; >> + struct rte_pci_addr *loc; >> >> if ((dr == NULL) || (dev == NULL)) >> return -EINVAL; >> >> - for (id_table = dr->id_table; id_table->vendor_id != 0; id_table++) { >> + loc = &dev->addr; >> >> - /* 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; >> + /* The device is not blacklisted; Check if driver supports it */ >> + ret = rte_pci_match(dr, dev); >> + if (ret) { >> + /* Match of device and driver failed */ >> + RTE_LOG(DEBUG, EAL, "Driver (%s) doesn't match the device\n", >> + dr->driver.name); >> + return 1; >> + } >> >> - struct rte_pci_addr *loc = &dev->addr; >> + RTE_LOG(INFO, EAL, "PCI device "PCI_PRI_FMT" on NUMA socket %i\n", >> + loc->domain, loc->bus, loc->devid, loc->function, >> + dev->device.numa_node); >> + >> + /* no initialization when blacklisted, return without error */ >> + if (dev->device.devargs != NULL && >> + dev->device.devargs->type == >> + RTE_DEVTYPE_BLACKLISTED_PCI) { >> + RTE_LOG(INFO, EAL, " Device is blacklisted, not" >> + " initializing\n"); >> + return 1; >> + } >> >> - 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(INFO, EAL, " probe 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, dr->driver.name); >> + if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) { >> + /* map resources for devices that use igb_uio */ >> + ret = rte_eal_pci_map_device(dev); >> + if (ret != 0) >> + return ret; >> + } >> >> - if (dr->remove && (dr->remove(dev) < 0)) >> - return -1; /* negative value is an error */ >> + /* reference driver structure */ >> + dev->driver = dr; >> >> - /* clear driver structure */ >> + /* call the driver probe() function */ >> + ret = dr->probe(dr, dev); >> + if (ret) { >> 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); >> + } >> >> - return 0; >> + return ret; >> +} >> + >> +/* >> + * 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) >> +{ >> + int ret; >> + struct rte_pci_addr *loc; >> + >> + if ((dr == NULL) || (dev == NULL)) >> + return -EINVAL; >> + >> + ret = rte_pci_match(dr, dev); >> + if (ret) { >> + /* Device and driver don't match */ >> + return 1; >> } >> >> - /* return positive value if driver doesn't support this device */ >> - return 1; >> + 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->device.numa_node); >> + >> + RTE_LOG(DEBUG, EAL, " remove driver: %x:%x %s\n", dev->id.vendor_id, >> + dev->id.device_id, dr->driver.name); >> + >> + if (dr->remove && (dr->remove(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 */ >> + rte_eal_pci_unmap_device(dev); >> + >> + return 0; >> } >> >> /* >> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h >> index 8557e47..adc20b9 100644 >> --- a/lib/librte_eal/common/include/rte_pci.h >> +++ b/lib/librte_eal/common/include/rte_pci.h >> @@ -371,6 +371,21 @@ rte_eal_compare_pci_addr(const struct rte_pci_addr *addr, >> int rte_eal_pci_scan(void); >> >> /** >> + * Match the PCI Driver and Device using the ID Table >> + * >> + * @param pci_drv >> + * PCI driver from which ID table would be extracted >> + * @param pci_dev >> + * PCI device to match against the driver >> + * @return >> + * 0 for successful match >> + * !0 for unsuccessful match >> + */ >> +int >> +rte_pci_match(const struct rte_pci_driver *pci_drv, >> + const struct rte_pci_device *pci_dev); > > I don't think this symbol needs to be exported and visible outside of EAL. Agree. 2 reasons why this exists: 1. Initial RFC has match as a callback. That doesn't exist now. 2. Earlier the idea was that other buses/device-types might use this function (buses which are PCI like), but that is not a good assumption. > >> + >> +/** >> * Probe the PCI bus for registered drivers. >> * >> * Scan the content of the PCI bus, and call the probe() function for >> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> index 69e0e7b..b557e7c 100644 >> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> @@ -188,5 +188,6 @@ DPDK_17.02 { >> rte_bus_register; >> rte_bus_scan; >> rte_bus_unregister; >> + rte_pci_match; >> >> } DPDK_16.11; >> -- >> 2.7.0 >> >