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

[Thomas Monjalon]

2016-03-01 10:08, Xie, Huawei:
> 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.

Yes it is.
Currently we have 3 cases:
- probe auto (best effort)
- whitelist (implicitly blacklist other devices)
- blacklist (implicitly whitelist other devices)

If you want to mix blacklist and best effort (auto probing), we must
set these requirements as per device flags instead of the current lists.

> Even that, the upper layer should explicitly put the non-blacklisted
> device into whitelist, i mean here we should only deal with whitelist.

It is not the way it is done currently. But some per-device flags could
be introduced later to make it explicit and deal only with the whitelist
flag (let's call it PROBE_FORCE).