PCI: Provide sensible irq vector alloc/free routines

Add a new pci_alloc_irq_vectors helper that allocates MSI-X or multi-MSI vectors for PCI device while isolating the driver from the arcane details.

This include handling both MSI-X, MSI and legacy interrupt fallbacks transparently, automatic capping to the available vectors as well as storing the information needed for request_irq in the PCI device itself so that a lot of boiler plate code in the driver can be removed.

In the future this will also allow us to automatically set up spreading for interrupt vectors without having to duplicate it in all the drivers.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  19 +++++++++
 2 files changed, 127 insertions(+)

drivers/pci/msi.c +38 -0

index a080f44..a510484 100644
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2003-2004 Intel
  * Copyright (C) Tom Long Nguyen (tom.l.nguyen at intel.com)
+ * Copyright (c) 2016 Christoph Hellwig.
  */
 
 #include <linux/err.h>

@@ -1120,6 +1121,113 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
 }
 EXPORT_SYMBOL(pci_enable_msix_range);
 
+static int __pci_enable_msix(struct pci_dev *dev, int nr_vecs)
+{
+	struct msix_entry *msix_entries;
+	int ret, i;
+
+	msix_entries = kcalloc(nr_vecs, sizeof(struct msix_entry), GFP_KERNEL);
+	if (!msix_entries)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_vecs; i++)
+		msix_entries[i].entry = i;
+
+	ret = msix_capability_init(dev, msix_entries, nr_vecs);
+	if (ret == 0) {
+		for (i = 0; i < nr_vecs; i++)
+			dev->irqs[i] = msix_entries[i].vector;
+	}
+
+	kfree(msix_entries);
+	return ret;
+}
+
+static int __pci_enable_msi(struct pci_dev *dev, int nr_vecs)
+{
+	int ret, i;
+
+	ret = msi_capability_init(dev, nr_vecs);
+	if (ret == 0) {
+		for (i = 0; i < nr_vecs; i++)
+			dev->irqs[i] = dev->irq + i;
+	}
+
+	return ret;
+}
+
+/**
+ * pci_alloc_irq_vectors - allocate multiple IRQs for a device

drivers/pci/msi.c +60 -0

+ */
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags)
+{
+	unsigned int ret;
+
+	if (WARN_ON_ONCE(dev->msi_enabled || dev->msix_enabled))
+		return -EINVAL;
+
+	if (!pci_msi_supported(dev, 1))
+		goto use_legacy_irq;
+
+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msix_vec_count(dev));
+	else if (dev->msi_cap)
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msi_vec_count(dev));
+	else
+		goto use_legacy_irq;
+
+	dev->irqs = kcalloc(nr_vecs, sizeof(u32), GFP_KERNEL);
+	if (!dev->irqs)
+		return -ENOMEM;
+
+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		ret = __pci_enable_msix(dev, nr_vecs);
+	else
+		ret = __pci_enable_msi(dev, nr_vecs);
+	if (ret)
+		goto out_free_irqs;
+
+	return 0;
+
+out_free_irqs:
+	kfree(dev->irqs);
+use_legacy_irq:
+	dev->irqs = &dev->irq;
+	return 1;
+}
+EXPORT_SYMBOL(pci_alloc_irq_vectors);
+
+/**
+ * pci_free_irq_vectors - free previously allocated IRQs for a device
+ * @dev:		PCI device to operate on
+ *
+ * Undoes the allocations and enabling in pci_alloc_irq_vectors().
+ */
+void pci_free_irq_vectors(struct pci_dev *dev)
+{
+	if (dev->msix_enabled)
+		pci_disable_msix(dev);
+	else if (dev->msi_enabled)
+		pci_disable_msi(dev);
+
+	if (dev->irqs != &dev->irq)
+		kfree(dev->irqs);
+	dev->irqs = NULL;
+}
+EXPORT_SYMBOL(pci_free_irq_vectors);
+
+
 struct pci_dev *msi_desc_to_pci_dev(struct msi_desc *desc)
 {
 	return to_pci_dev(desc->dev);

include/linux/pci.h +19 -0

index 932ec74..e201d0d 100644
@@ -322,6 +322,7 @@ struct pci_dev {
 	 * directly, use the values stored here. They might be different!
 	 */
 	unsigned int	irq;
+	unsigned int	*irqs;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
 
 	bool match_driver;		/* Skip attaching driver */

@@ -1255,6 +1256,8 @@ struct msix_entry {
 	u16	entry;	/* driver uses to specify entry, OS writes */
 };
 
+#define PCI_IRQ_NOMSIX		(1 << 0) /* don't try to use MSI-X interrupts */
+
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);

@@ -1283,6 +1286,10 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
 		return rc;
 	return 0;
 }
