pci: fix unhandled interrupt on shutdown

PATCH Superseded Michael S. Tsirkin 4 participants 15 messages Mar 24, 2015 – Mar 25, 2015

Subsystems (5)

Stable branch PCI Char and misc drivers virtio core +1 more subsystems

lore

Share & tools

lore

Michael S. Tsirkin

author maintainer

2015-03-24 15:42 UTC #0 lore raw

v3 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.

As a result, people reported VMs being hung on shutdown, with traces that look like this:

...

[<ffffffff815f367b>] ? _raw_spin_unlock_irqrestore+0x1b/0x40

  [<ffffffff812e068f>] pci_bus_read_config_word+0x9f/0xb0
  [<ffffffff812e205c>] pci_read_config_word+0x1c/0x20
  [<ffffffff812e5ea3>] pci_intx+0x33/0xb0
  [<ffffffff81304ddd>] pci_msix_shutdown+0x8d/0x90
  [<ffffffff812ead26>] pci_device_shutdown+0x46/0x60
  [<ffffffff813b4258>] device_shutdown+0xc8/0x180
  [<ffffffff810761c5>] kernel_power_off+0x35/0x80
  [<ffffffff81077ceb>] SYSC_reboot+0x18b/0x260
  [<ffffffff811c5bb5>] ? d_free+0x55/0x60
  [<ffffffff811cf414>] ? mntput+0x24/0x40
  [<ffffffff811b1613>] ? __fput+0x183/0x270
  [<ffffffff81077dee>] SyS_reboot+0xe/0x10
  [<ffffffff815fc819>] system_call_fastpath+0x16/0x1b

...

as well as the messages that look like this:

BUG: soft lockup - CPU#2 stuck for 22s!  [poweroff:1964]

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.

The patch that added the code in question is from

	d52877c7b1afb8c37ebe17e2005040b79cb618b0:
        "pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2"

from 2008, but it turns out that in 2011 the following commit
commit d5dea7d95c48d7bc951cee4910a7fd9c0cd26fb0
"PCI: msi: Disable msi interrupts when we initialize a pci device"
fixed the issue in a more robust way. The only thing left to do is to fix configurations with CONFIG_PCI_MSI disabled, and then d52877c7b1afb8c37ebe17e2005040b79cb618b0 can be reverted.

Status:

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:
move code from probe to device enumeration add patches to unexport pci_msi_off

Michael S. Tsirkin (10):
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                  | 26 ++++------------
 drivers/pci/probe.c                | 16 ++++++++++
 drivers/pci/quirks.c               |  2 --
 drivers/virtio/virtio_pci_common.c |  3 --
 10 files changed, 57 insertions(+), 88 deletions(-)

Signature · 1 line

MST

Michael S. Tsirkin

author maintainer

2015-03-24 15:42 UTC #1 lore raw

PATCH v3 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.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/pci/pci.h | 21 +++++++++++++++++++++
 drivers/pci/msi.c | 45 ++++++++++++---------------------------------
 2 files changed, 33 insertions(+), 33 deletions(-)

drivers/pci/pci.h +21 -0

index 4091f82..17f213d 100644
@@ -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

author maintainer

2015-03-24 15:42 UTC #2 lore raw

PATCH v3 2/10 pci: move pci_msi_init_pci_dev to pci.c

In reply to Michael S. Tsirkin

commit 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.

Cc [email protected]

Cc Eric W. Biederman <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 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

index 17f213d..a964569 100644
@@ -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~~

index 9942f68..ae3f7cc 100644
@@ -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

index 8d2f400..45d6d5c 100644
@@ -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

author maintainer

2015-03-24 15:42 UTC #3 lore raw

PATCH v3 3/10 pci: drop some duplicate code

In reply to Michael S. Tsirkin

pci_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.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/pci/pci.c   | 24 +++++-------------------
 drivers/pci/probe.c | 11 ++++-------
 2 files changed, 9 insertions(+), 26 deletions(-)

drivers/pci/pci.c +5 ~~-19~~

index 81f06e8..d5f297a 100644
@@ -3106,26 +3106,12 @@ 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);
-	}
+	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);
 }
 EXPORT_SYMBOL_GPL(pci_msi_off);

drivers/pci/probe.c +4 -7

index 45d6d5c..baf8ddd 100644
@@ -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

Bjorn Helgaas

maintainer

2015-03-24 23:33 UTC #4 lore raw

Re: [PATCH v3 03/10] pci: drop some duplicate code

Quoted 32 lines · Michael S. Tsirkin

