On Thu, May 14, 2009 at 05:02:06PM +0900, Hidetoshi Seto wrote:
> 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?
I disagree that the helper function improves readability. With it open-coded,
you can see clearly that we just keep modifying the control register, disabling
and enabling the various bits.
I think the real problem is that msix_capability_init() is 90 lines
of code with 11 local variables. The fix for this, however, is not
to encapsulate the function control elsewhere, but to create more
subfunctions. I shrunk it to 45 lines by doing that ... I'll send
that patch as a followup (note I didn't even boot the result, but it
does compile).
----
commit 7d4f9bdd8995e6718cb9dec57fd294bc16f17919
Author: Matthew Wilcox <matthew@xxxxxx>
Date: Wed May 13 17:31:11 2009 -0400
Fix the NIU MSI-X problem in a better way
The previous MSI-X fix (8d181018532dd709ec1f789e374cda92d7b01ce1) had
three bugs. First, 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, and 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.
Signed-off-by: Matthew Wilcox <willy@xxxxxxxxxxxxxxx>
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 3627732..44dadd9 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -321,22 +321,22 @@ static void __pci_restore_msix_state(struct pci_dev *dev)
if (!dev->msix_enabled)
return;
+ 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);
/* route the table */
pci_intx_for_msi(dev, 0);
- msix_set_enable(dev, 0);
+ control |= PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL;
+ pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
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);
}
@@ -427,11 +427,14 @@ 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 */
-
pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+ pci_read_config_word(dev, pos + PCI_MSIX_FLAGS, &control);
+
+ /* Ensure MSI-X is disabled while it is set up */
+ control &= ~PCI_MSIX_FLAGS_ENABLE;
+ pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
+
/* Request & Map MSI-X table region */
- pci_read_config_word(dev, msi_control_reg(pos), &control);
nr_entries = multi_msix_capable(control);
pci_read_config_dword(dev, msix_table_offset_reg(pos), &table_offset);
@@ -442,7 +445,6 @@ static int msix_capability_init(struct pci_dev *dev,
if (base == NULL)
return -ENOMEM;
- /* MSI-X Table Initialization */
for (i = 0; i < nvec; i++) {
entry = alloc_msi_entry(dev);
if (!entry)
@@ -455,7 +457,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);
}
@@ -480,22 +481,31 @@ static int msix_capability_init(struct pci_dev *dev,
return ret;
}
+ /*
+ * Some devices require MSI-X to be enabled before we can touch the
+ * MSI-X registers. We need to mask all the vectors to prevent
+ * interrupts coming in before they're fully set up.
+ */
+ control |= PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE;
+ pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
+
i = 0;
list_for_each_entry(entry, &dev->msi_list, list) {
entries[i].vector = entry->irq;
set_irq_msi(entry->irq, entry);
+ j = entries[i].entry;
+ entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE +
+ PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
+ msix_mask_irq(entry, 1);
i++;
}
- /* Set MSI-X enabled bits */
+
+ /* Set MSI-X enabled bits and unmask the function */
pci_intx_for_msi(dev, 0);
- msix_set_enable(dev, 1);
dev->msix_enabled = 1;
- 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);
- }
+ control &= ~PCI_MSIX_FLAGS_MASKALL;
+ pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
return 0;
}
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