+
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags);
+void pci_free_irq_vectors(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) { }

@@ -1306,6 +1313,18 @@ static inline int pci_enable_msix_range(struct pci_dev *dev,
 static inline int pci_enable_msix_exact(struct pci_dev *dev,
 		      struct msix_entry *entries, int nvec)
 { return -ENOSYS; }
+
+static inline int pci_alloc_irq_vectors(struct pci_dev *dev,
+		unsigned int nr_vecs, unsigned int flags)
+{
+	dev->irqs = &dev->irq;
+	return 1;
+}
+
+static inline void pci_free_irq_vectors(struct pci_dev *dev)
+{
+	dev->irqs = NULL;
+}
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS

Signature · 1 line

2.1.4

I like this a lot. One relatively minor comment below.

Quoted 1 line · Bjorn Helgaas

I like this a lot. One relatively minor comment below.

Do you want this addressed? The problem is that I probably will also need something like this flags argument for introducing the irq spreading as we can't simply do it for all callers directly. If you take the patch as-is I'd be happy to do another pass later to remove it and look into different no-MSI quirking once we're done with the MSI-X spreading work.

I don't know what sort of clues you'll need for IRQ spreading, but that sounds like a good reason for the argument. So if you will need the flags argument for something else, then I think it's OK as-is.

Add a new pci_alloc_irq_vectors helper that allocates MSI-X or multi-MSI vectors for PCI device while isolating the driver from the arcane details.

This include handling both MSI-X, MSI and legacy interrupt fallbacks transparently, automatic capping to the available vectors as well as storing the information needed for request_irq in the PCI device itself so that a lot of boiler plate code in the driver can be removed.

In the future this will also allow us to automatically set up spreading for interrupt vectors without having to duplicate it in all the drivers.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  19 +++++++++
 2 files changed, 127 insertions(+)

drivers/pci/msi.c +108 -0

index a080f44..a510484 100644
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2003-2004 Intel
  * Copyright (C) Tom Long Nguyen (tom.l.nguyen at intel.com)
+ * Copyright (c) 2016 Christoph Hellwig.
  */
 
 #include <linux/err.h>

@@ -1120,6 +1121,113 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
 }
 EXPORT_SYMBOL(pci_enable_msix_range);
 
