On Mon, Dec 31, 2018 at 04:00:59PM -0600, Bjorn Helgaas wrote:
> On Sat, Dec 29, 2018 at 11:26:48AM +0800, Ming Lei wrote:
> > The API of pci_alloc_irq_vectors_affinity() requires to return -ENOSPC
> > if leass than @min_vecs interrupt vectors are available for @dev.
>
> s/leass/fewer/
>
> > However, this way may be changed by falling back to
> > __pci_enable_msi_range(), for example, if the device isn't capable of
> > MSI, __pci_enable_msi_range() will return -EINVAL, and finally it is
> > returned to users of pci_alloc_irq_vectors_affinity() even though
> > there are quite MSIX vectors available. This way violates the interface.
>
> I *think* the above means:
>
> If a device supports MSI-X but not MSI and a caller requests
> @min_vecs that can't be satisfied by MSI-X, we previously returned
> -EINVAL (from the failed attempt to enable MSI), not -ENOSPC.
>
> and I agree that this doesn't match the documented API.
OK, will use the above comment log.
>
> > Users of pci_alloc_irq_vectors_affinity() may try to reduce irq
> > vectors and allocate vectors again in case that -ENOSPC is returned, such
> > as NVMe, so we need to respect the current interface and give preference to
> > -ENOSPC.
>
> I thought the whole point of the (min_vecs, max_vecs) tuple was to
> avoid this sort of "reduce and try again" iteration in the callers.
As Keith replied, in case of NVMe, we have to keep min_vecs same with
max_vecs.
>
> > Cc: Jens Axboe <axboe@xxxxxx>,
> > Cc: Keith Busch <keith.busch@xxxxxxxxx>,
> > Cc: linux-pci@xxxxxxxxxxxxxxx,
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>,
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > drivers/pci/msi.c | 20 +++++++++++---------
> > 1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 7a1c8a09efa5..91b4f03fee91 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -1168,7 +1168,8 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
> > const struct irq_affinity *affd)
> > {
> > static const struct irq_affinity msi_default_affd;
> > - int vecs = -ENOSPC;
> > + int msix_vecs = -ENOSPC;
> > + int msi_vecs = -ENOSPC;
> >
> > if (flags & PCI_IRQ_AFFINITY) {
> > if (!affd)
> > @@ -1179,16 +1180,17 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
> > }
> >
> > if (flags & PCI_IRQ_MSIX) {
> > - vecs = __pci_enable_msix_range(dev, NULL, min_vecs, max_vecs,
> > - affd);
> > - if (vecs > 0)
> > - return vecs;
> > + msix_vecs = __pci_enable_msix_range(dev, NULL, min_vecs,
> > + max_vecs, affd);
> > + if (msix_vecs > 0)
> > + return msix_vecs;
> > }
> >
> > if (flags & PCI_IRQ_MSI) {
> > - vecs = __pci_enable_msi_range(dev, min_vecs, max_vecs, affd);
> > - if (vecs > 0)
> > - return vecs;
> > + msi_vecs = __pci_enable_msi_range(dev, min_vecs, max_vecs,
> > + affd);
> > + if (msi_vecs > 0)
> > + return msi_vecs;
> > }
> >
> > /* use legacy irq if allowed */
> > @@ -1199,7 +1201,7 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
> > }
> > }
> >
> > - return vecs;
> > + return msix_vecs == -ENOSPC ? msix_vecs : msi_vecs;
>
> If you know you want to return -ENOSPC, just return that, not a
> variable that happens to contain it, i.e.,
>
> if (msix_vecs == -ENOSPC)
> return -ENOSPC;
> return msi_vecs;
OK.
Thanks,
Ming
