PCI: Provide sensible irq vector alloc/free routines

PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

Hi Bjorn,

find my revisted patch for the MSI-X allocator API.  There has been
some rework to use lower level APIs now that it has been moved to
msi.c.  The second patch converts nvme over to it.  If Keith and Jens
are fine with merging it through the PCI tree I'd be all for it,
if not we'll either need a common branch for it, or just defer it
until the next merge window.

PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

Hi Bjorn,

find my revisted patch for the MSI-X allocator API.  There has been
some rework to use lower level APIs now that it has been moved to
msi.c.  The second patch converts nvme over to it.  If Keith and Jens
are fine with merging it through the PCI tree I'd be all for it,
if not we'll either need a common branch for it, or just defer it
until the next merge window.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[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.

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@lst.de>
---
 drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  19 +++++++++
 2 files changed, 127 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a080f44..a510484 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2003-2004 Intel
  * Copyright (C) Tom Long Nguyen (tom.l.nguyen@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);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 932ec74..e201d0d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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
-- 
2.1.4

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[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.

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(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a080f44..a510484 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 932ec74..e201d0d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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
-- 
2.1.4

[PATCH 2/2] nvme: switch to use pci_alloc_irq_vectors

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 87 +++++++++++++------------------------------------
 1 file changed, 22 insertions(+), 65 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4fd733f..0219965 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -89,7 +89,6 @@ struct nvme_dev {
 	unsigned max_qid;
 	int q_depth;
 	u32 db_stride;
-	struct msix_entry *entry;
 	void __iomem *bar;
 	struct work_struct reset_work;
 	struct work_struct scan_work;
@@ -209,6 +208,11 @@ static unsigned int nvme_cmd_size(struct nvme_dev *dev)
 		nvme_iod_alloc_size(dev, NVME_INT_BYTES(dev), NVME_INT_PAGES);
 }
 
+static int nvmeq_irq(struct nvme_queue *nvmeq)
+{
+	return to_pci_dev(nvmeq->dev->dev)->irqs[nvmeq->cq_vector];
+}
+
 static int nvme_admin_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
 				unsigned int hctx_idx)
 {
@@ -1055,7 +1059,7 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 		spin_unlock_irq(&nvmeq->q_lock);
 		return 1;
 	}
-	vector = nvmeq->dev->entry[nvmeq->cq_vector].vector;
+	vector = nvmeq_irq(nvmeq);
 	nvmeq->dev->online_queues--;
 	nvmeq->cq_vector = -1;
 	spin_unlock_irq(&nvmeq->q_lock);
@@ -1063,7 +1067,6 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
 		blk_mq_stop_hw_queues(nvmeq->dev->ctrl.admin_q);
 
-	irq_set_affinity_hint(vector, NULL);
 	free_irq(vector, nvmeq);
 
 	return 0;
@@ -1182,11 +1185,11 @@ static int queue_request_irq(struct nvme_dev *dev, struct nvme_queue *nvmeq,
 							const char *name)
 {
 	if (use_threaded_interrupts)
-		return request_threaded_irq(dev->entry[nvmeq->cq_vector].vector,
-					nvme_irq_check, nvme_irq, IRQF_SHARED,
-					name, nvmeq);
-	return request_irq(dev->entry[nvmeq->cq_vector].vector, nvme_irq,
-				IRQF_SHARED, name, nvmeq);
+		return request_threaded_irq(nvmeq_irq(nvmeq), nvme_irq_check,
+				nvme_irq, IRQF_SHARED, name, nvmeq);
+	else
+		return request_irq(nvmeq_irq(nvmeq), nvme_irq, IRQF_SHARED,
+				name, nvmeq);
 }
 
 static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
@@ -1463,7 +1466,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 {
 	struct nvme_queue *adminq = dev->queues[0];
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	int result, i, vecs, nr_io_queues, size;
+	int result, nr_io_queues, size;
 
 	nr_io_queues = num_possible_cpus();
 	result = nvme_set_queue_count(&dev->ctrl, &nr_io_queues);
@@ -1506,29 +1509,17 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	}
 
 	/* Deregister the admin queue's interrupt */
-	free_irq(dev->entry[0].vector, adminq);
+	free_irq(pdev->irqs[0], adminq);
 
 	/*
 	 * If we enable msix early due to not intx, disable it again before
 	 * setting up the full range we need.
 	 */
-	if (pdev->msi_enabled)
-		pci_disable_msi(pdev);
-	else if (pdev->msix_enabled)
-		pci_disable_msix(pdev);
-
-	for (i = 0; i < nr_io_queues; i++)
-		dev->entry[i].entry = i;
-	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
-	if (vecs < 0) {
-		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
-		if (vecs < 0) {
-			vecs = 1;
-		} else {
-			for (i = 0; i < vecs; i++)
-				dev->entry[i].vector = i + pdev->irq;
-		}
-	}
+	pci_free_irq_vectors(pdev);
+	nr_io_queues = pci_alloc_irq_vectors(pdev, nr_io_queues, 0);
+	if (nr_io_queues <= 0)
+		return -EIO;
+	dev->max_qid = nr_io_queues;
 
 	/*
 	 * Should investigate if there's a performance win from allocating
@@ -1536,8 +1527,6 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	 * path to scale better, even if the receive path is limited by the
 	 * number of interrupts.
 	 */
-	nr_io_queues = vecs;
-	dev->max_qid = nr_io_queues;
 
 	result = queue_request_irq(dev, adminq, adminq->irqname);
 	if (result) {
@@ -1551,22 +1540,6 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	return result;
 }
 
-static void nvme_set_irq_hints(struct nvme_dev *dev)
-{
-	struct nvme_queue *nvmeq;
-	int i;
-
-	for (i = 0; i < dev->online_queues; i++) {
-		nvmeq = dev->queues[i];
-
-		if (!nvmeq->tags || !(*nvmeq->tags))
-			continue;
-
-		irq_set_affinity_hint(dev->entry[nvmeq->cq_vector].vector,
-					blk_mq_tags_cpumask(*nvmeq->tags));
-	}
-}
-
 static void nvme_dev_scan(struct work_struct *work)
 {
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, scan_work);
@@ -1574,7 +1547,6 @@ static void nvme_dev_scan(struct work_struct *work)
 	if (!dev->tagset.tags)
 		return;
 	nvme_scan_namespaces(&dev->ctrl);
-	nvme_set_irq_hints(dev);
 }
 
 static void nvme_del_queue_end(struct request *req, int error)
@@ -1713,15 +1685,9 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 	 * interrupts. Pre-enable a single MSIX or MSI vec for setup. We'll
 	 * adjust this later.
 	 */
-	if (pci_enable_msix(pdev, dev->entry, 1)) {
-		pci_enable_msi(pdev);
-		dev->entry[0].vector = pdev->irq;
-	}
-
-	if (!dev->entry[0].vector) {
-		result = -ENODEV;
-		goto disable;
-	}
+	result = pci_alloc_irq_vectors(pdev, 1, 0);
+	if (result < 0)
+		return result;
 
 	cap = lo_hi_readq(dev->bar + NVME_REG_CAP);
 
@@ -1763,10 +1729,7 @@ static void nvme_pci_disable(struct nvme_dev *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
-	if (pdev->msi_enabled)
-		pci_disable_msi(pdev);
-	else if (pdev->msix_enabled)
-		pci_disable_msix(pdev);
+	pci_free_irq_vectors(pdev);
 
 	if (pci_is_enabled(pdev)) {
 		pci_disable_pcie_error_reporting(pdev);
@@ -1835,7 +1798,6 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
 	if (dev->ctrl.admin_q)
 		blk_put_queue(dev->ctrl.admin_q);
 	kfree(dev->queues);
-	kfree(dev->entry);
 	kfree(dev);
 }
 
@@ -2010,10 +1972,6 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	dev = kzalloc_node(sizeof(*dev), GFP_KERNEL, node);
 	if (!dev)
 		return -ENOMEM;
-	dev->entry = kzalloc_node(num_possible_cpus() * sizeof(*dev->entry),
-							GFP_KERNEL, node);
-	if (!dev->entry)
-		goto free;
 	dev->queues = kzalloc_node((num_possible_cpus() + 1) * sizeof(void *),
 							GFP_KERNEL, node);
 	if (!dev->queues)
@@ -2056,7 +2014,6 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	nvme_dev_unmap(dev);
  free:
 	kfree(dev->queues);
-	kfree(dev->entry);
 	kfree(dev);
 	return result;
 }
-- 
2.1.4

[PATCH 2/2] nvme: switch to use pci_alloc_irq_vectors

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 87 +++++++++++++------------------------------------
 1 file changed, 22 insertions(+), 65 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4fd733f..0219965 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -89,7 +89,6 @@ struct nvme_dev {
 	unsigned max_qid;
 	int q_depth;
 	u32 db_stride;
-	struct msix_entry *entry;
 	void __iomem *bar;
 	struct work_struct reset_work;
 	struct work_struct scan_work;
@@ -209,6 +208,11 @@ static unsigned int nvme_cmd_size(struct nvme_dev *dev)
 		nvme_iod_alloc_size(dev, NVME_INT_BYTES(dev), NVME_INT_PAGES);
 }
 
+static int nvmeq_irq(struct nvme_queue *nvmeq)
+{
+	return to_pci_dev(nvmeq->dev->dev)->irqs[nvmeq->cq_vector];
+}
+
 static int nvme_admin_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
 				unsigned int hctx_idx)
 {
@@ -1055,7 +1059,7 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 		spin_unlock_irq(&nvmeq->q_lock);
 		return 1;
 	}