+static int __pci_enable_msix(struct pci_dev *dev, int nr_vecs)
+{
+	struct msix_entry *msix_entries;
+	int ret, i;
+
+	msix_entries = kcalloc(nr_vecs, sizeof(struct msix_entry), GFP_KERNEL);
+	if (!msix_entries)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_vecs; i++)
+		msix_entries[i].entry = i;
+
+	ret = msix_capability_init(dev, msix_entries, nr_vecs);
+	if (ret == 0) {
+		for (i = 0; i < nr_vecs; i++)
+			dev->irqs[i] = msix_entries[i].vector;
+	}
+
+	kfree(msix_entries);
+	return ret;
+}
+
+static int __pci_enable_msi(struct pci_dev *dev, int nr_vecs)
+{
+	int ret, i;
+
+	ret = msi_capability_init(dev, nr_vecs);
+	if (ret == 0) {
+		for (i = 0; i < nr_vecs; i++)
+			dev->irqs[i] = dev->irq + i;
+	}
+
+	return ret;
+}
+
+/**
+ * pci_alloc_irq_vectors - allocate multiple IRQs for a device
+ * @dev:		PCI device to operate on
+ * @nr_vecs:		number of vectors to operate on
+ * @flags:		flags or quirks for the allocation
+ *
+ * Allocate @nr_vecs interrupt vectors for @dev, using MSI-X or MSI
+ * vectors if available, and fall back to a single legacy vector
+ * if neither is available.  Return the number of vectors allocated
+ * (which might be smaller than @nr_vecs) if successful, or a negative
+ * error code on error.  The Linux irq numbers for the allocated
+ * vectors are stored in pdev->irqs.
+ */
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags)
+{
+	unsigned int ret;
+
+	if (WARN_ON_ONCE(dev->msi_enabled || dev->msix_enabled))
+		return -EINVAL;
+
+	if (!pci_msi_supported(dev, 1))
+		goto use_legacy_irq;
+
+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msix_vec_count(dev));
+	else if (dev->msi_cap)
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msi_vec_count(dev));
+	else
+		goto use_legacy_irq;
+
+	dev->irqs = kcalloc(nr_vecs, sizeof(u32), GFP_KERNEL);
+	if (!dev->irqs)
+		return -ENOMEM;
+
+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		ret = __pci_enable_msix(dev, nr_vecs);
+	else
+		ret = __pci_enable_msi(dev, nr_vecs);
+	if (ret)
+		goto out_free_irqs;
+
+	return 0;
+
+out_free_irqs:
+	kfree(dev->irqs);
+use_legacy_irq:
+	dev->irqs = &dev->irq;
+	return 1;
+}
+EXPORT_SYMBOL(pci_alloc_irq_vectors);
+
+/**
+ * pci_free_irq_vectors - free previously allocated IRQs for a device
+ * @dev:		PCI device to operate on
+ *
+ * Undoes the allocations and enabling in pci_alloc_irq_vectors().
+ */
+void pci_free_irq_vectors(struct pci_dev *dev)
+{
+	if (dev->msix_enabled)
+		pci_disable_msix(dev);
+	else if (dev->msi_enabled)
+		pci_disable_msi(dev);
+
+	if (dev->irqs != &dev->irq)
+		kfree(dev->irqs);
+	dev->irqs = NULL;
+}
+EXPORT_SYMBOL(pci_free_irq_vectors);
+
+
 struct pci_dev *msi_desc_to_pci_dev(struct msi_desc *desc)
 {
 	return to_pci_dev(desc->dev);

include/linux/pci.h +19 -0

index 932ec74..e201d0d 100644
@@ -322,6 +322,7 @@ struct pci_dev {
 	 * directly, use the values stored here. They might be different!
 	 */
 	unsigned int	irq;
+	unsigned int	*irqs;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
 
 	bool match_driver;		/* Skip attaching driver */

@@ -1255,6 +1256,8 @@ struct msix_entry {
 	u16	entry;	/* driver uses to specify entry, OS writes */
 };
 
+#define PCI_IRQ_NOMSIX		(1 << 0) /* don't try to use MSI-X interrupts */
+
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);

@@ -1283,6 +1286,10 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
 		return rc;
 	return 0;
 }
+
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags);
+void pci_free_irq_vectors(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) { }

@@ -1306,6 +1313,18 @@ static inline int pci_enable_msix_range(struct pci_dev *dev,
 static inline int pci_enable_msix_exact(struct pci_dev *dev,
 		      struct msix_entry *entries, int nvec)
 { return -ENOSYS; }
+
+static inline int pci_alloc_irq_vectors(struct pci_dev *dev,
+		unsigned int nr_vecs, unsigned int flags)
+{
+	dev->irqs = &dev->irq;
+	return 1;
+}
+
+static inline void pci_free_irq_vectors(struct pci_dev *dev)
+{
+	dev->irqs = NULL;
+}
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS

Signature · 1 line

2.1.4

Thanks, I applied this to pci/msi for v4.7. I'd be happy to apply the nvme change, too, given the appropriate acks.

Quoted 8 lines · Christoph Hellwig

Add a new pci_alloc_irq_vectors helper that allocates MSI-X or multi-MSI vectors for PCI device while isolating the driver from the arcane details.

This include handling both MSI-X, MSI and legacy interrupt fallbacks transparently, automatic capping to the available vectors as well as storing the information needed for request_irq in the PCI device itself so that a lot of boiler plate code in the driver can be removed.

Quoted 4 lines

In the future this will also allow us to automatically set up spreading for interrupt vectors without having to duplicate it in all the drivers.

Signed-off-by Christoph Hellwig <hch at lst.de>

Thanks, I applied this to pci/msi for v4.7.

Add a new pci_alloc_irq_vectors helper that allocates MSI-X or multi-MSI vectors for PCI device while isolating the driver from the arcane details.

This include handling both MSI-X, MSI and legacy interrupt fallbacks transparently, automatic capping to the available vectors as well as storing the information needed for request_irq in the PCI device itself so that a lot of boiler plate code in the driver can be removed.

In the future this will also allow us to automatically set up spreading for interrupt vectors without having to duplicate it in all the drivers.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  19 +++++++++
 2 files changed, 127 insertions(+)

drivers/pci/msi.c +65 -0

index a080f44..a510484 100644
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2003-2004 Intel
  * Copyright (C) Tom Long Nguyen (tom.l.nguyen at intel.com)
+ * Copyright (c) 2016 Christoph Hellwig.
  */
 
 #include <linux/err.h>

@@ -1120,6 +1121,113 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
 }
 EXPORT_SYMBOL(pci_enable_msix_range);
 
