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

[David Vrabel <david.vrabel@citrix.com>]

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?

> --- 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;
}

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

It is also possible in the above loop if slot reset is supported to
ignore sibling devices that are on different slots.

> +	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.

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

David