[PATCH v3 00/11] Introduce pcim_enable_msi*() family helpers

[PATCH v3 00/11] Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

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

Changes from v2 to v3:
  - new public interfaces commented in drivers/pci/msi.c;
  - patch "Make quota traversing and requesting race-safe" explained;
  - pci_enable_msi/msix() 'nvec' arg type changed from 'unsigned int' to 'int';
  - pcim_enable_msi*() arg 'nvec' renamed to 'maxvec' when upper limit passed;
  - pcim_enable_msi*(..., maxvec, minvec) arg order swapped to minvec, maxvec;
  - "PCI: Fail MSI/MSI-X initialization if device is not in PCI_D0" commit
    869a161 and "PCI/MSI: Factor out pci_get_msi_cap() interface" patch
    conflicts resolved;

Tejun, due to last three changes will you keep your "Acked-By" and
"Reviewed-by" on these patches?
  PCI/MSI: Factor out pci_get_msi_cap() interface
  PCI/MSI: Get rid of pci_enable_msi_block_auto() interface
  PCI/MSI: Convert pci_msix_table_size() to a public interface
  PCI/MSI: Introduce pcim_enable_msi*() family helpers

Currently many device drivers need contiguously call functions
pci_enable_msix() for MSI-X or pci_enable_msi_block() for MSI
in a loop until success or failure. This update generalizes
this usage pattern and introduces pcim_enable_msi*() family
helpers.

As result, device drivers do not have to deal with tri-state
return values from pci_enable_msix() and pci_enable_msi_block()
functions directly and expected to have more clearer and straight
code.

So i.e. the request loop described in the documentation...

	int foo_driver_enable_msix(struct foo_adapter *adapter,
				   int nvec)
	{
		while (nvec >= FOO_DRIVER_MINIMUM_NVEC) {
			rc = pci_enable_msix(adapter->pdev,
					     adapter->msix_entries,
					     nvec);
			if (rc > 0)
				nvec = rc;
			else
				return rc;
		}

		return -ENOSPC;
	}

...would turn into a single helper call....

	rc = pcim_enable_msix_range(adapter->pdev,
				    adapter->msix_entries,
				    FOO_DRIVER_MINIMUM_NVEC,
				    nvec);

Device drivers with more specific requirements (i.e. a number of
MSI-Xs which is a multiple of a certain number within a specified
range) would still need to implement the loop using the two old
functions.

Patches 1,2	- ACK'ed tweaks for s390 architecture
Patches 3,4	- fixes for PowerPC pSeries platform
Patches 5-11	- fixes, tweaks and changes of the generic MSI code

The tree could be found in "pci-next-msi-v3" branch in repo:
https://github.com/a-gordeev/linux.git

Alexander Gordeev (11):
  PCI/MSI/s390: Fix single MSI only check
  PCI/MSI/s390: Remove superfluous check of MSI type
  PCI/MSI/pSeries: Fix wrong error code reporting
  PCI/MSI/pSeries: Make quota traversing and requesting race-safe
  PCI/MSI: Fix return value when populate_msi_sysfs() failed
  PCI/MSI: Return -ENOSYS for unimplemented interfaces, not -1
  PCI/MSI: Make pci_enable_msi/msix() 'nvec' argument type as int
  PCI/MSI: Factor out pci_get_msi_cap() interface
  PCI/MSI: Get rid of pci_enable_msi_block_auto() interface
  PCI/MSI: Convert pci_msix_table_size() to a public interface
  PCI/MSI: Introduce pcim_enable_msi*() family helpers

 Documentation/PCI/MSI-HOWTO.txt      |  175 +++++++++++++++++++++++++++++-----
 arch/powerpc/platforms/pseries/msi.c |   26 +++++-
 arch/s390/pci/pci.c                  |    4 +-
 drivers/ata/ahci.c                   |   56 +++++++----
 drivers/pci/msi.c                    |  157 +++++++++++++++++++++++--------
 drivers/pci/pcie/portdrv_core.c      |    5 +-
 include/linux/pci.h                  |   72 ++++++++++++--
 7 files changed, 395 insertions(+), 100 deletions(-)

-- 
1.7.7.6

[PATCH v3 01/11] PCI/MSI/s390: Fix single MSI only check

[Alexander Gordeev]

Multiple MSIs have never been supported on s390 architecture,
but the platform code fails to report single MSI only.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/pci/pci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index f17a834..c79c6e4 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -427,6 +427,8 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	pr_debug("%s: requesting %d MSI-X interrupts...", __func__, nvec);
 	if (type != PCI_CAP_ID_MSIX && type != PCI_CAP_ID_MSI)
 		return -EINVAL;
+	if (type == PCI_CAP_ID_MSI && nvec > 1)
+		return 1;
 	msi_vecs = min(nvec, ZPCI_MSI_VEC_MAX);
 	msi_vecs = min_t(unsigned int, msi_vecs, CONFIG_PCI_NR_MSI);
 
-- 
1.7.7.6

[PATCH v3 02/11] PCI/MSI/s390: Remove superfluous check of MSI type

[Alexander Gordeev]

arch_setup_msi_irqs() hook can only be called from the generic
MSI code which ensures correct MSI type parameter.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/pci/pci.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index c79c6e4..61a3c2c 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -425,8 +425,6 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	int rc;
 
 	pr_debug("%s: requesting %d MSI-X interrupts...", __func__, nvec);
-	if (type != PCI_CAP_ID_MSIX && type != PCI_CAP_ID_MSI)
-		return -EINVAL;
 	if (type == PCI_CAP_ID_MSI && nvec > 1)
 		return 1;
 	msi_vecs = min(nvec, ZPCI_MSI_VEC_MAX);
-- 
1.7.7.6

[PATCH v3 03/11] PCI/MSI/pSeries: Fix wrong error code reporting

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arch/powerpc/platforms/pseries/msi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 6d2f0ab..009ec73 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -467,7 +467,7 @@ again:
 	list_for_each_entry(entry, &pdev->msi_list, list) {
 		hwirq = rtas_query_irq_number(pdn, i++);
 		if (hwirq < 0) {
-			pr_debug("rtas_msi: error (%d) getting hwirq\n", rc);
+			pr_debug("rtas_msi: error (%d) getting hwirq\n", hwirq);
 			return hwirq;
 		}
 
-- 
1.7.7.6

[PATCH v3 04/11] PCI/MSI/pSeries: Make quota traversing and requesting race-safe

[Alexander Gordeev]

The current MSI quota handling is not race-safe and might lead
to incoherent number of MSIs allocated between the firmware and
Linux MSI data structures. I.e. if the following chain is called
from concurrently loading drivers: rtas_setup_msi_irqs() ->
msi_quota_for_device() -> traverse_pci_devices() a driver might
get a stalled value of MSI limit for its device or possibly even
crash.

This update introduces "rtas_quota_mutex" and serializes all
accesses to msi_quota_for_device() function. As result, no driver
could eat into other device's MSI limit.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arch/powerpc/platforms/pseries/msi.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 009ec73..0e1d288 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -26,6 +26,8 @@ static int query_token, change_token;
 #define RTAS_CHANGE_MSIX_FN	4
 #define RTAS_CHANGE_32MSI_FN	5
 
+static DEFINE_MUTEX(rtas_quota_mutex);
+
 /* RTAS Helpers */
 
 static int rtas_change_msi(struct pci_dn *pdn, u32 func, u32 num_irqs)
@@ -345,7 +347,9 @@ static int rtas_msi_check_device(struct pci_dev *pdev, int nvec, int type)
 	if (rc)
 		return rc;
 
+	mutex_lock(&rtas_quota_mutex);
 	quota = msi_quota_for_device(pdev, nvec);
+	mutex_unlock(&rtas_quota_mutex);
 
 	if (quota && quota < nvec)
 		return quota;
@@ -399,6 +403,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
 	struct msi_msg msg;
 	int nvec = nvec_in;
 	int use_32bit_msi_hack = 0;
+	int quota;
 
 	pdn = pci_get_pdn(pdev);
 	if (!pdn)
@@ -407,13 +412,21 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
 	if (type == PCI_CAP_ID_MSIX && check_msix_entries(pdev))
 		return -EINVAL;
 
+	mutex_lock(&rtas_quota_mutex);
+
+	quota = msi_quota_for_device(pdev, nvec);
+	if (quota && quota < nvec) {
+		mutex_unlock(&rtas_quota_mutex);
+		return quota;
+	}
+
 	/*
 	 * Firmware currently refuse any non power of two allocation
 	 * so we round up if the quota will allow it.
 	 */
 	if (type == PCI_CAP_ID_MSIX) {
 		int m = roundup_pow_of_two(nvec);
-		int quota = msi_quota_for_device(pdev, m);
+		quota = msi_quota_for_device(pdev, m);
 
 		if (quota >= m)
 			nvec = m;
@@ -433,8 +446,11 @@ again:
 				 * We only want to run the 32 bit MSI hack below if
 				 * the max bus speed is Gen2 speed
 				 */
-				if (pdev->bus->max_bus_speed != PCIE_SPEED_5_0GT)
+				if (pdev->bus->max_bus_speed !=
+				    PCIE_SPEED_5_0GT) {
+					mutex_unlock(&rtas_quota_mutex);
 					return rc;
+				}
 
 				use_32bit_msi_hack = 1;
 			}
@@ -459,6 +475,7 @@ again:
 			nvec = nvec_in;
 			goto again;
 		}