+static int __pci_enable_msix(struct pci_dev *dev, int nr_vecs)
+{
+	struct msix_entry *msix_entries;
+	int ret, i;
+
+	msix_entries = kcalloc(nr_vecs, sizeof(struct msix_entry), GFP_KERNEL);
+	if (!msix_entries)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_vecs; i++)
+		msix_entries[i].entry = i;
+
+	ret = msix_capability_init(dev, msix_entries, nr_vecs);
+	if (ret == 0) {
+		for (i = 0; i < nr_vecs; i++)
+			dev->irqs[i] = msix_entries[i].vector;
+	}
+
+	kfree(msix_entries);
+	return ret;
+}
+
+static int __pci_enable_msi(struct pci_dev *dev, int nr_vecs)
+{
+	int ret, i;
+
+	ret = msi_capability_init(dev, nr_vecs);
+	if (ret == 0) {
+		for (i = 0; i < nr_vecs; i++)
+			dev->irqs[i] = dev->irq + i;
+	}
+
+	return ret;
+}
+
+/**
+ * pci_alloc_irq_vectors - allocate multiple IRQs for a device
+ * @dev:		PCI device to operate on
+ * @nr_vecs:		number of vectors to operate on
+ * @flags:		flags or quirks for the allocation
+ *
+ * Allocate @nr_vecs interrupt vectors for @dev, using MSI-X or MSI
+ * vectors if available, and fall back to a single legacy vector
+ * if neither is available.  Return the number of vectors allocated
+ * (which might be smaller than @nr_vecs) if successful, or a negative
+ * error code on error.  The Linux irq numbers for the allocated
+ * vectors are stored in pdev->irqs.
+ */
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags)
+{
+	unsigned int ret;
+
+	if (WARN_ON_ONCE(dev->msi_enabled || dev->msix_enabled))
+		return -EINVAL;
+
+	if (!pci_msi_supported(dev, 1))
+		goto use_legacy_irq;
+
+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msix_vec_count(dev));
+	else if (dev->msi_cap)
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msi_vec_count(dev));
+	else

drivers/pci/msi.c +33 -0

+	if (ret)
+		goto out_free_irqs;
+
+	return 0;
+
+out_free_irqs:
+	kfree(dev->irqs);
+use_legacy_irq:
+	dev->irqs = &dev->irq;
+	return 1;
+}
+EXPORT_SYMBOL(pci_alloc_irq_vectors);
+
+/**
+ * pci_free_irq_vectors - free previously allocated IRQs for a device
+ * @dev:		PCI device to operate on
+ *
+ * Undoes the allocations and enabling in pci_alloc_irq_vectors().
+ */
+void pci_free_irq_vectors(struct pci_dev *dev)
+{
+	if (dev->msix_enabled)
+		pci_disable_msix(dev);
+	else if (dev->msi_enabled)
+		pci_disable_msi(dev);
+
+	if (dev->irqs != &dev->irq)
+		kfree(dev->irqs);
+	dev->irqs = NULL;
+}
+EXPORT_SYMBOL(pci_free_irq_vectors);
+
+
 struct pci_dev *msi_desc_to_pci_dev(struct msi_desc *desc)
 {
 	return to_pci_dev(desc->dev);

include/linux/pci.h +19 -0

index 932ec74..e201d0d 100644
@@ -322,6 +322,7 @@ struct pci_dev {
 	 * directly, use the values stored here. They might be different!
 	 */
 	unsigned int	irq;
+	unsigned int	*irqs;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
 
 	bool match_driver;		/* Skip attaching driver */

@@ -1255,6 +1256,8 @@ struct msix_entry {
 	u16	entry;	/* driver uses to specify entry, OS writes */
 };
 
+#define PCI_IRQ_NOMSIX		(1 << 0) /* don't try to use MSI-X interrupts */
+
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);

@@ -1283,6 +1286,10 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
 		return rc;
 	return 0;
 }
+
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags);
+void pci_free_irq_vectors(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) { }

