Hi,
I don't have any platform with MSI-X capable PCI Express port, so I've
never tried the current MSI-X configuration code of PCIe port service
driver. But I think it has at least the following problems.
- To setup MSI-X for PCIe port services, we need to know which MSI-X
table entries are used for which services. For example, MSI-X entry
for Hot Plug and PME is indicated by Interrupt Message Number field
in the PCI Express Capabilities register, and MSI-X entry for
Advanced Error Reporting is indicated by Root Error Status register.
But current PCIe port service driver core is doing nothing about
that, and it would cause unexpected interrupt behaviour.
- Current PCIe port service driver tries to allocate vectors for Hot
Plug, PME, AER and VC individually. So PCIe port service driver
would request four vectors in maximum. But it should never be four
because Hot Plug and PME always share the interrupt. In addition,
I'm wondering if VC needs vectors, though I don't know about VC
very much.
I made a sample patch to explain the required fix I'm thinking. Please
note that it is *NOT* tested with MSI-X capable port because I don't
have any platform with MSI-X capable port as I mentioned above (I
confirmed MSI and INTx work with this patch).
Thanks,
Kenji Kaneshige
######################################################################
THIS IS A SAMPLE PATCH
THIS IS *NOT* TESTED ON THE MACHINE WITH MSI-X CAPABLE PORT
######################################################################
Fix MSI-X configuration code in PCIe port services.
I think current MSI-X configuration code in PCIe port service driver
has at least the following problems.
- To setup MSI-X for PCIe port services, we need to know which MSI-X
table entries are used for which services. For example, MSI-X entry
for Hot Plug and PME is indicated by Interrupt Message Number field
in the PCI Express Capabilities register, and MSI-X entry for
Advanced Error Reporting is indicated by Root Error Status register.
But current PCIe port service driver core is doing nothing about
that, and it would cause unexpected interrupt behaviour.
- Current PCIe port service driver tries to allocate vectors for Hot
Plug, PME, AER and VC individually. So PCIe port service driver
would reqest four vectors in muximum. But it should never be four
because Hot Plug and PME always share the interrupt. In addition,
I'm wondering if VC needs vectors, though I don't know about VC
very much.
Signed-off-by: Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>
---
drivers/pci/pcie/portdrv_core.c | 110 +++++++++++++++++++++++++++-------------
1 file changed, 75 insertions(+), 35 deletions(-)
Index: 20081217/drivers/pci/pcie/portdrv_core.c
===================================================================
--- 20081217.orig/drivers/pci/pcie/portdrv_core.c
+++ 20081217/drivers/pci/pcie/portdrv_core.c
@@ -127,51 +127,91 @@ static int is_msi_quirked(struct pci_dev
}
return quirk;
}
+
+static int port_msix_init(struct pci_dev *dev, int *vectors, int capabilities)
+{
+ int i, err, idx, idx_hppme = -1, idx_aer = -1;
+ struct msix_entry msix_entries[PCIE_PORT_DEVICE_MAXSERVICES];
+
+ if (!pci_find_capability(dev, PCI_CAP_ID_MSIX))
+ return -ENODEV;
+ /*
+ * We need to find out which MSI-X entries are used for which
+ * services before calling pci_enable_msix(). On the other
+ * hand, we need to enable MSI-X using pci_enable_msix() to
+ * find out this information. Against this dilemma, enabling
+ * MSI-X using pci_enable_msix() with a dummy msix_entry here.
+ */
+ msix_entries[0].entry = 0;
+ if ((err = pci_enable_msix(dev, msix_entries, 1)))
+ return err;
+ /* Find out which MSI-X entries are used for which services */
+ idx = 0;
+ if (capabilities & (PCIE_PORT_SERVICE_HP | PCIE_PORT_SERVICE_PME)) {
+ u16 reg16;
+ int pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
+ pci_read_config_word(dev, pos + PCIE_CAPABILITIES_REG, ®16);
+ msix_entries[idx].entry = (reg16 >> 9) & 0x1f;
+ idx_hppme = idx++;
+ }
+ if (capabilities & PCIE_PORT_SERVICE_AER) {
+ u32 reg32;
+ int pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+ pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, ®32);
+ msix_entries[idx].entry = (reg32 >> 27);
+ idx_aer = idx++;
+ }
+ /* Disable dummy MSI-X setting */
+ pci_disable_msix(dev);
+ /* Enable MSI-X with proper msix_entries */
+ if ((err = pci_enable_msix(dev, msix_entries, idx)))
+ return err;
+ for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
+ int service = (1 << i);
+ if (!(capabilities & service))
+ continue;
+ if (service & (PCIE_PORT_SERVICE_HP | PCIE_PORT_SERVICE_PME))
+ vectors[i] = msix_entries[idx_hppme].vector;
+ else if (service & PCIE_PORT_SERVICE_AER)
+ vectors[i] = msix_entries[idx_aer].vector;
+ else
+ vectors[i] = 0;
+ }
+ return 0;
+}
+
+static int port_msi_init(struct pci_dev *dev, int *vectors, int capabilities)
+{
+ int err, i;
+ if (!pci_find_capability(dev, PCI_CAP_ID_MSI))
+ return -ENODEV;
+ if ((err = pci_enable_msi(dev)))
+ return err;
+ for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
+ vectors[i] = dev->irq;
+ return 0;
+}
static int assign_interrupt_mode(struct pci_dev *dev, int *vectors, int mask)
{
- int i, pos, nvec, status = -EINVAL;
- int interrupt_mode = PCIE_PORT_INTx_MODE;
+ int i;
/* Set INTx as default */
- for (i = 0, nvec = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
- if (mask & (1 << i))
- nvec++;
+ for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
vectors[i] = dev->irq;
- }
/* Check MSI quirk */
if (is_msi_quirked(dev))
- return interrupt_mode;
+ return PCIE_PORT_INTx_MODE;
- /* Select MSI-X over MSI if supported */
- pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
- if (pos) {
- struct msix_entry msix_entries[PCIE_PORT_DEVICE_MAXSERVICES] =
- {{0, 0}, {0, 1}, {0, 2}, {0, 3}};
- status = pci_enable_msix(dev, msix_entries, nvec);
- if (!status) {
- int j = 0;
-
- interrupt_mode = PCIE_PORT_MSIX_MODE;
- for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
- if (mask & (1 << i))
- vectors[i] = msix_entries[j++].vector;
- }
- }
- }
- if (status) {
- pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
- if (pos) {
- status = pci_enable_msi(dev);
- if (!status) {
- interrupt_mode = PCIE_PORT_MSI_MODE;
- for (i = 0;i < PCIE_PORT_DEVICE_MAXSERVICES;i++)
- vectors[i] = dev->irq;
- }
- }
- }
- return interrupt_mode;
+ /* Select MSI-X over MSI if supported */
+ if (!port_msix_init(dev, vectors, mask))
+ return PCIE_PORT_MSIX_MODE;
+
+ if (!port_msi_init(dev, vectors, mask))
+ return PCIE_PORT_MSI_MODE;
+
+ return PCIE_PORT_INTx_MODE;
}
static int get_port_device_capability(struct pci_dev *dev)
--
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
