Re: [PATCH 6/8] pci: provide sensible irq vector alloc/free routines
From: Bjorn Helgaas
Date: Fri Apr 29 2016 - 17:16:51 EST
[+cc Alexander]
Sorry to be a pedant, but can you please edit the subject to be:
PCI: Provide sensible IRQ vector alloc/free routines
so it matches the drivers/pci convention?
I like this idea a lot. The MSI-X/MSI interfaces are much better than
they used to be, and I think this would be another significant
improvement. What do you think, Alexander? Here's the whole series
in case you don't have it handy:
http://lkml.kernel.org/r/1460770552-31260-1-git-send-email-hch@xxxxxx
On Fri, Apr 15, 2016 at 06:35:50PM -0700, Christoph Hellwig wrote:
> Hide all the MSI-X vs MSI vs legacy bullshit, and provide an array of
> interrupt vectors in the pci_dev structure, and ensure we get proper
> interrupt affinity by default.
This patch doesn't do anything for affinity by itself.
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> drivers/pci/irq.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/pci/msi.c | 2 +-
> drivers/pci/pci.h | 5 +++
> include/linux/pci.h | 5 +++
> 4 files changed, 99 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c
> index 6684f15..b683465 100644
> --- a/drivers/pci/irq.c
> +++ b/drivers/pci/irq.c
> @@ -1,7 +1,8 @@
> /*
> - * PCI IRQ failure handing code
> + * PCI IRQ handing code
s/handing/handling/ :)
> *
> * Copyright (c) 2008 James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> + * Copyright (c) 2016 Christoph Hellwig.
> */
>
> #include <linux/acpi.h>
> @@ -9,6 +10,92 @@
> #include <linux/kernel.h>
> #include <linux/export.h>
> #include <linux/pci.h>
> +#include <linux/interrupt.h>
> +#include "pci.h"
> +
> +static int pci_nr_irq_vectors(struct pci_dev *pdev)
> +{
> + int nr_entries;
> +
> + nr_entries = pci_msix_vec_count(pdev);
> + if (nr_entries <= 0 && pci_msi_supported(pdev, 1))
> + nr_entries = pci_msi_vec_count(pdev);
> + if (nr_entries <= 0)
> + nr_entries = 1;
> + return nr_entries;
> +}
> +
> +static int pci_enable_msix_range_wrapper(struct pci_dev *pdev, u32 *irqs,
> + int nr_vecs)
> +{
> + struct msix_entry *msix_entries;
> + int vecs, 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;
> +
> + vecs = pci_enable_msix_range(pdev, msix_entries, 1, nr_vecs);
> + if (vecs > 0) {
> + for (i = 0; i < vecs; i++)
> + irqs[i] = msix_entries[i].vector;
> + }
> +
> + kfree(msix_entries);
> + return vecs;
> +}
> +
> +int pci_alloc_irq_vectors(struct pci_dev *pdev, int nr_vecs)
> +{
> + int vecs, ret, i;
> + u32 *irqs;
> +
> + nr_vecs = min(nr_vecs, pci_nr_irq_vectors(pdev));
> +
> + irqs = kcalloc(nr_vecs, sizeof(u32), GFP_KERNEL);
> + if (!irqs)
> + return -ENOMEM;
> +
> + vecs = pci_enable_msix_range_wrapper(pdev, irqs, nr_vecs);
> + if (vecs <= 0) {
> + vecs = pci_enable_msi_range(pdev, 1, min(nr_vecs, 32));
I don't see one, but seems like we should have a #define for this
"32". I guess pci_enable_msi_range() already protects itself, so this
min() is probably not strictly necessary anyway.
> + if (vecs <= 0) {
> + ret = -EIO;
> + if (!pdev->irq)
> + goto out_free_irqs;
> +
> + /* use legacy irq */
> + vecs = 1;
> + }
> +
> + for (i = 0; i < vecs; i++)
> + irqs[i] = pdev->irq + i;
> + }
> +
> + pdev->irqs = irqs;
> + return vecs;
> +
> +out_free_irqs:
> + kfree(irqs);
> + return ret;
return -EIO;
and remove "ret".
> +}
> +EXPORT_SYMBOL(pci_alloc_irq_vectors);
> +
> +void pci_free_irq_vectors(struct pci_dev *pdev)
> +{
> + if (pdev->msi_enabled)
> + pci_disable_msi(pdev);
> + else if (pdev->msix_enabled)
> + pci_disable_msix(pdev);
> +
> + kfree(pdev->dev.irq_affinity);
> + pdev->dev.irq_affinity = NULL;
These two lines belong in a different patch.
> + kfree(pdev->irqs);
> +}
> +EXPORT_SYMBOL(pci_free_irq_vectors);
>
> static void pci_note_irq_problem(struct pci_dev *pdev, const char *reason)
> {
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..544d306 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -815,7 +815,7 @@ out_free:
> * to determine if MSI/-X are supported for the device. If MSI/-X is
> * supported return 1, else return 0.
> **/
> -static int pci_msi_supported(struct pci_dev *dev, int nvec)
> +int pci_msi_supported(struct pci_dev *dev, int nvec)
> {
> struct pci_bus *bus;
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index d0fb934..263422c 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -144,8 +144,13 @@ extern unsigned int pci_pm_d3_delay;
>
> #ifdef CONFIG_PCI_MSI
> void pci_no_msi(void);
> +int pci_msi_supported(struct pci_dev *dev, int nvec);
> #else
> static inline void pci_no_msi(void) { }
> +static int pci_msi_supported(struct pci_dev *dev, int nvec)
> +{
> + return 0;
> +}
> #endif
>
> static inline void pci_msi_set_enable(struct pci_dev *dev, int enable)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 004b813..4fbc14f 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 */
> @@ -1235,6 +1236,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);
>
> +int pci_alloc_irq_vectors(struct pci_dev *dev, int nr_vecs);
> +void pci_free_irq_vectors(struct pci_dev *pdev);
> +
> /* kmem_cache style wrapper around pci_alloc_consistent() */
>
> #include <linux/pci-dma.h>
> @@ -1282,6 +1286,7 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
> return rc;
> return 0;
> }
> +
> #else
> static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
> static inline void pci_msi_shutdown(struct pci_dev *dev) { }
> --
> 2.1.4
>