[dpdk-dev] [PATCH v4 4/4] virtio: return 1 to tell the upper layer we don't take over this device

[Xie, Huawei]

On 3/1/2016 6:09 PM, Xie, Huawei wrote:
> On 3/1/2016 5:57 PM, Thomas Monjalon wrote:
>> 2016-03-01 08:39, Xie, Huawei:
>>> On 3/1/2016 4:24 PM, Thomas Monjalon wrote:
>>>> 2016-03-01 07:53, Xie, Huawei:
>>>>> On 3/1/2016 3:18 PM, Thomas Monjalon wrote:
>>>>>> 2016-02-26 09:53, Huawei Xie:
>>>>>>> @@ -1037,8 +1039,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>>>>>  
>>>>>>>  	pci_dev = eth_dev->pci_dev;
>>>>>>>  
>>>>>>> -	if (vtpci_init(pci_dev, hw) < 0)
>>>>>>> -		return -1;
>>>>>>> +	ret = vtpci_init(pci_dev, hw);
>>>>>>> +	if (ret) {
>>>>>>> +		rte_free(eth_dev->data->mac_addrs);
>>>>>> The freeing seems not related to this patch.
>>>>> I can send a separate patch, ok within this patchset?
>>>> Yes
>>>>
>>>>>> [...]
>>>>>>>  	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
>>>>>>> -	if (legacy_virtio_resource_init(dev, hw) < 0)
>>>>>>> +	if (legacy_virtio_resource_init(dev, hw) < 0) {
>>>>>>> +		if (dev->kdrv == RTE_KDRV_UNKNOWN) {
>>>>>>> +			PMD_INIT_LOG(INFO,
>>>>>>> +				"skip kernel managed virtio device.");
>>>>>>> +			return 1;
>>>>>>> +		}
>>>>>>>  		return -1;
>>>>>>> +	}
>>>>>> You cannot skip a device if it was whitelisted.
>>>>>> I think you should check RTE_DEVTYPE_WHITELISTED_PCI and throw an error
>>>>>> in this case.
>>>>> I feel there is a subtle difference on the understanding of -w args. To
>>>>> me, without it, probe all devices; with it, only probe whiltelisted API.
>>>>> That is all.
>>>> I don't know if it is clearly documented indeed.
>>>>
>>>>> Do you mean that -w implies that devices whitelisted must be probed
>>>>> successfully otherwise we throw an error? If i get it right, then what
>>>>> about the devices whitelisted but without PMD driver?
>>>> Yes we should probably consider the whitelist as a "forced" init.
>>>> Later, we could introduce some device flags for probing/discovery:
>>>> PROBE_AUTO, PROBE_FORCE, PROBE_IGNORE. It would make white/black list
>>>> more precise.
>>>>
>>>>> I will fix, :).
>>>>> if (dev->kdrv == RTE_KDRV_UNKNOWN && dev->devargs->type !=
>>>>> RTE_DEVTYPE_WHITELISTED_PCI) {
>>>>>         ....
>>>>>         return 1;
>>>>> }
>>>> You should also consider the blacklist case: if there is a blacklist,
>>>> the not blacklisted devices must be initialised or throw an error.
>>>>
>>> Don't we already skip probing the blacklisted device in
>>> rte_eal_pci_probe_one_driver?
>> Yes, I'm talking about the devices which are not blacklisted.
>> Having some blacklisted devices imply that others are implicitly whitelisted.
> For blacklist, it only means the blacklisted device should be excluded
> from being probed. It doesn't mean all other devices should be probed
> either successfully or otherwise throw an error which cause DPDK exit.
> Even that, the upper layer should explicitly put the non-blacklisted
> device into whitelist, i mean here we should only deal with whitelist.

Thomas, comments?
Btw, i have reworked the patch, but git am always complains patch format
detection failed. Investigating. Will send it later.

>