-	vector = nvmeq->dev->entry[nvmeq->cq_vector].vector;
+	vector = nvmeq_irq(nvmeq);
 	nvmeq->dev->online_queues--;
 	nvmeq->cq_vector = -1;
 	spin_unlock_irq(&nvmeq->q_lock);
@@ -1063,7 +1067,6 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
 		blk_mq_stop_hw_queues(nvmeq->dev->ctrl.admin_q);
 
-	irq_set_affinity_hint(vector, NULL);
 	free_irq(vector, nvmeq);
 
 	return 0;
@@ -1182,11 +1185,11 @@ static int queue_request_irq(struct nvme_dev *dev, struct nvme_queue *nvmeq,
 							const char *name)
 {
 	if (use_threaded_interrupts)
-		return request_threaded_irq(dev->entry[nvmeq->cq_vector].vector,
-					nvme_irq_check, nvme_irq, IRQF_SHARED,
-					name, nvmeq);
-	return request_irq(dev->entry[nvmeq->cq_vector].vector, nvme_irq,
-				IRQF_SHARED, name, nvmeq);
+		return request_threaded_irq(nvmeq_irq(nvmeq), nvme_irq_check,
+				nvme_irq, IRQF_SHARED, name, nvmeq);
+	else
+		return request_irq(nvmeq_irq(nvmeq), nvme_irq, IRQF_SHARED,
+				name, nvmeq);
 }
 
 static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
@@ -1463,7 +1466,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 {
 	struct nvme_queue *adminq = dev->queues[0];
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	int result, i, vecs, nr_io_queues, size;
+	int result, nr_io_queues, size;
 
 	nr_io_queues = num_possible_cpus();
 	result = nvme_set_queue_count(&dev->ctrl, &nr_io_queues);
@@ -1506,29 +1509,17 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	}
 
 	/* Deregister the admin queue's interrupt */
-	free_irq(dev->entry[0].vector, adminq);
+	free_irq(pdev->irqs[0], adminq);
 
 	/*
 	 * If we enable msix early due to not intx, disable it again before
 	 * setting up the full range we need.
 	 */
-	if (pdev->msi_enabled)
-		pci_disable_msi(pdev);
-	else if (pdev->msix_enabled)
-		pci_disable_msix(pdev);
-
-	for (i = 0; i < nr_io_queues; i++)
-		dev->entry[i].entry = i;
-	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
-	if (vecs < 0) {
-		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
-		if (vecs < 0) {
-			vecs = 1;
-		} else {
-			for (i = 0; i < vecs; i++)
-				dev->entry[i].vector = i + pdev->irq;
-		}
-	}
+	pci_free_irq_vectors(pdev);
+	nr_io_queues = pci_alloc_irq_vectors(pdev, nr_io_queues, 0);
+	if (nr_io_queues <= 0)
+		return -EIO;
+	dev->max_qid = nr_io_queues;
 
 	/*
 	 * Should investigate if there's a performance win from allocating
@@ -1536,8 +1527,6 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	 * path to scale better, even if the receive path is limited by the
 	 * number of interrupts.
 	 */
