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;
--
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
