incremental fix for NIU MSI-X problem

Commit Message

Hidetoshi Seto May 14, 2009, 8:02 a.m. UTC

Hi Matthew and Michel,

I modified Matthew's patch based on Michel's feedback.
I add a helper function msix_set_enable_maskall() for readability
and I think it looks much better.  How about this?

Thanks,
H.Seto


[ This is against Jesse's for-linus tree ]

The previous MSI-X fix (8d181018532dd709ec1f789e374cda92d7b01ce1) had
three bugs:

 - It didn't move the write that disabled the vector.
   This led to writing garbage to the MSI-X vector.
   (spotted by Michael Ellerman).
 - It didn't fix the PCI resume case.
 - It had a race window where the device could generate an interrupt
   before the MSI-X registers were programmed (leading to a DMA to
   random addresses).

Fortunately, the MSI-X capability has a bit to mask all the vectors.
By setting this bit instead of clearing the enable bit, we can ensure
the device will not generate spurious interrupts.  Since the capability
is now enabled, the NIU device will not have a problem with the reads
and writes to the MSI-X registers being in the original order in the code.

(Original code from Matthew, modified based on Michael's feedback, and
 I introduce msix_set_enable_maskall() for readability  - HS)

Signed-off-by: Matthew Wilcox <[e-mail]>
Signed-off-by: Hidetoshi Seto <[e-mail]>
---
 drivers/pci/msi.c |   59 +++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 44 insertions(+), 15 deletions(-)

Patch

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 3627732..d7fbe24 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -108,6 +108,26 @@  static void msix_set_enable(struct pci_dev *dev, int enable)
 	}
 }
 
+static void msix_set_enable_maskall(struct pci_dev *dev, int enable, int mask)
+{
+	int pos;
+	u16 control;
+
+	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+	if (pos) {
+		pci_read_config_word(dev, pos + PCI_MSIX_FLAGS, &control);
+		if (enable)
+			control |= PCI_MSIX_FLAGS_ENABLE;
+		else
+			control &= ~PCI_MSIX_FLAGS_ENABLE;
+		if (mask)
+			control |= PCI_MSIX_FLAGS_MASKALL;
+		else
+			control &= ~PCI_MSIX_FLAGS_MASKALL;
+		pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
+	}
+}
+
 static inline __attribute_const__ u32 msi_mask(unsigned x)
 {
 	/* Don't shift by >= width of type */
@@ -315,29 +335,23 @@  static void __pci_restore_msi_state(struct pci_dev *dev)
 
 static void __pci_restore_msix_state(struct pci_dev *dev)
 {
-	int pos;
 	struct msi_desc *entry;
-	u16 control;
 
 	if (!dev->msix_enabled)
 		return;
 
+	BUG_ON(list_empty(&dev->msi_list));
+
 	/* route the table */
 	pci_intx_for_msi(dev, 0);
-	msix_set_enable(dev, 0);
+	msix_set_enable_maskall(dev, 1, 1);
 
 	list_for_each_entry(entry, &dev->msi_list, list) {
 		write_msi_msg(entry->irq, &entry->msg);
 		msix_mask_irq(entry, entry->masked);
 	}
 
-	BUG_ON(list_empty(&dev->msi_list));
-	entry = list_entry(dev->msi_list.next, struct msi_desc, list);
-	pos = entry->msi_attrib.pos;
-	pci_read_config_word(dev, pos + PCI_MSIX_FLAGS, &control);
-	control &= ~PCI_MSIX_FLAGS_MASKALL;
-	control |= PCI_MSIX_FLAGS_ENABLE;
-	pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
+	msix_set_enable_maskall(dev, 1, 0);
 }
 
 void pci_restore_msi_state(struct pci_dev *dev)
@@ -427,7 +441,8 @@  static int msix_capability_init(struct pci_dev *dev,
 	u8 bir;
 	void __iomem *base;
 
-	msix_set_enable(dev, 0);/* Ensure msix is disabled as I set it up */
+	/* Ensure msix is disabled as I set it up */
+	msix_set_enable(dev, 0);
 
    	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	/* Request & Map MSI-X table region */
@@ -455,7 +470,6 @@  static int msix_capability_init(struct pci_dev *dev,
 		entry->msi_attrib.default_irq = dev->irq;
 		entry->msi_attrib.pos = pos;
 		entry->mask_base = base;
-		msix_mask_irq(entry, 1);
 
 		list_add_tail(&entry->list, &dev->msi_list);
 	}
@@ -486,17 +500,32 @@  static int msix_capability_init(struct pci_dev *dev,
 		set_irq_msi(entry->irq, entry);
 		i++;
 	}
-	/* Set MSI-X enabled bits */
+
+	/* Set MSI-X enabled bits with function masked */
 	pci_intx_for_msi(dev, 0);
-	msix_set_enable(dev, 1);
-	dev->msix_enabled = 1;
+	msix_set_enable_maskall(dev, 1, 1);
 
+	/*
+	 * The value of bits[31::01] of Vector Control for MSI-X Table Entries
+	 * must be preserved by software.  Refer PCI spec 3.0, 6.8.2.9.
+	 * (Some of these bits are now used.  Refer PCIe spec 2.1, 7.7.1.)
+	 *
+	 * Some devices require MSI-X to be enabled before we can touch the
+	 * MSI-X registers.  So we need to mask all the vectors to prevent
+	 * interrupts coming in before they're fully set up.
+	 */
 	list_for_each_entry(entry, &dev->msi_list, list) {
 		int vector = entry->msi_attrib.entry_nr;
 		entry->masked = readl(base + vector * PCI_MSIX_ENTRY_SIZE +
 					PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
+		/* Make sure it is masked */
+		msix_mask_irq(entry, 1);
 	}
 
+	/* Unmask the function */
+	msix_set_enable_maskall(dev, 1, 0);
+	dev->msix_enabled = 1;
+
 	return 0;
 }