From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource Date: Thu, 05 Mar 2015 20:51:52 -0700 Message-ID: <1425613912.5200.344.camel@redhat.com> References: <20150305210529.6393.29546.stgit@gimli.home> <54F90790.7060204@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54F90790.7060204@linux.intel.com> Sender: linux-pci-owner@vger.kernel.org To: Jiang Liu , Bjorn Helgaas Cc: x86@kernel.org, rjw@rjwysocki.net, mingo@redhat.com, bp@alien8.de, lv.zheng@intel.com, hpa@zytor.com, tglx@linutronix.de, yinghai@kernel.org, lenb@kernel.org, linux-pci@vger.kernel.org, tony.luck@intel.com, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-acpi@vger.kernel.org On Fri, 2015-03-06 at 09:49 +0800, Jiang Liu wrote: > On 2015/3/6 5:06, Alex Williamson wrote: > > The IRQ resource for a device is established when pci_enabled_device() > > is called on a fully disabled device (ie. enable_cnt == 0). With > > commit b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ > > resources") this same IRQ resource is released when the driver is > > unbound from the device, regardless of the enable_cnt. This presents > > the situation that an ill-behaved driver can now make a device > > unusable to subsequent drivers by an imbalance in their use of > > pci_enable/disable_device(). It's one thing to break your own device > > if you're one of these ill-behaved drivers, but it's a serious > > regression for secondary drivers like vfio-pci, which are innocent > > of the transgressions of the previous driver. > > > > Resolve by pushing the device to a fully disabled state before > > releasing the IRQ resource. > > > > Fixes: b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ resources") > > Signed-off-by: Alex Williamson > > Cc: Jiang Liu > > --- > > arch/x86/pci/common.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > > index 3d2612b..4810194 100644 > > --- a/arch/x86/pci/common.c > > +++ b/arch/x86/pci/common.c > > @@ -527,8 +527,19 @@ static int pci_irq_notifier(struct notifier_block *nb, unsigned long action, > > if (action != BUS_NOTIFY_UNBOUND_DRIVER) > > return NOTIFY_DONE; > > > > - if (pcibios_disable_irq) > > + if (pcibios_disable_irq) { > > + /* > > + * Broken drivers may allow a device to be .remove()'d while > > + * still enabled. pci_enable_device() will only re-establish > > + * dev->irq if the devices is fully disabled. So if we want > > + * to release the IRQ, we need to make sure the next driver > > + * can re-establish it using pci_enable_device(). > > + */ > > + while (pci_is_enabled(dev)) > > + pci_disable_device(dev); > > + > > pcibios_disable_irq(dev); > > + } > Hi Alex, > Thanks for debugging and fixing it. > Will it be feasible to give a debug message to remind those > driver authors to correctly disable PCI when unbinding? I can certainly add a warning to the loop, it loses a bit of its teeth here though since we can't specify which driver to blame at this point. Maybe that warning and perhaps this enabling roll-back should happen in drivers/pci/pci-driver.c:pci_device_remove(). Bjorn, would you prefer it be done generically there? Thanks, Alex