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; }
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