pci_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.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/pci/pci.c   | 24 +++++-------------------
 drivers/pci/probe.c | 11 ++++-------
 2 files changed, 9 insertions(+), 26 deletions(-)

drivers/pci/pci.c +2 ~~-10~~

index 81f06e8..d5f297a 100644
@@ -3106,26 +3106,12 @@ 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);

drivers/pci/pci.c EXPORT_SYMBOL_GPL(pci_check_and_unmask_intx); · line 3112

Quoted 10 lines

drivers/pci/pci.c +1 -9

-		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);
-	}
+	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);

I think the above line is superfluous, isn't it? You already do the same in pci_msi_init_pci_dev() (below).

Quoted 34 lines

drivers/pci/pci.c +2 -0

+	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

index 45d6d5c..baf8ddd 100644
@@ -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

Michael S. Tsirkin

author maintainer

2015-03-25 05:40 UTC #5 lore raw

Re: [PATCH v3 03/10] pci: drop some duplicate code

Quoted 38 lines · Bjorn Helgaas

Quoted 37 lines · Michael S. Tsirkin

pci_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.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/pci/pci.c   | 24 +++++-------------------
 drivers/pci/probe.c | 11 ++++-------
 2 files changed, 9 insertions(+), 26 deletions(-)

drivers/pci/pci.c +2 ~~-15~~

index 81f06e8..d5f297a 100644
@@ -3106,26 +3106,12 @@ 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) {

Quoted 8 lines

drivers/pci/pci.c EXPORT_SYMBOL_GPL(pci_check_and_unmask_intx); · line 3112
Quoted 5 lines
drivers/pci/pci.c +1 -4
-		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);
-	}
+	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
I think the above line is superfluous, isn't it? You already do the same in pci_msi_init_pci_dev() (below).

Ugh. Yea. I'll fix that up, thanks!

Quoted 34 lines

drivers/pci/pci.c +2 -0

+	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

index 45d6d5c..baf8ddd 100644
@@ -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

Michael S. Tsirkin

author maintainer

2015-03-24 15:42 UTC #6 lore raw

PATCH v3 4/10 pci: don't disable msi/msix at shutdown

In reply to Michael S. Tsirkin

This 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.

Cc Yinghai Lu <[email protected]>

Cc Ulrich Obergfell <[email protected]>

Cc Rusty Russell <[email protected]>

Reported-by Fam Zheng <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/pci/pci-driver.c | 2 --
 1 file changed, 2 deletions(-)

drivers/pci/pci-driver.c +0 -2

index 3cb2210..38a602c 100644
@@ -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

author maintainer

2015-03-24 15:42 UTC #7 lore raw

PATCH v3 5/10 pci: make msi/msix shutdown functions static

In reply to Michael S. Tsirkin

pci_msi_shutdown and pci_msix_shutdown are now internal to msi.c, drop them from header and make them static.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/pci.h | 4 ----
 drivers/pci/msi.c   | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

include/linux/pci.h +0 -4

index 211e9da..a34df45 100644
@@ -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

index ae3f7cc..a353222 100644
@@ -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

author maintainer

2015-03-24 15:42 UTC #8 lore raw

PATCH v3 6/10 virtio_pci: drop msi_off on probe

In reply to Michael S. Tsirkin

pci core now disables msi on probe automatically,
drop this from device-specific code.

Cc Bjorn Helgaas <[email protected]>

Cc [email protected]

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_pci_common.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/virtio/virtio_pci_common.c +0 -3

index e894eb2..806bb2c 100644
@@ -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

author maintainer

2015-03-24 15:42 UTC #9 lore raw

PATCH v3 7/10 ntb: drop pci_msi_off call on probe

In reply to Michael S. Tsirkin

pci core now disables msi on probe automatically,
drop this from device-specific code.

Cc Bjorn Helgaas <[email protected]>

Cc [email protected]

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/ntb/ntb_hw.c | 2 --
 1 file changed, 2 deletions(-)

drivers/ntb/ntb_hw.c +0 -2

index cd29b10..8225cbc 100644
@@ -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

Jiang, Dave

maintainer Acked-by on v3 7/10

2015-03-24 16:12 UTC #10 lore raw

Re: [PATCH v3 07/10] ntb: drop pci_msi_off call on probe

Quoted 6 lines · Michael S. Tsirkin

pci core now disables msi on probe automatically,
drop this from device-specific code.
Cc Bjorn Helgaas <[email protected]>
Cc [email protected]
Signed-off-by Michael S. Tsirkin <[email protected]>

