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 15:44:16 +0530 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"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Ferruh Yigit , Return-path: Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-cys01nam02on0072.outbound.protection.outlook.com [104.47.37.72]) by dpdk.org (Postfix) with ESMTP id CAB01108F for ; Tue, 17 Jan 2017 11:10:54 +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 Tuesday 17 January 2017 03:28 PM, Ferruh Yigit wrote: > 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. Reverted to existing behavior. It was a miss from my side. I had moved this log message _before_ matching unlike the original code. Pushed v7 to ML. > >> >>> >>>> >>>> - 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 >> > >