* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-05 14:04 ` [PATCH 1/2] " Christoph Hellwig
@ 2016-05-06 16:04 ` Bjorn Helgaas
2016-05-06 16:28 ` Christoph Hellwig
2016-05-09 22:46 ` Bjorn Helgaas
` (2 subsequent siblings)
3 siblings, 1 reply; 27+ messages in thread
From: Bjorn Helgaas @ 2016-05-06 16:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: agordeev, pjw, axboe, keith.busch, linux-pci, linux-nvme
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-06 16:04 ` Bjorn Helgaas
@ 2016-05-06 16:28 ` Christoph Hellwig
2016-05-06 16:35 ` Bjorn Helgaas
0 siblings, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-06 16:28 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Christoph Hellwig, agordeev, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-06 16:28 ` Christoph Hellwig
@ 2016-05-06 16:35 ` Bjorn Helgaas
2016-05-08 9:05 ` Christoph Hellwig
0 siblings, 1 reply; 27+ messages in thread
From: Bjorn Helgaas @ 2016-05-06 16:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: agordeev, pjw, axboe, keith.busch, linux-pci, linux-nvme
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-06 16:35 ` Bjorn Helgaas
@ 2016-05-08 9:05 ` Christoph Hellwig
0 siblings, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-08 9:05 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Christoph Hellwig, agordeev, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-05 14:04 ` [PATCH 1/2] " Christoph Hellwig
2016-05-06 16:04 ` Bjorn Helgaas
@ 2016-05-09 22:46 ` Bjorn Helgaas
2016-05-11 8:52 ` Christoph Hellwig
2016-05-16 19:39 ` Bjorn Helgaas
2016-05-11 7:45 ` Alexander Gordeev
2016-05-12 11:04 ` Alexander Gordeev
3 siblings, 2 replies; 27+ messages in thread
From: Bjorn Helgaas @ 2016-05-09 22:46 UTC (permalink / raw)
To: Christoph Hellwig
Cc: agordeev, pjw, axboe, keith.busch, linux-pci, linux-nvme
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-09 22:46 ` Bjorn Helgaas
@ 2016-05-11 8:52 ` Christoph Hellwig
2016-05-16 19:39 ` Bjorn Helgaas
1 sibling, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-11 8:52 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Christoph Hellwig, agordeev, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-09 22:46 ` Bjorn Helgaas
2016-05-11 8:52 ` Christoph Hellwig
@ 2016-05-16 19:39 ` Bjorn Helgaas
2016-05-17 16:45 ` Christoph Hellwig
1 sibling, 1 reply; 27+ messages in thread
From: Bjorn Helgaas @ 2016-05-16 19:39 UTC (permalink / raw)
To: Christoph Hellwig
Cc: agordeev, pjw, axboe, keith.busch, linux-pci, linux-nvme
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-05 14:04 ` [PATCH 1/2] " Christoph Hellwig
2016-05-06 16:04 ` Bjorn Helgaas
2016-05-09 22:46 ` Bjorn Helgaas
@ 2016-05-11 7:45 ` Alexander Gordeev
2016-05-11 8:50 ` Christoph Hellwig
2016-05-12 11:04 ` Alexander Gordeev
3 siblings, 1 reply; 27+ messages in thread
From: Alexander Gordeev @ 2016-05-11 7:45 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: helgaas, pjw, axboe, keith.busch, linux-pci, linux-nvme
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-11 7:45 ` Alexander Gordeev
@ 2016-05-11 8:50 ` Christoph Hellwig
2016-05-11 9:44 ` Alexander Gordeev
0 siblings, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-11 8:50 UTC (permalink / raw)
To: Alexander Gordeev
Cc: Christoph Hellwig, helgaas, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-11 8:50 ` Christoph Hellwig
@ 2016-05-11 9:44 ` Alexander Gordeev
2016-05-12 7:35 ` Christoph Hellwig
0 siblings, 1 reply; 27+ messages in thread
From: Alexander Gordeev @ 2016-05-11 9:44 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: helgaas, pjw, axboe, keith.busch, linux-pci, linux-nvme
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.
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-11 9:44 ` Alexander Gordeev
@ 2016-05-12 7:35 ` Christoph Hellwig
2016-05-12 9:44 ` Alexander Gordeev
0 siblings, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-12 7:35 UTC (permalink / raw)
To: Alexander Gordeev
Cc: Christoph Hellwig, helgaas, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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;
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-12 7:35 ` Christoph Hellwig
@ 2016-05-12 9:44 ` Alexander Gordeev
2016-05-12 11:03 ` Christoph Hellwig
0 siblings, 1 reply; 27+ messages in thread
From: Alexander Gordeev @ 2016-05-12 9:44 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: helgaas, pjw, axboe, keith.busch, linux-pci, linux-nvme
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;
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-12 9:44 ` Alexander Gordeev
@ 2016-05-12 11:03 ` Christoph Hellwig
2016-05-12 12:11 ` Alexander Gordeev
0 siblings, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-12 11:03 UTC (permalink / raw)
To: Alexander Gordeev
Cc: Christoph Hellwig, helgaas, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-12 11:03 ` Christoph Hellwig
@ 2016-05-12 12:11 ` Alexander Gordeev
2016-05-12 12:19 ` Christoph Hellwig
0 siblings, 1 reply; 27+ messages in thread
From: Alexander Gordeev @ 2016-05-12 12:11 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: helgaas, pjw, axboe, keith.busch, linux-pci, linux-nvme
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)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-12 12:11 ` Alexander Gordeev
@ 2016-05-12 12:19 ` Christoph Hellwig
2016-05-12 14:29 ` Christoph Hellwig
0 siblings, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-12 12:19 UTC (permalink / raw)
To: Alexander Gordeev
Cc: Christoph Hellwig, helgaas, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-12 12:19 ` Christoph Hellwig
@ 2016-05-12 14:29 ` Christoph Hellwig
2016-05-13 8:29 ` Alexander Gordeev
0 siblings, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-12 14:29 UTC (permalink / raw)
To: Alexander Gordeev
Cc: Christoph Hellwig, helgaas, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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; }
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-12 14:29 ` Christoph Hellwig
@ 2016-05-13 8:29 ` Alexander Gordeev
2016-06-11 1:14 ` Bjorn Helgaas
0 siblings, 1 reply; 27+ messages in thread
From: Alexander Gordeev @ 2016-05-13 8:29 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: helgaas, pjw, axboe, keith.busch, linux-pci, linux-nvme
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; }
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-13 8:29 ` Alexander Gordeev
@ 2016-06-11 1:14 ` Bjorn Helgaas
2016-06-13 15:15 ` Christoph Hellwig
2016-06-21 14:33 ` Christoph Hellwig
0 siblings, 2 replies; 27+ messages in thread
From: Bjorn Helgaas @ 2016-06-11 1:14 UTC (permalink / raw)
To: Alexander Gordeev
Cc: Christoph Hellwig, pjw, axboe, keith.busch, linux-pci, linux-nvme
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-06-11 1:14 ` Bjorn Helgaas
@ 2016-06-13 15:15 ` Christoph Hellwig
2016-06-21 14:33 ` Christoph Hellwig
1 sibling, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Alexander Gordeev, Christoph Hellwig, pjw, axboe, keith.busch,
linux-pci, linux-nvme
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-06-11 1:14 ` Bjorn Helgaas
2016-06-13 15:15 ` Christoph Hellwig
@ 2016-06-21 14:33 ` Christoph Hellwig
1 sibling, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2016-06-21 14:33 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Alexander Gordeev, Christoph Hellwig, pjw, axboe, keith.busch,
linux-pci, linux-nvme
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?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-05 14:04 ` [PATCH 1/2] " Christoph Hellwig
` (2 preceding siblings ...)
2016-05-11 7:45 ` Alexander Gordeev
@ 2016-05-12 11:04 ` Alexander Gordeev
2016-05-12 11:04 ` Christoph Hellwig
3 siblings, 1 reply; 27+ messages in thread
From: Alexander Gordeev @ 2016-05-12 11:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: helgaas, pjw, axboe, keith.busch, linux-pci, linux-nvme
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/2] PCI: Provide sensible irq vector alloc/free routines
2016-05-12 11:04 ` Alexander Gordeev
@ 2016-05-12 11:04 ` Christoph Hellwig
0 siblings, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2016-05-12 11:04 UTC (permalink / raw)
To: Alexander Gordeev
Cc: Christoph Hellwig, helgaas, pjw, axboe, keith.busch, linux-pci,
linux-nvme
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!)
^ permalink raw reply [flat|nested] 27+ messages in thread