From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <ferruh.yigit@intel.com>
Received: from mga07.intel.com (mga07.intel.com [134.134.136.100])
by dpdk.org (Postfix) with ESMTP id BF114108F
for <dev@dpdk.org>; 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 <shreyansh.jain@nxp.com>, 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>
<d98a7c1d-d73b-4e5f-d0c1-bd56cffbfe57@intel.com>
<7d803d0d-7ffb-2be8-1ad9-d9003ac0a796@nxp.com>
Cc: dev@dpdk.org, thomas.monjalon@6wind.com
From: Ferruh Yigit <ferruh.yigit@intel.com>
Message-ID: <f78be8b4-543f-d888-ca24-4a15b14373d4@intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <shreyansh.jain@nxp.com>
>>
>> <...>
>>
>>> /*
>>> - * 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
>