+		mutex_unlock(&rtas_quota_mutex);
 		pr_debug("rtas_msi: rtas_change_msi() failed\n");
 		return rc;
 	}
@@ -467,6 +484,7 @@ again:
 	list_for_each_entry(entry, &pdev->msi_list, list) {
 		hwirq = rtas_query_irq_number(pdn, i++);
 		if (hwirq < 0) {
+			mutex_unlock(&rtas_quota_mutex);
 			pr_debug("rtas_msi: error (%d) getting hwirq\n", hwirq);
 			return hwirq;
 		}
@@ -474,6 +492,7 @@ again:
 		virq = irq_create_mapping(NULL, hwirq);
 
 		if (virq == NO_IRQ) {
+			mutex_unlock(&rtas_quota_mutex);
 			pr_debug("rtas_msi: Failed mapping hwirq %d\n", hwirq);
 			return -ENOSPC;
 		}
@@ -486,6 +505,7 @@ again:
 		entry->msg = msg;
 	}
 
+	mutex_unlock(&rtas_quota_mutex);
 	return 0;
 }
 
-- 
1.7.7.6

Re: [PATCH v3 04/11] PCI/MSI/pSeries: Make quota traversing and requesting race-safe

[Bjorn Helgaas]

On Tue, Nov 26, 2013 at 10:09:53AM +0100, Alexander Gordeev wrote:
> The current MSI quota handling is not race-safe and might lead
> to incoherent number of MSIs allocated between the firmware and
> Linux MSI data structures. I.e. if the following chain is called
> from concurrently loading drivers: rtas_setup_msi_irqs() ->
> msi_quota_for_device() -> traverse_pci_devices() a driver might
> get a stalled value of MSI limit for its device or possibly even
> crash.

Can you outline the race and the scenario that leads to incorrect results
or a crash?  I looked through rtas_setup_msi_irqs() (briefly) and I didn't
see the way that concurrent calls for different devices could interfere
with each other.

I was looking for some place that modifies state, where concurrent calls
might trample on each other, but it looks like msi_quota_for_device() is
pretty safe: it traverses a tree, but everything it computes is on the
stack and it doesn't seem to save results anywhere.  Maybe I'm barking up
the wrong tree?

Bjorn

> This update introduces "rtas_quota_mutex" and serializes all
> accesses to msi_quota_for_device() function. As result, no driver
> could eat into other device's MSI limit.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  arch/powerpc/platforms/pseries/msi.c |   24 ++++++++++++++++++++++--
>  1 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index 009ec73..0e1d288 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -26,6 +26,8 @@ static int query_token, change_token;
>  #define RTAS_CHANGE_MSIX_FN	4
>  #define RTAS_CHANGE_32MSI_FN	5
>  
> +static DEFINE_MUTEX(rtas_quota_mutex);
> +
>  /* RTAS Helpers */
>  
>  static int rtas_change_msi(struct pci_dn *pdn, u32 func, u32 num_irqs)
> @@ -345,7 +347,9 @@ static int rtas_msi_check_device(struct pci_dev *pdev, int nvec, int type)
>  	if (rc)
>  		return rc;
>  
> +	mutex_lock(&rtas_quota_mutex);
>  	quota = msi_quota_for_device(pdev, nvec);
> +	mutex_unlock(&rtas_quota_mutex);
>  
>  	if (quota && quota < nvec)
>  		return quota;
> @@ -399,6 +403,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
>  	struct msi_msg msg;
>  	int nvec = nvec_in;
>  	int use_32bit_msi_hack = 0;
> +	int quota;
>  
>  	pdn = pci_get_pdn(pdev);
>  	if (!pdn)
> @@ -407,13 +412,21 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
>  	if (type == PCI_CAP_ID_MSIX && check_msix_entries(pdev))
>  		return -EINVAL;
>  
> +	mutex_lock(&rtas_quota_mutex);
> +
> +	quota = msi_quota_for_device(pdev, nvec);
> +	if (quota && quota < nvec) {
> +		mutex_unlock(&rtas_quota_mutex);
> +		return quota;
> +	}
> +
>  	/*
>  	 * Firmware currently refuse any non power of two allocation
>  	 * so we round up if the quota will allow it.
>  	 */
>  	if (type == PCI_CAP_ID_MSIX) {
>  		int m = roundup_pow_of_two(nvec);
> -		int quota = msi_quota_for_device(pdev, m);
> +		quota = msi_quota_for_device(pdev, m);
>  
>  		if (quota >= m)
>  			nvec = m;
> @@ -433,8 +446,11 @@ again:
>  				 * We only want to run the 32 bit MSI hack below if
>  				 * the max bus speed is Gen2 speed
>  				 */
> -				if (pdev->bus->max_bus_speed != PCIE_SPEED_5_0GT)
> +				if (pdev->bus->max_bus_speed !=
> +				    PCIE_SPEED_5_0GT) {
> +					mutex_unlock(&rtas_quota_mutex);
>  					return rc;
> +				}
>  
>  				use_32bit_msi_hack = 1;
>  			}
> @@ -459,6 +475,7 @@ again:
>  			nvec = nvec_in;
>  			goto again;
>  		}
> +		mutex_unlock(&rtas_quota_mutex);
>  		pr_debug("rtas_msi: rtas_change_msi() failed\n");
>  		return rc;
>  	}
> @@ -467,6 +484,7 @@ again:
>  	list_for_each_entry(entry, &pdev->msi_list, list) {
>  		hwirq = rtas_query_irq_number(pdn, i++);
>  		if (hwirq < 0) {
> +			mutex_unlock(&rtas_quota_mutex);
>  			pr_debug("rtas_msi: error (%d) getting hwirq\n", hwirq);
>  			return hwirq;
>  		}
> @@ -474,6 +492,7 @@ again:
>  		virq = irq_create_mapping(NULL, hwirq);
>  
>  		if (virq == NO_IRQ) {
> +			mutex_unlock(&rtas_quota_mutex);
>  			pr_debug("rtas_msi: Failed mapping hwirq %d\n", hwirq);
>  			return -ENOSPC;
>  		}
> @@ -486,6 +505,7 @@ again:
>  		entry->msg = msg;
>  	}
>  
> +	mutex_unlock(&rtas_quota_mutex);
>  	return 0;
>  }
>  
> -- 
> 1.7.7.6
> 

Re: [PATCH v3 04/11] PCI/MSI/pSeries: Make quota traversing and requesting race-safe

[Alexander Gordeev]

On Tue, Dec 10, 2013 at 03:30:20PM -0700, Bjorn Helgaas wrote:
> Can you outline the race and the scenario that leads to incorrect results
> or a crash?  I looked through rtas_setup_msi_irqs() (briefly) and I didn't
> see the way that concurrent calls for different devices could interfere
> with each other.
> 
> I was looking for some place that modifies state, where concurrent calls
> might trample on each other, but it looks like msi_quota_for_device() is
> pretty safe: it traverses a tree, but everything it computes is on the
> stack and it doesn't seem to save results anywhere.  Maybe I'm barking up
> the wrong tree?

Hmm. I've assumed the number of MSIs for a device is cached, and therefore
concurrent calls to msi_quota_for_device() and rtas_change_msi() could race.
But it seems msi_quota_for_device() indeed computes a quota while reading
only device's properties and gains constant result (well, assuming the device
tree is not updated, but this is a different story). Which makes me confused
about this note from a earlier thread:

[quote]
On Sat, 2013-10-05 at 16:20 +0200, Alexander Gordeev wrote:
> So my point is - drivers should first obtain a number of MSIs they *can*
> get, then *derive* a number of MSIs the device is fine with and only then
> request that number. Not terribly different from memory or any other type
> of resource allocation ;)

What if the limit is for a group of devices ? Your interface is racy in
that case, another driver could have eaten into the limit in between the
calls.

Ben.
[/quote]

Some comment from Ben would be nice, but I think the patch could be dropped
for now.

Thanks, Bjorn!

> Bjorn

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v3 05/11] PCI/MSI: Fix return value when populate_msi_sysfs() failed

[Alexander Gordeev]

