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 <prarit@xxxxxxxxxx>
> 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 <prarit@xxxxxxxxxx>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> CC: Alex Williamson <alex.williamson@xxxxxxxxxx>
> CC: David Arcari <darcari@xxxxxxxxxx>
> CC: Myron Stowe <mstowe@xxxxxxxxxx>
> CC: Lukas Wunner <lukas@xxxxxxxxx>
> CC: Keith Busch <keith.busch@xxxxxxxxx>
> CC: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>
> 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 <bhelgaas@xxxxxxxxxx>
> 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 <bhelgaas@xxxxxxxxxx>
>
> 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; }
>
