| Message ID | 1465934346-20648-8-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Jun 14, 2016 at 09:59:00PM +0200, Christoph Hellwig wrote: > Add a helper 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 > helper. > > The next patch will also add automatic spreading of MSI / MSI-X > vectors to this function. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/pci/msi.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 18 +++++++++ New APIs should be documented in Documentation/PCI/MSI-HOWTO.txt, I guess. > 2 files changed, 128 insertions(+) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index a080f44..a33adec 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,115 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, > } > EXPORT_SYMBOL(pci_enable_msix_range); > > +static unsigned 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; > +} This function is strange, because it: (a) does not consider PCI_IRQ_NOMSIX flag; (b) only calls pci_msi_supported() for MSI case; (c) calls pci_msi_supported() with just one vector; (d) might return suboptimal number of vectors (number of MSI-X used later for MSI or vice versa) Overall, I would suggest simply return maximum between MSI-X and MSI numbers and let the rest of the code (i.e the two range functions) handle a-d. > +static int pci_enable_msix_range_wrapper(struct pci_dev *pdev, u32 *irqs, > + unsigned int min_vecs, unsigned int max_vecs) > +{ > + struct msix_entry *msix_entries; > + int vecs, i; > + > + 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; > + > + vecs = pci_enable_msix_range(pdev, msix_entries, min_vecs, max_vecs); > + if (vecs > 0) { This condition check is unneeded. > + for (i = 0; i < vecs; i++) > + irqs[i] = msix_entries[i].vector; > + } > + > + kfree(msix_entries); > + return vecs; > +} > + > +/** > + * 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. The Linux irq numbers for the allocated > + * vectors are stored in pdev->irqs. If less than @min_vecs interrupt > + * vectors are available for @dev the function will fail with -ENOSPC. > + */ > +int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, > + unsigned int max_vecs, unsigned int flags) > +{ > + unsigned int vecs, i; > + u32 *irqs; > + > + max_vecs = min(max_vecs, pci_nr_irq_vectors(dev)); Optionally, you could move this assignment to pci_nr_irq_vectors() and simply let it handle number of vectors to request. > + irqs = kcalloc(max_vecs, sizeof(u32), GFP_KERNEL); > + if (!irqs) > + return -ENOMEM; > + > + if (!(flags & PCI_IRQ_NOMSIX)) { > + vecs = pci_enable_msix_range_wrapper(dev, irqs, min_vecs, > + max_vecs); > + if (vecs > 0) > + goto done; > + } > + > + vecs = pci_enable_msi_range(dev, min_vecs, max_vecs); > + if (vecs > 0) { > + for (i = 0; i < vecs; i++) > + irqs[i] = dev->irq + i; > + goto done; > + } > + > + if (min_vecs > 1) > + return -ENOSPC; irqs is leaked if (min_vecs > 1) You can get rid of this check at all if you reorganize your code i.e. like this: ... vecs = pci_enable_msi_range(dev, min_vecs, max_vecs); if (vecs < 0) goto legacy; for (i = 0; i < vecs; i++) irqs[i] = dev->irq + i; done: ... legacy: ... > + > + /* use legacy irq */ > + kfree(irqs); > + dev->irqs = &dev->irq; > + return 1; > + > +done: > + dev->irqs = irqs; > + return vecs; > +} > +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); The checks are probably redundant or incomplete. Redundant - because pci_disable_msi()/pci_disable_msix() do it anyways: if (!pci_msi_enable || !dev || !dev->msi_enabled) return; Incomplete - because the two other conditions are not checked. > + if (dev->irqs != &dev->irq) > + kfree(dev->irqs); Unset dev->irqs? BTW, since (dev->irqs == &dev->irq) effectively checks if MSI/MSI-X was enabled this function could bail out in case they did not. > +} > +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..84a20fc 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; > + unsigned int *irqs; > struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */ > > bool match_driver; /* Skip attaching driver */ > @@ -1237,6 +1238,8 @@ 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_NOMSIX (1 << 0) /* don't try to use MSI-X interrupts */ BTW, why PCI_IRQ_NOMSIX only and no PCI_IRQ_NOMSI? > /* kmem_cache style wrapper around pci_alloc_consistent() */ > > #include <linux/pci-dma.h> > @@ -1284,6 +1287,9 @@ 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); > #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 +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 min_vecs, unsigned int max_vecs, > + unsigned int flags) > +{ > + if (min_vecs > 1) > + return -ENOSPC; > + dev->irqs = &dev->irq; > + return 1; > +} > +static inline void pci_free_irq_vectors(struct pci_dev *dev) > +{ Unset dev->irqs? > +} > #endif > > #ifdef CONFIG_PCIEPORTBUS > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 23, 2016 at 01:16:10PM +0200, Alexander Gordeev wrote: > New APIs should be documented in Documentation/PCI/MSI-HOWTO.txt, I guess. Ok, done. > > +static unsigned 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; > > +} > > This function is strange, because it: > (a) does not consider PCI_IRQ_NOMSIX flag; > (b) only calls pci_msi_supported() for MSI case; > (c) calls pci_msi_supported() with just one vector; > (d) might return suboptimal number of vectors (number of MSI-X used > later for MSI or vice versa) > > Overall, I would suggest simply return maximum between MSI-X and MSI > numbers and let the rest of the code (i.e the two range functions) > handle a-d. Ok, fixed except for (c) - the only thing pci_msi_supported does with nvec is to check for it being less than 1, which we don't care about, and which really shouldn't be in this function to start with. > > + struct msix_entry *msix_entries; > > + int vecs, i; > > + > > + 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; > > + > > + vecs = pci_enable_msix_range(pdev, msix_entries, min_vecs, max_vecs); > > + if (vecs > 0) { > > This condition check is unneeded. Why? We could get -ENOSPC back. Oh, because our for loop will terminate immediately. I can update it, but I think removing it is less readable than keeping it around. > > + if (!(flags & PCI_IRQ_NOMSIX)) { > > + vecs = pci_enable_msix_range_wrapper(dev, irqs, min_vecs, > > + max_vecs); > > + if (vecs > 0) > > + goto done; > > + } > > + > > + vecs = pci_enable_msi_range(dev, min_vecs, max_vecs); > > + if (vecs > 0) { > > + for (i = 0; i < vecs; i++) > > + irqs[i] = dev->irq + i; > > + goto done; > > + } > > + > > + if (min_vecs > 1) > > + return -ENOSPC; > > irqs is leaked if (min_vecs > 1) > > You can get rid of this check at all if you reorganize your code i.e. > like this: > > ... > > vecs = pci_enable_msi_range(dev, min_vecs, max_vecs); > if (vecs < 0) > goto legacy; > > for (i = 0; i < vecs; i++) > irqs[i] = dev->irq + i; > > done: > ... > > > legacy: > ... I've just moved the if below the kfree. > > +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); > > The checks are probably redundant or incomplete. Redundant - because > pci_disable_msi()/pci_disable_msix() do it anyways: > > if (!pci_msi_enable || !dev || !dev->msi_enabled) > return; > > Incomplete - because the two other conditions are not checked. Ok, I've dropped the check. > > > + if (dev->irqs != &dev->irq) > > + kfree(dev->irqs); > > Unset dev->irqs? Fine with me, added. > > +#define PCI_IRQ_NOMSIX (1 << 0) /* don't try to use MSI-X interrupts */ > > BTW, why PCI_IRQ_NOMSIX only and no PCI_IRQ_NOMSI? Because there is no need to call this API if your device only supports a single legacy vector anyway. > > + if (min_vecs > 1) > > + return -ENOSPC; > > + dev->irqs = &dev->irq; > > + return 1; > > +} > > +static inline void pci_free_irq_vectors(struct pci_dev *dev) > > +{ > > Unset dev->irqs? Ok. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 30, 2016 at 06:54:17PM +0200, Christoph Hellwig wrote: > > > +static unsigned 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; > > > +} > > > > This function is strange, because it: > > (a) does not consider PCI_IRQ_NOMSIX flag; > > (b) only calls pci_msi_supported() for MSI case; > > (c) calls pci_msi_supported() with just one vector; > > (d) might return suboptimal number of vectors (number of MSI-X used > > later for MSI or vice versa) > > > > Overall, I would suggest simply return maximum between MSI-X and MSI > > numbers and let the rest of the code (i.e the two range functions) > > handle a-d. > > Ok, fixed except for (c) - the only thing pci_msi_supported does with > nvec is to check for it being less than 1, which we don't care about, > and which really shouldn't be in this function to start with. Yes, but we should not rely on our knowledge of pci_msi_supported() internals, aren't we? We need to follow the API which asks nvecs for whatever reason. Anyway, if you return maximum of the two it does not matter. > > > + struct msix_entry *msix_entries; > > > + int vecs, i; > > > + > > > + 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; > > > + > > > + vecs = pci_enable_msix_range(pdev, msix_entries, min_vecs, max_vecs); > > > + if (vecs > 0) { > > > > This condition check is unneeded. > > Why? We could get -ENOSPC back. Oh, because our for loop will > terminate immediately. I can update it, but I think removing it > is less readable than keeping it around. Yes, I think you are right. > > > + if (!(flags & PCI_IRQ_NOMSIX)) { > > > + vecs = pci_enable_msix_range_wrapper(dev, irqs, min_vecs, > > > + max_vecs); > > > + if (vecs > 0) > > > + goto done; > > > + } > > > + > > > + vecs = pci_enable_msi_range(dev, min_vecs, max_vecs); > > > + if (vecs > 0) { > > > + for (i = 0; i < vecs; i++) > > > + irqs[i] = dev->irq + i; > > > + goto done; > > > + } > > > + > > > + if (min_vecs > 1) > > > + return -ENOSPC; > > > > irqs is leaked if (min_vecs > 1) > > > > You can get rid of this check at all if you reorganize your code i.e. > > like this: > > > > ... > > > > vecs = pci_enable_msi_range(dev, min_vecs, max_vecs); > > if (vecs < 0) > > goto legacy; > > > > for (i = 0; i < vecs; i++) > > irqs[i] = dev->irq + i; > > > > done: > > ... > > > > > > legacy: > > ... > > I've just moved the if below the kfree. I think I need to look at the updated version :) > > > +#define PCI_IRQ_NOMSIX (1 << 0) /* don't try to use MSI-X interrupts */ > > > > BTW, why PCI_IRQ_NOMSIX only and no PCI_IRQ_NOMSI? > > Because there is no need to call this API if your device only supports > a single legacy vector anyway. What if a device reports (up to 32) MSIs and MSI-X allocation failed? The driver might prefer the legacy single (i.e. due to errata in MSI), but there is no flag to ask for it. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 30, 2016 at 07:28:09PM +0200, Alexander Gordeev wrote: > I think I need to look at the updated version :) I've astarted processing the comments for the whole series and plan to post an update tomorrow. > > > > +#define PCI_IRQ_NOMSIX (1 << 0) /* don't try to use MSI-X interrupts */ > > > > > > BTW, why PCI_IRQ_NOMSIX only and no PCI_IRQ_NOMSI? > > > > Because there is no need to call this API if your device only supports > > a single legacy vector anyway. > > What if a device reports (up to 32) MSIs and MSI-X allocation failed? The > driver might prefer the legacy single (i.e. due to errata in MSI), but > there is no flag to ask for it. Ok, I'll add it for now. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index a080f44..a33adec 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,115 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries, } EXPORT_SYMBOL(pci_enable_msix_range); +static unsigned 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, + unsigned int min_vecs, unsigned int max_vecs) +{ + struct msix_entry *msix_entries; + int vecs, i; + + 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; + + vecs = pci_enable_msix_range(pdev, msix_entries, min_vecs, max_vecs); + if (vecs > 0) { + for (i = 0; i < vecs; i++) + irqs[i] = msix_entries[i].vector; + } + + kfree(msix_entries); + return vecs; +} + +/** + * 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. The Linux irq numbers for the allocated + * vectors are stored in pdev->irqs. If less than @min_vecs interrupt + * vectors are available for @dev the function will fail with -ENOSPC. + */ +int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, + unsigned int max_vecs, unsigned int flags) +{ + unsigned int vecs, i; + u32 *irqs; + + max_vecs = min(max_vecs, pci_nr_irq_vectors(dev)); + + irqs = kcalloc(max_vecs, sizeof(u32), GFP_KERNEL); + if (!irqs) + return -ENOMEM; + + if (!(flags & PCI_IRQ_NOMSIX)) { + vecs = pci_enable_msix_range_wrapper(dev, irqs, min_vecs, + max_vecs); + if (vecs > 0) + goto done; + } + + vecs = pci_enable_msi_range(dev, min_vecs, max_vecs); + if (vecs > 0) { + for (i = 0; i < vecs; i++) + irqs[i] = dev->irq + i; + goto done; + } + + if (min_vecs > 1) + return -ENOSPC; + + /* use legacy irq */ + kfree(irqs); + dev->irqs = &dev->irq; + return 1; + +done: + dev->irqs = irqs; + return vecs; +} +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); +} +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..84a20fc 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; + unsigned int *irqs; struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */ bool match_driver; /* Skip attaching driver */ @@ -1237,6 +1238,8 @@ 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_NOMSIX (1 << 0) /* don't try to use MSI-X interrupts */ + /* kmem_cache style wrapper around pci_alloc_consistent() */ #include <linux/pci-dma.h> @@ -1284,6 +1287,9 @@ 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); #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 +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 min_vecs, unsigned int max_vecs, + unsigned int flags) +{ + if (min_vecs > 1) + return -ENOSPC; + dev->irqs = &dev->irq; + return 1; +} +static inline void pci_free_irq_vectors(struct pci_dev *dev) +{ +} #endif #ifdef CONFIG_PCIEPORTBUS
Add a helper 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 helper. The next patch will also add automatic spreading of MSI / MSI-X vectors to this function. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/pci/msi.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 18 +++++++++ 2 files changed, 128 insertions(+)