-	nr_io_queues = vecs;
-	dev->max_qid = nr_io_queues;
 
 	result = queue_request_irq(dev, adminq, adminq->irqname);
 	if (result) {
@@ -1551,22 +1540,6 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	return result;
 }
 
-static void nvme_set_irq_hints(struct nvme_dev *dev)
-{
-	struct nvme_queue *nvmeq;
-	int i;
-
-	for (i = 0; i < dev->online_queues; i++) {
-		nvmeq = dev->queues[i];
-
-		if (!nvmeq->tags || !(*nvmeq->tags))
-			continue;
-
-		irq_set_affinity_hint(dev->entry[nvmeq->cq_vector].vector,
-					blk_mq_tags_cpumask(*nvmeq->tags));
-	}
-}
-
 static void nvme_dev_scan(struct work_struct *work)
 {
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, scan_work);
@@ -1574,7 +1547,6 @@ static void nvme_dev_scan(struct work_struct *work)
 	if (!dev->tagset.tags)
 		return;
 	nvme_scan_namespaces(&dev->ctrl);
-	nvme_set_irq_hints(dev);
 }
 
 static void nvme_del_queue_end(struct request *req, int error)
@@ -1713,15 +1685,9 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 	 * interrupts. Pre-enable a single MSIX or MSI vec for setup. We'll
 	 * adjust this later.
 	 */
-	if (pci_enable_msix(pdev, dev->entry, 1)) {
-		pci_enable_msi(pdev);
-		dev->entry[0].vector = pdev->irq;
-	}
-
-	if (!dev->entry[0].vector) {
-		result = -ENODEV;
-		goto disable;
-	}
+	result = pci_alloc_irq_vectors(pdev, 1, 0);
+	if (result < 0)
+		return result;
 
 	cap = lo_hi_readq(dev->bar + NVME_REG_CAP);
 
@@ -1763,10 +1729,7 @@ static void nvme_pci_disable(struct nvme_dev *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
-	if (pdev->msi_enabled)
-		pci_disable_msi(pdev);
-	else if (pdev->msix_enabled)
-		pci_disable_msix(pdev);
+	pci_free_irq_vectors(pdev);
 
 	if (pci_is_enabled(pdev)) {
 		pci_disable_pcie_error_reporting(pdev);
@@ -1835,7 +1798,6 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
 	if (dev->ctrl.admin_q)
 		blk_put_queue(dev->ctrl.admin_q);
 	kfree(dev->queues);
-	kfree(dev->entry);
 	kfree(dev);
 }
 
@@ -2010,10 +1972,6 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	dev = kzalloc_node(sizeof(*dev), GFP_KERNEL, node);
 	if (!dev)
 		return -ENOMEM;
-	dev->entry = kzalloc_node(num_possible_cpus() * sizeof(*dev->entry),
-							GFP_KERNEL, node);
-	if (!dev->entry)
-		goto free;
 	dev->queues = kzalloc_node((num_possible_cpus() + 1) * sizeof(void *),
 							GFP_KERNEL, node);
 	if (!dev->queues)
@@ -2056,7 +2014,6 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	nvme_dev_unmap(dev);
  free:
 	kfree(dev->queues);
-	kfree(dev->entry);
 	kfree(dev);
 	return result;
 }
-- 
2.1.4

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Thu, May 05, 2016 at 04:04:55PM +0200, Christoph Hellwig wrote:
> 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.

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

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  19 +++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..a510484 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2003-2004 Intel
>   * Copyright (C) Tom Long Nguyen (tom.l.nguyen@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.

I think the "flags" argument penalizes working devices unnecessarily.
Everybody that implements MSI-X correctly has to pass that zero
argument:

  pci_alloc_irq_vectors(pdev, nr_io_queues, 0);

instead of putting the burden on the broken folks like this:

  pdev->msix_broken = 1;
  pci_alloc_irq_vectors(pdev, nr_io_queues);

If we remember this via a bit in struct pci_dev, we can also make sure
pci_enable_msix(), pci_enable_msix_range(), etc. fail as they should.

> + */
> +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);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..e201d0d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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
> -- 
> 2.1.4
> 

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Thu, May 05, 2016@04:04:55PM +0200, Christoph Hellwig wrote:
> 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.

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

> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>  drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  19 +++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..a510484 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -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.

I think the "flags" argument penalizes working devices unnecessarily.
Everybody that implements MSI-X correctly has to pass that zero
argument:

  pci_alloc_irq_vectors(pdev, nr_io_queues, 0);

instead of putting the burden on the broken folks like this:

  pdev->msix_broken = 1;
  pci_alloc_irq_vectors(pdev, nr_io_queues);

If we remember this via a bit in struct pci_dev, we can also make sure
pci_enable_msix(), pci_enable_msix_range(), etc. fail as they should.

> + */
> +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);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..e201d0d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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
> -- 
> 2.1.4
> 

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Fri, May 06, 2016 at 11:04:08AM -0500, Bjorn Helgaas wrote:
> 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.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Fri, May 06, 2016@11:04:08AM -0500, Bjorn Helgaas wrote:
> 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.

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Fri, May 06, 2016 at 06:28:23PM +0200, Christoph Hellwig wrote:
> On Fri, May 06, 2016 at 11:04:08AM -0500, Bjorn Helgaas wrote:
> > 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.  

Bjorn

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Fri, May 06, 2016@06:28:23PM +0200, Christoph Hellwig wrote:
> On Fri, May 06, 2016@11:04:08AM -0500, Bjorn Helgaas wrote:
> > 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.  

Bjorn

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Fri, May 06, 2016 at 11:35:10AM -0500, Bjorn Helgaas wrote:
> 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.  

The basic idea is that we want to spread the MSI(-X) vectors around
properly right when allocating them.  See the series around this
patch when I last posted it.  For for now various drivers use different
ways of spreading them, so we'll need to make it opt-in in the beginning.

In the long run we'll hopefully have everyone converted to the common
algorithm and can get rid of the flag, but that might take a while.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Fri, May 06, 2016@11:35:10AM -0500, Bjorn Helgaas wrote:
> 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.  

The basic idea is that we want to spread the MSI(-X) vectors around
properly right when allocating them.  See the series around this
patch when I last posted it.  For for now various drivers use different
ways of spreading them, so we'll need to make it opt-in in the beginning.

