From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP; 17 Jan 2017 01:58:26 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,243,1477983600"; d="scan'208";a="1113818849" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.38]) ([10.237.220.38]) by fmsmga002.fm.intel.com with ESMTP; 17 Jan 2017 01:58:25 -0800 To: Shreyansh Jain , david.marchand@6wind.com 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> Cc: dev@dpdk.org, thomas.monjalon@6wind.com From: Ferruh Yigit Message-ID: Date: Tue, 17 Jan 2017 09:58:24 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <7d803d0d-7ffb-2be8-1ad9-d9003ac0a796@nxp.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 3/8] pci: split match and probe function X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2017 09:58:29 -0000 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 >