| Message ID | 1480558504-18691-1-git-send-email-dennis.chen@arm.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(-)