In the long run we'll hopefully have everyone converted to the common
algorithm and can get rid of the flag, but that might take a while.

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Thu, May 05, 2016 at 04:04:55PM +0200, Christoph Hellwig wrote:
> 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@lst.de>

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

> ---
>  drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  19 +++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..a510484 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2003-2004 Intel
>   * Copyright (C) Tom Long Nguyen (tom.l.nguyen@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);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..e201d0d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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
> -- 
> 2.1.4
> 

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Thu, May 05, 2016@04:04:55PM +0200, Christoph Hellwig wrote:
> 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>

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

> ---
>  drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  19 +++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..a510484 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -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);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..e201d0d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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
> -- 
> 2.1.4
> 

Re: [PATCH 2/2] nvme: switch to use pci_alloc_irq_vectors

[Keith Busch]

On Thu, May 05, 2016 at 04:04:56PM +0200, Christoph Hellwig wrote:
> -static void nvme_set_irq_hints(struct nvme_dev *dev)
> -{
> -	struct nvme_queue *nvmeq;
> -	int i;
> -
> -	for (i = 0; i < dev->online_queues; i++) {
> -		nvmeq = dev->queues[i];
> -
> -		if (!nvmeq->tags || !(*nvmeq->tags))
> -			continue;
> -
> -		irq_set_affinity_hint(dev->entry[nvmeq->cq_vector].vector,
> -					blk_mq_tags_cpumask(*nvmeq->tags));
> -	}
> -}

The above doesn't merge with linux-block/for-next since the scanning was
moved to core. If this is going through linux-pci, then it merges fine,
but someone will have to fix this later.

Otherwise, looks good.

Acked-by: Keith Busch <keith.busch@intel.com>

[PATCH 2/2] nvme: switch to use pci_alloc_irq_vectors

[Keith Busch]

On Thu, May 05, 2016@04:04:56PM +0200, Christoph Hellwig wrote:
> -static void nvme_set_irq_hints(struct nvme_dev *dev)
> -{
> -	struct nvme_queue *nvmeq;
> -	int i;
> -
> -	for (i = 0; i < dev->online_queues; i++) {
> -		nvmeq = dev->queues[i];
> -
> -		if (!nvmeq->tags || !(*nvmeq->tags))
> -			continue;
> -
> -		irq_set_affinity_hint(dev->entry[nvmeq->cq_vector].vector,
> -					blk_mq_tags_cpumask(*nvmeq->tags));
> -	}
> -}

The above doesn't merge with linux-block/for-next since the scanning was
moved to core. If this is going through linux-pci, then it merges fine,
but someone will have to fix this later.

Otherwise, looks good.

Acked-by: Keith Busch <keith.busch at intel.com>

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 05, 2016 at 04:04:55PM +0200, Christoph Hellwig wrote:
> 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.

Hi Christoph,

Sorry for jumping in that late.

The MSI/MSI-X enabling pattern you introduce seems quite common
so I think it worth a separate function.

However, it is not exactly what is stated in the changelog above,
since it does not really do any fallbacks (see [1] below).

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.

May be it is what NVMe driver needs, but (once again) it is not
what the changelog claims.

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  19 +++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..a510484 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2003-2004 Intel
>   * Copyright (C) Tom Long Nguyen (tom.l.nguyen@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);

1. No fallbacks.

> +	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);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..e201d0d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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
> -- 
> 2.1.4
> 

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 05, 2016@04:04:55PM +0200, Christoph Hellwig wrote:
> 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.

Hi Christoph,

Sorry for jumping in that late.

The MSI/MSI-X enabling pattern you introduce seems quite common
so I think it worth a separate function.

However, it is not exactly what is stated in the changelog above,
since it does not really do any fallbacks (see [1] below).

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.

May be it is what NVMe driver needs, but (once again) it is not
what the changelog claims.

> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>  drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  19 +++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..a510484 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -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);

1. No fallbacks.

> +	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);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..e201d0d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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
> -- 
> 2.1.4
> 

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

Hi Alexander,

> 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:

> > +	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;

> > +	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.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

Hi Alexander,

> 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:

> > +	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;

> > +	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.

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Mon, May 09, 2016 at 05:46:31PM -0500, Bjorn Helgaas wrote:
> Thanks, I applied this to pci/msi for v4.7.  I'd be happy to apply the
> nvme change, too, given the appropriate acks.

Given the merge conflict that Keith pointed out I suspect it'll be better
to defer it until the next merge window.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Mon, May 09, 2016@05:46:31PM -0500, Bjorn Helgaas wrote:
> Thanks, I applied this to pci/msi for v4.7.  I'd be happy to apply the
> nvme change, too, given the appropriate acks.

Given the merge conflict that Keith pointed out I suspect it'll be better
to defer it until the next merge window.

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Wed, May 11, 2016 at 10:50:49AM +0200, Christoph Hellwig wrote:
> Hi Alexander,
> 
> > 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:

Nope. This check verifies MSI info, but fail to call arch-specific
arch_setup_msi_irqs() function - that is where an arch actually
does MSI interrupt allocation. I.e. some PPC platforms are known
for its run-time quota which fails to allocate as many vectors as
device MSI header reports. Such conditions are circumvented with a
cycle in range family functions:

	do {
		/* pci_enable_msix() calls arch_setup_msi_irqs() */
		rc = pci_enable_msix(dev, entries, nvec);
		if (rc < 0) {
			return rc;
		} else if (rc > 0) {
			if (rc < minvec)
				return -ENOSPC;
			nvec = rc;
		}
	} while (rc);

> > > +	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;

So above and below you choose either MSI-X or MSI. That is not a fallback
from MSI-X to MSI, nor most possible number of whatever type of MSI vectors
obtained from the underlying architecture. By contrast, the current NVMe
driver implementation does it:

	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
	if (vecs < 0) {
		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
		if (vecs < 0) {
			vecs = 1;
		} else {
			for (i = 0; i < vecs; i++)
				dev->entry[i].vector = i + pdev->irq;
		}
	}

Again, I do not know what change to NVMe driver is needed. I am just
pointing out that the changlog for pci_alloc_irq_vectors() function is
not entirely correct and the change to NVMe driver might be undesirable.

> > > +	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.
> 

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Wed, May 11, 2016@10:50:49AM +0200, Christoph Hellwig wrote:
> Hi Alexander,
> 
> > 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:

