| Message ID | 1480558504-18691-1-git-send-email-dennis.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dennis, I've fixed ahci to treat all errors the same in the meantime, please try latest Linux tree. That being said I don't like the different error returns from __pci_enable_msi_range (and __pci_enable_msix_range), but they have been there for a while.
On Thu, Dec 01, 2016 at 09:52:43AM +0100, Christoph Hellwig wrote: > Hi Dennis, > > I've fixed ahci to treat all errors the same in the meantime, please > try latest Linux tree. That being said I don't like the different > error returns from __pci_enable_msi_range (and __pci_enable_msix_range), > but they have been there for a while. Ah, I've noticed that you have the fix recently which is somehow to weaken the necessary of the change. But, that also being said that I don't like we insist at least the inconsistent either just because something has been there *for a while*. Both below comments from cpi_alloc_irq_vectors_affinity() and the logic itself leads us to think that the correct return value is -NOSPC: /** *... *Return the number of vectors allocated, * (which might be smaller than @max_vecs) if successful, or a negative * error code on error. If less than @min_vecs interrupt vectors are * available for @dev the function will fail with -ENOSPC. * ... */ People maybe argue that almost has no device drivers depending on the different return value, then why we still need to do that? Thanks, Dennis
On Thu, Dec 01, 2016 at 10:15:04AM +0800, Dennis Chen wrote: > The __pci_enable_msi_range() should return -ENOSPC instead of -EINVAL > when the device doesn't have enough vectors as required, just as the > MSI-X vector allocator does in __pci_enable_msix_range(). Otherwise, > some drivers depending on that return value will probably fallback to > the legacy interrupt directly, for example, in commit 17a51f12cfbd2814 > ("ahci: only try to use multi-MSI mode if there is more than 1 port"), the > ahci driver will fallback to single MSI mode only when the return value > is -ENOSPC in case of required vectors is not enough, else the driver will > use legacy interrupt which has been observed on a x86 box with 6-port SATA > controller. Unless Christoph objects, I'll apply this, but I don't understand the situation with 17a51f12cfbd. That commit doesn't check for EINVAL or ENOSPC so I don't know what the connection with this patch is. I know Christoph said he changed something in ahci to treat all errors the same, but I don't know where that is, either. If there's a revision of Linus' tree that is broken, please give the details so I can at least describe which versions are broken, when it got fixed by Christoph, and figure out whether we need stable backports or anything. Bjorn > With this patch, when a MSI-capable device doesn't have enough MSI > vectors as requested, it will fallback to single MSI mode while not > legacy interrupt. > > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > Cc: Tejun Heo <tj@kernel.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Tom Long Nguyen <tom.l.nguyen@intel.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Steve Capper <steve.capper@arm.com> > Cc: linux-ide@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > --- > drivers/pci/msi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index ad70507..da37113 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -1084,7 +1084,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, > if (nvec < 0) > return nvec; > if (nvec < minvec) > - return -EINVAL; > + return -ENOSPC; > > if (nvec > maxvec) > nvec = maxvec; > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 11, 2017 at 12:18:53PM -0600, Bjorn Helgaas wrote: > Unless Christoph objects, I'll apply this, but I don't understand the > situation with 17a51f12cfbd. That commit doesn't check for EINVAL or > ENOSPC so I don't know what the connection with this patch is. I don't think that commit is the culprit. > I know Christoph said he changed something in ahci to treat all errors > the same, but I don't know where that is, either. "ahci: always fall back to single-MSI mode"
On Thu, Dec 01, 2016 at 10:15:04AM +0800, Dennis Chen wrote: > The __pci_enable_msi_range() should return -ENOSPC instead of -EINVAL > when the device doesn't have enough vectors as required, just as the > MSI-X vector allocator does in __pci_enable_msix_range(). Otherwise, > some drivers depending on that return value will probably fallback to > the legacy interrupt directly, for example, in commit 17a51f12cfbd2814 > ("ahci: only try to use multi-MSI mode if there is more than 1 port"), the > ahci driver will fallback to single MSI mode only when the return value > is -ENOSPC in case of required vectors is not enough, else the driver will > use legacy interrupt which has been observed on a x86 box with 6-port SATA > controller. > > With this patch, when a MSI-capable device doesn't have enough MSI > vectors as requested, it will fallback to single MSI mode while not > legacy interrupt. > > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > Cc: Tejun Heo <tj@kernel.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Tom Long Nguyen <tom.l.nguyen@intel.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Steve Capper <steve.capper@arm.com> > Cc: linux-ide@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org Applied to pci/msi for v4.11, thanks, Dennis! > --- > drivers/pci/msi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index ad70507..da37113 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -1084,7 +1084,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, > if (nvec < 0) > return nvec; > if (nvec < minvec) > - return -EINVAL; > + return -ENOSPC; > > if (nvec > maxvec) > nvec = maxvec; > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index ad70507..da37113 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -1084,7 +1084,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec, if (nvec < 0) return nvec; if (nvec < minvec) - return -EINVAL; + return -ENOSPC; if (nvec > maxvec) nvec = maxvec;
The __pci_enable_msi_range() should return -ENOSPC instead of -EINVAL when the device doesn't have enough vectors as required, just as the MSI-X vector allocator does in __pci_enable_msix_range(). Otherwise, some drivers depending on that return value will probably fallback to the legacy interrupt directly, for example, in commit 17a51f12cfbd2814 ("ahci: only try to use multi-MSI mode if there is more than 1 port"), the ahci driver will fallback to single MSI mode only when the return value is -ENOSPC in case of required vectors is not enough, else the driver will use legacy interrupt which has been observed on a x86 box with 6-port SATA controller. With this patch, when a MSI-capable device doesn't have enough MSI vectors as requested, it will fallback to single MSI mode while not legacy interrupt. Signed-off-by: Dennis Chen <dennis.chen@arm.com> Cc: Tejun Heo <tj@kernel.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Tom Long Nguyen <tom.l.nguyen@intel.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Steve Capper <steve.capper@arm.com> Cc: linux-ide@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org --- drivers/pci/msi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)