Skip to contentSkip to conversation

pci: fix unhandled interrupt on shutdown

PATCH In review Michael S. Tsirkin 4 participants 22 messages Mar 29, 2015 – Apr 12, 2015

Subsystems (4)
lore
Share & tools
lore

Michael S. Tsirkin

author maintainer
#0 lore raw

v5 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: the problem was actually observed in the field, although the BZ in question isn't public yet.

Patches 7-10 compiled only, will need maintainer ack.

Please review, and consider at least 1-6 for 4.1.

changes from v4:
Yijing Wang <[email protected]> noted that early fixups rely on pci_msi_off.
Split out the functionality and move off the required part to run early during pci_device_setup.
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 (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                  | 57 ++++++++------------------------------
 drivers/pci/pci-driver.c           |  2 --
 drivers/pci/pci.c                  | 25 ++++-------------
 drivers/pci/probe.c                | 15 ++++++++++
 drivers/pci/quirks.c               |  2 --
 drivers/virtio/virtio_pci_common.c |  3 --
 10 files changed, 57 insertions(+), 81 deletions(-)
Signature · 1 line
MST
author maintainer
#1 lore raw

PATCH v5 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
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
Reviewed-by on v5 1/10
#2 lore raw

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

Quoted 2 lines · 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.

if

s/will be used which/when/

then

Quoted 177 lines
---
 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
index c3e7dfc..9942f68 100644
@@ -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);
 }
 
 /**
Signature · 1 line
MST
author maintainer
#3 lore raw

PATCH v5 2/10 pci: move pci_msi_init_pci_dev to probe.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 part of the functionality probe.c, and compiling it unconditionally.

---
 drivers/pci/msi.c   | 12 ------------
 drivers/pci/probe.c | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 12 deletions(-)
drivers/pci/msi.c +0 -12
index 9942f68..f66be86 100644
@@ -1041,18 +1041,6 @@ 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);
 }
 
 /**
drivers/pci/probe.c +16 -0
index 8d2f400..50dd934 100644
@@ -1483,10 +1483,26 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	return dev;
 }
 
+static void pci_msi_setup_pci_dev(struct pci_dev *dev)
+{
+	/* 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 */
 	pci_msi_init_pci_dev(dev);
+	pci_msi_setup_pci_dev(dev);
 
 	/* Buffers for saving PCIe and PCI-X capabilities */
 	pci_allocate_cap_save_buffers(dev);
Signature · 1 line
MST
Reviewed-by on v5 2/10
#4 lore raw

Re: [PATCH v5 02/10] pci: move pci_msi_init_pci_dev to probe.c

Quoted 10 lines · 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 part of the functionality probe.c, and compiling it unconditionally.

Quoted 65 lines
---
 drivers/pci/msi.c   | 12 ------------
 drivers/pci/probe.c | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 12 deletions(-)
drivers/pci/msi.c +0 -12
index 9942f68..f66be86 100644
@@ -1041,18 +1041,6 @@ 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);
 }
 
 /**
drivers/pci/probe.c +16 -0
index 8d2f400..50dd934 100644
@@ -1483,10 +1483,26 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	return dev;
 }
 
+static void pci_msi_setup_pci_dev(struct pci_dev *dev)
+{
+	/* 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 */
 	pci_msi_init_pci_dev(dev);
+	pci_msi_setup_pci_dev(dev);
 
 	/* Buffers for saving PCIe and PCI-X capabilities */
 	pci_allocate_cap_save_buffers(dev);
Signature · 1 line
MST
#5 lore raw

Re: [PATCH v5 02/10] pci: move pci_msi_init_pci_dev to probe.c

Quoted 5 lines · Fam Zheng
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 6 lines

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 part of the functionality probe.c, and compiling it unconditionally.

I am a little surprised that there are systems that compile out MSI support, but I do remember the problem being a limitation of the code when wrote it and given how nasty screaming irqs are during boot up it seems well worth it to have a little extra code to turn them off.

Eric

Quoted 64 lines
Quoted 64 lines
---
 drivers/pci/msi.c   | 12 ------------
 drivers/pci/probe.c | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 12 deletions(-)
drivers/pci/msi.c +0 -12
index 9942f68..f66be86 100644
@@ -1041,18 +1041,6 @@ 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);
 }
 
 /**
drivers/pci/probe.c +16 -0
index 8d2f400..50dd934 100644
@@ -1483,10 +1483,26 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	return dev;
 }
 
+static void pci_msi_setup_pci_dev(struct pci_dev *dev)
+{
+	/* 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 */
 	pci_msi_init_pci_dev(dev);