Nope. This check verifies MSI info, but fail to call arch-specific
arch_setup_msi_irqs() function - that is where an arch actually
does MSI interrupt allocation. I.e. some PPC platforms are known
for its run-time quota which fails to allocate as many vectors as
device MSI header reports. Such conditions are circumvented with a
cycle in range family functions:

	do {
		/* pci_enable_msix() calls arch_setup_msi_irqs() */
		rc = pci_enable_msix(dev, entries, nvec);
		if (rc < 0) {
			return rc;
		} else if (rc > 0) {
			if (rc < minvec)
				return -ENOSPC;
			nvec = rc;
		}
	} while (rc);

> > > +	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;

So above and below you choose either MSI-X or MSI. That is not a fallback
from MSI-X to MSI, nor most possible number of whatever type of MSI vectors
obtained from the underlying architecture. By contrast, the current NVMe
driver implementation does it:

	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
	if (vecs < 0) {
		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
		if (vecs < 0) {
			vecs = 1;
		} else {
			for (i = 0; i < vecs; i++)
				dev->entry[i].vector = i + pdev->irq;
		}
	}

Again, I do not know what change to NVMe driver is needed. I am just
pointing out that the changlog for pci_alloc_irq_vectors() function is
not entirely correct and the change to NVMe driver might be undesirable.

