Question on using MSI in PCI driver

I'm looking at implementing MSI/MSI-X support in a PCI device driver I'm working on. However, I've run into an issue with the MSI API that I would like some clarification on.

When I call pci_enable_msi, since my device is MSI-X capable, the kernel calls msix_capability_init, which works out the memory region where vectors should be written and then calls request_region. (In fact it calls

request_mem_region(phys_addr,
dev_msi_cap * PCI_MSIX_ENTRY_SIZE, "MSI-X iomap Failure"))

which leads to a bizarre entry in /proc/iomem with the name "MSI-X iomap Failure")

The problem is that if I follow the standard route in my driver and call pci_request_regions() during init (since I want to claim my whole device), the request_mem_region in msix_capability_init will fail. Now, for my device, the MSI-X table happens to fall in the middle of a BAR, and I need to access stuff on both sides of it in that BAR. To make things even worse for me, my device has two more BARs I want to claim.

So it seems I am forced to turn my nice clean pci_request_regions() call into two calls to request_mem_region() (to get the beginning and end of the BAR with the MSI-X table in it) and two more calls to pci_request_region() (to get the other two BARs).

This isn't the end of the world but it feels suboptimal to me. Anyone have an idea for a better way to do this? (I'm happy to write a patch to the kernel if someone suggests how to change the MSI API)

Thanks,
Roland

Not answering your question directly, but...

You are breaking new ground by adding MSI support to a driver. I thank you for this -- alot -- but you should realize there will probably be a little bit of PCI core work necessary in order to get things the way you want them.

Feel free to propose changes to the PCI core to accomodate your MSI driver.

Jeff

Jeff> You are breaking new ground by adding MSI support to a Jeff> driver. I thank you for this -- alot -- but you should Jeff> realize there will probably be a little bit of PCI core work Jeff> necessary in order to get things the way you want them.

Yes, I noticed only a couple of hotplug drivers seem to be using MSI, and no one is using multiple vector support. My motivation is twofold. First, because my device can generate interrupts for different reasons, and I'm noticing that doing the MMIO read to the cause register is taking a lot of time, so I'm wondering whether avoiding this by having separate MSI-X messages will be a win. Second, for the fun of trying it :)

Jeff> Feel free to propose changes to the PCI core to accomodate Jeff> your MSI driver.

First patch coming up now :)

- Roland

Cool. If you are feeling generous or motivated, update Documentation/pci.txt too ;-)

Sharing your experiences will save time for others.

Jeff

As a followup to my previous post about the request_mem_region in msi.c, I noticed that the region is only released in msi_remove_pci_irq_vectors(). Based on the fact that this function is declared in linux/pci.h (and stubbed out if CONFIG_PCI_USE_VECTOR is not defined), I'm guessing that the intent is for a device driver to unconditionally call this when exiting.

However, a module can't call msi_remove_pci_irq_vectors unless the symbol is exported... so if this is the way to do things, please apply this patch.

On the other hand, MSI-HOWTO.txt seems to imply that the 0th MSI vector should be cleaned up just by calling free_irq... so should pci_disable_msi be calling msi_remove_pci_irq_vectors?

- Roland

Index: linux-2.6.7/drivers/pci/msi.c
===================================================================
@@ -1011,3 +1011,4 @@
 EXPORT_SYMBOL(pci_enable_msi);
 EXPORT_SYMBOL(msi_alloc_vectors);
 EXPORT_SYMBOL(msi_free_vectors);
+EXPORT_SYMBOL(msi_remove_pci_irq_vectors);

Jeff> Cool. If you are feeling generous or motivated, update Jeff> Documentation/pci.txt too ;-)

Will do, once I know what I'm doing :)

- R.

msix_capability_init() puts the offset of the MSI-X capability into pos, then uses pos as a loop index to clear the MSI-X vector table, and then tries to use pos as the offset again, which results in writing the MSI-X enable bit off into space.

This patch fixes that by adding a new loop index variable and using that to clear the vector table.

- Roland

Index: linux-2.6.7/drivers/pci/msi.c
===================================================================
@@ -569,7 +569,7 @@
 	struct msi_desc *entry;
 	struct msg_address address;
 	struct msg_data data;
-	int vector = 0, pos, dev_msi_cap;
+	int vector = 0, pos, dev_msi_cap, i;
 	u32 phys_addr, table_offset;
 	u32 control;
 	u8 bir;

@@ -629,12 +629,12 @@
 	writel(address.hi_address, base + PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET);
 	writel(*(u32*)&data, base + PCI_MSIX_ENTRY_DATA_OFFSET);
 	/* Initialize all entries from 1 up to 0 */
-	for (pos = 1; pos < dev_msi_cap; pos++) {
-		writel(0, base + pos * PCI_MSIX_ENTRY_SIZE +
+	for (i = 1; i < dev_msi_cap; i++) {
+		writel(0, base + i * PCI_MSIX_ENTRY_SIZE +
 			PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
-		writel(0, base + pos * PCI_MSIX_ENTRY_SIZE +
+		writel(0, base + i * PCI_MSIX_ENTRY_SIZE +
 			PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET);
-		writel(0, base + pos * PCI_MSIX_ENTRY_SIZE +
+		writel(0, base + i * PCI_MSIX_ENTRY_SIZE +
 			PCI_MSIX_ENTRY_DATA_OFFSET);
 	}
 	attach_msi_entry(entry, vector);

I think so.

Applied, thanks.

greg k-h

Greg> Applied, thanks.

Great... I would suggest looking at applying the big patch that Long posted today, too, as it has lots of improvements.

- Roland

Yes, I saw it, but it's just out for review, not ready to apply just yet. :)

thanks,

greg k-h