+	pci_msi_setup_pci_dev(dev);
 
 	/* Buffers for saving PCIe and PCI-X capabilities */
 	pci_allocate_cap_save_buffers(dev);
Signature · 1 line
MST
author maintainer
#6 lore raw

PATCH v5 3/10 pci: drop some duplicate code

In reply to Michael S. Tsirkin

pci_msi_setup_pci_dev and pci_msi_off share a lot of code. This used to be justified since pci_msi_setup_pci_dev wasn't compiled in when CONFIG_PCI_MSI is off. Now that it is, let's reuse code.

Since pci_msi_off is used by early quirks, doing this requires calling pci_msi_setup_pci_dev early after device allocation, so move the call to pci_setup_device.

---
 drivers/pci/pci.c   | 23 ++++-------------------
 drivers/pci/probe.c | 31 +++++++++++++++----------------
 2 files changed, 19 insertions(+), 35 deletions(-)
drivers/pci/pci.c +4 -19
index 81f06e8..fcee8ea 100644
@@ -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 +15 -16
index 50dd934..120772c 100644
@@ -1084,6 +1084,18 @@ int pci_cfg_space_size(struct pci_dev *dev)
 
 #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
 
+static void pci_msi_setup_pci_dev(struct pci_dev *dev)
+{
+	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.
+	 */
+	pci_msi_off(dev);
+}
+
 /**
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill
@@ -1139,6 +1151,9 @@ int pci_setup_device(struct pci_dev *dev)
 	/* "Unknown power state" */
 	dev->current_state = PCI_UNKNOWN;
 
+	/* MSI/MSI-X setup has to be done early since it's used by quirks. */
+	pci_msi_setup_pci_dev(dev);
+
 	/* Early fixups, before probing the BARs */
 	pci_fixup_device(pci_fixup_early, dev);
 	/* device class may be changed after fixup */
@@ -1483,26 +1498,10 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	return dev;
 }
 
-static void pci_msi_setup_pci_dev(struct pci_dev *dev)
-{
-	/* 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 */
 	pci_msi_init_pci_dev(dev);
-	pci_msi_setup_pci_dev(dev);
 
 	/* Buffers for saving PCIe and PCI-X capabilities */
 	pci_allocate_cap_save_buffers(dev);
Signature · 1 line
MST
#7 lore raw

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

Quoted 8 lines · Michael S. Tsirkin

pci_msi_setup_pci_dev and pci_msi_off share a lot of code. This used to be justified since pci_msi_setup_pci_dev wasn't compiled in when CONFIG_PCI_MSI is off. Now that it is, let's reuse code.

Since pci_msi_off is used by early quirks, doing this requires calling pci_msi_setup_pci_dev early after device allocation, so move the call to pci_setup_device.

The dropping of duplicated code looks good, but doesn't the moving of pci_msi_setup_pci_dev call belong to a separate patch?

Fam

Quoted 105 lines
---
 drivers/pci/pci.c   | 23 ++++-------------------
 drivers/pci/probe.c | 31 +++++++++++++++----------------
 2 files changed, 19 insertions(+), 35 deletions(-)
drivers/pci/pci.c +4 -19
index 81f06e8..fcee8ea 100644
@@ -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 +15 -16
index 50dd934..120772c 100644
@@ -1084,6 +1084,18 @@ int pci_cfg_space_size(struct pci_dev *dev)
 
 #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
 
+static void pci_msi_setup_pci_dev(struct pci_dev *dev)
+{
+	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.
+	 */
+	pci_msi_off(dev);
+}
+
 /**
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill
@@ -1139,6 +1151,9 @@ int pci_setup_device(struct pci_dev *dev)
 	/* "Unknown power state" */
 	dev->current_state = PCI_UNKNOWN;
 
+	/* MSI/MSI-X setup has to be done early since it's used by quirks. */
+	pci_msi_setup_pci_dev(dev);
+
 	/* Early fixups, before probing the BARs */
 	pci_fixup_device(pci_fixup_early, dev);
 	/* device class may be changed after fixup */
@@ -1483,26 +1498,10 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	return dev;
 }
 
-static void pci_msi_setup_pci_dev(struct pci_dev *dev)
-{
-	/* 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 */
 	pci_msi_init_pci_dev(dev);
-	pci_msi_setup_pci_dev(dev);
 
 	/* Buffers for saving PCIe and PCI-X capabilities */
 	pci_allocate_cap_save_buffers(dev);
Signature · 1 line
MST
author maintainer
#8 lore raw

PATCH v5 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]>
---
 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
Reviewed-by Tested-by on v5 4/10
#9 lore raw

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