If populate_msi_sysfs() function failed msix_capability_init()
must return the error code, but it returns the success instead.
This update fixes the described misbehaviour.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 drivers/pci/msi.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 5e63645..babe503 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -729,7 +729,7 @@ static int msix_capability_init(struct pci_dev *dev,
 
 	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
 	if (ret)
-		goto error;
+		goto out_avail;
 
 	/*
 	 * Some devices require MSI-X to be enabled before we can touch the
@@ -742,10 +742,8 @@ static int msix_capability_init(struct pci_dev *dev,
 	msix_program_entries(dev, entries);
 
 	ret = populate_msi_sysfs(dev);
-	if (ret) {
-		ret = 0;
-		goto error;
-	}
+	if (ret)
+		goto out_free;
 
 	/* Set MSI-X enabled bits and unmask the function */
 	pci_intx_for_msi(dev, 0);
@@ -756,7 +754,7 @@ static int msix_capability_init(struct pci_dev *dev,
 
 	return 0;
 
-error:
+out_avail:
 	if (ret < 0) {
 		/*
 		 * If we had some success, report the number of irqs
@@ -773,6 +771,7 @@ error:
 			ret = avail;
 	}
 
+out_free:
 	free_msi_irqs(dev);
 
 	return ret;
-- 
1.7.7.6

[PATCH v3 06/11] PCI/MSI: Return -ENOSYS for unimplemented interfaces, not -1

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Suggested-by: Ben Hutchings <bhutchings@solarflare.com>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 include/linux/pci.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 835ec7b..d5cc0d3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1157,13 +1157,13 @@ struct msix_entry {
 #ifndef CONFIG_PCI_MSI
 static inline int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
 {
-	return -1;
+	return -ENOSYS;
 }
 
 static inline int
 pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec)
 {
-	return -1;
+	return -ENOSYS;
 }
 
 static inline void pci_msi_shutdown(struct pci_dev *dev)
@@ -1178,7 +1178,7 @@ static inline int pci_msix_table_size(struct pci_dev *dev)
 static inline int pci_enable_msix(struct pci_dev *dev,
 				  struct msix_entry *entries, int nvec)
 {
-	return -1;
+	return -ENOSYS;
 }
 
 static inline void pci_msix_shutdown(struct pci_dev *dev)
-- 
1.7.7.6

[PATCH v3 07/11] PCI/MSI: Make pci_enable_msi/msix() 'nvec' argument type as int

[Alexander Gordeev]

Make pci_enable_msi_block(), pci_enable_msi_block_auto() and
pci_enable_msix() consistent with regard to the type of 'nvec'
argument.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 Documentation/PCI/MSI-HOWTO.txt |    2 +-
 drivers/pci/msi.c               |    4 ++--
 include/linux/pci.h             |    8 ++++----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index a091780..a4d174e 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -129,7 +129,7 @@ call to succeed.
 
 4.2.3 pci_enable_msi_block_auto
 
-int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *count)
+int pci_enable_msi_block_auto(struct pci_dev *dev, int *count)
 
 This variation on pci_enable_msi() call allows a device driver to request
 the maximum possible number of MSIs.  The MSI specification only allows
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index babe503..4656b88 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -835,7 +835,7 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
  * updates the @dev's irq member to the lowest new interrupt number; the
  * other interrupt numbers allocated to this device are consecutive.
  */
-int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
+int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 {
 	int status, maxvec;
 	u16 msgctl;
@@ -866,7 +866,7 @@ int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
 }
 EXPORT_SYMBOL(pci_enable_msi_block);
 
-int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec)
+int pci_enable_msi_block_auto(struct pci_dev *dev, int *maxvec)
 {
 	int ret, nvec;
 	u16 msgctl;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d5cc0d3..6ba0deb 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1155,13 +1155,13 @@ struct msix_entry {
 
 
 #ifndef CONFIG_PCI_MSI
-static inline int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
+static inline int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 {
 	return -ENOSYS;
 }
 
 static inline int
-pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec)
+pci_enable_msi_block_auto(struct pci_dev *dev, int *maxvec)
 {
 	return -ENOSYS;
 }
@@ -1196,8 +1196,8 @@ static inline int pci_msi_enabled(void)
 	return 0;
 }
 #else
-int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec);
-int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec);
+int pci_enable_msi_block(struct pci_dev *dev, int nvec);
+int pci_enable_msi_block_auto(struct pci_dev *dev, int *maxvec);
 void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
 int pci_msix_table_size(struct pci_dev *dev);
-- 
1.7.7.6

Re: [PATCH v3 07/11] PCI/MSI: Make pci_enable_msi/msix() 'nvec' argument type as int

[Tejun Heo]

On Tue, Nov 26, 2013 at 10:09:56AM +0100, Alexander Gordeev wrote:
> Make pci_enable_msi_block(), pci_enable_msi_block_auto() and
> pci_enable_msix() consistent with regard to the type of 'nvec'
> argument.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH v3 08/11] PCI/MSI: Factor out pci_get_msi_cap() interface

[Alexander Gordeev]

Device drivers can use this interface to obtain maximum number
of MSI interrupts the device supports and use that number i.e.
in a following call to pci_enable_msi_block() interface.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 Documentation/PCI/MSI-HOWTO.txt |   15 ++++++++++++++
 drivers/pci/msi.c               |   41 +++++++++++++++++++++++++++++++-------
 include/linux/pci.h             |    6 +++++
 3 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index a4d174e..9c1282e 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -169,6 +169,21 @@ on any interrupt for which it previously called request_irq().
 Failure to do so results in a BUG_ON(), leaving the device with
 MSI enabled and thus leaking its vector.
 
