[PATCH v3 00/10] pci: fix unhandled interrupt on shutdown

[PATCH v3 00/10] pci: fix unhandled interrupt on shutdown

[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

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

-- 
MST

[PATCH v3 01/10] pci: export functions for msi/msix ctrl

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 drivers/pci/pci.h | 21 +++++++++++++++++++++
 drivers/pci/msi.c | 45 ++++++++++++---------------------------------
 2 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4091f82..17f213d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index c3e7dfc..9942f68 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -185,27 +185,6 @@ void __weak arch_restore_msi_irqs(struct pci_dev *dev)
 	return default_restore_msi_irqs(dev);
 }
 
-static void 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 void 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);
-}
-
 static inline __attribute_const__ u32 msi_mask(unsigned x)
 {
 	/* Don't shift by >= width of type */
@@ -452,7 +431,7 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
 	entry = irq_get_msi_desc(dev->irq);
 
 	pci_intx_for_msi(dev, 0);
-	msi_set_enable(dev, 0);
+	pci_msi_set_enable(dev, 0);
 	arch_restore_msi_irqs(dev);
 
 	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
@@ -473,14 +452,14 @@ static void __pci_restore_msix_state(struct pci_dev *dev)
 
 	/* route the table */
 	pci_intx_for_msi(dev, 0);
-	msix_clear_and_set_ctrl(dev, 0,
+	pci_msix_clear_and_set_ctrl(dev, 0,
 				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
 
 	arch_restore_msi_irqs(dev);
 	list_for_each_entry(entry, &dev->msi_list, list)
 		msix_mask_irq(entry, entry->masked);
 
-	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
+	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
 }
 
 void pci_restore_msi_state(struct pci_dev *dev)
@@ -647,7 +626,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 	int ret;
 	unsigned mask;
 
-	msi_set_enable(dev, 0);	/* Disable MSI during set up */
+	pci_msi_set_enable(dev, 0);	/* Disable MSI during set up */
 
 	entry = msi_setup_entry(dev, nvec);
 	if (!entry)
@@ -683,7 +662,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 
 	/* Set MSI enabled bits	 */
 	pci_intx_for_msi(dev, 0);
-	msi_set_enable(dev, 1);
+	pci_msi_set_enable(dev, 1);
 	dev->msi_enabled = 1;
 
 	dev->irq = entry->irq;
@@ -775,7 +754,7 @@ static int msix_capability_init(struct pci_dev *dev,
 	void __iomem *base;
 
 	/* Ensure MSI-X is disabled while it is set up */
-	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
+	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
 
 	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
 	/* Request & Map MSI-X table region */
@@ -801,7 +780,7 @@ static int msix_capability_init(struct pci_dev *dev,
 	 * MSI-X registers.  We need to mask all the vectors to prevent
 	 * interrupts coming in before they're fully set up.
 	 */
-	msix_clear_and_set_ctrl(dev, 0,
+	pci_msix_clear_and_set_ctrl(dev, 0,
 				PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
 
 	msix_program_entries(dev, entries);
@@ -814,7 +793,7 @@ static int msix_capability_init(struct pci_dev *dev,
 	pci_intx_for_msi(dev, 0);
 	dev->msix_enabled = 1;
 
-	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
+	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
 
 	return 0;
 
@@ -919,7 +898,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
 	BUG_ON(list_empty(&dev->msi_list));
 	desc = list_first_entry(&dev->msi_list, struct msi_desc, list);
 
-	msi_set_enable(dev, 0);
+	pci_msi_set_enable(dev, 0);
 	pci_intx_for_msi(dev, 1);
 	dev->msi_enabled = 0;
 
@@ -1027,7 +1006,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
 		__pci_msix_desc_mask_irq(entry, 1);
 	}
 
-	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
+	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
 	pci_intx_for_msi(dev, 1);
 	dev->msix_enabled = 0;
 }
@@ -1069,11 +1048,11 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
 	 */
 	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
 	if (dev->msi_cap)
-		msi_set_enable(dev, 0);
+		pci_msi_set_enable(dev, 0);
 
 	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	if (dev->msix_cap)
-		msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
+		pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
 }
 
 /**
-- 
MST

[PATCH v3 02/10] pci: move pci_msi_init_pci_dev to pci.c

[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: stable@kernel.org
Cc: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/pci/pci.h   |  2 --
 drivers/pci/msi.c   | 17 -----------------
 drivers/pci/probe.c | 19 +++++++++++++++++++
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 17f213d..a964569 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 9942f68..ae3f7cc 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8d2f400..45d6d5c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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 */
-- 
MST

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

[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 <mst@redhat.com>
---
 drivers/pci/pci.c   | 24 +++++-------------------
 drivers/pci/probe.c | 11 ++++-------
 2 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 81f06e8..d5f297a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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);
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 45d6d5c..baf8ddd 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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)
-- 
MST

[PATCH v3 04/10] pci: don't disable msi/msix at shutdown

[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 <yhlu.kernel.send@gmail.com>
Cc: Ulrich Obergfell <uobergfe@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/pci/pci-driver.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 3cb2210..38a602c 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -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
 	/*
-- 
MST

[PATCH v3 05/10] pci: make msi/msix shutdown functions static

[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 <mst@redhat.com>
---
 include/linux/pci.h | 4 ----
 drivers/pci/msi.c   | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 211e9da..a34df45 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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; }
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index ae3f7cc..a353222 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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;
 
-- 
MST

[PATCH v3 06/10] virtio_pci: drop msi_off on probe

[Michael S. Tsirkin]

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

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci_common.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index e894eb2..806bb2c 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -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)
-- 
MST

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

[Michael S. Tsirkin]

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

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/ntb/ntb_hw.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index cd29b10..8225cbc 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -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);
 
-- 
MST

[PATCH v3 08/10] mic: drop pci_msi_off call on probe

[Michael S. Tsirkin]

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

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/misc/mic/host/mic_intr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/misc/mic/host/mic_intr.c b/drivers/misc/mic/host/mic_intr.c
index d686f28..b4ca6c8 100644
--- a/drivers/misc/mic/host/mic_intr.c
+++ b/drivers/misc/mic/host/mic_intr.c
@@ -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);
-- 
MST

[PATCH v3 09/10] pci: drop pci_msi_off calls from quirks

[Michael S. Tsirkin]

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

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/pci/quirks.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 85f247e..df3e718 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -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");
 }
-- 
MST

[PATCH v3 10/10] pci: unexport pci_msi_off

[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 <mst@redhat.com>
---
 drivers/pci/pci.h   | 2 ++
 include/linux/pci.h | 1 -
 drivers/pci/pci.c   | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a964569..dd17615 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a34df45..ef15f91 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d5f297a..04063cc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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)
 {
-- 
MST

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

[Jiang, Dave]

T24gVHVlLCAyMDE1LTAzLTI0IGF0IDE2OjQyICswMTAwLCBNaWNoYWVsIFMuIFRzaXJraW4gd3Jv
dGU6DQo+IHBjaSBjb3JlIG5vdyBkaXNhYmxlcyBtc2kgb24gcHJvYmUgYXV0b21hdGljYWxseSwN
Cj4gZHJvcCB0aGlzIGZyb20gZGV2aWNlLXNwZWNpZmljIGNvZGUuDQo+IA0KPiBDYzogQmpvcm4g
SGVsZ2FhcyA8YmhlbGdhYXNAZ29vZ2xlLmNvbT4NCj4gQ2M6IGxpbnV4LXBjaUB2Z2VyLmtlcm5l
bC5vcmcNCj4gU2lnbmVkLW9mZi1ieTogTWljaGFlbCBTLiBUc2lya2luIDxtc3RAcmVkaGF0LmNv
bT4NCg0KQWNrZWQtYnk6IERhdmUgSmlhbmcgPGRhdmUuamlhbmdAaW50ZWwuY29tPg0KDQo+IC0t
LQ0KPiAgZHJpdmVycy9udGIvbnRiX2h3LmMgfCAyIC0tDQo+ICAxIGZpbGUgY2hhbmdlZCwgMiBk
ZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL250Yi9udGJfaHcuYyBiL2Ry
aXZlcnMvbnRiL250Yl9ody5jDQo+IGluZGV4IGNkMjliMTAuLjgyMjVjYmMgMTAwNjQ0DQo+IC0t
LSBhL2RyaXZlcnMvbnRiL250Yl9ody5jDQo+ICsrKyBiL2RyaXZlcnMvbnRiL250Yl9ody5jDQo+
IEBAIC0xMzEzLDggKzEzMTMsNiBAQCBzdGF0aWMgaW50IG50Yl9zZXR1cF9pbnR4KHN0cnVjdCBu
dGJfZGV2aWNlICpuZGV2KQ0KPiAgCXN0cnVjdCBwY2lfZGV2ICpwZGV2ID0gbmRldi0+cGRldjsN
Cj4gIAlpbnQgcmM7DQo+ICANCj4gLQlwY2lfbXNpX29mZihwZGV2KTsNCj4gLQ0KPiAgCS8qIFZl
cmlmeSBpbnR4IGlzIGVuYWJsZWQgKi8NCj4gIAlwY2lfaW50eChwZGV2LCAxKTsNCj4gIA0K

Re: [PATCH v3 00/10] pci: fix unhandled interrupt on shutdown

[Greg KH]

On Tue, Mar 24, 2015 at 04:42:25PM +0100, Michael S. Tsirkin wrote:
> 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(-)


<formletter>

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>

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

[Bjorn Helgaas]

On Tue, Mar 24, 2015 at 04:42:35PM +0100, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
>  drivers/pci/pci.c   | 24 +++++-------------------
>  drivers/pci/probe.c | 11 ++++-------
>  2 files changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 81f06e8..d5f297a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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);

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

> +	if (dev->msix_cap)
> +		pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
>  }
>  EXPORT_SYMBOL_GPL(pci_msi_off);
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 45d6d5c..baf8ddd 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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)
> -- 
> MST
> 

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

[Michael S. Tsirkin]

On Tue, Mar 24, 2015 at 06:33:57PM -0500, Bjorn Helgaas wrote:
> On Tue, Mar 24, 2015 at 04:42:35PM +0100, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> > ---
> >  drivers/pci/pci.c   | 24 +++++-------------------
> >  drivers/pci/probe.c | 11 ++++-------
> >  2 files changed, 9 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 81f06e8..d5f297a 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -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);
> 
> 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!

> > +	if (dev->msix_cap)
> > +		pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> >  }
> >  EXPORT_SYMBOL_GPL(pci_msi_off);
> >  
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 45d6d5c..baf8ddd 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -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)
> > -- 
> > MST
> >