12/07/2017 22:39, Gaëtan Rivet:
> Hi Thomas,
>
> Nice idea. A few remarks below:
>
> On Wed, Jul 12, 2017 at 08:28:12PM +0200, Thomas Monjalon wrote:
> > FOREACH_SUBDEV(sdev, i, dev) {
> > if (sdev->state != DEV_PARSED)
> > continue;
> > da = &sdev->devargs;
> > +
>
> Superfluous line.
I don't think so :) It is isolating the "skip" block with its comment.
> > + /* skip plugged out devices */
> > + if (! first_init
> > + && sdev->cmdline == NULL
> > + && strcmp(da->bus->name, "vdev") != 0) {
>
> Use first_init == false instead of negation.
> && should be at the end of the line instead of the start of the next
> one.
Yes
> Indentation is wrong.
No, the coding style is to put 2 tabs for continuation lines.
> > + da->bus->scan();
> > + if (bus->find_device(NULL, cmp_dev_name, da->name) ==
> > NULL)
> > + continue; /* device not found */
>
> da->bus->find_device instead of bus->find_device.
> This function cannot find the device back currently on the PCI bus,
> blocking the plugging of VF.
>
> The PCI bus will scan the VF while no rte_devargs exists to
> describe it within the global list. If the device exists, it will
> detect it, allocate it and then set its name.
> Without any rte_devargs, the name of a PCI device falls back to its
> canonical name (DomBDF instead of BDF). The name comparison with
> da->name can only succeed if the slave was declared using the DomBDF
> format.
>
> The fix is to do a deep copy of the rte_devargs (the API has been
> sent previously with the rte_devargs rework but I have since removed
> it) and insert it using rte_eal_devargs_insert(). This is essentially
> the solution I used for the rte_eal_hotplug_add() fix[1].
>
> The alternative fix is to propose an API for buses to transform device
> names into their canonical form on demand... And it would certainly only
> be useful for the PCI bus.
>
> The issue is discussed there:
> [1]: http://dpdk.org/ml/archives/dev/2017-July/071155.html
OK, I was not aware of this exact issue.
So I will wait above fix.