+4.2.5 pci_get_msi_cap
+
+int pci_get_msi_cap(struct pci_dev *dev)
+
+This function could be used to retrieve the number of MSI vectors the
+device requested (via the Multiple Message Capable register). The MSI
+specification only allows the returned value to be a power of two,
+up to a maximum of 2^5 (32).
+
+If this function returns a negative number, it indicates the device is
+not capable of sending MSIs.
+
+If this function returns a positive number, it indicates the maximum
+number of MSI interrupt vectors that could be allocated.
+
 4.3 Using MSI-X
 
 The MSI-X capability is much more flexible than the MSI capability.
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 4656b88..3558b45 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -823,6 +823,31 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 }
 
 /**
+ * pci_get_msi_cap - Return the number of MSI vectors device is capable to send
+ * @dev: device to report about
+ *
+ * This function returns the number of MSI vectors a device requested via
+ * Multiple Message Capable register. It returns a negative errno if the
+ * device is not capable sending MSI interrupts. Otherwise, the call succeeds
+ * and returns a power of two, up to a maximum of 2^5 (32), according to the
+ * MSI specification.
+ **/
+int pci_get_msi_cap(struct pci_dev *dev)
+{
+	int ret;
+	u16 msgctl;
+
+	if (!dev->msi_cap)
+		return -EINVAL;
+
+	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
+	ret = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
+
+	return ret;
+}
+EXPORT_SYMBOL(pci_get_msi_cap);
+
+/**
  * pci_enable_msi_block - configure device's MSI capability structure
  * @dev: device to configure
  * @nvec: number of interrupts to configure
@@ -838,13 +863,13 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 {
 	int status, maxvec;
-	u16 msgctl;
 
-	if (!dev->msi_cap || dev->current_state != PCI_D0)
+	if (dev->current_state != PCI_D0)
 		return -EINVAL;
 
-	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
-	maxvec = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
+	maxvec = pci_get_msi_cap(dev);
+	if (maxvec < 0)
+		return maxvec;
 	if (nvec > maxvec)
 		return maxvec;
 
@@ -869,13 +894,13 @@ EXPORT_SYMBOL(pci_enable_msi_block);
 int pci_enable_msi_block_auto(struct pci_dev *dev, int *maxvec)
 {
 	int ret, nvec;
-	u16 msgctl;
 
-	if (!dev->msi_cap || dev->current_state != PCI_D0)
+	if (dev->current_state != PCI_D0)
 		return -EINVAL;
 
-	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
-	ret = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
+	ret = pci_get_msi_cap(dev);
+	if (ret < 0)
+		return ret;
 
 	if (maxvec)
 		*maxvec = ret;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6ba0deb..e337086 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1155,6 +1155,11 @@ struct msix_entry {
 
 
 #ifndef CONFIG_PCI_MSI
+static inline int pci_get_msi_cap(struct pci_dev *dev)
+{
+	return -ENOSYS;
+}
+
 static inline int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 {
 	return -ENOSYS;
@@ -1196,6 +1201,7 @@ static inline int pci_msi_enabled(void)
 	return 0;
 }
 #else
+int pci_get_msi_cap(struct pci_dev *dev);
 int pci_enable_msi_block(struct pci_dev *dev, int nvec);
 int pci_enable_msi_block_auto(struct pci_dev *dev, int *maxvec);
 void pci_msi_shutdown(struct pci_dev *dev);
-- 
1.7.7.6

[PATCH v3 09/11] PCI/MSI: Get rid of pci_enable_msi_block_auto() interface

[Alexander Gordeev]

As result of introduction of pci_get_msi_cap() interface
pci_enable_msi_block_auto() function became superflous.

To enable maximum possible number of MSIs drivers will
first obtain that number from pci_get_msi_cap() function
and then call pci_enable_msi_block() interface.

Function pci_enable_msi_block_auto() has been introduced
recently and its only user is AHCI driver, which is also
updated by this change.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 Documentation/PCI/MSI-HOWTO.txt |   39 ++++-----------------------
 drivers/ata/ahci.c              |   56 ++++++++++++++++++++++++--------------
 drivers/pci/msi.c               |   25 -----------------
 include/linux/pci.h             |    7 -----
 4 files changed, 41 insertions(+), 86 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 9c1282e..1fe4900 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -127,49 +127,22 @@ on the number of vectors that can be allocated; pci_enable_msi_block()
 returns as soon as it finds any constraint that doesn't allow the
 call to succeed.
 
-4.2.3 pci_enable_msi_block_auto
-
-int pci_enable_msi_block_auto(struct pci_dev *dev, int *count)
-
-This variation on pci_enable_msi() call allows a device driver to request
-the maximum possible number of MSIs.  The MSI specification only allows
-interrupts to be allocated in powers of two, up to a maximum of 2^5 (32).
-
-If this function returns a positive number, it indicates that it has
-succeeded and the returned value is the number of allocated interrupts. In
-this case, the function enables MSI on this device and updates dev->irq to
-be the lowest of the new interrupts assigned to it.  The other interrupts
-assigned to the device are in the range dev->irq to dev->irq + returned
-value - 1.
-
-If this function returns a negative number, it indicates an error and
-the driver should not attempt to request any more MSI interrupts for
-this device.
-
-If the device driver needs to know the number of interrupts the device
-supports it can pass the pointer count where that number is stored. The
-device driver must decide what action to take if pci_enable_msi_block_auto()
-succeeds, but returns a value less than the number of interrupts supported.
-If the device driver does not need to know the number of interrupts
-supported, it can set the pointer count to NULL.
-
-4.2.4 pci_disable_msi
+4.2.3 pci_disable_msi
 
 void pci_disable_msi(struct pci_dev *dev)
 
 This function should be used to undo the effect of pci_enable_msi() or
-pci_enable_msi_block() or pci_enable_msi_block_auto().  Calling it restores
-dev->irq to the pin-based interrupt number and frees the previously
-allocated message signaled interrupt(s).  The interrupt may subsequently be
-assigned to another device, so drivers should not cache the value of
-dev->irq.
+pci_enable_msi_block().  Calling it restores dev->irq to the pin-based
+interrupt number and frees the previously allocated message signaled
+interrupt(s).  The interrupt may subsequently be assigned to another
+device, so drivers should not cache the value of dev->irq.
 
 Before calling this function, a device driver must always call free_irq()
 on any interrupt for which it previously called request_irq().
 Failure to do so results in a BUG_ON(), leaving the device with
 MSI enabled and thus leaking its vector.
 
-4.2.5 pci_get_msi_cap
+4.2.4 pci_get_msi_cap
 
 int pci_get_msi_cap(struct pci_dev *dev)
 
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 9d715ae..f1c8838 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1091,26 +1091,40 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
 {}
 #endif
 
-int ahci_init_interrupts(struct pci_dev *pdev, struct ahci_host_priv *hpriv)
+int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+			 struct ahci_host_priv *hpriv)
 {
-	int rc;
-	unsigned int maxvec;
+	int rc, nvec;
 
-	if (!(hpriv->flags & AHCI_HFLAG_NO_MSI)) {
-		rc = pci_enable_msi_block_auto(pdev, &maxvec);
-		if (rc > 0) {
-			if ((rc == maxvec) || (rc == 1))
-				return rc;
-			/*
-			 * Assume that advantage of multipe MSIs is negated,
-			 * so fallback to single MSI mode to save resources
-			 */
-			pci_disable_msi(pdev);
-			if (!pci_enable_msi(pdev))
-				return 1;
-		}
-	}
+	if (hpriv->flags & AHCI_HFLAG_NO_MSI)
+		goto intx;
+
+	rc = pci_get_msi_cap(pdev);
+	if (rc < 0)
+		goto intx;
+
+	/*
+	 * If number of MSIs is less than number of ports then Sharing Last
+	 * 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)
+		goto single_msi;
+
+	nvec = rc;
+	rc = pci_enable_msi_block(pdev, nvec);
+	if (rc)
+		goto intx;
 
+	return nvec;
+
+single_msi:
+	rc = pci_enable_msi(pdev);
+	if (rc)
+		goto intx;
+	return 1;
+
+intx:
 	pci_intx(pdev, 1);
 	return 0;
 }
@@ -1277,10 +1291,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	hpriv->mmio = pcim_iomap_table(pdev)[ahci_pci_bar];
 
-	n_msis = ahci_init_interrupts(pdev, hpriv);
-	if (n_msis > 1)
-		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
-
 	/* save initial config */
 	ahci_pci_save_initial_config(pdev, hpriv);
 
@@ -1335,6 +1345,10 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 */
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
+	n_msis = ahci_init_interrupts(pdev, n_ports, hpriv);
+	if (n_msis > 1)
+		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
+
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
 	if (!host)
 		return -ENOMEM;
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 3558b45..e4b02ac 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -891,31 +891,6 @@ int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 }
 EXPORT_SYMBOL(pci_enable_msi_block);
 
-int pci_enable_msi_block_auto(struct pci_dev *dev, int *maxvec)
-{
-	int ret, nvec;
-
-	if (dev->current_state != PCI_D0)
-		return -EINVAL;
-
-	ret = pci_get_msi_cap(dev);
-	if (ret < 0)
-		return ret;
-
-	if (maxvec)
-		*maxvec = ret;
-
-	do {
-		nvec = ret;
-		ret = pci_enable_msi_block(dev, nvec);
-	} while (ret > 0);
-
-	if (ret < 0)
-		return ret;
-	return nvec;
-}
-EXPORT_SYMBOL(pci_enable_msi_block_auto);
-
 void pci_msi_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *desc;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e337086..9ab1692 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1165,12 +1165,6 @@ static inline int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 	return -ENOSYS;
 }
 
-static inline int
-pci_enable_msi_block_auto(struct pci_dev *dev, int *maxvec)
-{
-	return -ENOSYS;
-}
-
 static inline void pci_msi_shutdown(struct pci_dev *dev)
 { }
 static inline void pci_disable_msi(struct pci_dev *dev)
@@ -1203,7 +1197,6 @@ static inline int pci_msi_enabled(void)
 #else
 int pci_get_msi_cap(struct pci_dev *dev);
 int pci_enable_msi_block(struct pci_dev *dev, int nvec);
-int pci_enable_msi_block_auto(struct pci_dev *dev, int *maxvec);
 void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
 int pci_msix_table_size(struct pci_dev *dev);
-- 
1.7.7.6

[PATCH v3 10/11] PCI/MSI: Convert pci_msix_table_size() to a public interface

[Alexander Gordeev]

Make pci_msix_table_size() function to return a negative errno
if device does not support MSI-X interrupts. After this update
pci_msix_table_size() can fail and callers must always check
the returned value.

This update is needed to create a consistent MSI-X counterpart
for pci_get_msi_cap() MSI interface. Device drivers can use this
function to obtain maximum number of MSI-X interrupts the device
supports and i.e. use that number in a following call to
pci_enable_msix() interface.

The only user of pci_msix_table_size() function is PCI-Express
port driver, which is also updated by this change.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 Documentation/PCI/MSI-HOWTO.txt |   13 +++++++++++++
 drivers/pci/msi.c               |   12 ++++++++++--
 drivers/pci/pcie/portdrv_core.c |    5 +++--
 include/linux/pci.h             |    2 +-
 4 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 1fe4900..5955389 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -243,6 +243,19 @@ MSI-X Table.  This address is mapped by the PCI subsystem, and should not
 be accessed directly by the device driver.  If the driver wishes to
 mask or unmask an interrupt, it should call disable_irq() / enable_irq().
 
+4.3.4 pci_msix_table_size
+
+int pci_msix_table_size(struct pci_dev *dev)
+
+This function could be used to retrieve number of entries in the device
+MSI-X table.
+
+If this function returns a negative number, it indicates the device is
+not capable of sending MSI-Xs.
+
+If this function returns a positive number, it indicates the maximum
+number of MSI-X interrupt vectors that could be allocated.
+
 4.4 Handling devices implementing both MSI and MSI-X capabilities
 
 If a device implements both MSI and MSI-X capabilities, it can
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index e4b02ac..6fe0add 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -932,17 +932,23 @@ EXPORT_SYMBOL(pci_disable_msi);
 /**
  * pci_msix_table_size - return the number of device's MSI-X table entries
  * @dev: pointer to the pci_dev data structure of MSI-X device function
- */
+
+ * This function returns the number of device's MSI-X table entries and
+ * therefore the number of MSI-X vectors device is capable to send.
+ * It returns a negative errno if the device is not capable sending MSI-X
+ * interrupts.
+ **/
 int pci_msix_table_size(struct pci_dev *dev)
 {
 	u16 control;
 
 	if (!dev->msix_cap)
-		return 0;
+		return -EINVAL;
 
 	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
 	return msix_table_size(control);
 }
