From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Message-ID: <58C29C80.8040904@redhat.com> Date: Fri, 10 Mar 2017 07:30:56 -0500 From: Prarit Bhargava MIME-Version: 1.0 To: Bjorn Helgaas CC: linux-pci@vger.kernel.org, alex.williamson@redhat.com, darcari@redhat.com, mstowe@redhat.com, bhelgaas@google.com, lukas@wunner.de, keith.busch@intel.com, mika.westerberg@linux.intel.com Subject: Re: [PATCH] pci: Only disable MSI/X and enable INTx if shutdown function has been called References: <1485457667-791-1-git-send-email-prarit@redhat.com> <20170309215718.GC19517@bhelgaas-glaptop.roam.corp.google.com> In-Reply-To: <20170309215718.GC19517@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset=windows-1252 List-ID: On 03/09/2017 04:57 PM, Bjorn Helgaas wrote: > Hi Prarit, > > My abject apologies for taking so long to deal with this. np. It's only two lines but it is also complex code and I know you're busy. >> drivers/pci/pci-driver.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c >> index 1ccce1cd6aca..87c35db5a564 100644 >> --- a/drivers/pci/pci-driver.c >> +++ b/drivers/pci/pci-driver.c >> @@ -461,10 +461,11 @@ static void pci_device_shutdown(struct device *dev) >> >> pm_runtime_resume(dev); >> >> - if (drv && drv->shutdown) >> + if (drv && drv->shutdown) { >> drv->shutdown(pci_dev); >> - pci_msi_shutdown(pci_dev); >> - pci_msix_shutdown(pci_dev); >> + pci_msi_shutdown(pci_dev); >> + pci_msix_shutdown(pci_dev); >> + } > > I love this patch because it cleans up pci_device_shutdown(). You > mentioned that you've also tested a patch that just removes the calls > to pci_msi_shutdown() and pci_msix_shutdown() completely. I like that > even more. > > As Keith pointed out, the driver remains bound to the device even > after we call pci_device_shutdown(), and the PCI core should not > change the configuration of the device behind the back of the driver. > > I think these commits are important pieces: > > 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel > doesn't support MSI") > e80e7edc55ba ("PCI/MSI: Initialize MSI capability for all > architectures") > > because they ensure that a kexeced kernel can deal with MSIs being > left enabled. > > What do you think of the following two patches? Thanks for all the > details in your changelog -- I think they finally helped me gel all > the pieces in my mind, and it all seems obvious now. I tried to > distill it down to just the critical pieces. > I'm good with these two patches. P. > Bjorn > > > commit fda78d7a0ead144f4b2cdb582dcba47911f4952c > Author: Prarit Bhargava > Date: Thu Jan 26 14:07:47 2017 -0500 > > PCI/MSI: Stop disabling MSI/MSI-X in pci_device_shutdown() > > The pci_bus_type .shutdown method, pci_device_shutdown(), is called from > device_shutdown() in the kernel restart and shutdown paths. > > Previously, pci_device_shutdown() called pci_msi_shutdown() and > pci_msix_shutdown(). This disables MSI and MSI-X, which causes the device > to fall back to raising interrupts via INTx. But the driver is still bound > to the device, it doesn't know about this change, and it likely doesn't > have an INTx handler, so these INTx interrupts cause "nobody cared" > warnings like this: > > irq 16: nobody cared (try booting with the "irqpoll" option) > CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.8.2-1.el7_UNSUPPORTED.x86_64 #1 > Hardware name: Hewlett-Packard HP Z820 Workstation/158B, BIOS J63 v03.90 06/ > ... > > The MSI disabling code was added by d52877c7b1af ("pci/irq: let > pci_device_shutdown to call pci_msi_shutdown v2") because a driver left MSI > enabled and kdump failed because the kexeced kernel wasn't prepared to > receive the MSI interrupts. > > Subsequent commits 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even > if kernel doesn't support MSI") and e80e7edc55ba ("PCI/MSI: Initialize MSI > capability for all architectures") changed the kexeced kernel to disable > all MSIs itself so it no longer depends on the crashed kernel to clean up > after itself. > > Stop disabling MSI/MSI-X in pci_device_shutdown(). This resolves the > "nobody cared" unhandled IRQ issue above. It also allows PCI serial > devices, which may rely on the MSI interrupts, to continue outputting > messages during reboot/shutdown. > > [bhelgaas: changelog, drop pci_msi_shutdown() and pci_msix_shutdown() calls > altogether] > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=187351 > Signed-off-by: Prarit Bhargava > Signed-off-by: Bjorn Helgaas > CC: Alex Williamson > CC: David Arcari > CC: Myron Stowe > CC: Lukas Wunner > CC: Keith Busch > CC: Mika Westerberg > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index afa72717a979..8ec136164e93 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -461,8 +461,6 @@ static void pci_device_shutdown(struct device *dev) > > if (drv && drv->shutdown) > drv->shutdown(pci_dev); > - pci_msi_shutdown(pci_dev); > - pci_msix_shutdown(pci_dev); > > /* > * If this is a kexec reboot, turn off Bus Master bit on the > > commit 688769f643bfce894f14dc7141bfc6c010f52750 > Author: Bjorn Helgaas > Date: Thu Mar 9 15:45:14 2017 -0600 > > PCI/MSI: Make pci_msi_shutdown() and pci_msix_shutdown() static > > pci_msi_shutdown() and pci_msix_shutdown() are used only in > drivers/pci/msi.c, so make them static. > > Signed-off-by: Bjorn Helgaas > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index d571bc330686..4d062c3bf5f0 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -882,7 +882,7 @@ int pci_msi_vec_count(struct pci_dev *dev) > } > EXPORT_SYMBOL(pci_msi_vec_count); > > -void pci_msi_shutdown(struct pci_dev *dev) > +static void pci_msi_shutdown(struct pci_dev *dev) > { > struct msi_desc *desc; > u32 mask; > @@ -994,7 +994,7 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec) > } > EXPORT_SYMBOL(pci_enable_msix); > > -void pci_msix_shutdown(struct pci_dev *dev) > +static void pci_msix_shutdown(struct pci_dev *dev) > { > struct msi_desc *entry; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index eb3da1a04e6c..10917c122974 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1297,11 +1297,9 @@ struct msix_entry { > > #ifdef CONFIG_PCI_MSI > int pci_msi_vec_count(struct pci_dev *dev); > -void pci_msi_shutdown(struct pci_dev *dev); > void pci_disable_msi(struct pci_dev *dev); > int pci_msix_vec_count(struct pci_dev *dev); > int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec); > -void pci_msix_shutdown(struct pci_dev *dev); > void pci_disable_msix(struct pci_dev *dev); > void pci_restore_msi_state(struct pci_dev *dev); > int pci_msi_enabled(void); > @@ -1327,13 +1325,11 @@ int pci_irq_get_node(struct pci_dev *pdev, int vec); > > #else > static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; } > -static inline void pci_msi_shutdown(struct pci_dev *dev) { } > static inline void pci_disable_msi(struct pci_dev *dev) { } > static inline int pci_msix_vec_count(struct pci_dev *dev) { return -ENOSYS; } > static inline int pci_enable_msix(struct pci_dev *dev, > struct msix_entry *entries, int nvec) > { return -ENOSYS; } > -static inline void pci_msix_shutdown(struct pci_dev *dev) { } > static inline void pci_disable_msix(struct pci_dev *dev) { } > static inline void pci_restore_msi_state(struct pci_dev *dev) { } > static inline int pci_msi_enabled(void) { return 0; } >