[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 3/7] ahci: Use new interfaces for MSI/MSI-X enablement

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/ata/ahci.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 8516f4d..cfdb079 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1098,13 +1098,13 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
 int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 			 struct ahci_host_priv *hpriv)
 {
-	int rc, nvec;
+	int nvec;
 
 	if (hpriv->flags & AHCI_HFLAG_NO_MSI)
 		goto intx;
 
-	rc = pci_msi_vec_count(pdev);
-	if (rc < 0)
+	nvec = pci_msi_vec_count(pdev);
+	if (nvec < 0)
 		goto intx;
 
 	/*
@@ -1112,19 +1112,16 @@ int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 	 * Message mode could be enforced. In this case assume that advantage
 	 * of multipe MSIs is negated and use single MSI mode instead.
 	 */
-	if (rc < n_ports)
+	if (nvec < n_ports)
 		goto single_msi;
 
-	nvec = rc;
-	rc = pci_enable_msi_block(pdev, nvec);
-	if (rc)
+	if (pci_enable_msi_range(pdev, nvec, nvec) < 0)
 		goto intx;
 
 	return nvec;
 
 single_msi:
-	rc = pci_enable_msi(pdev);
-	if (rc)
+	if (pci_enable_msi_range(pdev, 1, 1) < 0)
 		goto intx;
 	return 1;
 
-- 
1.7.7.6

Re: [PATCH 3/7] ahci: Use new interfaces for MSI/MSI-X enablement

[Bjorn Helgaas]

On Tue, Jan 07, 2014 at 07:05:38PM +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/ata/ahci.c |   15 ++++++---------
>  1 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 8516f4d..cfdb079 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1098,13 +1098,13 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
>  int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
>  			 struct ahci_host_priv *hpriv)
>  {
> -	int rc, nvec;
> +	int nvec;
>  
>  	if (hpriv->flags & AHCI_HFLAG_NO_MSI)
>  		goto intx;
>  
> -	rc = pci_msi_vec_count(pdev);
> -	if (rc < 0)
> +	nvec = pci_msi_vec_count(pdev);
> +	if (nvec < 0)
>  		goto intx;
>  
>  	/*
> @@ -1112,19 +1112,16 @@ int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
>  	 * Message mode could be enforced. In this case assume that advantage
>  	 * of multipe MSIs is negated and use single MSI mode instead.
>  	 */
> -	if (rc < n_ports)
> +	if (nvec < n_ports)
>  		goto single_msi;
>  
> -	nvec = rc;
> -	rc = pci_enable_msi_block(pdev, nvec);
> -	if (rc)
> +	if (pci_enable_msi_range(pdev, nvec, nvec) < 0)
>  		goto intx;
>  
>  	return nvec;
>  
>  single_msi:
> -	rc = pci_enable_msi(pdev);
> -	if (rc)
> +	if (pci_enable_msi_range(pdev, 1, 1) < 0)

This part doesn't seem like an improvement.  There are a hundred or so
callers of pci_enable_msi() that only want a single MSI.  Is there any
benefit in changing them to use pci_enable_msi_range()?

I guess I agreed (maybe even suggested) to deprecate pci_enable_msi(),
but it doesn't suffer from the tri-state return value problem, and I'm
having second thoughts.

>  		goto intx;
>  	return 1;
>  
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] ahci: Use new interfaces for MSI/MSI-X enablement

[Alexander Gordeev]

On Mon, Jan 13, 2014 at 12:12:20PM -0700, Bjorn Helgaas wrote:
> > -	nvec = rc;
> > -	rc = pci_enable_msi_block(pdev, nvec);
> > -	if (rc)
> > +	if (pci_enable_msi_range(pdev, nvec, nvec) < 0)
> >  		goto intx;
> >  
> >  	return nvec;
> >  
> >  single_msi:
> > -	rc = pci_enable_msi(pdev);
> > -	if (rc)
> > +	if (pci_enable_msi_range(pdev, 1, 1) < 0)
> 
> This part doesn't seem like an improvement.  There are a hundred or so
> callers of pci_enable_msi() that only want a single MSI.  Is there any
> benefit in changing them to use pci_enable_msi_range()?

