From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v6 3/8] pci: split match and probe function Date: Tue, 17 Jan 2017 09:58:24 +0000 Message-ID: References: <1484581107-2025-1-git-send-email-shreyansh.jain@nxp.com> <1484581107-2025-4-git-send-email-shreyansh.jain@nxp.com> <7d803d0d-7ffb-2be8-1ad9-d9003ac0a796@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, thomas.monjalon@6wind.com To: Shreyansh Jain , david.marchand@6wind.com Return-path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id BF114108F for ; Tue, 17 Jan 2017 10:58:28 +0100 (CET) In-Reply-To: <7d803d0d-7ffb-2be8-1ad9-d9003ac0a796@nxp.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 1/17/2017 4:54 AM, Shreyansh Jain wrote: > 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? I would prefer existing behavior, INFO level for successfully probed device and driver, but no strong opinion. > >> >>> >>> - 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. OK. > >> >>> >>> } DPDK_16.11; >>> >> >> > > - > Shreyansh >