+EXPORT_SYMBOL(pci_msix_table_size);
 
 /**
  * pci_enable_msix - configure device's MSI-X capability structure
@@ -972,6 +978,8 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 		return status;
 
 	nr_entries = pci_msix_table_size(dev);
+	if (nr_entries < 0)
+		return nr_entries;
 	if (nvec > nr_entries)
 		return nr_entries;
 
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 08d131f..5bebeec 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -80,8 +80,9 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *vectors, int mask)
 	u32 reg32;
 
 	nr_entries = pci_msix_table_size(dev);
-	if (!nr_entries)
-		return -EINVAL;
+	if (nr_entries < 0)
+		return nr_entries;
+	BUG_ON(!nr_entries);
 	if (nr_entries > PCIE_PORT_MAX_MSIX_ENTRIES)
 		nr_entries = PCIE_PORT_MAX_MSIX_ENTRIES;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9ab1692..8af1217 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1172,7 +1172,7 @@ static inline void pci_disable_msi(struct pci_dev *dev)
 
 static inline int pci_msix_table_size(struct pci_dev *dev)
 {
-	return 0;
+	return -ENOSYS;
 }
 static inline int pci_enable_msix(struct pci_dev *dev,
 				  struct msix_entry *entries, int nvec)
-- 
1.7.7.6

Re: [PATCH v3 10/11] PCI/MSI: Convert pci_msix_table_size() to a public interface

[Bjorn Helgaas]

On Tue, Nov 26, 2013 at 10:09:59AM +0100, Alexander Gordeev wrote:
> Make pci_msix_table_size() function to return a negative errno
> if device does not support MSI-X interrupts. After this update
> pci_msix_table_size() can fail and callers must always check
> the returned value.
> 
> This update is needed to create a consistent MSI-X counterpart
> for pci_get_msi_cap() MSI interface. Device drivers can use this
> function to obtain maximum number of MSI-X interrupts the device
> supports and i.e. use that number in a following call to
> pci_enable_msix() interface.

If pci_msix_table_size() is a counterpart to pci_get_msi_cap(), can we make
the names similar?

> The only user of pci_msix_table_size() function is PCI-Express
> port driver, which is also updated by this change.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Reviewed-by: Tejun Heo <tj@kernel.org>
> ---
>  Documentation/PCI/MSI-HOWTO.txt |   13 +++++++++++++
>  drivers/pci/msi.c               |   12 ++++++++++--
>  drivers/pci/pcie/portdrv_core.c |    5 +++--
>  include/linux/pci.h             |    2 +-
>  4 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
> index 1fe4900..5955389 100644
> --- a/Documentation/PCI/MSI-HOWTO.txt
> +++ b/Documentation/PCI/MSI-HOWTO.txt
> @@ -243,6 +243,19 @@ MSI-X Table.  This address is mapped by the PCI subsystem, and should not
>  be accessed directly by the device driver.  If the driver wishes to
>  mask or unmask an interrupt, it should call disable_irq() / enable_irq().
>  
> +4.3.4 pci_msix_table_size
> +
> +int pci_msix_table_size(struct pci_dev *dev)
> +
> +This function could be used to retrieve number of entries in the device
> +MSI-X table.
> +
> +If this function returns a negative number, it indicates the device is
> +not capable of sending MSI-Xs.
> +
> +If this function returns a positive number, it indicates the maximum
> +number of MSI-X interrupt vectors that could be allocated.
> +
>  4.4 Handling devices implementing both MSI and MSI-X capabilities
>  
>  If a device implements both MSI and MSI-X capabilities, it can
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index e4b02ac..6fe0add 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -932,17 +932,23 @@ EXPORT_SYMBOL(pci_disable_msi);
>  /**
>   * pci_msix_table_size - return the number of device's MSI-X table entries
>   * @dev: pointer to the pci_dev data structure of MSI-X device function
> - */
> +
> + * This function returns the number of device's MSI-X table entries and
> + * therefore the number of MSI-X vectors device is capable to send.
> + * It returns a negative errno if the device is not capable sending MSI-X
> + * interrupts.
> + **/
>  int pci_msix_table_size(struct pci_dev *dev)
>  {
>  	u16 control;
>  
>  	if (!dev->msix_cap)
> -		return 0;
> +		return -EINVAL;
>  
>  	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
>  	return msix_table_size(control);
>  }
> +EXPORT_SYMBOL(pci_msix_table_size);
>  
>  /**
>   * pci_enable_msix - configure device's MSI-X capability structure
> @@ -972,6 +978,8 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
>  		return status;
>  
>  	nr_entries = pci_msix_table_size(dev);
> +	if (nr_entries < 0)
> +		return nr_entries;
>  	if (nvec > nr_entries)
>  		return nr_entries;
>  
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 08d131f..5bebeec 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -80,8 +80,9 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *vectors, int mask)
>  	u32 reg32;
>  
>  	nr_entries = pci_msix_table_size(dev);
> -	if (!nr_entries)
> -		return -EINVAL;
> +	if (nr_entries < 0)
> +		return nr_entries;
> +	BUG_ON(!nr_entries);
>  	if (nr_entries > PCIE_PORT_MAX_MSIX_ENTRIES)
>  		nr_entries = PCIE_PORT_MAX_MSIX_ENTRIES;
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 9ab1692..8af1217 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1172,7 +1172,7 @@ static inline void pci_disable_msi(struct pci_dev *dev)
>  
>  static inline int pci_msix_table_size(struct pci_dev *dev)
>  {
> -	return 0;
> +	return -ENOSYS;
>  }
>  static inline int pci_enable_msix(struct pci_dev *dev,
>  				  struct msix_entry *entries, int nvec)
> -- 
> 1.7.7.6
> 

Re: [PATCH v3 10/11] PCI/MSI: Convert pci_msix_table_size() to a public interface

[Alexander Gordeev]

On Tue, Dec 10, 2013 at 04:08:47PM -0700, Bjorn Helgaas wrote:
> If pci_msix_table_size() is a counterpart to pci_get_msi_cap(), can we make
> the names similar?

Tejun also pointed this out. What about pci_get_msi_vec_count() and
pci_get_msix_vec_count()?

Nevertheless, the existing names are more informative for me, since they
hint where the MSI/MSI-X vector count comes from. Up to you.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v3 10/11] PCI/MSI: Convert pci_msix_table_size() to a public interface

[Bjorn Helgaas]

On Thu, Dec 12, 2013 at 9:06 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Tue, Dec 10, 2013 at 04:08:47PM -0700, Bjorn Helgaas wrote:
>> If pci_msix_table_size() is a counterpart to pci_get_msi_cap(), can we make
>> the names similar?
>
> Tejun also pointed this out. What about pci_get_msi_vec_count() and
> pci_get_msix_vec_count()?

Sounds perfect to me.  I think the fact that they return the same type
of information is more important than where the information came from.

> Nevertheless, the existing names are more informative for me, since they
> hint where the MSI/MSI-X vector count comes from. Up to you.
>
> --
> Regards,
> Alexander Gordeev
> agordeev@redhat.com

[PATCH v3 11/11] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

Currently many device drivers need contiguously call functions
pci_enable_msix() for MSI-X or pci_enable_msi_block() for MSI
in a loop until success or failure. This update generalizes
this usage pattern and introduces pcim_enable_msi*() family
helpers.

As result, device drivers do not have to deal with tri-state
return values from pci_enable_msix() and pci_enable_msi_block()
functions directly and expected to have more clearer and straight
code.

So i.e. the request loop described in the documentation...

	int foo_driver_enable_msix(struct foo_adapter *adapter,
				   int nvec)
	{
		while (nvec >= FOO_DRIVER_MINIMUM_NVEC) {
			rc = pci_enable_msix(adapter->pdev,
					     adapter->msix_entries,
					     nvec);
			if (rc > 0)
				nvec = rc;
			else
				return rc;
		}

		return -ENOSPC;
	}

...would turn into a single helper call....

	rc = pcim_enable_msix_range(adapter->pdev,
				    adapter->msix_entries,
				    FOO_DRIVER_MINIMUM_NVEC,
				    nvec);

Device drivers with more specific requirements (i.e. a number of
MSI-Xs which is a multiple of a certain number within a specified
range) would still need to implement the loop using the two old
functions.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Suggested-by: Ben Hutchings <bhutchings@solarflare.com>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 Documentation/PCI/MSI-HOWTO.txt |  134 +++++++++++++++++++++++++++++++++++++--
 drivers/pci/msi.c               |   74 +++++++++++++++++++++
 include/linux/pci.h             |   55 ++++++++++++++++
 3 files changed, 258 insertions(+), 5 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 5955389..044f9cd 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -127,7 +127,62 @@ on the number of vectors that can be allocated; pci_enable_msi_block()
 returns as soon as it finds any constraint that doesn't allow the
 call to succeed.
 
-4.2.3 pci_disable_msi
+4.2.3 pcim_enable_msi_range
+
+int pcim_enable_msi_range(struct pci_dev *dev, struct msix_entry *entries,
+			  int minvec, int maxvec)
+
+This variation on pci_enable_msi_block() call allows a device driver to
+request any number of MSIs within specified range 'minvec' to 'maxvec'.
+Whenever possible device drivers are encouraged to use this function
+rather than explicit request loop calling pci_enable_msi_block().
+
+If this function returns a negative number, it indicates an error and
+the driver should not attempt to request any more MSI interrupts for
+this device.
+
+If this function returns a positive number it indicates at least the
+returned number of MSI interrupts have been successfully allocated (it may
+have allocated more in order to satisfy the power-of-two requirement).
+Device drivers can use this number to further initialize devices.
+
+4.2.4 pcim_enable_msi
+
+int pcim_enable_msi(struct pci_dev *dev,
+		    struct msix_entry *entries, int maxvec)
+
+This variation on pci_enable_msi_block() call allows a device driver to
+request any number of MSIs up to 'maxvec'. Whenever possible device drivers
+are encouraged to use this function rather than explicit request loop
+calling pci_enable_msi_block().
+
+If this function returns a negative number, it indicates an error and
+the driver should not attempt to request any more MSI interrupts for
+this device.
+
+If this function returns a positive number it indicates at least the
+returned number of MSI interrupts have been successfully allocated (it may
+have allocated more in order to satisfy the power-of-two requirement).
+Device drivers can use this number to further initialize devices.
+
+4.2.5 pcim_enable_msi_exact
+
+int pcim_enable_msi_exact(struct pci_dev *dev,
+			  struct msix_entry *entries, int nvec)
+
+This variation on pci_enable_msi_block() call allows a device driver to
+request exactly 'nvec' MSIs.
+
+If this function returns a negative number, it indicates an error and
+the driver should not attempt to request any more MSI interrupts for
+this device.
+
+If this function returns the value of 'nvec' it indicates MSI interrupts
+have been successfully allocated. No other value in case of success could
+be returned. Device drivers can use this value to further allocate and
+initialize device resources.
+
+4.2.6 pci_disable_msi
 
 void pci_disable_msi(struct pci_dev *dev)
 
@@ -142,7 +197,7 @@ on any interrupt for which it previously called request_irq().
 Failure to do so results in a BUG_ON(), leaving the device with
 MSI enabled and thus leaking its vector.
 
-4.2.4 pci_get_msi_cap
+4.2.7 pci_get_msi_cap
 
 int pci_get_msi_cap(struct pci_dev *dev)
 
@@ -222,7 +277,76 @@ static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
 	return -ENOSPC;
 }
 
-4.3.2 pci_disable_msix
+4.3.2 pcim_enable_msix_range
+
+int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
+			   int minvec, int maxvec)
+
+This variation on pci_enable_msix() call allows a device driver to request
+any number of MSI-Xs within specified range 'minvec' to 'maxvec'. Whenever
+possible device drivers are encouraged to use this function rather than
+explicit request loop calling pci_enable_msix().
+
+If this function returns a negative number, it indicates an error and
+the driver should not attempt to allocate any more MSI-X interrupts for
+this device.
+
+If this function returns a positive number it indicates the number of
+MSI-X interrupts that have been successfully allocated. Device drivers
+can use this number to further allocate and initialize device resources.
+
+A modified function calling pci_enable_msix() in a loop might look like:
+
+static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
+{
+	rc = pcim_enable_msix_range(adapter->pdev, adapter->msix_entries,
+				    FOO_DRIVER_MINIMUM_NVEC, nvec);
+	if (rc < 0)
+		return rc;
+
+	rc = foo_driver_init_other(adapter, rc);
+	if (rc < 0)
+		pci_disable_msix(adapter->pdev);
+
+	return rc;
+}
+
+4.3.3 pcim_enable_msix
+
+int pcim_enable_msix(struct pci_dev *dev,
+		     struct msix_entry *entries, int maxvec)
+
+This variation on pci_enable_msix() call allows a device driver to request
+any number of MSI-Xs up to 'maxvec'. Whenever possible device drivers are
+encouraged to use this function rather than explicit request loop calling
+pci_enable_msix().
+
+If this function returns a negative number, it indicates an error and
+the driver should not attempt to allocate any more MSI-X interrupts for
+this device.
+
+If this function returns a positive number it indicates the number of
+MSI-X interrupts that have been successfully allocated. Device drivers
+can use this number to further allocate and initialize device resources.
+
+4.3.4 pcim_enable_msix_exact
+
+int pcim_enable_msix_exact(struct pci_dev *dev,
+			   struct msix_entry *entries, int nvec)
+
+This variation on pci_enable_msix() call allows a device driver to request
+exactly 'nvec' MSI-Xs.
+
+If this function returns a negative number, it indicates an error and
+the driver should not attempt to allocate any more MSI-X interrupts for
+this device.
+
+If this function returns the value of 'nvec' it indicates MSI-X interrupts
+have been successfully allocated. No other value in case of success could
+be returned. Device drivers can use this value to further allocate and
+initialize device resources.
+
+4.3.5 pci_disable_msix
 
 void pci_disable_msix(struct pci_dev *dev)
 
@@ -236,14 +360,14 @@ on any interrupt for which it previously called request_irq().
 Failure to do so results in a BUG_ON(), leaving the device with
 MSI-X enabled and thus leaking its vector.
 
-4.3.3 The MSI-X Table
+4.3.6 The MSI-X Table
 
 The MSI-X capability specifies a BAR and offset within that BAR for the
 MSI-X Table.  This address is mapped by the PCI subsystem, and should not
 be accessed directly by the device driver.  If the driver wishes to
 mask or unmask an interrupt, it should call disable_irq() / enable_irq().
 
-4.3.4 pci_msix_table_size
+4.3.7 pci_msix_table_size
 
 int pci_msix_table_size(struct pci_dev *dev)
 
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 6fe0add..c5fb4fb 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1086,3 +1086,77 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
 	if (dev->msix_cap)
 		msix_set_enable(dev, 0);
 }
+
+/**
+ * pcim_enable_msi_range - configure device's MSI capability structure
+ * @dev: device to configure
+ * @minvec: minimal number of interrupts to configure
+ * @maxvec: maximum number of interrupts to configure
+ *
+ * This function tries to allocate a maximum possible number of interrupts in a
+ * range between @minvec and @maxvec. It returns a negative errno if an error
+ * occurs. If it succeeds, it returns the actual number of interrupts allocated
+ * and updates the @dev's irq member to the lowest new interrupt number;
+ * the other interrupt numbers allocated to this device are consecutive.
+ **/
+int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
+{
+	int nvec = maxvec;
+	int rc;
+
+	if (maxvec < minvec)
+		return -ERANGE;
+
+	do {
+		rc = pci_enable_msi_block(dev, nvec);
+		if (rc < 0) {
+			return rc;
+		} else if (rc > 0) {
+			if (rc < minvec)
+				return -ENOSPC;
+			nvec = rc;
+		}
+	} while (rc);
+
+	return nvec;
+}
+EXPORT_SYMBOL(pcim_enable_msi_range);
+
+/**
+ * pcim_enable_msix_range - configure device's MSI-X capability structure
+ * @dev: pointer to the pci_dev data structure of MSI-X device function
+ * @entries: pointer to an array of MSI-X entries
+ * @minvec: minimum number of MSI-X irqs requested
+ * @maxvec: maximum number of MSI-X irqs requested
+ *
+ * Setup the MSI-X capability structure of device function with a maximum
+ * possible number of interrupts in the range between @minvec and @maxvec
+ * upon its software driver call to request for MSI-X mode enabled on its
+ * hardware device function. It returns a negative errno if an error occurs.
+ * If it succeeds, it returns the actual number of interrupts allocated and
+ * indicates the successful configuration of MSI-X capability structure
+ * with new allocated MSI-X interrupts.
+ **/
+int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
+			   int minvec, int maxvec)
+{
+	int nvec = maxvec;
+	int rc;
+
+	if (maxvec < minvec)
+		return -ERANGE;
+
+	do {
+		rc = pci_enable_msix(dev, entries, nvec);
+		if (rc < 0) {
+			return rc;
+		} else if (rc > 0) {
+			if (rc < minvec)
+				return -ENOSPC;
+			nvec = rc;
+		}
+	} while (rc);
+
+	return nvec;
+}
+EXPORT_SYMBOL(pcim_enable_msix_range);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8af1217..4e02d88 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1194,6 +1194,37 @@ static inline int pci_msi_enabled(void)
 {
 	return 0;
 }
