[PATCH 0/7] Phase out pci_enable_msi_block()

[PATCH 0/7] Phase out pci_enable_msi_block()

[Alexander Gordeev]

As result of recent deprecation of MSI-X/MSI enablement
interfaces pci_enable_msi_block(), pci_enable_msi() and
pci_enable_msix() all drivers need to be updated to use
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

This is the first in a series of updates, to phase out
pci_enable_msi_block() function.

This series is against pci/msi branch in Bjorn Helgaas's repo:
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git

Thanks!

Alexander Gordeev (7):
  ipr: Do not call pci_disable_msi/msix() if pci_enable_msi/msix()
    failed
  ipr: Use new interfaces for MSI/MSI-X enablement
  ahci: Use new interfaces for MSI/MSI-X enablement
  nvme: Use new interfaces for MSI/MSI-X enablement
  vfio: Use new interfaces for MSI/MSI-X enablement
  ath10k: Use new interfaces for MSI enablement
  wil6210: Use new interfaces for MSI enablement

 drivers/ata/ahci.c                          |   15 +++-----
 drivers/block/nvme-core.c                   |   33 ++++-------------
 drivers/net/wireless/ath/ath10k/pci.c       |   22 ++++++------
 drivers/net/wireless/ath/wil6210/pcie_bus.c |   36 ++++++++++---------
 drivers/scsi/ipr.c                          |   51 +++++++++-----------------
 drivers/vfio/pci/vfio_pci_intrs.c           |    8 ++--
 6 files changed, 66 insertions(+), 99 deletions(-)

-- 
1.7.7.6

[PATCH 5/7] vfio: Use new interfaces for MSI/MSI-X enablement

[Alexander Gordeev]

This update also fixes a bug when deprecated pci_enable_msix()
and pci_enable_msi_block() functions return a positive return
value which indicats the number of interrupts that could have
been allocated rather than a successful allocation. The driver
misinterpreted this value and assumed MSI-X/MSIs are enabled,
although in fact it were not.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/vfio/pci/vfio_pci_intrs.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 641bc87..66d1746 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -482,15 +482,15 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
 		for (i = 0; i < nvec; i++)
 			vdev->msix[i].entry = i;
 