> > > +	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.
> 

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[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:

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a510484..32ce65e 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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;
 
+retry:
 	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: */
+	if (!ret)
+		return nr_vecs;
 
-out_free_irqs:
 	kfree(dev->irqs);
+	/* if ret is positive it's the numbers of vectors we can use, retry: */
+	if (ret > 0) {
+		nr_vecs = ret;
+		goto retry;
+	}
+
+	/* no MSI or MSI-X vectors available, fall back to the legacy IRQ: */
 use_legacy_irq:
 	dev->irqs = &dev->irq;
 	return 1;

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[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:

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a510484..32ce65e 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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;
 
+retry:
 	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: */
+	if (!ret)
+		return nr_vecs;
 
-out_free_irqs:
 	kfree(dev->irqs);
+	/* if ret is positive it's the numbers of vectors we can use, retry: */
+	if (ret > 0) {
+		nr_vecs = ret;
+		goto retry;
+	}
+
+	/* no MSI or MSI-X vectors available, fall back to the legacy IRQ: */
 use_legacy_irq:
 	dev->irqs = &dev->irq;
 	return 1;

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 12, 2016 at 09:35:52AM +0200, Christoph Hellwig wrote:
> 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:
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a510484..32ce65e 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -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;
>  
> +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).

>  	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: */
> +	if (!ret)
> +		return nr_vecs;
>  
> -out_free_irqs:
>  	kfree(dev->irqs);
> +	/* if ret is positive it's the numbers of vectors we can use, retry: */
> +	if (ret > 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;

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

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 12, 2016@09:35:52AM +0200, Christoph Hellwig wrote:
> 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:
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a510484..32ce65e 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -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;
>  
> +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).

>  	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: */
> +	if (!ret)
> +		return nr_vecs;
>  
> -out_free_irqs:
>  	kfree(dev->irqs);
> +	/* if ret is positive it's the numbers of vectors we can use, retry: */
> +	if (ret > 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;

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

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Thu, May 12, 2016 at 11:44:33AM +0200, Alexander Gordeev wrote:
> On Thu, May 12, 2016 at 09:35:52AM +0200, Christoph Hellwig wrote:
> > 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:
> > 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index a510484..32ce65e 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -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;
> >  
> > +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).

I can easily skip the pci_msi_supported check, but we either need
to re-read the number of vectors, as that is done differently for
MSI vs MSIX, or totally restructure the code, which doesn't seem worth it,
especially as the existing code also re-reads it every time.

> > -out_free_irqs:
> >  	kfree(dev->irqs);
> > +	/* if ret is positive it's the numbers of vectors we can use, retry: */
> > +	if (ret > 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.

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

> 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?

>   (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.

>   (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.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Thu, May 12, 2016@11:44:33AM +0200, Alexander Gordeev wrote:
> On Thu, May 12, 2016@09:35:52AM +0200, Christoph Hellwig wrote:
> > 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:
> > 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index a510484..32ce65e 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -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;
> >  
> > +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).

I can easily skip the pci_msi_supported check, but we either need
to re-read the number of vectors, as that is done differently for
MSI vs MSIX, or totally restructure the code, which doesn't seem worth it,
especially as the existing code also re-reads it every time.

> > -out_free_irqs:
> >  	kfree(dev->irqs);
> > +	/* if ret is positive it's the numbers of vectors we can use, retry: */
> > +	if (ret > 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.

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

> 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?

>   (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.

>   (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.

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Thu, May 12, 2016 at 01:04:58PM +0200, Alexander Gordeev wrote:
> Because MSI vectors are allocated sequentially we do not really
> need pdev->irqs[] for MSI. It is 32 items at most, but still.

The allocator itself doesn't "need" it.  Any driver that doesn't want
to special case MSI vs MSI-X all through the code needs it, though.


(oh, and please don't full quote the whole email, thank you!)

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Thu, May 12, 2016@01:04:58PM +0200, Alexander Gordeev wrote:
> Because MSI vectors are allocated sequentially we do not really
> need pdev->irqs[] for MSI. It is 32 items at most, but still.

The allocator itself doesn't "need" it.  Any driver that doesn't want
to special case MSI vs MSI-X all through the code needs it, though.


(oh, and please don't full quote the whole email, thank you!)

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 05, 2016 at 04:04:55PM +0200, Christoph Hellwig wrote:
> 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@lst.de>
> ---
>  drivers/pci/msi.c   | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  19 +++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..a510484 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2003-2004 Intel
>   * Copyright (C) Tom Long Nguyen (tom.l.nguyen@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.

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

> + */
> +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);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..e201d0d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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
> -- 
> 2.1.4
> 

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 05, 2016@04:04:55PM +0200, Christoph Hellwig wrote:
> 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(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..a510484 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -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.

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

> + */
> +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);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 932ec74..e201d0d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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
> -- 
> 2.1.4
> 

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 12, 2016 at 01:03:18PM +0200, Christoph Hellwig wrote:
> > 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.

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. 

> > 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.

> >   (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 ;)

> >   (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.

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?

(Sorry for the weird quotting)

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 12, 2016@01:03:18PM +0200, Christoph Hellwig wrote:
> > 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.

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. 

> > 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.

> >   (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 ;)

> >   (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.

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?

(Sorry for the weird quotting)

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Thu, May 12, 2016 at 02:11:57PM +0200, Alexander Gordeev wrote:
> 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. 

I can go back to just calling the existing exported functions like
the first version did for now, that'll handle it for now.

> > > 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.

We're requesting multiple vectors, so I think the naming should be fine.

> > >   (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 ;)

A quick audit shows that there are indeed a few users of this
interface.  I can add pci_alloc_irq_vectors_range that allows passing
a minvec, and the old pci_alloc_irq_vectors interface for everyone
who wants to keep it simple.

> > 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?

Or we can just look at pdev->irqs as in my patch.  That'll work without
much overhead for the single IRQ case as it can just point to ->irq,
and gives a neat interface for MSI-X and the rare multi-MSI case.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Thu, May 12, 2016@02:11:57PM +0200, Alexander Gordeev wrote:
> 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. 

I can go back to just calling the existing exported functions like
the first version did for now, that'll handle it for now.

> > > 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.

We're requesting multiple vectors, so I think the naming should be fine.

> > >   (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 ;)

A quick audit shows that there are indeed a few users of this
interface.  I can add pci_alloc_irq_vectors_range that allows passing
a minvec, and the old pci_alloc_irq_vectors interface for everyone
who wants to keep it simple.

> > 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?

Or we can just look at pdev->irqs as in my patch.  That'll work without
much overhead for the single IRQ case as it can just point to ->irq,
and gives a neat interface for MSI-X and the rare multi-MSI case.

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

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


diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a510484..cee3962 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1121,98 +1121,131 @@ 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)
+static int __pci_enable_msi(struct pci_dev *dev, unsigned int min_vecs,
+		unsigned int max_vecs)
 {
-	struct msix_entry *msix_entries;
-	int ret, i;
+	int nr_vecs, i;
 
-	msix_entries = kcalloc(nr_vecs, sizeof(struct msix_entry), GFP_KERNEL);
-	if (!msix_entries)
-		return -ENOMEM;
+	nr_vecs = pci_msi_vec_count(dev);
+	if (nr_vecs <= 0)
+		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);
+			return -ENOMEM;
+		}
 
-	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;
+			dev->irqs[i] = dev->irq + i;
+	} else if (nr_vecs == 1) {
+		dev->irqs = &dev->irq;
 	}
 
-	kfree(msix_entries);
-	return ret;
+	WARN_ON_ONCE(nr_vecs == 0);
+	return nr_vecs;
 }
 
-static int __pci_enable_msi(struct pci_dev *dev, int nr_vecs)
+static int __pci_enable_msix(struct pci_dev *dev, unsigned int min_vecs,
+		unsigned int max_vecs)
 {
-	int ret, i;
+	struct msix_entry *msix_entries;
+	int nr_vecs, i;
 
-	ret = msi_capability_init(dev, nr_vecs);
-	if (ret == 0) {
-		for (i = 0; i < nr_vecs; i++)
-			dev->irqs[i] = dev->irq + i;
+	nr_vecs = pci_msix_vec_count(dev);
+	if (nr_vecs <= 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);
+		nr_vecs = -ENOMEM;
+		goto out_free_entries;
 	}
 
-	return ret;
+	for (i = 0; i < nr_vecs; i++)
+		dev->irqs[i] = msix_entries[i].vector;
+
+out_free_entries:
+	WARN_ON_ONCE(nr_vecs == 0);
+	kfree(msix_entries);
+	return nr_vecs;
 }
 
 /**
  * pci_alloc_irq_vectors - allocate multiple IRQs for a device
  * @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,
-		unsigned int flags)
+int pci_alloc_irq_vectors_range(struct pci_dev *dev, unsigned int min_vecs,
+		unsigned int max_vecs, unsigned int flags)
 {
-	unsigned int ret;
-
+	if (WARN_ON_ONCE(min_vecs == 0))
+		return -EINVAL;
+	if (WARN_ON_ONCE(max_vecs < min_vecs))
+		return -EINVAL;
 	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;
+	if (pci_msi_supported(dev, 1)) {
+		if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX)) {
+			int ret = __pci_enable_msix(dev, min_vecs, max_vecs);
+			if (ret > 0)
+				return ret;
+		}
+		if (dev->msi_cap) {
+			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;
 	return 1;
 }
+EXPORT_SYMBOL(pci_alloc_irq_vectors_range);
+
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags)
+{
+	return pci_alloc_irq_vectors_range(dev, 1, nr_vecs, flags);
+}
 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().
+ * 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)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e201d0d..cd5800a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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; }

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

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


diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a510484..cee3962 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1121,98 +1121,131 @@ 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)
+static int __pci_enable_msi(struct pci_dev *dev, unsigned int min_vecs,
+		unsigned int max_vecs)
 {
-	struct msix_entry *msix_entries;
-	int ret, i;
+	int nr_vecs, i;
 
-	msix_entries = kcalloc(nr_vecs, sizeof(struct msix_entry), GFP_KERNEL);
-	if (!msix_entries)
-		return -ENOMEM;
+	nr_vecs = pci_msi_vec_count(dev);
+	if (nr_vecs <= 0)
+		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);
+			return -ENOMEM;
+		}
 
-	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;
+			dev->irqs[i] = dev->irq + i;
+	} else if (nr_vecs == 1) {
+		dev->irqs = &dev->irq;
 	}
 
-	kfree(msix_entries);
-	return ret;
+	WARN_ON_ONCE(nr_vecs == 0);
+	return nr_vecs;
 }
 
-static int __pci_enable_msi(struct pci_dev *dev, int nr_vecs)
+static int __pci_enable_msix(struct pci_dev *dev, unsigned int min_vecs,
+		unsigned int max_vecs)
 {
-	int ret, i;
+	struct msix_entry *msix_entries;
+	int nr_vecs, i;
 
-	ret = msi_capability_init(dev, nr_vecs);
-	if (ret == 0) {
-		for (i = 0; i < nr_vecs; i++)
-			dev->irqs[i] = dev->irq + i;
+	nr_vecs = pci_msix_vec_count(dev);
+	if (nr_vecs <= 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);
+		nr_vecs = -ENOMEM;
+		goto out_free_entries;
 	}
 
-	return ret;
+	for (i = 0; i < nr_vecs; i++)
+		dev->irqs[i] = msix_entries[i].vector;
+
+out_free_entries:
+	WARN_ON_ONCE(nr_vecs == 0);
+	kfree(msix_entries);
+	return nr_vecs;
 }
 
 /**
  * pci_alloc_irq_vectors - allocate multiple IRQs for a device
  * @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,
-		unsigned int flags)
+int pci_alloc_irq_vectors_range(struct pci_dev *dev, unsigned int min_vecs,
+		unsigned int max_vecs, unsigned int flags)
 {
-	unsigned int ret;
-
+	if (WARN_ON_ONCE(min_vecs == 0))
+		return -EINVAL;
+	if (WARN_ON_ONCE(max_vecs < min_vecs))
+		return -EINVAL;
 	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;
+	if (pci_msi_supported(dev, 1)) {
+		if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX)) {
+			int ret = __pci_enable_msix(dev, min_vecs, max_vecs);
+			if (ret > 0)
+				return ret;
+		}
+		if (dev->msi_cap) {
+			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;
 	return 1;
 }
+EXPORT_SYMBOL(pci_alloc_irq_vectors_range);
+
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
+		unsigned int flags)
+{
+	return pci_alloc_irq_vectors_range(dev, 1, nr_vecs, flags);
+}
 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().
+ * 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)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e201d0d..cd5800a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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; }

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 12, 2016 at 04:29:02PM +0200, Christoph Hellwig wrote:
> 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.

> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a510484..cee3962 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1121,98 +1121,131 @@ 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)
> +static int __pci_enable_msi(struct pci_dev *dev, unsigned int min_vecs,
> +		unsigned int max_vecs)

__pci_enable_msi_range()?

>  {
> -	struct msix_entry *msix_entries;
> -	int ret, i;
> +	int nr_vecs, i;
>  
> -	msix_entries = kcalloc(nr_vecs, sizeof(struct msix_entry), GFP_KERNEL);
> -	if (!msix_entries)
> -		return -ENOMEM;
> +	nr_vecs = pci_msi_vec_count(dev);
> +	if (nr_vecs <= 0)

pci_msi_vec_count() can not return 0

> +		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);
> +			return -ENOMEM;
> +		}
>  
> -	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;
> +			dev->irqs[i] = dev->irq + i;
> +	} else if (nr_vecs == 1) {
> +		dev->irqs = &dev->irq;

So if you do not want conserve on (up to) 32 entries for MSI case
is there a point to conserve on just 1? :) The code would be clearer
without this branch.

>  	}
>  
> -	kfree(msix_entries);
> -	return ret;
> +	WARN_ON_ONCE(nr_vecs == 0);

nr_vecs can not be 0 at this point

> +	return nr_vecs;
>  }
>  
> -static int __pci_enable_msi(struct pci_dev *dev, int nr_vecs)
> +static int __pci_enable_msix(struct pci_dev *dev, unsigned int min_vecs,
> +		unsigned int max_vecs)

__pci_enable_msix_range()?

>  {
> -	int ret, i;
> +	struct msix_entry *msix_entries;
> +	int nr_vecs, i;
>  
> -	ret = msi_capability_init(dev, nr_vecs);
> -	if (ret == 0) {
> -		for (i = 0; i < nr_vecs; i++)
> -			dev->irqs[i] = dev->irq + i;
> +	nr_vecs = pci_msix_vec_count(dev);
> +	if (nr_vecs <= 0)

pci_msix_vec_count() can not return 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);
> +		nr_vecs = -ENOMEM;
> +		goto out_free_entries;
>  	}
>  
> -	return ret;
> +	for (i = 0; i < nr_vecs; i++)
> +		dev->irqs[i] = msix_entries[i].vector;
> +
> +out_free_entries:
> +	WARN_ON_ONCE(nr_vecs == 0);

nr_vecs can not be 0 at this point

> +	kfree(msix_entries);
> +	return nr_vecs;
>  }
>  
>  /**
>   * pci_alloc_irq_vectors - allocate multiple IRQs for a device

pci_alloc_irq_vectors_range()

This function needs to be documented in Documentation/PCI/MSI-HOWTO.txt
I guess.

>   * @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,
> -		unsigned int flags)
> +int pci_alloc_irq_vectors_range(struct pci_dev *dev, unsigned int min_vecs,
> +		unsigned int max_vecs, unsigned int flags)
>  {
> -	unsigned int ret;
> -
> +	if (WARN_ON_ONCE(min_vecs == 0))
> +		return -EINVAL;
> +	if (WARN_ON_ONCE(max_vecs < min_vecs))
> +		return -EINVAL;

These two checks are unnecessary, since they are done in pci_msi_supported()

>  	if (WARN_ON_ONCE(dev->msi_enabled || dev->msix_enabled))
>  		return -EINVAL;

This check could be omitted since it is done within the range functions.

> -	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;
> +	if (pci_msi_supported(dev, 1)) {

The 2nd argument should be min_vecs. But is is still unnecessary, since
pci_enable_msix_range() calls it anyway.
	i

> +		if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX)) {

dev->msix_cap check is unnecessary since pci_msix_vec_count() does it.

> +			int ret = __pci_enable_msix(dev, min_vecs, max_vecs);
> +			if (ret > 0)
> +				return ret;
> +		}
> +		if (dev->msi_cap) {

pci_msix_vec_count() checks dev->msi_cap.

> +			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;
>  	return 1;
>  }
> +EXPORT_SYMBOL(pci_alloc_irq_vectors_range);
> +
> +int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
> +		unsigned int flags)
> +{
> +	return pci_alloc_irq_vectors_range(dev, 1, nr_vecs, flags);
> +}
>  EXPORT_SYMBOL(pci_alloc_irq_vectors);

Any reason not to make it an inline?

>  
>  /**
>   * 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)
>  {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e201d0d..cd5800a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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; }

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Alexander Gordeev]

On Thu, May 12, 2016@04:29:02PM +0200, Christoph Hellwig wrote:
> 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.

> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a510484..cee3962 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1121,98 +1121,131 @@ 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)
> +static int __pci_enable_msi(struct pci_dev *dev, unsigned int min_vecs,
> +		unsigned int max_vecs)

__pci_enable_msi_range()?

>  {
> -	struct msix_entry *msix_entries;
> -	int ret, i;
> +	int nr_vecs, i;
>  
> -	msix_entries = kcalloc(nr_vecs, sizeof(struct msix_entry), GFP_KERNEL);
> -	if (!msix_entries)
> -		return -ENOMEM;
> +	nr_vecs = pci_msi_vec_count(dev);
> +	if (nr_vecs <= 0)

pci_msi_vec_count() can not return 0

> +		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);
> +			return -ENOMEM;
> +		}
>  
> -	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;
> +			dev->irqs[i] = dev->irq + i;
> +	} else if (nr_vecs == 1) {
> +		dev->irqs = &dev->irq;

So if you do not want conserve on (up to) 32 entries for MSI case
is there a point to conserve on just 1? :) The code would be clearer
without this branch.

>  	}
>  
> -	kfree(msix_entries);
> -	return ret;
> +	WARN_ON_ONCE(nr_vecs == 0);

nr_vecs can not be 0 at this point

> +	return nr_vecs;
>  }
>  
> -static int __pci_enable_msi(struct pci_dev *dev, int nr_vecs)
> +static int __pci_enable_msix(struct pci_dev *dev, unsigned int min_vecs,
> +		unsigned int max_vecs)

__pci_enable_msix_range()?

>  {
> -	int ret, i;
> +	struct msix_entry *msix_entries;
> +	int nr_vecs, i;
>  
> -	ret = msi_capability_init(dev, nr_vecs);
> -	if (ret == 0) {
> -		for (i = 0; i < nr_vecs; i++)
> -			dev->irqs[i] = dev->irq + i;
> +	nr_vecs = pci_msix_vec_count(dev);
> +	if (nr_vecs <= 0)

pci_msix_vec_count() can not return 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);
> +		nr_vecs = -ENOMEM;
> +		goto out_free_entries;
>  	}
>  
> -	return ret;
> +	for (i = 0; i < nr_vecs; i++)
> +		dev->irqs[i] = msix_entries[i].vector;
> +
> +out_free_entries:
> +	WARN_ON_ONCE(nr_vecs == 0);

nr_vecs can not be 0 at this point

> +	kfree(msix_entries);
> +	return nr_vecs;
>  }
>  
>  /**
>   * pci_alloc_irq_vectors - allocate multiple IRQs for a device

pci_alloc_irq_vectors_range()

This function needs to be documented in Documentation/PCI/MSI-HOWTO.txt
I guess.

>   * @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,
> -		unsigned int flags)
> +int pci_alloc_irq_vectors_range(struct pci_dev *dev, unsigned int min_vecs,
> +		unsigned int max_vecs, unsigned int flags)
>  {
> -	unsigned int ret;
> -
> +	if (WARN_ON_ONCE(min_vecs == 0))
> +		return -EINVAL;
> +	if (WARN_ON_ONCE(max_vecs < min_vecs))
> +		return -EINVAL;

These two checks are unnecessary, since they are done in pci_msi_supported()

>  	if (WARN_ON_ONCE(dev->msi_enabled || dev->msix_enabled))
>  		return -EINVAL;

This check could be omitted since it is done within the range functions.

> -	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;
> +	if (pci_msi_supported(dev, 1)) {

The 2nd argument should be min_vecs. But is is still unnecessary, since
pci_enable_msix_range() calls it anyway.
	i

> +		if (dev->msix_cap && !(flags & PCI_IRQ_NOMSIX)) {

dev->msix_cap check is unnecessary since pci_msix_vec_count() does it.

> +			int ret = __pci_enable_msix(dev, min_vecs, max_vecs);
> +			if (ret > 0)
> +				return ret;
> +		}
> +		if (dev->msi_cap) {

pci_msix_vec_count() checks dev->msi_cap.

> +			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;
>  	return 1;
>  }
> +EXPORT_SYMBOL(pci_alloc_irq_vectors_range);
> +
> +int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int nr_vecs,
> +		unsigned int flags)
> +{
> +	return pci_alloc_irq_vectors_range(dev, 1, nr_vecs, flags);
> +}
>  EXPORT_SYMBOL(pci_alloc_irq_vectors);

Any reason not to make it an inline?

>  
>  /**
>   * 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)
>  {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e201d0d..cd5800a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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; }

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Mon, May 09, 2016 at 05:46:31PM -0500, Bjorn Helgaas wrote:
> On Thu, May 05, 2016 at 04:04:55PM +0200, Christoph Hellwig wrote:
> > 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@lst.de>
> 
> Thanks, I applied this to pci/msi for v4.7.

Given the subsequent discussion, it seems a little premature to merge
this for v4.7, so I'm dropping this for now.

Bjorn

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Mon, May 09, 2016@05:46:31PM -0500, Bjorn Helgaas wrote:
> On Thu, May 05, 2016@04:04:55PM +0200, Christoph Hellwig wrote:
> > 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>
> 
> Thanks, I applied this to pci/msi for v4.7.

Given the subsequent discussion, it seems a little premature to merge
this for v4.7, so I'm dropping this for now.

Bjorn

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

Ok, I'll have an updated version ready early for the next merge
window.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

Ok, I'll have an updated version ready early for the next merge
window.

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Fri, May 13, 2016 at 10:29:48AM +0200, Alexander Gordeev wrote:
> On Thu, May 12, 2016 at 04:29:02PM +0200, Christoph Hellwig wrote:
> > 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?

Bjorn

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Bjorn Helgaas]

On Fri, May 13, 2016@10:29:48AM +0200, Alexander Gordeev wrote:
> On Thu, May 12, 2016@04:29:02PM +0200, Christoph Hellwig wrote:
> > 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?

Bjorn

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Fri, Jun 10, 2016 at 08:14:50PM -0500, Bjorn Helgaas wrote:
> Hi Christoph,
> 
> Where are we at with this?  Did I miss a newer version?

I've been respinning the whole MSI-X spreading series after a discussion
with Thomas.  I'll be able to post it soon.

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Fri, Jun 10, 2016@08:14:50PM -0500, Bjorn Helgaas wrote:
> Hi Christoph,
> 
> Where are we at with this?  Did I miss a newer version?

I've been respinning the whole MSI-X spreading series after a discussion
with Thomas.  I'll be able to post it soon.

Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Fri, Jun 10, 2016 at 08:14:50PM -0500, Bjorn Helgaas wrote:
> Where are we at with this?  Did I miss a newer version?

Any comments on the repost from last week?

[PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines

[Christoph Hellwig]

On Fri, Jun 10, 2016@08:14:50PM -0500, Bjorn Helgaas wrote:
> Where are we at with this?  Did I miss a newer version?

Any comments on the repost from last week?