@@ -1306,6 +1313,18 @@ static inline int pci_enable_msix_range(struct pci_dev *dev,
 static inline int pci_enable_msix_exact(struct pci_dev *dev,
 		      struct msix_entry *entries, int nvec)
 { return -ENOSYS; }
+
+static inline int pci_alloc_irq_vectors(struct pci_dev *dev,
+		unsigned int nr_vecs, unsigned int flags)
+{
+	dev->irqs = &dev->irq;
+	return 1;
+}
+
+static inline void pci_free_irq_vectors(struct pci_dev *dev)
+{
+	dev->irqs = NULL;
+}
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS

Signature · 1 line

2.1.4

Moreover, patch 2/2 not only removed pci_enable_msi[x]_range() internal fallback logic (from desired number of interrupts to available number of interrupts), but it also removed MSI-X to MSI fallback.

Quoted 6 lines

drivers/pci/msi.c +6 -0

+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msix_vec_count(dev));
+	else if (dev->msi_cap)
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msi_vec_count(dev));
+	else
+		goto use_legacy_irq;

drivers/pci/msi.c int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, · line 1197

Quoted 4 lines

drivers/pci/msi.c +4 -0

+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		ret = __pci_enable_msix(dev, nr_vecs);
+	else
+		ret = __pci_enable_msi(dev, nr_vecs);

1. No fallbacks.

Hi Alexander,

Quoted 4 lines

Moreover, patch 2/2 not only removed pci_enable_msi[x]_range() internal fallback logic (from desired number of interrupts to available number of interrupts), but it also removed MSI-X to MSI fallback.

That is done in the very begining of the function, see the quoted part of the patch just below:

Quoted 6 lines

drivers/pci/msi.c int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, · line 1188

Quoted 6 lines

drivers/pci/msi.c +6 -0

+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msix_vec_count(dev));
+	else if (dev->msi_cap)
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msi_vec_count(dev));
+	else
+		goto use_legacy_irq;

Quoted 6 lines

drivers/pci/msi.c int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, · line 1197

Quoted 4 lines

drivers/pci/msi.c +4 -0

+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		ret = __pci_enable_msix(dev, nr_vecs);
+	else
+		ret = __pci_enable_msi(dev, nr_vecs);

1. No fallbacks.

I read through the code in msi.c in detail and could not find a legitimate case where we have msix_cap but actually fail the MSI-X allocation. If that is a valid case I can happily add the fallback here as well.

Hi Alex,

what do you think about the incremental patch below? This should address the concerns about the strange PPC bridges, although I don't have a way to test one:

drivers/pci/msi.c +0 -0

index a510484..32ce65e 100644

drivers/pci/msi.c +6 -5

 	if (!pci_msi_supported(dev, 1))
 		goto use_legacy_irq;
 

@@ -1191,17 +1192,26 @@ int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
 	if (!dev->irqs)
 		return -ENOMEM;
 
-	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX)) {
 		ret = __pci_enable_msix(dev, nr_vecs);
-	else
+		if (ret < 0)
+			flags |= PCI_IRQ_NOMSIX;
+	} else {
 		ret = __pci_enable_msi(dev, nr_vecs);
-	if (ret)
-		goto out_free_irqs;
+	}
 
-	return 0;
+	/* if we succeeded getting all vectors return the number we got: */

drivers/pci/msi.c +2 -0

+
+	/* no MSI or MSI-X vectors available, fall back to the legacy IRQ: */
 use_legacy_irq:
 	dev->irqs = &dev->irq;
 	return 1;

Quoted 14 lines · Christoph Hellwig

Hi Alex,

what do you think about the incremental patch below? This should address the concerns about the strange PPC bridges, although I don't have a way to test one:

drivers/pci/msi.c +0 -0

index a510484..32ce65e 100644
@@ -1177,6 +1177,7 @@ int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
 	if (WARN_ON_ONCE(dev->msi_enabled || dev->msix_enabled))
 		return -EINVAL;
 

drivers/pci/msi.c int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs, · line 1180

Quoted 1 line

drivers/pci/msi.c +1 -0

+retry:

A retry does not seem needs checking MSI support each time, nor reading MSI header info like number of vectors (not visible in this patch).

Quoted 4 lines

drivers/pci/msi.c +2 -1

-out_free_irqs:
 	kfree(dev->irqs);
+	/* if ret is positive it's the numbers of vectors we can use, retry: */
+	if (ret > 0) {

drivers/pci/msi.c int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs, · line 1212

Quoted 3 lines

drivers/pci/msi.c +3 -0

+		nr_vecs = ret;
+		goto retry;
+	}

Okay, now we have a subtlety here. You switch from MSI-X to MSI in case MSI-X retries exhausted. At this point nr_vecs is lowest (likely 1). So you start trying MSI from 1 rather from 32. I would suggest two subsequent loops instead.

Another thing, pci_alloc_irq_vectors() tries to allocate vectors in a range from 1 to nr_vecs now. So this function implicitly falls into the other two range functions family and therefore:

(a) pci_alloc_irq_vectors() name is not perfec;

(b) why not introduce 'minvec' minimal number of interrupts then?

      We could have a handy pci_enable_irq_range() as result;

(c) if you do (b) then PCI_IRQ_NOMSIX flag becomes redundant, since

      caller would invoke pci_enable_msi_range() instead;

Quoted 4 lines

Okay, now we have a subtlety here. You switch from MSI-X to MSI in case MSI-X retries exhausted. At this point nr_vecs is lowest (likely 1). So you start trying MSI from 1 rather from 32. I would suggest two subsequent loops instead.

So bridges could support less MSI-X entries than MSI ones? Yikes.

Quoted 5 lines

Another thing, pci_alloc_irq_vectors() tries to allocate vectors in a range from 1 to nr_vecs now. So this function implicitly falls into the other two range functions family and therefore:

(a) pci_alloc_irq_vectors() name is not perfec;

What would you call it instead?

Quoted 2 lines

(b) why not introduce 'minvec' minimal number of interrupts then?

      We could have a handy pci_enable_irq_range() as result;

That seems pretty pointless, when the caller can simply treat a too small number as failure and use the existing failure path for that.

Quoted 2 lines

(c) if you do (b) then PCI_IRQ_NOMSIX flag becomes redundant, since

      caller would invoke pci_enable_msi_range() instead;

pci_enable_msi_range still has a horrible API that forces the caller to deal with the irq numbers differently than the MSI-X case, so it should also go away in the long run.

Not this way. MSI vectors could be a scarce resource in a platform. So even though devices could support more MSI-Xs than MSIs the underlying platform might fail to fulfil a device request.

Quoted 7 lines

Quoted 5 lines

Another thing, pci_alloc_irq_vectors() tries to allocate vectors in a range from 1 to nr_vecs now. So this function implicitly falls into the other two range functions family and therefore:

(a) pci_alloc_irq_vectors() name is not perfec;

What would you call it instead?

I do not know, really :( I would expect "range" within the name since a range requested indeed, but I am just hinting here.

Quoted 5 lines

Quoted 2 lines

(b) why not introduce 'minvec' minimal number of interrupts then?

      We could have a handy pci_enable_irq_range() as result;

That seems pretty pointless, when the caller can simply treat a too small number as failure and use the existing failure path for that.

There was a huge discussion on this few years ago, when the range functions were introduced. Actually, the prototypes of these two is the outcome of that discussion. I almost sure your point was expressed by many at the time ;)

Quoted 3 lines

pci_enable_msi_range still has a horrible API that forces the caller to deal with the irq numbers differently than the MSI-X case, so it should also go away in the long run.

Well, if we introduce pci_enable_irq_range() (or something) and pci_get_dev_irq(int vector) (or something) that covers MSI-X, MSI and legacy IRQs then we can get it done now. Your pci_alloc_irq_vectors() is just few steps from there, huh?

Alexander, what do you think about the version below? Survived quick testing with nvme so far.

drivers/pci/msi.c +7 -2

+		return -EINVAL;
+	max_vecs = min_t(unsigned int, max_vecs, nr_vecs);
 
