Re: [PATCH v6 3/8] pci: split match and probe function

[Shreyansh Jain <shreyansh.jain@nxp.com>]

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

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