Re: [Xen-devel] [RFC PATCH 5/5] xen/pciback: PCI reset slot or bus

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Dec 16, 2013 at 11:34:32AM +0000, David Vrabel wrote:
> On 13/12/13 16:09, Konrad Rzeszutek Wilk wrote:
> > The life-cycle of a PCI device in Xen pciback is a bit complex.
> > 
> > It starts with the device being binded to us - for which
> > we do a device function reset.
> > 
> > If the device is unbinded from us - we also do a function
> > reset.
> 
> Spelling: bound and unbound.
> 
> > The reset is done when all of the functions of a device
> > are binded to Xen pciback. Or when a device is un-assigned
> > from a guest. We do this by having a 'completion' workqueue
> > on which the users of the PCI device wait. This workqueue
> > is started once the device has been 'binded' or de-assigned
> > from a guest.
> 
> The use of a work item and a completion baffles me.  What problem does
> this solve?

Avoiding of deadlock. Whenever you do any SysFS operations on on PCI
devices it ends up locking pci_dev and then we try to use it .. and
end up dead-locking. I should have clarified that more.

I could add code to pci (Generic) to have an non-locking variant - but
there is soo much of it I think doing it in a work item and completion
would be much simpler.

> 
> > --- a/drivers/xen/xen-pciback/pci_stub.c
> > +++ b/drivers/xen/xen-pciback/pci_stub.c
> [...]
> > +	/* We expect X amount of slots (this would also find out
> > +	 * if we do not have all of the slots assigned to us).
> > +	 */
> > +	list_for_each_entry(pci_dev, &dev->bus->devices, bus_list)
> > +		slots++;
> > +
> > +	spin_lock_irqsave(&pcistub_devices_lock, flags);
> > +	/* Iterate over the existing devices to ascertain whether
> > +	 * all of them are under the bridge and not in use. */
> > +	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
> > +		if (!psdev->dev)
> > +			continue;
> > +
> > +		if (pci_domain_nr(psdev->dev->bus) == pci_domain_nr(dev->bus) &&
> > +		    psdev->dev->bus->number == dev->bus->number &&
> > +		    PCI_SLOT(psdev->dev->devfn) == PCI_SLOT(dev->devfn)) {
> > +			slots--;
> > +			/* Ignore ourselves in case hadn't cleaned up yet */
> > +			if (psdev->pdev && psdev->dev != dev)
> > +				inuse++;
> > +		}
> > +	}
> 
> This check looks broken.  A device added to pciback but still bound to
> another driver will be considered as safe to reset.
> 
> I think you want something like:
> 
> list_for_each_entry(pdev, &dev->bus->devices, bus_list)
> {
>     if (pdev != dev && pdev->driver
>         && pdev->driver != xen_pcibk_pci_driver))
>         return -ENOTTY;
> }

Good catch!
> 
> It is safe to reset unbound devices (even if they're not (or intended)
> to be available to pciback).

OK, we can check for that.
> 
> It is also possible in the above loop if slot reset is supported to
> ignore sibling devices that are on different slots.

Not sure what you mean?
> 
> > +	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
> > +	/* Slots should be zero (all slots under the bridge are
> > +	 * accounted for), and inuse should be zero (not assigned
> > +	 * to anybody). */
> > +	if (!slots && !inuse) {
> > +		int rc = 0, bus = 0;
> > +		list_for_each_entry(pci_dev, &dev->bus->devices, bus_list) {
> > +			dev_dbg(&pci_dev->dev, "resetting the slot device\n");
> > +			if (!pci_probe_reset_slot(pci_dev->slot))
> > +				rc = pci_reset_slot(pci_dev->slot);
> > +			else
> > +				bus = 1;
> > +			if (rc)
> > +				dev_info(&pci_dev->dev, "resetting slot failed with %d\n", rc);
> > +		}
> 
> Why are you resetting every slot on the bus?  You only need to reset the
> slot that the device is on.

Bug.
> 
> Take a look at the vfio-pci driver.  It does this bus/slot reset choice
> in a much more straightforward way.
> 
> David