Re: [dpdk-dev] [PATCH] net/failsafe: do not probe device if plugged out

[Thomas Monjalon <thomas@monjalon.net>]

13/07/2017 10:14, Gaëtan Rivet:
> On Thu, Jul 13, 2017 at 08:52:33AM +0200, Thomas Monjalon wrote:
> > 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.
> 
> The fix above will not solve this issue.
> If you are waiting for a proper, general fix, I don't think it will
> arrive anytime soon.
> 
> I have the rte_eal_devargs_clone and a working version that I can send
> in-reply-to if you want. But that's an API extension while in RC2 so...

No worries, it can be fixed in 17.11.