+
+int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int pcim_enable_msi(struct pci_dev *dev, int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int pcim_enable_msi_exact(struct pci_dev *dev, int nvec)
+{
+	return -ENOSYS;
+}
+
+static inline int
+pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
+		       int minvec, int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int
+pcim_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int
+pcim_enable_msix_exact(struct pci_dev *dev,
+		       struct msix_entry *entries, int nvec)
+{
+	return -ENOSYS;
+}
 #else
 int pci_get_msi_cap(struct pci_dev *dev);
 int pci_enable_msi_block(struct pci_dev *dev, int nvec);
@@ -1206,6 +1237,30 @@ void pci_disable_msix(struct pci_dev *dev);
 void msi_remove_pci_irq_vectors(struct pci_dev *dev);
 void pci_restore_msi_state(struct pci_dev *dev);
 int pci_msi_enabled(void);
+
+int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec);
+static inline int pcim_enable_msi(struct pci_dev *dev, int maxvec)
+{
+	return pcim_enable_msi_range(dev, 1, maxvec);
+}
+static inline int pcim_enable_msi_exact(struct pci_dev *dev, int nvec)
+{
+	return pcim_enable_msi_range(dev, nvec, nvec);
+}
+
+int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
+			   int minvec, int maxvec);
+static inline int
+pcim_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int maxvec)
+{
+	return pcim_enable_msix_range(dev, entries, 1, maxvec);
+}
+static inline int
+pcim_enable_msix_exact(struct pci_dev *dev,
+		       struct msix_entry *entries, int nvec)
+{
+	return pcim_enable_msix_range(dev, entries, nvec, nvec);
+}
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS
-- 
1.7.7.6

