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

[Shreyansh Jain]

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?

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

>
>>
>>  } DPDK_16.11;
>>
>
>

-
Shreyansh