[dpdk-dev] [PATCH v6 3/8] pci: split match and probe function

[Ferruh Yigit]

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 at 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
>