Re: [PATCH v3 11/11] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Bjorn Helgaas]

On Tue, Nov 26, 2013 at 10:10:00AM +0100, Alexander Gordeev wrote:
> Currently many device drivers need contiguously call functions
> pci_enable_msix() for MSI-X or pci_enable_msi_block() for MSI
> in a loop until success or failure. This update generalizes
> this usage pattern and introduces pcim_enable_msi*() family
> helpers.

What's the reason for using the "pcim_" prefix?  To me, that suggests that
this is a "managed" interface in the sense described in
Documentation/driver-model/devres.txt, where resources are automatically
deallocated when the driver detaches.  But I don't see anything like that
happening in this patch.

> As result, device drivers do not have to deal with tri-state
> return values from pci_enable_msix() and pci_enable_msi_block()
> functions directly and expected to have more clearer and straight
> code.
> 
> So i.e. the request loop described in the documentation...
> 
> 	int foo_driver_enable_msix(struct foo_adapter *adapter,
> 				   int nvec)
> 	{
> 		while (nvec >= FOO_DRIVER_MINIMUM_NVEC) {
> 			rc = pci_enable_msix(adapter->pdev,
> 					     adapter->msix_entries,
> 					     nvec);
> 			if (rc > 0)
> 				nvec = rc;
> 			else
> 				return rc;
> 		}
> 
> 		return -ENOSPC;
> 	}
> 
> ...would turn into a single helper call....
> 
> 	rc = pcim_enable_msix_range(adapter->pdev,
> 				    adapter->msix_entries,
> 				    FOO_DRIVER_MINIMUM_NVEC,
> 				    nvec);
> 
> Device drivers with more specific requirements (i.e. a number of
> MSI-Xs which is a multiple of a certain number within a specified
> range) would still need to implement the loop using the two old
> functions.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Suggested-by: Ben Hutchings <bhutchings@solarflare.com>
> Reviewed-by: Tejun Heo <tj@kernel.org>
> ---
>  Documentation/PCI/MSI-HOWTO.txt |  134 +++++++++++++++++++++++++++++++++++++--
>  drivers/pci/msi.c               |   74 +++++++++++++++++++++
>  include/linux/pci.h             |   55 ++++++++++++++++
>  3 files changed, 258 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
> index 5955389..044f9cd 100644
> --- a/Documentation/PCI/MSI-HOWTO.txt
> +++ b/Documentation/PCI/MSI-HOWTO.txt
> @@ -127,7 +127,62 @@ on the number of vectors that can be allocated; pci_enable_msi_block()
>  returns as soon as it finds any constraint that doesn't allow the
>  call to succeed.
>  
> -4.2.3 pci_disable_msi
> +4.2.3 pcim_enable_msi_range
> +
> +int pcim_enable_msi_range(struct pci_dev *dev, struct msix_entry *entries,
> +			  int minvec, int maxvec)
> +
> +This variation on pci_enable_msi_block() call allows a device driver to
> +request any number of MSIs within specified range 'minvec' to 'maxvec'.
> +Whenever possible device drivers are encouraged to use this function
> +rather than explicit request loop calling pci_enable_msi_block().
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to request any more MSI interrupts for
> +this device.
> +
> +If this function returns a positive number it indicates at least the
> +returned number of MSI interrupts have been successfully allocated (it may
> +have allocated more in order to satisfy the power-of-two requirement).
> +Device drivers can use this number to further initialize devices.
> +
> +4.2.4 pcim_enable_msi
> +
> +int pcim_enable_msi(struct pci_dev *dev,
> +		    struct msix_entry *entries, int maxvec)
> +
> +This variation on pci_enable_msi_block() call allows a device driver to
> +request any number of MSIs up to 'maxvec'. Whenever possible device drivers
> +are encouraged to use this function rather than explicit request loop
> +calling pci_enable_msi_block().
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to request any more MSI interrupts for
> +this device.
> +
> +If this function returns a positive number it indicates at least the
> +returned number of MSI interrupts have been successfully allocated (it may
> +have allocated more in order to satisfy the power-of-two requirement).
> +Device drivers can use this number to further initialize devices.
> +
> +4.2.5 pcim_enable_msi_exact
> +
> +int pcim_enable_msi_exact(struct pci_dev *dev,
> +			  struct msix_entry *entries, int nvec)
> +
> +This variation on pci_enable_msi_block() call allows a device driver to
> +request exactly 'nvec' MSIs.
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to request any more MSI interrupts for
> +this device.
> +
> +If this function returns the value of 'nvec' it indicates MSI interrupts
> +have been successfully allocated. No other value in case of success could
> +be returned. Device drivers can use this value to further allocate and
> +initialize device resources.
> +
> +4.2.6 pci_disable_msi
>  
>  void pci_disable_msi(struct pci_dev *dev)
>  
> @@ -142,7 +197,7 @@ on any interrupt for which it previously called request_irq().
>  Failure to do so results in a BUG_ON(), leaving the device with
>  MSI enabled and thus leaking its vector.
>  
> -4.2.4 pci_get_msi_cap
> +4.2.7 pci_get_msi_cap
>  
>  int pci_get_msi_cap(struct pci_dev *dev)
>  
> @@ -222,7 +277,76 @@ static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
>  	return -ENOSPC;
>  }
>  
> -4.3.2 pci_disable_msix
> +4.3.2 pcim_enable_msix_range
> +
> +int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +			   int minvec, int maxvec)
> +
> +This variation on pci_enable_msix() call allows a device driver to request
> +any number of MSI-Xs within specified range 'minvec' to 'maxvec'. Whenever
> +possible device drivers are encouraged to use this function rather than
> +explicit request loop calling pci_enable_msix().
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to allocate any more MSI-X interrupts for
> +this device.
> +
> +If this function returns a positive number it indicates the number of
> +MSI-X interrupts that have been successfully allocated. Device drivers
> +can use this number to further allocate and initialize device resources.
> +
> +A modified function calling pci_enable_msix() in a loop might look like:
> +
> +static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
> +{
> +	rc = pcim_enable_msix_range(adapter->pdev, adapter->msix_entries,
> +				    FOO_DRIVER_MINIMUM_NVEC, nvec);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = foo_driver_init_other(adapter, rc);
> +	if (rc < 0)
> +		pci_disable_msix(adapter->pdev);
> +
> +	return rc;
> +}
> +
> +4.3.3 pcim_enable_msix
> +
> +int pcim_enable_msix(struct pci_dev *dev,
> +		     struct msix_entry *entries, int maxvec)
> +
> +This variation on pci_enable_msix() call allows a device driver to request
> +any number of MSI-Xs up to 'maxvec'. Whenever possible device drivers are
> +encouraged to use this function rather than explicit request loop calling
> +pci_enable_msix().
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to allocate any more MSI-X interrupts for
> +this device.
> +
> +If this function returns a positive number it indicates the number of
> +MSI-X interrupts that have been successfully allocated. Device drivers
> +can use this number to further allocate and initialize device resources.
> +
> +4.3.4 pcim_enable_msix_exact
> +
> +int pcim_enable_msix_exact(struct pci_dev *dev,
> +			   struct msix_entry *entries, int nvec)
> +
> +This variation on pci_enable_msix() call allows a device driver to request
> +exactly 'nvec' MSI-Xs.
> +
> +If this function returns a negative number, it indicates an error and
> +the driver should not attempt to allocate any more MSI-X interrupts for
> +this device.
> +
> +If this function returns the value of 'nvec' it indicates MSI-X interrupts
> +have been successfully allocated. No other value in case of success could
> +be returned. Device drivers can use this value to further allocate and
> +initialize device resources.
> +
> +4.3.5 pci_disable_msix
>  
>  void pci_disable_msix(struct pci_dev *dev)
>  
> @@ -236,14 +360,14 @@ on any interrupt for which it previously called request_irq().
>  Failure to do so results in a BUG_ON(), leaving the device with
>  MSI-X enabled and thus leaking its vector.
>  
> -4.3.3 The MSI-X Table
> +4.3.6 The MSI-X Table
>  
>  The MSI-X capability specifies a BAR and offset within that BAR for the
>  MSI-X Table.  This address is mapped by the PCI subsystem, and should not
>  be accessed directly by the device driver.  If the driver wishes to
>  mask or unmask an interrupt, it should call disable_irq() / enable_irq().
>  
> -4.3.4 pci_msix_table_size
> +4.3.7 pci_msix_table_size
>  
>  int pci_msix_table_size(struct pci_dev *dev)
>  
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 6fe0add..c5fb4fb 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1086,3 +1086,77 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
>  	if (dev->msix_cap)
>  		msix_set_enable(dev, 0);
>  }
> +
> +/**
> + * pcim_enable_msi_range - configure device's MSI capability structure
> + * @dev: device to configure
> + * @minvec: minimal number of interrupts to configure
> + * @maxvec: maximum number of interrupts to configure
> + *
> + * This function tries to allocate a maximum possible number of interrupts in a
> + * range between @minvec and @maxvec. It returns a negative errno if an error
> + * occurs. If it succeeds, it returns the actual number of interrupts allocated
> + * and updates the @dev's irq member to the lowest new interrupt number;
> + * the other interrupt numbers allocated to this device are consecutive.
> + **/
> +int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
> +{
> +	int nvec = maxvec;
> +	int rc;
> +
> +	if (maxvec < minvec)
> +		return -ERANGE;
> +
> +	do {
> +		rc = pci_enable_msi_block(dev, nvec);
> +		if (rc < 0) {
> +			return rc;
> +		} else if (rc > 0) {
> +			if (rc < minvec)
> +				return -ENOSPC;
> +			nvec = rc;
> +		}
> +	} while (rc);
> +
> +	return nvec;
> +}
> +EXPORT_SYMBOL(pcim_enable_msi_range);
> +
> +/**
> + * pcim_enable_msix_range - configure device's MSI-X capability structure
> + * @dev: pointer to the pci_dev data structure of MSI-X device function
> + * @entries: pointer to an array of MSI-X entries
> + * @minvec: minimum number of MSI-X irqs requested
> + * @maxvec: maximum number of MSI-X irqs requested
> + *
> + * Setup the MSI-X capability structure of device function with a maximum
> + * possible number of interrupts in the range between @minvec and @maxvec
> + * upon its software driver call to request for MSI-X mode enabled on its
> + * hardware device function. It returns a negative errno if an error occurs.
> + * If it succeeds, it returns the actual number of interrupts allocated and
> + * indicates the successful configuration of MSI-X capability structure
> + * with new allocated MSI-X interrupts.
> + **/
> +int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +			   int minvec, int maxvec)
> +{
> +	int nvec = maxvec;
> +	int rc;
> +
> +	if (maxvec < minvec)
> +		return -ERANGE;
> +
> +	do {
> +		rc = pci_enable_msix(dev, entries, nvec);
> +		if (rc < 0) {
> +			return rc;
> +		} else if (rc > 0) {
> +			if (rc < minvec)
> +				return -ENOSPC;
> +			nvec = rc;
> +		}
> +	} while (rc);
> +
> +	return nvec;
> +}
> +EXPORT_SYMBOL(pcim_enable_msix_range);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8af1217..4e02d88 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1194,6 +1194,37 @@ static inline int pci_msi_enabled(void)
>  {
>  	return 0;
>  }
> +
> +int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int pcim_enable_msi(struct pci_dev *dev, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int pcim_enable_msi_exact(struct pci_dev *dev, int nvec)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int
> +pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +		       int minvec, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int
> +pcim_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int
> +pcim_enable_msix_exact(struct pci_dev *dev,
> +		       struct msix_entry *entries, int nvec)
> +{
> +	return -ENOSYS;
> +}
>  #else
>  int pci_get_msi_cap(struct pci_dev *dev);
>  int pci_enable_msi_block(struct pci_dev *dev, int nvec);
> @@ -1206,6 +1237,30 @@ void pci_disable_msix(struct pci_dev *dev);
>  void msi_remove_pci_irq_vectors(struct pci_dev *dev);
>  void pci_restore_msi_state(struct pci_dev *dev);
>  int pci_msi_enabled(void);
> +
> +int pcim_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec);
> +static inline int pcim_enable_msi(struct pci_dev *dev, int maxvec)
> +{
> +	return pcim_enable_msi_range(dev, 1, maxvec);
> +}
> +static inline int pcim_enable_msi_exact(struct pci_dev *dev, int nvec)
> +{
> +	return pcim_enable_msi_range(dev, nvec, nvec);
> +}
> +
> +int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +			   int minvec, int maxvec);
> +static inline int
> +pcim_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int maxvec)
> +{
> +	return pcim_enable_msix_range(dev, entries, 1, maxvec);
> +}
> +static inline int
> +pcim_enable_msix_exact(struct pci_dev *dev,
> +		       struct msix_entry *entries, int nvec)
> +{
> +	return pcim_enable_msix_range(dev, entries, nvec, nvec);
> +}
>  #endif
>  
>  #ifdef CONFIG_PCIEPORTBUS
> -- 
> 1.7.7.6
> 