-		ret = pci_enable_msix(pdev, vdev->msix, nvec);
-		if (ret) {
+		ret = pci_enable_msix_range(pdev, vdev->msix, nvec, nvec);
+		if (ret < 0) {
 			kfree(vdev->msix);
 			kfree(vdev->ctx);
 			return ret;
 		}
 	} else {
-		ret = pci_enable_msi_block(pdev, nvec);
-		if (ret) {
+		ret = pci_enable_msi_range(pdev, nvec, nvec);
+		if (ret < 0) {
 			kfree(vdev->ctx);
 			return ret;
 		}
-- 
1.7.7.6

Re: [PATCH 5/7] vfio: Use new interfaces for MSI/MSI-X enablement

[Alex Williamson]

On Tue, 2014-01-07 at 19:05 +0100, Alexander Gordeev wrote:
> This update also fixes a bug when deprecated pci_enable_msix()
> and pci_enable_msi_block() functions return a positive return
> value which indicats the number of interrupts that could have
> been allocated rather than a successful allocation. The driver
> misinterpreted this value and assumed MSI-X/MSIs are enabled,
> although in fact it were not.

No, the driver interpreted it correctly, which is why anything other
than zero is handled as an error.  This patch looks incorrect if the new
interfaces follow the same return convention.  Thanks,

Alex

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci_intrs.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 641bc87..66d1746 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -482,15 +482,15 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
>  		for (i = 0; i < nvec; i++)
>  			vdev->msix[i].entry = i;
>  
> -		ret = pci_enable_msix(pdev, vdev->msix, nvec);
> -		if (ret) {
> +		ret = pci_enable_msix_range(pdev, vdev->msix, nvec, nvec);
> +		if (ret < 0) {
>  			kfree(vdev->msix);
>  			kfree(vdev->ctx);
>  			return ret;
>  		}
>  	} else {
> -		ret = pci_enable_msi_block(pdev, nvec);
> -		if (ret) {
> +		ret = pci_enable_msi_range(pdev, nvec, nvec);
> +		if (ret < 0) {
>  			kfree(vdev->ctx);
>  			return ret;
>  		}

Re: [PATCH 5/7] vfio: Use new interfaces for MSI/MSI-X enablement

[Alexander Gordeev]

On Tue, Jan 07, 2014 at 11:34:13AM -0700, Alex Williamson wrote:
> On Tue, 2014-01-07 at 19:05 +0100, Alexander Gordeev wrote:
> > This update also fixes a bug when deprecated pci_enable_msix()
> > and pci_enable_msi_block() functions return a positive return
> > value which indicats the number of interrupts that could have
> > been allocated rather than a successful allocation. The driver
> > misinterpreted this value and assumed MSI-X/MSIs are enabled,
> > although in fact it were not.
> 
> No, the driver interpreted it correctly, which is why anything other
> than zero is handled as an error.  This patch looks incorrect if the new
> interfaces follow the same return convention.  Thanks,

The new interfaces differ wrt the return value - it is eigher a negative
error code or a positive number of successfuly allocated vectors. 

If the user level makes use of a number of vectors that could have been
allocated then it should cease doing it, since only 0 or a negative error
code is returned after this update.

The changelog is incorrect as the driver indeed bailes out on positive
return values. I will send a updated version.

> Alex

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v2 5/7] vfio: Use new interfaces for MSI/MSI-X enablement

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/vfio/pci/vfio_pci_intrs.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 641bc87..66d1746 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -482,15 +482,15 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
 		for (i = 0; i < nvec; i++)
 			vdev->msix[i].entry = i;
 
-		ret = pci_enable_msix(pdev, vdev->msix, nvec);
-		if (ret) {
+		ret = pci_enable_msix_range(pdev, vdev->msix, nvec, nvec);
+		if (ret < 0) {
 			kfree(vdev->msix);
 			kfree(vdev->ctx);
 			return ret;
 		}
 	} else {
-		ret = pci_enable_msi_block(pdev, nvec);
-		if (ret) {
+		ret = pci_enable_msi_range(pdev, nvec, nvec);
+		if (ret < 0) {
 			kfree(vdev->ctx);
 			return ret;
 		}
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 5/7] vfio: Use new interfaces for MSI/MSI-X enablement

[Alex Williamson]

On Wed, 2014-01-08 at 08:57 +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci_intrs.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 641bc87..66d1746 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -482,15 +482,15 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
>  		for (i = 0; i < nvec; i++)
>  			vdev->msix[i].entry = i;
>  
> -		ret = pci_enable_msix(pdev, vdev->msix, nvec);
> -		if (ret) {
> +		ret = pci_enable_msix_range(pdev, vdev->msix, nvec, nvec);
> +		if (ret < 0) {
>  			kfree(vdev->msix);
>  			kfree(vdev->ctx);
>  			return ret;
>  		}
>  	} else {
> -		ret = pci_enable_msi_block(pdev, nvec);
> -		if (ret) {
> +		ret = pci_enable_msi_range(pdev, nvec, nvec);
> +		if (ret < 0) {
>  			kfree(vdev->ctx);
>  			return ret;
>  		}

Based on your description, this is a user visible API change.  We now
return success so long as we allocated at least a single vector and the
user has no way to know that they didn't get all the vectors they
requested.  That's unacceptable, userspace expects the old API - setup
the requested vectors or setup none and tell me how many to retry with.
To maintain the same API exposed to userspace, I'd think we need
something like:

if (ret != nvec) {
	if (ret > 0)
		pci_disable...
	kfree(...
	kfree(...
	return ret;
}

Thanks,
Alex

[PATCH v3 5/7] vfio: Use new interfaces for MSI/MSI-X enablement

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/vfio/pci/vfio_pci_intrs.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 641bc87..4a9db1d 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -482,15 +482,19 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
 		for (i = 0; i < nvec; i++)
 			vdev->msix[i].entry = i;
 
-		ret = pci_enable_msix(pdev, vdev->msix, nvec);
-		if (ret) {
+		ret = pci_enable_msix_range(pdev, vdev->msix, 1, nvec);
+		if (ret < nvec) {
+			if (ret > 0)
+				pci_disable_msix(pdev);
 			kfree(vdev->msix);
 			kfree(vdev->ctx);
 			return ret;
 		}
 	} else {
-		ret = pci_enable_msi_block(pdev, nvec);
-		if (ret) {
+		ret = pci_enable_msi_range(pdev, 1, nvec);
+		if (ret < nvec) {
+			if (ret > 0)
+				pci_disable_msi(pdev);
 			kfree(vdev->ctx);
 			return ret;
 		}
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v3 5/7] vfio: Use new interfaces for MSI/MSI-X enablement

[Alex Williamson]

On Fri, 2014-01-10 at 08:42 +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

Thanks for the changes.

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> ---
>  drivers/vfio/pci/vfio_pci_intrs.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 641bc87..4a9db1d 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -482,15 +482,19 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
>  		for (i = 0; i < nvec; i++)
>  			vdev->msix[i].entry = i;
>  
> -		ret = pci_enable_msix(pdev, vdev->msix, nvec);
> -		if (ret) {
> +		ret = pci_enable_msix_range(pdev, vdev->msix, 1, nvec);
> +		if (ret < nvec) {
> +			if (ret > 0)
> +				pci_disable_msix(pdev);
>  			kfree(vdev->msix);
>  			kfree(vdev->ctx);
>  			return ret;
>  		}
>  	} else {
> -		ret = pci_enable_msi_block(pdev, nvec);
> -		if (ret) {
> +		ret = pci_enable_msi_range(pdev, 1, nvec);
> +		if (ret < nvec) {
> +			if (ret > 0)
> +				pci_disable_msi(pdev);
>  			kfree(vdev->ctx);
>  			return ret;
>  		}
> -- 
> 1.7.7.6
>