Re: [PATCH 07/13] pci: Provide sensible irq vector alloc/free routines
From: Alexander Gordeev <hidden>
Date: 2016-07-06 08:06:06
Also in:
linux-nvme, linux-pci, lkml
On Mon, Jul 04, 2016 at 05:39:28PM +0900, Christoph Hellwig wrote:
Add a function to allocate a range of interrupt vectors, which will transparently use MSI-X and MSI if available or fallback to legacy vectors. The interrupts are available in a core managed array in the pci_dev structure, and can also be released using a similar function. A new helper, __pci_enable_msix_range, is introduced to allow allocating the array of msix descriptors in the core PCIe code at the exact number of vectors supported by the PCI host complex, and to also help with the automatic IRQ affinity assignment that will be added in the next commit. Signed-off-by: Christoph Hellwig <hch@lst.de> --- Documentation/PCI/MSI-HOWTO.txt | 437 ++++------------------------------------
[...]
+ pci_enable_msi, pci_enable_msi_range, pci_enable_msi_exact, pci_disable_msi, + pci_msi_vec_count, pci_enable_msix_range, pci_enable_msix_exact, + pci_disable_msix, pci_msix_vec_count
Description of these functions can be removed when all drivers migrated to the new API. Also implementation descriptions + examples would still be needed AFAICT. [...]
quoted hunk ↗ jump to hunk
--- 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,98 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, } EXPORT_SYMBOL(pci_enable_msix_range); +static int __pci_enable_msix_range(struct pci_dev *dev, unsigned int min_vecs, + unsigned int max_vecs) +{ + int vecs = max_vecs, ret, i; + +retry: + if (vecs < min_vecs) + return -ENOSPC; + + dev->msix_vectors = kmalloc_array(vecs, sizeof(struct msix_entry), + GFP_KERNEL); + if (!dev->msix_vectors) + return -ENOMEM; + + for (i = 0; i < vecs; i++) + dev->msix_vectors[i].entry = i; + + ret = pci_enable_msix(dev, dev->msix_vectors, vecs); + if (ret) + goto out_fail; + + return vecs; + +out_fail: + kfree(dev->msix_vectors); + dev->msix_vectors = NULL; + + if (ret >= 0) { + /* retry with the actually supported number of vectors */ + vecs = ret; + goto retry; + } + + return ret; +}
This function's code almost matches the existing pci_enable_msix_range()
so pci_enable_msix_range() should be reworked instead IMHO.
I took a look at the existing code and the rework does not appear
necessary as __pci_enable_msix_range() could look like this:
static int __pci_enable_msix_range(struct pci_dev *dev, unsigned int min_vecs,
unsigned int max_vecs)
{
int ret, i;
struct msix_entry *entries;
entries = kmalloc_array(max_vecs, sizeof(entries[0]), GFP_KERNEL);
if (!entries)
return -ENOMEM;
for (i = 0; i < max_vecs; i++)
entries[i].entry = i;
ret = pci_enable_msix_range(dev, entries, min_vecs, max_vecs);
kfree(entries);
return ret;
}
We do not need to keep msix_entry array, since it only needed for
pci_irq_vector() function. But the same info could be retrieved from
msi_desc::irq.
+/**
+ * pci_alloc_irq_vectors - allocate multiple IRQs for a device
+ * @dev: PCI device to operate on
+ * @min_vecs: minimum number of vectors required (must be >= 1)
+ * @max_vecs: maximum (desired) number of vectors
+ * @flags: flags or quirks for the allocation
+ *
+ * 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. If less than @min_vecs interrupt vectors are
+ * available for @dev the function will fail with -ENOSPC.
+ *
+ * To get the Linux irq number used for a vector that can be passed to
+ * request_irq use the pci_irq_vector() helper.
+ */
+int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
+ unsigned int max_vecs, unsigned int flags)
+{
+ int vecs;
+
+ if (!(flags & PCI_IRQ_NOMSIX)) {
+ vecs = __pci_enable_msix_range(dev, min_vecs, max_vecs);
+ if (vecs > 0)
+ return vecs;
+ }
+
+ if (!(flags & PCI_IRQ_NOMSI)) {
+ vecs = pci_enable_msi_range(dev, min_vecs, max_vecs);
+ if (vecs > 0)
+ return vecs;
+ }
+
+ /* use legacy irq if allowed */
+ if (min_vecs == 1)
+ return 1;
+ return -ENOSPC;The original error code (in vecs) would be overridden with -ENOSPC here.
quoted hunk ↗ jump to hunk
+} +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) + kfree(dev->msix_vectors); + pci_disable_msix(dev); + pci_disable_msi(dev); +} +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 b67e4df..129871f 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h@@ -320,6 +320,7 @@ struct pci_dev { * directly, use the values stored here. They might be different! */ unsigned int irq; + struct msix_entry *msix_vectors;
As suggested above, it is not needed.
quoted hunk ↗ jump to hunk
struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */ bool match_driver; /* Skip attaching driver */@@ -1237,6 +1238,9 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno); int pci_set_vga_state(struct pci_dev *pdev, bool decode, unsigned int command_bits, u32 flags); +#define PCI_IRQ_NOMSI (1 << 0) /* don't try to use MSI interrupts */ +#define PCI_IRQ_NOMSIX (1 << 1) /* don't try to use MSI-X interrupts */ + /* kmem_cache style wrapper around pci_alloc_consistent() */ #include <linux/pci-dma.h>@@ -1284,6 +1288,24 @@ 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 min_vecs, + unsigned int max_vecs, unsigned int flags); +void pci_free_irq_vectors(struct pci_dev *dev); + + +/** + * pci_irq_vector - return Linux IRQ number of a device vector + * @dev: PCI device to operate on + * @nr: device-relative interrupt vector index (0-based). + */ +static inline unsigned int pci_irq_vector(struct pci_dev *dev, unsigned int nr) +{ + if (dev->msix_enabled) + return dev->msix_vectors[nr].vector;
So here you could search with for_each_pci_msi_entry() instead.
+ WARN_ON_ONCE(!dev->msi_enabled && nr > 0); + return dev->irq + nr;
I think this function should check irq number existence and return the vector number or -EINVAL;
quoted hunk ↗ jump to hunk
+} #else static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; } static inline void pci_msi_shutdown(struct pci_dev *dev) { }@@ -1307,6 +1329,23 @@ 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 min_vecs, unsigned int max_vecs, + unsigned int flags) +{ + if (min_vecs > 1) + return -ENOSPC;
In case CONFIG_PCI_MSI is unset min_vecs > 1 is -EINVAL;
+ return 1;
+}
+static inline void pci_free_irq_vectors(struct pci_dev *dev)
+{
+}
+
+static inline unsigned int pci_irq_vector(struct pci_dev *dev, unsigned int nr)
+{
+ WARN_ON_ONCE(nr > 0);As suggested above, it would rather -EINVAL;
+ return dev->irq; +} #endif #ifdef CONFIG_PCIEPORTBUS -- 2.1.4