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 <[email protected]>
---
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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
> ---
> 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 <[email protected]>
> ---
> drivers/pci/msi/msi.c | 5 +++++
> 1 file changed, 5 insertions(+)
Reviewed-by: Jason Gunthorpe <[email protected]>
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.
The following commit has been merged into the irq/core branch of tip:
Commit-ID: fe97f59a78fefda6c4a8c8ee6a070b1f769fc801
Gitweb: https://git.kernel.org/tip/fe97f59a78fefda6c4a8c8ee6a070b1f769fc801
Author: Thomas Gleixner <[email protected]>
AuthorDate: Fri, 11 Nov 2022 14:54:15 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Thu, 17 Nov 2022 15:15:18 +01:00
PCI/MSI: Check for MSI enabled in __pci_msix_enable()
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 <[email protected]>
Reviewed-by: Ashok Raj <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/pci/msi/msi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index fdd2ec0..160af9f 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -935,6 +935,11 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
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;
> From: Thomas Gleixner <[email protected]>
> 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 <[email protected]>
> ---
> 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.