-	for (i = 0; i < nr_vecs; i++)
-		msix_entries[i].entry = i;
+	nr_vecs = pci_enable_msi_range(dev, min_vecs, max_vecs);
+	if (nr_vecs > 1) {
+		dev->irqs = kcalloc(nr_vecs, sizeof(u32), GFP_KERNEL);
+		if (!dev->irqs) {
+			pci_disable_msi(dev);

drivers/pci/msi.c +17 -0

+		return -EINVAL;
+	max_vecs = min_t(unsigned int, max_vecs, nr_vecs);
+
+	msix_entries = kcalloc(max_vecs, sizeof(struct msix_entry), GFP_KERNEL);
+	if (!msix_entries)
+		return -ENOMEM;
+
+	for (i = 0; i < max_vecs; i++)
+		msix_entries[i].entry = i;
+
+	nr_vecs = pci_enable_msix_range(dev, msix_entries, min_vecs, max_vecs);
+	if (nr_vecs < 0)
+		goto out_free_entries;
+
+	dev->irqs = kcalloc(nr_vecs, sizeof(u32), GFP_KERNEL);
+	if (!dev->irqs) {
+		pci_disable_msix(dev);

drivers/pci/msi.c +11 -8

  * @dev:		PCI device to operate on
- * @nr_vecs:		number of vectors to operate on
+ * @min_vecs:		minimum vectors required
+ * @max_vecs:		maximum vectors requested
  * @flags:		flags or quirks for the allocation
  *
- * Allocate @nr_vecs interrupt vectors for @dev, using MSI-X or MSI
- * vectors if available, and fall back to a single legacy vector
- * if neither is available.  Return the number of vectors allocated
- * (which might be smaller than @nr_vecs) if successful, or a negative
- * error code on error.  The Linux irq numbers for the allocated
- * vectors are stored in pdev->irqs.
+ * Allocate up to @max_vecs interrupt vectors for @dev, using MSI-X or MSI
+ * vectors if available, and fall back to a single legacy vector if neither
+ * is available.  Return the number of vectors allocated (which might be
+ * smaller than @max_vecs) if successful, or a negative error code on error.
+ *
+ * The Linux irq numbers for the allocated vectors are stored in pdev->irqs.
+ *
+ * If @min_vecs is set to a number bigger than zero the function will fail
+ * with -%ENOSPC if les than @min_vecs vectors are available.
  */
-int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,

drivers/pci/msi.c +0 -13

-	if (!pci_msi_supported(dev, 1))
-		goto use_legacy_irq;
-
-	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
-		nr_vecs = min_t(unsigned int, nr_vecs, pci_msix_vec_count(dev));
-	else if (dev->msi_cap)
-		nr_vecs = min_t(unsigned int, nr_vecs, pci_msi_vec_count(dev));
-	else
-		goto use_legacy_irq;
-
-	dev->irqs = kcalloc(nr_vecs, sizeof(u32), GFP_KERNEL);
-	if (!dev->irqs)
-		return -ENOMEM;

drivers/pci/msi.c +6 -3

+			int ret = __pci_enable_msi(dev, min_vecs, max_vecs);
+			if (ret > 0)
+				return ret;
+		}
+	}
 
-out_free_irqs:
-	kfree(dev->irqs);
-use_legacy_irq:
+	/* no MSI or MSI-X vectors available, use the legacy IRQ: */
 	dev->irqs = &dev->irq;

drivers/pci/msi.c +2 -1

 /**
  * pci_free_irq_vectors - free previously allocated IRQs for a device
  * @dev:		PCI device to operate on
  *
- * Undoes the allocations and enabling in pci_alloc_irq_vectors().
+ * Undoes the allocations and enabling in pci_alloc_irq_vectors() or
+ * pci_alloc_irq_vectors_range().
  */
 void pci_free_irq_vectors(struct pci_dev *dev)
 {

include/linux/pci.h +2 -0

index e201d0d..cd5800a 100644
@@ -1289,6 +1289,8 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
 
 int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
 		unsigned int flags);
+int pci_alloc_irq_vectors_range(struct pci_dev *dev, unsigned int min_vecs,
+		unsigned int max_vecs, unsigned int flags);
 void pci_free_irq_vectors(struct pci_dev *dev);
 #else
 static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }

Quoted 2 lines · Christoph Hellwig

Alexander, what do you think about the version below? Survived quick testing with nvme so far.

Hi Christoph,

Please find my comments below. Sorry in advance if missed something - an incremental patch is bit difficult to review.

Hi Christoph,

Where are we at with this? Did I miss a newer version?

Where are we at with this? Did I miss a newer version?

Add a new pci_alloc_irq_vectors helper that allocates MSI-X or multi-MSI vectors for PCI device while isolating the driver from the arcane details.

This include handling both MSI-X, MSI and legacy interrupt fallbacks transparently, automatic capping to the available vectors as well as storing the information needed for request_irq in the PCI device itself so that a lot of boiler plate code in the driver can be removed.

In the future this will also allow us to automatically set up spreading for interrupt vectors without having to duplicate it in all the drivers.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  19 +++++++++
 2 files changed, 127 insertions(+)

drivers/pci/msi.c +38 -0

index a080f44..a510484 100644
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2003-2004 Intel
  * Copyright (C) Tom Long Nguyen (tom.l.nguyen at intel.com)
+ * Copyright (c) 2016 Christoph Hellwig.
  */
 
 #include <linux/err.h>

@@ -1120,6 +1121,113 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
 }
 EXPORT_SYMBOL(pci_enable_msix_range);
 