In this particular case it reads better to me as one sees on the screen
pci_enable_msi_range(pdev, nvec, nvec) and pci_enable_msi_range(pdev, 1, 1)
calls. That allows to avoid switching in mind between negative-or-positive
return in the former call and negative-or-zero return from pci_enable_msi()
if we had it.

But in most cases when single MSI is enabled we do cause complication
with the patterns below (which I expect I am going be hated for ;) ):


-	rc = pci_enable_msi(pdev);
-	if (rc)
+	rc = pci_enable_msi_range(pdev, 1, 1);
+	if (rc < 0)
		...


-	rc = pci_enable_msi(pdev);
-	if (!rc)
+	rc = pci_enable_msi_range(pdev, 1, 1);
+	if (rc > 0)
		...

 
I think we have a tradeoff between the interface simplicity and code clarity.
What if we try to address both goals by making pci_enable_msi() a helper over
pci_enable_msi_range(pdev, 1, 1)? In this case the return value will match
negative-or-positive binary semantics while reads almost as good as it used to:


-	rc = pci_enable_msi(pdev);
-	if (rc)
+	rc = pci_enable_msi(pdev);
+	if (rc < 0)
		...


-	rc = pci_enable_msi(pdev);
-	if (!rc)
+	rc = pci_enable_msi(pdev);
+	if (rc > 0)
		...


The whole interface would not be inflated as well, with just:

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index a8d0100..fa0b27d 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -158,6 +158,9 @@ static int foo_driver_enable_single_msi(struct pci_dev *pdev)
 	return pci_enable_msi_range(pdev, 1, 1);
 }
 
+A helper function pci_enable_msi() could be used instead. Note, as just
+one MSI is requested it could return either a negative errno or 1.
+
 4.2.2 pci_disable_msi
 
 void pci_disable_msi(struct pci_dev *dev)


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 3/7] ahci: Use new interfaces for MSI/MSI-X enablement

[Alexander Gordeev]

On Tue, Jan 14, 2014 at 09:50:40AM +0100, Alexander Gordeev wrote:
> What if we try to address both goals by making pci_enable_msi() a helper over
> pci_enable_msi_range(pdev, 1, 1)?

Or pci_enable_msi_single() to help reading and avoid drivers conversion mess.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 3/7] ahci: Use new interfaces for MSI/MSI-X enablement

[Bjorn Helgaas]

On Tue, Jan 14, 2014 at 1:50 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Mon, Jan 13, 2014 at 12:12:20PM -0700, Bjorn Helgaas wrote:
>> > -   nvec = rc;
>> > -   rc = pci_enable_msi_block(pdev, nvec);
>> > -   if (rc)
>> > +   if (pci_enable_msi_range(pdev, nvec, nvec) < 0)
>> >             goto intx;
>> >
>> >     return nvec;
>> >
>> >  single_msi:
>> > -   rc = pci_enable_msi(pdev);
>> > -   if (rc)
>> > +   if (pci_enable_msi_range(pdev, 1, 1) < 0)
>>
>> This part doesn't seem like an improvement.  There are a hundred or so
>> callers of pci_enable_msi() that only want a single MSI.  Is there any
>> benefit in changing them to use pci_enable_msi_range()?
>
> In this particular case it reads better to me as one sees on the screen
> pci_enable_msi_range(pdev, nvec, nvec) and pci_enable_msi_range(pdev, 1, 1)
> calls. That allows to avoid switching in mind between negative-or-positive
> return in the former call and negative-or-zero return from pci_enable_msi()
> if we had it.
>
> But in most cases when single MSI is enabled we do cause complication
> with the patterns below (which I expect I am going be hated for ;) ):

I don't want to touch those hundred pci_enable_msi() callers at all.
So I think we should have something like this:

    /* Return 0 for success (one MSI enabled) or negative errno */
    static inline int pci_enable_msi(struct pci_dev *dev)
    {
        int rc;

        rc = pci_enable_msi_range(pdev, 1, 1);
        if (rc == 1)
            return 0;
        return rc;
    }