| Message ID | 20221111122013.653556720@linutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | genirq, PCI/MSI: Support for per device MSI and PCI/IMS - Part 1 cleanups | expand |
On Fri, Nov 11, 2022 at 02:54:15PM +0100, Thomas Gleixner wrote: > PCI/MSI and PCI/MSI-X are mutually exclusive, but the MSI-X enable code > lacks a check for already enabled MSI. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/pci/msi/msi.c | 5 +++++ > 1 file changed, 5 insertions(+) > > --- a/drivers/pci/msi/msi.c > +++ b/drivers/pci/msi/msi.c > @@ -935,6 +935,11 @@ static int __pci_enable_msix_range(struc > if (maxvec < minvec) > return -ERANGE; > > + if (dev->msi_enabled) { > + pci_info(dev, "can't enable MSI-X (MSI already enabled)\n"); > + return -EINVAL; > + } > + nit: Can the pre-enabled checks for msi and msix be moved up before any vector range check? not that it matters for how it fails, does EBUSY sound better? looks good. Reviewed-by: Ashok Raj <ashok.raj@intel.com> > if (WARN_ON_ONCE(dev->msix_enabled)) > return -EINVAL;
On Fri, Nov 11, 2022 at 02:54:15PM +0100, Thomas Gleixner wrote: > PCI/MSI and PCI/MSI-X are mutually exclusive, but the MSI-X enable code > lacks a check for already enabled MSI. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/msi/msi.c | 5 +++++ > 1 file changed, 5 insertions(+) > > --- a/drivers/pci/msi/msi.c > +++ b/drivers/pci/msi/msi.c > @@ -935,6 +935,11 @@ static int __pci_enable_msix_range(struc > if (maxvec < minvec) > return -ERANGE; > > + if (dev->msi_enabled) { > + pci_info(dev, "can't enable MSI-X (MSI already enabled)\n"); > + return -EINVAL; > + } > + > if (WARN_ON_ONCE(dev->msix_enabled)) > return -EINVAL; > >
On Fri, Nov 11, 2022 at 02:54:15PM +0100, Thomas Gleixner wrote: > PCI/MSI and PCI/MSI-X are mutually exclusive, but the MSI-X enable code > lacks a check for already enabled MSI. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/pci/msi/msi.c | 5 +++++ > 1 file changed, 5 insertions(+) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Wed, Nov 16 2022 at 07:39, Ashok Raj wrote: > On Fri, Nov 11, 2022 at 02:54:15PM +0100, Thomas Gleixner wrote: > > Can the pre-enabled checks for msi and msix be moved up before any vector > range check? > > not that it matters for how it fails, does EBUSY sound better? Does any caller care about the error code or about the ordering in which the caller stupity is detected?
On Thu, Nov 17, 2022 at 02:07:33PM +0100, Thomas Gleixner wrote: > On Wed, Nov 16 2022 at 07:39, Ashok Raj wrote: > > On Fri, Nov 11, 2022 at 02:54:15PM +0100, Thomas Gleixner wrote: > > > > Can the pre-enabled checks for msi and msix be moved up before any vector > > range check? > > > > not that it matters for how it fails, does EBUSY sound better? > > Does any caller care about the error code or about the ordering in which > the caller stupity is detected? No, I don't think so. That's why I prefixed it with "not that it matters" :-) Just thought it would be good hygiene, but doesn't change anything functionally.
> From: Thomas Gleixner <tglx@linutronix.de> > Sent: Friday, November 11, 2022 9:54 PM > > PCI/MSI and PCI/MSI-X are mutually exclusive, but the MSI-X enable code > lacks a check for already enabled MSI. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/pci/msi/msi.c | 5 +++++ > 1 file changed, 5 insertions(+) > > --- a/drivers/pci/msi/msi.c > +++ b/drivers/pci/msi/msi.c > @@ -935,6 +935,11 @@ static int __pci_enable_msix_range(struc > if (maxvec < minvec) > return -ERANGE; > > + if (dev->msi_enabled) { > + pci_info(dev, "can't enable MSI-X (MSI already enabled)\n"); > + return -EINVAL; > + } > + > if (WARN_ON_ONCE(dev->msix_enabled)) > return -EINVAL; > a same check remains in __pci_enable_msix(): /* Check whether driver already requested for MSI IRQ */ if (dev->msi_enabled) { pci_info(dev, "can't enable MSI-X (MSI IRQ already assigned)\n"); return -EINVAL; } return msix_capability_init(dev, entries, nvec, affd); It's removed later in patch33 when sanitizing MSI-X checks. But logically the removal can come with this patch.
--- a/drivers/pci/msi/msi.c +++ b/drivers/pci/msi/msi.c @@ -935,6 +935,11 @@ static int __pci_enable_msix_range(struc if (maxvec < minvec) return -ERANGE; + if (dev->msi_enabled) { + pci_info(dev, "can't enable MSI-X (MSI already enabled)\n"); + return -EINVAL; + } + if (WARN_ON_ONCE(dev->msix_enabled)) return -EINVAL;
PCI/MSI and PCI/MSI-X are mutually exclusive, but the MSI-X enable code lacks a check for already enabled MSI. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- drivers/pci/msi/msi.c | 5 +++++ 1 file changed, 5 insertions(+)