From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v6 3/8] pci: split match and probe function Date: Tue, 17 Jan 2017 10:24:05 +0530 Message-ID: <7d803d0d-7ffb-2be8-1ad9-d9003ac0a796@nxp.com> References: <1484581107-2025-1-git-send-email-shreyansh.jain@nxp.com> <1484581107-2025-4-git-send-email-shreyansh.jain@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Ferruh Yigit , Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0063.outbound.protection.outlook.com [104.47.41.63]) by dpdk.org (Postfix) with ESMTP id 0DC482BF2 for ; Tue, 17 Jan 2017 05:50:42 +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" Hello Ferruh, On Tuesday 17 January 2017 01:23 AM, Ferruh Yigit wrote: > On 1/16/2017 3:38 PM, Shreyansh Jain wrote: >> 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 > > <...> > >> /* >> - * 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; >> + 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); > > This cause bunch of log printed during app startup, what about printing > this log when probed device found? Only thing I did was move around this log message without adding anything new. Maybe earlier it was in some conditional (match) and now it isn't. I will check again and move to match-only case. > >> >> - struct rte_pci_addr *loc = &dev->addr; >> + /* 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; >> + } >> + >> + /* 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); > > Same for this one, this line cause printing all registered drivers for > each device during app initialization, only matched one can be logged. Same. Will post v7 shortly with only match case printing. What about DEBUG for all cases? > >> >> - 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; >> +} > > <...> > >> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> index b553b13..5ed2589 100644 >> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map >> @@ -186,5 +186,6 @@ DPDK_17.02 { >> rte_bus_dump; >> rte_bus_register; >> rte_bus_unregister; >> + rte_pci_match; > > I think this is internal API, should library expose this API? Idea is that pci_match be useable outside of PCI for any other PCI-like bus (BDF compliant). For example, one of NXP's devices are very close to PCI (but not exactly PCI) and they too rely on BDF for addressing/naming. > >> >> } DPDK_16.11; >> > > - Shreyansh