Quoted 9 lines · 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.

Quoted 7 lines

In particular, this was observed with virtio.

Cc Yinghai Lu <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Rusty Russell <[email protected]>
Quoted 20 lines
---
 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
maintainer
#10 lore raw

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

In reply to Michael S. Tsirkin

Hi Michael,

Quoted 10 lines · 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.

I'm not questioning that this hang happens, but would you mind outlining *how* it happens in a little more detail? I'm not an IRQ expert, so I expected an "irq %d: nobody cared" message or something similar. It seems like a kernel hang is a pretty severe way to deal with an unexpected interrupt.

Is virtio the only way the hang could happen, or is it just coincidence that it was involved?

It'd be really nice if we could reference the bug report here. I think you said the original report was private. Can we open a kernel.org bugzilla that contains just the public information?

Quoted 25 lines
Cc Yinghai Lu <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Rusty Russell <[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
author maintainer
#11 lore raw

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

Quoted 12 lines · Bjorn Helgaas

Hi Michael,

Quoted 9 lines · 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.

Quoted 7 lines
Quoted 1 line

In particular, this was observed with virtio.

I'm not questioning that this hang happens, but would you mind outlining *how* it happens in a little more detail? I'm not an IRQ expert, so I expected an "irq %d: nobody cared" message or something similar. It seems like a kernel hang is a pretty severe way to deal with an unexpected interrupt.

True. I intend to look into how this interacts with spurious interrupt detection some more. Avoiding spurious interrupts seems like a worthwhile goal in any case, right?

It seems clear how this will cause hangs when noirqdebug is set (later leads to softlockup detected messages, or crash if softlockup_panic=1 is set).

Quoted 2 lines

Is virtio the only way the hang could happen, or is it just coincidence that it was involved?

Well, you need a driver which doesn't handle level IRQs when it enables MSI. virtio is one such driver.

Quoted 3 lines

It'd be really nice if we could reference the bug report here. I think you said the original report was private. Can we open a kernel.org bugzilla that contains just the public information?

Ulrich Obergfell did most of the work on reproducing this, Fam Zheng did most debugging, so I'd like one of them to do this, so they get the appropriate credit. Fam, Ulrich?

Quoted 25 lines
Quoted 25 lines
Cc Yinghai Lu <[email protected]>
Cc Ulrich Obergfell <[email protected]>
Cc Rusty Russell <[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
author maintainer
#12 lore raw

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

---
 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 f66be86..ea78a07 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
Reviewed-by on v5 5/10
#13 lore raw

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

Quoted 2 lines · Michael S. Tsirkin

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

Quoted 62 lines
---
 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 f66be86..ea78a07 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
author maintainer
#14 lore raw

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

---
 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
Reviewed-by on v5 6/10
#15 lore raw

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

Quoted 27 lines · Michael S. Tsirkin

pci 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
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
Reviewed-by on v5 6/10
#16 lore raw

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

In reply to Michael S. Tsirkin
Quoted 27 lines · Michael S. Tsirkin

pci 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
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
author maintainer
#17 lore raw

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

---
 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
author maintainer
#18 lore raw

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

---
 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
author maintainer
#19 lore raw

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

---
 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
author maintainer
#20 lore raw

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

---
 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 17f213d..620fcad 100644
@@ -146,6 +146,8 @@ static inline void pci_no_msi(void) { }
 static inline void pci_msi_init_pci_dev(struct pci_dev *dev) { }
 #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 fcee8ea..54cefb4 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)
 {
@@ -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
author maintainer
#21 lore raw
In reply to Michael S. Tsirkin
Quoted 12 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.

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.

Quoted 8 lines

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: the problem was actually observed in the field, although the BZ in question isn't public yet.
Patches 7-10 compiled only, will need maintainer ack.

Looks like people don't bother to review until it's merged.

Bjorn, what do you think about this patchset?
Could you merge it for 4.1?

Quoted 44 lines

Please review, and consider at least 1-6 for 4.1.

changes from v4:
Yijing Wang <[email protected]> noted that early fixups rely on pci_msi_off.
Split out the functionality and move off the required part to run early during pci_device_setup.
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 (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                  | 57 ++++++++------------------------------
 drivers/pci/pci-driver.c           |  2 --
 drivers/pci/pci.c                  | 25 ++++-------------
 drivers/pci/probe.c                | 15 ++++++++++
 drivers/pci/quirks.c               |  2 --
 drivers/virtio/virtio_pci_common.c |  3 --
 10 files changed, 57 insertions(+), 81 deletions(-)
Signature · 1 line
MST