RE: [patch 01/39] PCI/MSI: Check for MSI enabled in __pci_msix_enable()

2022-11-17 Thread Tian, Kevin 
> From: Thomas Gleixner 
> 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 
> ---
>  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.

Re: [patch 01/39] PCI/MSI: Check for MSI enabled in __pci_msix_enable()

2022-11-17 Thread Ashok Raj 
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.

Re: [patch 01/39] PCI/MSI: Check for MSI enabled in __pci_msix_enable()

2022-11-17 Thread Thomas Gleixner 
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?

Re: [patch 01/39] PCI/MSI: Check for MSI enabled in __pci_msix_enable()

2022-11-16 Thread Jason Gunthorpe 
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 
> ---
>  drivers/pci/msi/msi.c |5 +
>  1 file changed, 5 insertions(+)

Reviewed-by: Jason Gunthorpe 

Jason

Re: [patch 01/39] PCI/MSI: Check for MSI enabled in __pci_msix_enable()

2022-11-16 Thread Bjorn Helgaas 
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 

Acked-by: Bjorn Helgaas 

> ---
>  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;
>  
>

Re: [patch 01/39] PCI/MSI: Check for MSI enabled in __pci_msix_enable()

2022-11-16 Thread Ashok Raj 
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 
> ---
>  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 

>   if (WARN_ON_ONCE(dev->msix_enabled))
>   return -EINVAL;

[patch 01/39] PCI/MSI: Check for MSI enabled in __pci_msix_enable()

2022-11-11 Thread Thomas Gleixner 
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 
---
 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;