Acked-by Dave Jiang <[email protected]>

Quoted 17 lines

---
 drivers/ntb/ntb_hw.c | 2 --
 1 file changed, 2 deletions(-)

drivers/ntb/ntb_hw.c +0 -2

index cd29b10..8225cbc 100644
@@ -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);

Michael S. Tsirkin

author maintainer

2015-03-24 15:42 UTC #11 lore raw

PATCH v3 8/10 mic: drop pci_msi_off call on probe

In reply to Michael S. Tsirkin

pci core now disables msi on probe automatically,
drop this from device-specific code.

Cc Bjorn Helgaas <[email protected]>

Cc [email protected]

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/misc/mic/host/mic_intr.c | 2 --
 1 file changed, 2 deletions(-)

drivers/misc/mic/host/mic_intr.c +0 -2

index d686f28..b4ca6c8 100644
@@ -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

author maintainer

2015-03-24 15:42 UTC #12 lore raw

PATCH v3 9/10 pci: drop pci_msi_off calls from quirks

In reply to Michael S. Tsirkin

pci core now disables msi on probe automatically,
drop this from device-specific quirks.

Cc Bjorn Helgaas <[email protected]>

Cc [email protected]

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/pci/quirks.c | 2 --
 1 file changed, 2 deletions(-)

drivers/pci/quirks.c +0 -2

index 85f247e..df3e718 100644
@@ -1600,7 +1600,6 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_EESSC,	quirk_a
 
 static 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

author maintainer

2015-03-24 15:43 UTC #13 lore raw

PATCH v3 10/10 pci: unexport pci_msi_off

In reply to Michael S. Tsirkin

Now one should be using pci_msi_off now, unexport it and move the declaration to internal header to prevent misuse.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 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

index a964569..dd17615 100644
@@ -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

index a34df45..ef15f91 100644
@@ -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

index d5f297a..04063cc 100644
@@ -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)
 {

@@ -3113,7 +3114,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

Greg KH

2015-03-24 17:02 UTC #14 lore raw

In reply to Michael S. Tsirkin

Quoted 76 lines · Michael S. Tsirkin

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.
As a result, people reported VMs being hung on shutdown, with traces that look like this:
...
[<ffffffff815f367b>] ? _raw_spin_unlock_irqrestore+0x1b/0x40
  [<ffffffff812e068f>] pci_bus_read_config_word+0x9f/0xb0
  [<ffffffff812e205c>] pci_read_config_word+0x1c/0x20
  [<ffffffff812e5ea3>] pci_intx+0x33/0xb0
  [<ffffffff81304ddd>] pci_msix_shutdown+0x8d/0x90
  [<ffffffff812ead26>] pci_device_shutdown+0x46/0x60
  [<ffffffff813b4258>] device_shutdown+0xc8/0x180
  [<ffffffff810761c5>] kernel_power_off+0x35/0x80
  [<ffffffff81077ceb>] SYSC_reboot+0x18b/0x260
  [<ffffffff811c5bb5>] ? d_free+0x55/0x60
  [<ffffffff811cf414>] ? mntput+0x24/0x40
  [<ffffffff811b1613>] ? __fput+0x183/0x270
  [<ffffffff81077dee>] SyS_reboot+0xe/0x10
  [<ffffffff815fc819>] system_call_fastpath+0x16/0x1b
...
as well as the messages that look like this:
BUG: soft lockup - CPU#2 stuck for 22s!  [poweroff:1964]
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.
The patch that added the code in question is from
	d52877c7b1afb8c37ebe17e2005040b79cb618b0:
        "pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2"
from 2008, but it turns out that in 2011 the following commit
commit d5dea7d95c48d7bc951cee4910a7fd9c0cd26fb0
"PCI: msi: Disable msi interrupts when we initialize a pci device"
fixed the issue in a more robust way. The only thing left to do is to fix configurations with CONFIG_PCI_MSI disabled, and then d52877c7b1afb8c37ebe17e2005040b79cb618b0 can be reverted.
Status:
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:
move code from probe to device enumeration add patches to unexport pci_msi_off
Michael S. Tsirkin (10):
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

Quoted 11 lines

 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                  | 26 ++++------------
 drivers/pci/probe.c                | 16 ++++++++++
 drivers/pci/quirks.c               |  2 --
 drivers/virtio/virtio_pci_common.c |  3 --
 10 files changed, 57 insertions(+), 88 deletions(-)

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.

</formletter>