lkml.org 
[lkml]   [2013]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [Xen-devel] [RFC PATCH 5/5] xen/pciback: PCI reset slot or bus
    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


    \
     
     \ /
      Last update: 2013-12-16 16:21    [W:4.448 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site