Re: [PATCH v3 11/11] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Tue, Dec 10, 2013 at 04:16:02PM -0700, Bjorn Helgaas wrote:
> What's the reason for using the "pcim_" prefix?  To me, that suggests that
> this is a "managed" interface in the sense described in
> Documentation/driver-model/devres.txt, where resources are automatically
> deallocated when the driver detaches.  But I don't see anything like that
> happening in this patch.

Oh.. right. What about pci_auto_ prefix to indicate automatic fallback
logic? Or may be even pcia_ ?

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v3 11/11] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Tejun Heo]

On Thu, Dec 12, 2013 at 05:11:01PM +0100, Alexander Gordeev wrote:
> On Tue, Dec 10, 2013 at 04:16:02PM -0700, Bjorn Helgaas wrote:
> > What's the reason for using the "pcim_" prefix?  To me, that suggests that
> > this is a "managed" interface in the sense described in
> > Documentation/driver-model/devres.txt, where resources are automatically
> > deallocated when the driver detaches.  But I don't see anything like that
> > happening in this patch.
> 
> Oh.. right. What about pci_auto_ prefix to indicate automatic fallback
> logic? Or may be even pcia_ ?

I'd vote for pci_auto, one-off single char pre/postfixes usually get
pretty confusing.

Thanks.

-- 
tejun

Re: [PATCH v3 11/11] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Bjorn Helgaas]

On Thu, Dec 12, 2013 at 9:11 AM, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Dec 12, 2013 at 05:11:01PM +0100, Alexander Gordeev wrote:
>> On Tue, Dec 10, 2013 at 04:16:02PM -0700, Bjorn Helgaas wrote:
>> > What's the reason for using the "pcim_" prefix?  To me, that suggests that
>> > this is a "managed" interface in the sense described in
>> > Documentation/driver-model/devres.txt, where resources are automatically
>> > deallocated when the driver detaches.  But I don't see anything like that
>> > happening in this patch.
>>
>> Oh.. right. What about pci_auto_ prefix to indicate automatic fallback
>> logic? Or may be even pcia_ ?
>
> I'd vote for pci_auto, one-off single char pre/postfixes usually get
> pretty confusing.

Sounds good to me.

Re: [PATCH v3 00/11] Introduce pcim_enable_msi*() family helpers

[Tejun Heo]

Hello,

On Tue, Nov 26, 2013 at 10:09:49AM +0100, Alexander Gordeev wrote:
> Tejun, due to last three changes will you keep your "Acked-By" and
> "Reviewed-by" on these patches?
>   PCI/MSI: Factor out pci_get_msi_cap() interface
>   PCI/MSI: Get rid of pci_enable_msi_block_auto() interface
>   PCI/MSI: Convert pci_msix_table_size() to a public interface
>   PCI/MSI: Introduce pcim_enable_msi*() family helpers

Sure, the whole series looks good to me.

Thanks for doing this!

-- 
tejun

Re: [PATCH v3 00/11] Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Tue, Nov 26, 2013 at 10:09:49AM +0100, Alexander Gordeev wrote:
> Patches 3,4	- fixes for PowerPC pSeries platform
[...]
>   PCI/MSI/pSeries: Fix wrong error code reporting
>   PCI/MSI/pSeries: Make quota traversing and requesting race-safe

Hi Bjorn,

The two unreviewed PowerPC patches is not a blocker for the rest of the
series as they are completely independent with the rest of the changes.
I can rework them later if needed.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com