+static int __pci_enable_msix(struct pci_dev *dev, int nr_vecs)
+{
+	struct msix_entry *msix_entries;
+	int ret, i;
+
+	msix_entries = kcalloc(nr_vecs, sizeof(struct msix_entry), GFP_KERNEL);
+	if (!msix_entries)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_vecs; i++)
+		msix_entries[i].entry = i;
+
+	ret = msix_capability_init(dev, msix_entries, nr_vecs);
+	if (ret == 0) {
+		for (i = 0; i < nr_vecs; i++)
+			dev->irqs[i] = msix_entries[i].vector;
+	}
+
+	kfree(msix_entries);
+	return ret;
+}
+
+static int __pci_enable_msi(struct pci_dev *dev, int nr_vecs)
+{
+	int ret, i;
+
+	ret = msi_capability_init(dev, nr_vecs);
+	if (ret == 0) {
+		for (i = 0; i < nr_vecs; i++)
+			dev->irqs[i] = dev->irq + i;
+	}
+
+	return ret;
+}
+
+/**
+ * pci_alloc_irq_vectors - allocate multiple IRQs for a device

drivers/pci/msi.c +60 -0

+ */
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags)
+{
+	unsigned int ret;
+
+	if (WARN_ON_ONCE(dev->msi_enabled || dev->msix_enabled))
+		return -EINVAL;
+
+	if (!pci_msi_supported(dev, 1))
+		goto use_legacy_irq;
+
+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msix_vec_count(dev));
+	else if (dev->msi_cap)
+		nr_vecs = min_t(unsigned int, nr_vecs, pci_msi_vec_count(dev));
+	else
+		goto use_legacy_irq;
+
+	dev->irqs = kcalloc(nr_vecs, sizeof(u32), GFP_KERNEL);
+	if (!dev->irqs)
+		return -ENOMEM;
+
+	if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX))
+		ret = __pci_enable_msix(dev, nr_vecs);
+	else
+		ret = __pci_enable_msi(dev, nr_vecs);
+	if (ret)
+		goto out_free_irqs;
+
+	return 0;
+
+out_free_irqs:
+	kfree(dev->irqs);
+use_legacy_irq:
+	dev->irqs = &dev->irq;
+	return 1;
+}
+EXPORT_SYMBOL(pci_alloc_irq_vectors);
+
+/**
+ * pci_free_irq_vectors - free previously allocated IRQs for a device
+ * @dev:		PCI device to operate on
+ *
+ * Undoes the allocations and enabling in pci_alloc_irq_vectors().
+ */
+void pci_free_irq_vectors(struct pci_dev *dev)
+{
+	if (dev->msix_enabled)
+		pci_disable_msix(dev);
+	else if (dev->msi_enabled)
+		pci_disable_msi(dev);
+
+	if (dev->irqs != &dev->irq)
+		kfree(dev->irqs);
+	dev->irqs = NULL;
+}
+EXPORT_SYMBOL(pci_free_irq_vectors);
+
+
 struct pci_dev *msi_desc_to_pci_dev(struct msi_desc *desc)
 {
 	return to_pci_dev(desc->dev);

include/linux/pci.h +19 -0

index 932ec74..e201d0d 100644
@@ -322,6 +322,7 @@ struct pci_dev {
 	 * directly, use the values stored here. They might be different!
 	 */
 	unsigned int	irq;
+	unsigned int	*irqs;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
 
 	bool match_driver;		/* Skip attaching driver */

@@ -1255,6 +1256,8 @@ struct msix_entry {
 	u16	entry;	/* driver uses to specify entry, OS writes */
 };
 
+#define PCI_IRQ_NOMSIX		(1 << 0) /* don't try to use MSI-X interrupts */
+
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);

@@ -1283,6 +1286,10 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
 		return rc;
 	return 0;
 }
+
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags);
+void pci_free_irq_vectors(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) { }

@@ -1306,6 +1313,18 @@ static inline int pci_enable_msix_range(struct pci_dev *dev,
 static inline int pci_enable_msix_exact(struct pci_dev *dev,
 		      struct msix_entry *entries, int nvec)
 { return -ENOSYS; }
+
+static inline int pci_alloc_irq_vectors(struct pci_dev *dev,
+		unsigned int nr_vecs, unsigned int flags)
+{
+	dev->irqs = &dev->irq;
+	return 1;
+}
+
+static inline void pci_free_irq_vectors(struct pci_dev *dev)
+{
+	dev->irqs = NULL;
+}
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS

Signature · 1 line

2.1.4

Because MSI vectors are allocated sequentially we do not really need pdev->irqs[] for MSI. It is 32 items at most, but still.

drivers/nvme/host/pci.c +0 -5

-static void nvme_set_irq_hints(struct nvme_dev *dev)
-{
-	struct nvme_queue *nvmeq;
-	int i;
-