All of lore.kernel.org
 help / color / mirror / Atom feed
From: helgaas@kernel.org (Bjorn Helgaas)
Subject: [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL
Date: Mon, 31 Dec 2018 15:51:47 -0600	[thread overview]
Message-ID: <20181231215147.GH159477@google.com> (raw)
In-Reply-To: <20181227130834.GA22967@lst.de>

On Thu, Dec 27, 2018@02:08:34PM +0100, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018@04:21:38PM +0800, Ming Lei wrote:
> > On Wed, Dec 26, 2018@07:20:27PM +0100, Christoph Hellwig wrote:
> > > On Wed, Dec 26, 2018@06:37:55PM +0800, Ming Lei wrote:
> > > > It is observed on QEMU that pci_alloc_irq_vectors_affinity() may
> > > > returns -EINVAL when the requested number is too big(such as 64).
> > > 
> > > Which is not how this API is supposed to work and documented to work.
> > > 
> > > We need to fix pci_alloc_irq_vectors_affinity to not return a spurious
> > > error and just return the allocated number of vectors instead of
> > > hacking around that in drivers.
> > 
> > Yeah, you are right.
> > 
> > The issue is that QEMU nvme-pci is MSIX-capable only, and hasn't MSI
> > capability.
> > 
> > __pci_enable_msix_range() actually returns -ENOSPC, but __pci_enable_msi_range()
> > returns -EINVAL because dev->msi_cap is zero.
> > 
> > Maybe we need the following fix?
> 
> Should it matter?  We still get a negative vecs back, and still fall
> back to the next option.  

I'm not sure how it matters either, since
pci_alloc_irq_vectors_affinity() will fail either way.  It *would* be
nice to return the correct error in case the caller uses it to emit a
message.

But if the caller wants to use -ENOSPC to reduce @min_vecs and try
again, that sounds like an incorrect use of the interface -- the
caller should have just used the smaller @min_vecs the first time
around.

> Unless ther are no irqs available at all
> for the selected types pci_alloc_irq_vectors_affinity should never
> return an error.

I don't quite understand this last sentence.  If @min_vecs == 5 and
the device only supports 4 MSI-X and 4 MSI vectors, the function
comment says we should fail with -ENOSPC.

Bjorn

WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>, Jens Axboe <axboe@fb.com>,
	linux-pci@vger.kernel.org, linux-nvme@lists.infradead.org,
	Keith Busch <keith.busch@intel.com>
Subject: Re: [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL
Date: Mon, 31 Dec 2018 15:51:47 -0600	[thread overview]
Message-ID: <20181231215147.GH159477@google.com> (raw)
In-Reply-To: <20181227130834.GA22967@lst.de>

On Thu, Dec 27, 2018 at 02:08:34PM +0100, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 04:21:38PM +0800, Ming Lei wrote:
> > On Wed, Dec 26, 2018 at 07:20:27PM +0100, Christoph Hellwig wrote:
> > > On Wed, Dec 26, 2018 at 06:37:55PM +0800, Ming Lei wrote:
> > > > It is observed on QEMU that pci_alloc_irq_vectors_affinity() may
> > > > returns -EINVAL when the requested number is too big(such as 64).
> > > 
> > > Which is not how this API is supposed to work and documented to work.
> > > 
> > > We need to fix pci_alloc_irq_vectors_affinity to not return a spurious
> > > error and just return the allocated number of vectors instead of
> > > hacking around that in drivers.
> > 
> > Yeah, you are right.
> > 
> > The issue is that QEMU nvme-pci is MSIX-capable only, and hasn't MSI
> > capability.
> > 
> > __pci_enable_msix_range() actually returns -ENOSPC, but __pci_enable_msi_range()
> > returns -EINVAL because dev->msi_cap is zero.
> > 
> > Maybe we need the following fix?
> 
> Should it matter?  We still get a negative vecs back, and still fall
> back to the next option.  

I'm not sure how it matters either, since
pci_alloc_irq_vectors_affinity() will fail either way.  It *would* be
nice to return the correct error in case the caller uses it to emit a
message.

But if the caller wants to use -ENOSPC to reduce @min_vecs and try
again, that sounds like an incorrect use of the interface -- the
caller should have just used the smaller @min_vecs the first time
around.

> Unless ther are no irqs available at all
> for the selected types pci_alloc_irq_vectors_affinity should never
> return an error.

I don't quite understand this last sentence.  If @min_vecs == 5 and
the device only supports 4 MSI-X and 4 MSI vectors, the function
comment says we should fail with -ENOSPC.

Bjorn

  parent reply	other threads:[~2018-12-31 21:51 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-26 10:37 [PATCH 0/2] nvme pci: two fixes on nvme_setup_irqs Ming Lei
2018-12-26 10:37 ` [PATCH 1/2] nvme pci: fix nvme_setup_irqs() Ming Lei
2018-12-26 10:37 ` [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL Ming Lei
2018-12-26 18:20   ` Christoph Hellwig
2018-12-27  8:21     ` Ming Lei
2018-12-27  8:21       ` Ming Lei
2018-12-27 13:08       ` Christoph Hellwig
2018-12-27 13:08         ` Christoph Hellwig
2018-12-27 22:16         ` Ming Lei
2018-12-27 22:16           ` Ming Lei
2018-12-31 21:51         ` Bjorn Helgaas [this message]
2018-12-31 21:51           ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181231215147.GH159477@google.com \
    --to=helgaas@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.