From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [RFC PATCH 5/5] xen/pciback: PCI reset slot or bus Date: Mon, 16 Dec 2013 11:34:32 +0000 Message-ID: <52AEE548.8010800__18726.4155866162$1387193784$gmane$org@citrix.com> References: <1386950978-8628-1-git-send-email-konrad.wilk@oracle.com> <1386950978-8628-6-git-send-email-konrad.wilk@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VsWRY-00006E-R9 for xen-devel@lists.xenproject.org; Mon, 16 Dec 2013 11:34:37 +0000 In-Reply-To: <1386950978-8628-6-git-send-email-konrad.wilk@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: xen-devel@lists.xenproject.org, gordan@bobich.net, linux-kernel@vger.kernel.org List-Id: xen-devel@lists.xenproject.org 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