pci: fix unhandled interrupt on shutdown
Subsystems (4)
Michael S. Tsirkin
maintainerv4 cover pci: fix unhandled interrupt on shutdown
Fam Zheng noticed that pci shutdown disables msi and msix of a device while device is still active. This was intended to fix kexec with fusion devices but had the unintended effect of breaking even regular shutdown when using virtio.
The same problem would affect any driver which doesn't register a level interrupt handler when using msix.
I think the fix is to avoid touching device on shutdown:
we clear bus master anyway, so we won't get any more msi interrupts, and bus reset will clear the msi/msix state eventually anyway.
Patches 1-6 work well for me.
Given they affect all pci devices, and the bug has been there since 2.6 times, I think there's no rush: we can merge them for 4.1.
At the same time, once merged, patches 1-4 will likely make a good stable candidate.
Patches 7-10 compiled only, will need maintainer ack.
Please review, and consider at least 1-6 for 4.1.
Changes from v3:
fix a copy-and-paste error in
pci: drop some duplicate code
other patches are unchanged
drop Cc stable for now
Changes from v2:
move code from probe to device enumeration add patches to unexport pci_msi_off
Michael S. Tsirkin (11):
pci: export functions for msi/msix ctrl
pci: move pci_msi_init_pci_dev to pci.c
pci: drop some duplicate code
pci: don't disable msi/msix at shutdown
pci: make msi/msix shutdown functions static
virtio_pci: drop msi_off on probe
ntb: drop pci_msi_off call on probe
mic: drop pci_msi_off call on probe
pci: drop pci_msi_off calls from quirks
pci: unexport pci_msi_off
drivers/pci/pci.h | 25 +++++++++++++-- include/linux/pci.h | 5 --- drivers/misc/mic/host/mic_intr.c | 2 -- drivers/ntb/ntb_hw.c | 2 -- drivers/pci/msi.c | 62 ++++++++------------------------------ drivers/pci/pci-driver.c | 2 -- drivers/pci/pci.c | 24 +++------------ drivers/pci/probe.c | 16 ++++++++++ drivers/pci/quirks.c | 2 -- drivers/virtio/virtio_pci_common.c | 3 -- 10 files changed, 56 insertions(+), 88 deletions(-)
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 1/10 pci: export functions for msi/msix ctrl
move pci_msi_set_enable and pci_msix_clear_and_set_ctrl out of msi.c, so we can use them will be used which MSI isn't configured in kernel.
--- drivers/pci/pci.h | 21 +++++++++++++++++++++ drivers/pci/msi.c | 45 ++++++++++++--------------------------------- 2 files changed, 33 insertions(+), 33 deletions(-)
drivers/pci/pci.h +21 -0
@@ -146,6 +146,27 @@ static inline void pci_no_msi(void) { }static inline void pci_msi_init_pci_dev (struct pci_dev *dev) { }#endif +static inline void pci_msi_set_enable (struct pci_dev *dev,int enable) +{ +u16 control; + +pci_read_config_word (dev, dev->msi_cap +PCI_MSI_FLAGS , &control); + control &= ~PCI_MSI_FLAGS_ENABLE ; +if (enable) + control |=PCI_MSI_FLAGS_ENABLE ; +pci_write_config_word (dev, dev->msi_cap +PCI_MSI_FLAGS , control); +} + +static inline void pci_msix_clear_and_set_ctrl (struct pci_dev *dev,u16 clear,u16 set) +{ +u16 ctrl; + +pci_read_config_word (dev, dev->msix_cap +PCI_MSIX_FLAGS , &ctrl); + ctrl &= ~clear; + ctrl |= set; +pci_write_config_word (dev, dev->msix_cap +PCI_MSIX_FLAGS , ctrl); +} +void pci_realloc_get_opt (char *);static inline int pci_no_d1d2 (struct pci_dev *dev)
drivers/pci/msi.c +12 -33
120 lines not on this page · raw message
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 2/10 pci: move pci_msi_init_pci_dev to pci.c
In reply to Michael S. Tsirkincommit d5dea7d95c48d7bc951cee4910a7fd9c0cd26fb0
"PCI: msi: Disable msi interrupts when we initialize a pci device"
fixes kexec when the booting kernel does not enable msi interupts.
Unfortunately the relevant functionality is in msi.c so it isn't compiled in when CONFIG_PCI_MSI is off, which means such configurations would still get interrupt storms.
Fix by moving the function to pci.c, and compiling it unconditionally.
--- drivers/pci/pci.h | 2 -- drivers/pci/msi.c | 17 ----------------- drivers/pci/probe.c | 19 +++++++++++++++++++ 3 files changed, 19 insertions(+), 19 deletions(-)
drivers/pci/pci.h +0 -2
@@ -140,10 +140,8 @@ extern unsigned int pci_pm_d3_delay;#ifdef CONFIG_PCI_MSI void pci_no_msi (void ); -void pci_msi_init_pci_dev (struct pci_dev *dev);#else static inline void pci_no_msi (void ) { } -static inline void pci_msi_init_pci_dev (struct pci_dev *dev) { }#endif static inline void pci_msi_set_enable (struct pci_dev *dev,int enable)
drivers/pci/msi.c +0 -17
@@ -1038,23 +1038,6 @@ int pci_msi_enabled(void) }EXPORT_SYMBOL (pci_msi_enabled); -void pci_msi_init_pci_dev (struct pci_dev *dev) -{ -INIT_LIST_HEAD (&dev->msi_list); - -/* Disable the msi hardware to avoid screaming interrupts -* during boot. This is the power on reset default so -* usually this should be a noop. -*/ - dev->msi_cap =pci_find_capability (dev,PCI_CAP_ID_MSI ); -if (dev->msi_cap) -pci_msi_set_enable (dev,0 ); - - dev->msix_cap =pci_find_capability (dev,PCI_CAP_ID_MSIX ); -if (dev->msix_cap) -pci_msix_clear_and_set_ctrl (dev,PCI_MSIX_FLAGS_ENABLE ,0 ); -} -/** * pci_enable_msi_range - configure device's MSI capability structure * @dev: device to configure
drivers/pci/probe.c +19 -0
@@ -1483,6 +1483,25 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)return dev; } +static void pci_msi_init_pci_dev (struct pci_dev *dev) +{ +#ifdef CONFIG_PCI_MSI +INIT_LIST_HEAD (&dev->msi_list); +#endif + +/* Disable the msi hardware to avoid screaming interrupts +* during boot. This is the power on reset default so +* usually this should be a noop. +*/ + dev->msi_cap =pci_find_capability (dev,PCI_CAP_ID_MSI ); +if (dev->msi_cap) +pci_msi_set_enable (dev,0 ); + + dev->msix_cap =pci_find_capability (dev,PCI_CAP_ID_MSIX ); +if (dev->msix_cap) +pci_msix_clear_and_set_ctrl (dev,PCI_MSIX_FLAGS_ENABLE ,0 ); +} +static void pci_init_capabilities (struct pci_dev *dev) {/* MSI/MSI-X list */
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 3/10 pci: drop some duplicate code
In reply to Michael S. Tsirkinpci_msi_init_pci_dev and pci_msi_off share a lot of code. This used to be justified since pci_msi_init_pci_dev wasn't compiled in when CONFIG_PCI_MSI is off. Now that it is, let's reuse code.
--- drivers/pci/pci.c | 23 ++++------------------- drivers/pci/probe.c | 11 ++++------- 2 files changed, 8 insertions(+), 26 deletions(-)
drivers/pci/pci.c +4 -19
@@ -3106,26 +3106,11 @@ EXPORT_SYMBOL_GPL(pci_check_and_unmask_intx);*/ void pci_msi_off (struct pci_dev *dev) { -int pos; -u16 control; +if (dev->msi_cap) +pci_msi_set_enable (dev,0 ); -/* -* This looks like it could go in msi.c, but we need it even when -* CONFIG_PCI_MSI=n. For the same reason, we can't use -* dev->msi_cap or dev->msix_cap here. -*/ - pos =pci_find_capability (dev,PCI_CAP_ID_MSI ); -if (pos) { -pci_read_config_word (dev, pos +PCI_MSI_FLAGS , &control); - control &= ~PCI_MSI_FLAGS_ENABLE ; -pci_write_config_word (dev, pos +PCI_MSI_FLAGS , control); - } - pos =pci_find_capability (dev,PCI_CAP_ID_MSIX ); -if (pos) { -pci_read_config_word (dev, pos +PCI_MSIX_FLAGS , &control); - control &= ~PCI_MSIX_FLAGS_ENABLE ; -pci_write_config_word (dev, pos +PCI_MSIX_FLAGS , control); - } +if (dev->msix_cap) +pci_msix_clear_and_set_ctrl (dev,PCI_MSIX_FLAGS_ENABLE ,0 ); }EXPORT_SYMBOL_GPL (pci_msi_off);
drivers/pci/probe.c +4 -7
@@ -1489,17 +1489,14 @@ static void pci_msi_init_pci_dev(struct pci_dev *dev)INIT_LIST_HEAD (&dev->msi_list);#endif + dev->msi_cap =pci_find_capability (dev,PCI_CAP_ID_MSI ); + dev->msix_cap =pci_find_capability (dev,PCI_CAP_ID_MSIX ); +/* Disable the msi hardware to avoid screaming interrupts * during boot. This is the power on reset default so * usually this should be a noop. */ - dev->msi_cap =pci_find_capability (dev,PCI_CAP_ID_MSI ); -if (dev->msi_cap) -pci_msi_set_enable (dev,0 ); - - dev->msix_cap =pci_find_capability (dev,PCI_CAP_ID_MSIX ); -if (dev->msix_cap) -pci_msix_clear_and_set_ctrl (dev,PCI_MSIX_FLAGS_ENABLE ,0 ); +pci_msi_off (dev); }static void pci_init_capabilities (struct pci_dev *dev)
Signature · 1 line
MST
Re: [PATCH v4 03/10] pci: drop some duplicate code
Quoted 4 lines · Michael S. Tsirkin
pci_msi_init_pci_devandpci_msi_offshare a lot of code. This used to be justified sincepci_msi_init_pci_devwasn't compiled in whenCONFIG_PCI_MSIis off. Now that it is, let's reuse code.
It's not correct, pci_msi_off() would be used in quirk when the msi_cap is not initialized.
For example:
/*
* It's possible for the MSI to get corrupted if shpc and acpi
* are used together on certain PXH-based systems. */
static void quirk_pcie_pxh(struct pci_dev *dev)
{
pci_msi_off(dev);
dev->no_msi = 1;
dev_warn(&dev->dev, "PXH quirk detected; SHPC device MSI disabled\n");
}DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHD_0, quirk_pcie_pxh);
Quoted 69 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- drivers/pci/pci.c | 23 ++++------------------- drivers/pci/probe.c | 11 ++++------- 2 files changed, 8 insertions(+), 26 deletions(-)drivers/pci/pci.c +4
-19@@ -3106,26 +3106,11 @@ EXPORT_SYMBOL_GPL(pci_check_and_unmask_intx);*/ void pci_msi_off (struct pci_dev *dev) { -int pos; -u16 control; +if (dev->msi_cap) +pci_msi_set_enable (dev,0 ); -/* -* This looks like it could go in msi.c, but we need it even when -* CONFIG_PCI_MSI=n. For the same reason, we can't use -* dev->msi_cap or dev->msix_cap here. -*/ - pos =pci_find_capability (dev,PCI_CAP_ID_MSI ); -if (pos) { -pci_read_config_word (dev, pos +PCI_MSI_FLAGS , &control); - control &= ~PCI_MSI_FLAGS_ENABLE ; -pci_write_config_word (dev, pos +PCI_MSI_FLAGS , control); - } - pos =pci_find_capability (dev,PCI_CAP_ID_MSIX ); -if (pos) { -pci_read_config_word (dev, pos +PCI_MSIX_FLAGS , &control); - control &= ~PCI_MSIX_FLAGS_ENABLE ; -pci_write_config_word (dev, pos +PCI_MSIX_FLAGS , control); - } +if (dev->msix_cap) +pci_msix_clear_and_set_ctrl (dev,PCI_MSIX_FLAGS_ENABLE ,0 ); }EXPORT_SYMBOL_GPL (pci_msi_off);drivers/pci/probe.c +4
-7@@ -1489,17 +1489,14 @@ static void pci_msi_init_pci_dev(struct pci_dev *dev)INIT_LIST_HEAD (&dev->msi_list);#endif + dev->msi_cap =pci_find_capability (dev,PCI_CAP_ID_MSI ); + dev->msix_cap =pci_find_capability (dev,PCI_CAP_ID_MSIX ); +/* Disable the msi hardware to avoid screaming interrupts * during boot. This is the power on reset default so * usually this should be a noop. */ - dev->msi_cap =pci_find_capability (dev,PCI_CAP_ID_MSI ); -if (dev->msi_cap) -pci_msi_set_enable (dev,0 ); - - dev->msix_cap =pci_find_capability (dev,PCI_CAP_ID_MSIX ); -if (dev->msix_cap) -pci_msix_clear_and_set_ctrl (dev,PCI_MSIX_FLAGS_ENABLE ,0 ); +pci_msi_off (dev); }static void pci_init_capabilities (struct pci_dev *dev)
Signature · 2 lines
Thanks! Yijing
Michael S. Tsirkin
maintainerPATCH v4 4/10 pci: don't disable msi/msix at shutdown
In reply to Michael S. TsirkinThis partially reverts commit d52877c7b1afb8c37ebe17e2005040b79cb618b0:
"pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2"
It's un-necessary now that we disable msi at start, and it actually turns out to cause problems: some device drivers don't register a level interrupt handler when they detect msi/msix capability, switching off msi while device is going causes device to assert a level interrupt which is never de-asserted, causing a kernel hang.
In particular, this was observed with virtio.
--- drivers/pci/pci-driver.c | 2 -- 1 file changed, 2 deletions(-)
drivers/pci/pci-driver.c +0 -2
@@ -450,8 +450,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);#ifdef CONFIG_KEXEC /*
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 5/10 pci: make msi/msix shutdown functions static
In reply to Michael S. Tsirkinpci_msi_shutdown and pci_msix_shutdown are now internal to msi.c, drop them from header and make them static.
--- include/linux/pci.h | 4 ---- drivers/pci/msi.c | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-)
include/linux/pci.h +0 -4
@@ -1209,11 +1209,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 );
@@ -1237,13 +1235,11 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev, }#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 ; }
drivers/pci/msi.c +2 -2
@@ -887,7 +887,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;
@@ -993,7 +993,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;
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 6/10 virtio_pci: drop msi_off on probe
In reply to Michael S. Tsirkinpci core now disables msi on probe automatically,
drop this from device-specific code.
--- drivers/virtio/virtio_pci_common.c | 3 --- 1 file changed, 3 deletions(-)
drivers/virtio/virtio_pci_common.c +0 -3
@@ -501,9 +501,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,INIT_LIST_HEAD (&vp_dev->virtqueues);spin_lock_init (&vp_dev->lock); -/* Disable MSI/MSIX to bring device to a known good state. */ -pci_msi_off (pci_dev); -/* enable the device */ rc =pci_enable_device (pci_dev);if (rc)
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 7/10 ntb: drop pci_msi_off call on probe
In reply to Michael S. Tsirkinpci core now disables msi on probe automatically,
drop this from device-specific code.
--- drivers/ntb/ntb_hw.c | 2 -- 1 file changed, 2 deletions(-)
drivers/ntb/ntb_hw.c +0 -2
@@ -1313,8 +1313,6 @@ static int ntb_setup_intx(struct ntb_device *ndev)struct pci_dev *pdev = ndev->pdev;int rc; -pci_msi_off (pdev); -/* Verify intx is enabled */ pci_intx (pdev,1 );
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 8/10 mic: drop pci_msi_off call on probe
In reply to Michael S. Tsirkinpci core now disables msi on probe automatically,
drop this from device-specific code.
--- drivers/misc/mic/host/mic_intr.c | 2 -- 1 file changed, 2 deletions(-)
drivers/misc/mic/host/mic_intr.c +0 -2
@@ -363,8 +363,6 @@ static int mic_setup_intx(struct mic_device *mdev, struct pci_dev *pdev) {int rc; -pci_msi_off (pdev); -/* Enable intx */ pci_intx (pdev,1 ); rc =mic_setup_callbacks (mdev);
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 9/10 pci: drop pci_msi_off calls from quirks
In reply to Michael S. Tsirkinpci core now disables msi on probe automatically,
drop this from device-specific quirks.
--- drivers/pci/quirks.c | 2 -- 1 file changed, 2 deletions(-)
drivers/pci/quirks.c +0 -2
@@ -1600,7 +1600,6 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EESSC, quirk_astatic void quirk_pcie_mch (struct pci_dev *pdev) { -pci_msi_off (pdev); pdev->no_msi =1 ; }DECLARE_PCI_FIXUP_FINAL (PCI_VENDOR_ID_INTEL ,PCI_DEVICE_ID_INTEL_E7520_MCH , quirk_pcie_mch);
@@ -1614,7 +1613,6 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7525_MCH, quir*/ static void quirk_pcie_pxh (struct pci_dev *dev) { -pci_msi_off (dev); dev->no_msi =1 ;dev_warn (&dev->dev,"PXH quirk detected; SHPC device MSI disabled\n" ); }
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v4 10/10 pci: unexport pci_msi_off
In reply to Michael S. TsirkinNow one should be using pci_msi_off now, unexport it and move the declaration to internal header to prevent misuse.
--- drivers/pci/pci.h | 2 ++ include/linux/pci.h | 1 - drivers/pci/pci.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)
drivers/pci/pci.h +2 -0
@@ -144,6 +144,8 @@ void pci_no_msi(void);static inline void pci_no_msi (void ) { }#endif +void pci_msi_off (struct pci_dev *dev); +static inline void pci_msi_set_enable (struct pci_dev *dev,int enable) {u16 control;
include/linux/pci.h +0 -1
@@ -970,7 +970,6 @@ void pci_intx(struct pci_dev *dev, int enable);bool pci_intx_mask_supported (struct pci_dev *dev);bool pci_check_and_mask_intx (struct pci_dev *dev);bool pci_check_and_unmask_intx (struct pci_dev *dev); -void pci_msi_off (struct pci_dev *dev);int pci_set_dma_max_seg_size (struct pci_dev *dev,unsigned int size);int pci_set_dma_seg_boundary (struct pci_dev *dev,unsigned long mask);int pci_wait_for_pending (struct pci_dev *dev,int pos,u16 mask);
drivers/pci/pci.c +1 -1
@@ -3103,6 +3103,7 @@ EXPORT_SYMBOL_GPL(pci_check_and_unmask_intx);* If you want to use MSI, see pci_enable_msi() and friends. * This is a lower-level primitive that allows us to disable * MSI operation at the device level. +* Not for use by drivers. */ void pci_msi_off (struct pci_dev *dev) {
@@ -3112,7 +3113,6 @@ void pci_msi_off(struct pci_dev *dev)if (dev->msix_cap)pci_msix_clear_and_set_ctrl (dev,PCI_MSIX_FLAGS_ENABLE ,0 ); } -EXPORT_SYMBOL_GPL (pci_msi_off);int pci_set_dma_max_seg_size (struct pci_dev *dev,unsigned int size) {
Signature · 1 line
MST