[PATCH v4 0/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[PATCH v4 0/9] PCI/MSI: Introduce pci_auto_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 v3 to v4:
  - pcim_enable_msi* functions renamed to pci_auto_enable_msi* ones
  - PowerPC patches dropped
  - pci_get_msi_cap()     renamed to pci_get_msix_vec_count() and
    pci_msix_table_size() renamed to pci_get_msix_vec_count()

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;

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 pci_auto_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 = pci_auto_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.

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


Alexander Gordeev (9):
  PCI/MSI/s390: Fix single MSI only check
  PCI/MSI/s390: Remove superfluous check of MSI type
  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_vec_count() interface
  PCI/MSI: Get rid of pci_enable_msi_block_auto() interface
  PCI/MSI: Introduce pci_get_msix_vec_count() interface
  PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

 Documentation/PCI/MSI-HOWTO.txt |  175 +++++++++++++++++++++++++++++++++------
 arch/s390/pci/pci.c             |    4 +-
 drivers/ata/ahci.c              |   56 ++++++++-----
 drivers/pci/msi.c               |  163 +++++++++++++++++++++++++++---------
 drivers/pci/pcie/portdrv_core.c |    7 +-
 include/linux/pci.h             |   78 +++++++++++++++---
 6 files changed, 380 insertions(+), 103 deletions(-)

-- 
1.7.7.6

[PATCH v4 1/9] 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 bf7c73d..6f3788e 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -409,6 +409,8 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 
 	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 v4 2/9] 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 6f3788e..4859c40 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -407,8 +407,6 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	struct msi_msg msg;
 	int rc;
 
-	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 v4 3/9] 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 c0baa3d..623322d 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -740,7 +740,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
@@ -753,10 +753,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);
@@ -767,7 +765,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
@@ -784,6 +782,7 @@ error:
 			ret = avail;
 	}
 
+out_free:
 	free_msi_irqs(dev);
 
 	return ret;
-- 
1.7.7.6

[PATCH v4 4/9] 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 e335f21..39493b7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1156,13 +1156,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)
@@ -1177,7 +1177,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 v4 5/9] 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>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 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 623322d..c0e2259 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -846,7 +846,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;
@@ -877,7 +877,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 39493b7..8cf7b15 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1154,13 +1154,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;
 }
@@ -1195,8 +1195,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

[PATCH v4 6/9] PCI/MSI: Factor out pci_get_msi_vec_count() 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..c03b815 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_vec_count
+
+int pci_get_msi_vec_count(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 c0e2259..8915edb 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -834,6 +834,31 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 }
 
 /**
+ * pci_get_msi_vec_count - Return the number of MSI vectors a device can 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_vec_count(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_vec_count);
+
+/**
  * pci_enable_msi_block - configure device's MSI capability structure
  * @dev: device to configure
  * @nvec: number of interrupts to configure
@@ -849,13 +874,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_vec_count(dev);
+	if (maxvec < 0)
+		return maxvec;
 	if (nvec > maxvec)
 		return maxvec;
 
@@ -880,13 +905,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_vec_count(dev);
+	if (ret < 0)
+		return ret;
 
 	if (maxvec)
 		*maxvec = ret;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8cf7b15..997751d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1154,6 +1154,11 @@ struct msix_entry {
 
 
 #ifndef CONFIG_PCI_MSI
+static inline int pci_get_msi_vec_count(struct pci_dev *dev)
+{
+	return -ENOSYS;
+}
+
 static inline int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 {
 	return -ENOSYS;
@@ -1195,6 +1200,7 @@ static inline int pci_msi_enabled(void)
 	return 0;
 }
 #else
+int pci_get_msi_vec_count(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 v4 7/9] PCI/MSI: Get rid of pci_enable_msi_block_auto() interface

[Alexander Gordeev]

As result of introduction of pci_get_msi_vec_count()
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_vec_count()
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 c03b815..8d35d62 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_vec_count
+4.2.4 pci_get_msi_vec_count
 
 int pci_get_msi_vec_count(struct pci_dev *dev)
 
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index e2903d0..e490d42 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1095,26 +1095,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_vec_count(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;
 }
@@ -1281,10 +1295,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);
 
@@ -1339,6 +1349,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 8915edb..de1acb9 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -902,31 +902,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_vec_count(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 997751d..8d9eb6b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1164,12 +1164,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)
@@ -1202,7 +1196,6 @@ static inline int pci_msi_enabled(void)
 #else
 int pci_get_msi_vec_count(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 v4 8/9] PCI/MSI: Introduce pci_get_msix_vec_count() interface

[Alexander Gordeev]

This update is needed to create a consistent MSI-X counterpart
for pci_get_msi_vec_count() 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.

Interface pci_get_msix_vec_count() supersedes pci_msix_table_size()
function and returns a negative errno if device does not support
MSI-X interrupts. After this update callers must always check the
returned value.

The only user of pci_msix_table_size() function was 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               |   18 +++++++++++++-----
 drivers/pci/pcie/portdrv_core.c |    7 ++++---
 include/linux/pci.h             |    6 +++---
 4 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 8d35d62..7d19656 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_get_msix_vec_count
+
+int pci_get_msix_vec_count(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 de1acb9..18e877f5 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -939,19 +939,25 @@ void pci_disable_msi(struct pci_dev *dev)
 EXPORT_SYMBOL(pci_disable_msi);
 
 /**
- * pci_msix_table_size - return the number of device's MSI-X table entries
+ * pci_get_msix_vec_count - return the number of device's MSI-X table entries
  * @dev: pointer to the pci_dev data structure of MSI-X device function
- */
-int pci_msix_table_size(struct pci_dev *dev)
+
+ * 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_get_msix_vec_count(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_get_msix_vec_count);
 
 /**
  * pci_enable_msix - configure device's MSI-X capability structure
@@ -980,7 +986,9 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 	if (status)
 		return status;
 
-	nr_entries = pci_msix_table_size(dev);
+	nr_entries = pci_get_msix_vec_count(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 ce9d9ae..8ba9db0 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -79,9 +79,10 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *vectors, int mask)
 	u16 reg16;
 	u32 reg32;
 
-	nr_entries = pci_msix_table_size(dev);
-	if (!nr_entries)
-		return -EINVAL;
+	nr_entries = pci_get_msix_vec_count(dev);
+	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 8d9eb6b..7941f06 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1169,9 +1169,9 @@ static inline void pci_msi_shutdown(struct pci_dev *dev)
 static inline void pci_disable_msi(struct pci_dev *dev)
 { }
 
-static inline int pci_msix_table_size(struct pci_dev *dev)
+static inline int pci_get_msix_vec_count(struct pci_dev *dev)
 {
-	return 0;
+	return -ENOSYS;
 }
 static inline int pci_enable_msix(struct pci_dev *dev,
 				  struct msix_entry *entries, int nvec)
@@ -1198,7 +1198,7 @@ int pci_get_msi_vec_count(struct pci_dev *dev);
 int pci_enable_msi_block(struct pci_dev *dev, int nvec);
 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);
+int pci_get_msix_vec_count(struct pci_dev *dev);
 int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec);
 void pci_msix_shutdown(struct pci_dev *dev);
 void pci_disable_msix(struct pci_dev *dev);
-- 
1.7.7.6

[PATCH v4 9/9] PCI/MSI: Introduce pci_auto_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 pci_auto_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 = pci_auto_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             |   57 +++++++++++++++++
 3 files changed, 260 insertions(+), 5 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 7d19656..168d9c3 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 pci_auto_enable_msi_range
+
+int pci_auto_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 pci_auto_enable_msi
+
+int pci_auto_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 pci_auto_enable_msi_exact
+
+int pci_auto_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_vec_count
+4.2.7 pci_get_msi_vec_count
 
 int pci_get_msi_vec_count(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 pci_auto_enable_msix_range
+
+int pci_auto_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 = pci_auto_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 pci_auto_enable_msix
+
+int pci_auto_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 pci_auto_enable_msix_exact
+
+int pci_auto_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_get_msix_vec_count
+4.3.7 pci_get_msix_vec_count
 
 int pci_get_msix_vec_count(struct pci_dev *dev)
 
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 18e877f5..ccfd49b 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1093,3 +1093,77 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
 	if (dev->msix_cap)
 		msix_set_enable(dev, 0);
 }
+
+/**
+ * pci_auto_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 pci_auto_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(pci_auto_enable_msi_range);
+
+/**
+ * pci_auto_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 pci_auto_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(pci_auto_enable_msix_range);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7941f06..7e30b52 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1193,6 +1193,38 @@ static inline int pci_msi_enabled(void)
 {
 	return 0;
 }
+
+int pci_auto_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int pci_auto_enable_msi(struct pci_dev *dev, int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int pci_auto_enable_msi_exact(struct pci_dev *dev, int nvec)
+{
+	return -ENOSYS;
+}
+
+static inline int
+pci_auto_enable_msix_range(struct pci_dev *dev,
+			   struct msix_entry *entries, int minvec, int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int
+pci_auto_enable_msix(struct pci_dev *dev,
+		     struct msix_entry *entries, int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int
+pci_auto_enable_msix_exact(struct pci_dev *dev,
+			   struct msix_entry *entries, int nvec)
+{
+	return -ENOSYS;
+}
 #else
 int pci_get_msi_vec_count(struct pci_dev *dev);
 int pci_enable_msi_block(struct pci_dev *dev, int nvec);
@@ -1205,6 +1237,31 @@ 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 pci_auto_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec);
+static inline int pci_auto_enable_msi(struct pci_dev *dev, int maxvec)
+{
+	return pci_auto_enable_msi_range(dev, 1, maxvec);
+}
+static inline int pci_auto_enable_msi_exact(struct pci_dev *dev, int nvec)
+{
+	return pci_auto_enable_msi_range(dev, nvec, nvec);
+}
+
+int pci_auto_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
+			       int minvec, int maxvec);
+static inline int
+pci_auto_enable_msix(struct pci_dev *dev,
+		     struct msix_entry *entries, int maxvec)
+{
+	return pci_auto_enable_msix_range(dev, entries, 1, maxvec);
+}
+static inline int
+pci_auto_enable_msix_exact(struct pci_dev *dev,
+			   struct msix_entry *entries, int nvec)
+{
+	return pci_auto_enable_msix_range(dev, entries, nvec, nvec);
+}
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS
-- 
1.7.7.6

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Bjorn Helgaas]

On Mon, Dec 16, 2013 at 09:35:02AM +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 pci_auto_enable_msi*() family
> helpers.

I think the idea of this series is excellent and will really make MSI/MSI-X
easier to use and less error-prone for drivers, so I don't want this to
sound discouraging.  I haven't been paying attention to this in detail, so
likely some of my questions have already been hashed out and I missed the
answers.

After this patch, we would have:

    pci_enable_msi()				# existing (1 vector)
    pci_enable_msi_block(nvec)			# existing
    pci_enable_msi_block_auto(maxvec)		# existing (removed)

    pci_auto_enable_msi(maxvec)			# new	(1-maxvec)
    pci_auto_enable_msi_range(minvec, maxvec)	# new
    pci_auto_enable_msi_exact(nvec)		# new	(nvec-nvec)

    pci_enable_msix(nvec)			# existing

    pci_auto_enable_msix(maxvec)		# new	(1-maxvec)
    pci_auto_enable_msix_range(minvec, maxvec)	# new
    pci_auto_enable_msix_exact(nvec)		# new	(nvec-nvec)

That seems like a lot of interfaces to document and understand, especially
since most of them are built on each other.  I'd prefer just these:

    pci_enable_msi()				# existing (1 vector)
    pci_enable_msi_range(minvec, maxvec)	# new

    pci_enable_msix(nvec)			# existing
    pci_enable_msix_range(minvec, maxvec)	# new

with examples in the documentation about how to call them with ranges like
(1, maxvec), (nvec, nvec), etc.  I think that will be easier than
understanding several interfaces.

I don't think the "auto" in the names really adds anything, does it?  The
whole point of supplying a range is that the core has the flexibility to
choose any number of vectors within the range.

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

I only see five users of pci_enable_msi_block() (nvme, ath10k, wil6210,
ipr, vfio); we can easily convert those to use pci_enable_msi_range() and
then remove pci_enable_msi_block().

pci_enable_msi() itself can simply be pci_enable_msi_range(1, 1).

There are nearly 80 callers of pci_enable_msix(), so that's a bit harder.
Can we deprecate that somehow, and incrementally convert callers to use
pci_enable_msix_range() instead?  Maybe you're already planning that; I
know you dropped some driver patches from the series for now, and I didn't
look to see exactly what they did.

It would be good if pci_enable_msix() could be implemented in terms of
pci_enable_msix_range(nvec, nvec), with a little extra glue to handle the
positive return values.

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

I think we should remove this example from the documentation because we
want to get rid of the tri-state return idea completely.  I think the same
thing could be accomplished with pci_enable_msix_range() (correct me if I'm
wrong).

> ...would turn into a single helper call....
> 
> 	rc = pci_auto_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             |   57 +++++++++++++++++
>  3 files changed, 260 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
> index 7d19656..168d9c3 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 pci_auto_enable_msi_range
> +
> +int pci_auto_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().

I think we should remove pci_enable_msi_block() completely, including its
mention here.

> +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).

I assume this means the return value may be larger than the "maxvec"
requested, right?  And the driver is free to use all the vectors up to the
return value, even those above maxvec, right?

> +Device drivers can use this number to further initialize devices.
> +
> +4.2.4 pci_auto_enable_msi
> +
> +int pci_auto_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 pci_auto_enable_msi_exact
> +
> +int pci_auto_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_vec_count
> +4.2.7 pci_get_msi_vec_count
>  
>  int pci_get_msi_vec_count(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 pci_auto_enable_msix_range
> +
> +int pci_auto_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().

I guess maybe I'm wrong, and there *are* cases where
pci_enable_msix_range() isn't sufficient to replace pci_enable_msix()?
Can you remind me what they are?

> +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:

There's no loop in this example.  Don't use "A modified function"; that
only makes sense during the transition from pci_enable_msix() to
pci_enable_msix_range().  "A function using pci_enable_msix_range() might
look like this:" should be sufficient.

> +static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
> +{
> +	rc = pci_auto_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 pci_auto_enable_msix
> +
> +int pci_auto_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 pci_auto_enable_msix_exact
> +
> +int pci_auto_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_get_msix_vec_count
> +4.3.7 pci_get_msix_vec_count
>  
>  int pci_get_msix_vec_count(struct pci_dev *dev)
>  
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 18e877f5..ccfd49b 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1093,3 +1093,77 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
>  	if (dev->msix_cap)
>  		msix_set_enable(dev, 0);
>  }
> +
> +/**
> + * pci_auto_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 pci_auto_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(pci_auto_enable_msi_range);
> +
> +/**
> + * pci_auto_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 pci_auto_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;

I think it would be better to make pci_enable_msix_range() the fundamental
implementation, with pci_enable_msix() built on top of it.  That way we
could deprecate and eventually remove pci_enable_msix() and its tri-state
return values.

Bjorn

> +}
> +EXPORT_SYMBOL(pci_auto_enable_msix_range);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7941f06..7e30b52 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1193,6 +1193,38 @@ static inline int pci_msi_enabled(void)
>  {
>  	return 0;
>  }
> +
> +int pci_auto_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int pci_auto_enable_msi(struct pci_dev *dev, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int pci_auto_enable_msi_exact(struct pci_dev *dev, int nvec)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int
> +pci_auto_enable_msix_range(struct pci_dev *dev,
> +			   struct msix_entry *entries, int minvec, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int
> +pci_auto_enable_msix(struct pci_dev *dev,
> +		     struct msix_entry *entries, int maxvec)
> +{
> +	return -ENOSYS;
> +}
> +static inline int
> +pci_auto_enable_msix_exact(struct pci_dev *dev,
> +			   struct msix_entry *entries, int nvec)
> +{
> +	return -ENOSYS;
> +}
>  #else
>  int pci_get_msi_vec_count(struct pci_dev *dev);
>  int pci_enable_msi_block(struct pci_dev *dev, int nvec);
> @@ -1205,6 +1237,31 @@ 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 pci_auto_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec);
> +static inline int pci_auto_enable_msi(struct pci_dev *dev, int maxvec)
> +{
> +	return pci_auto_enable_msi_range(dev, 1, maxvec);
> +}
> +static inline int pci_auto_enable_msi_exact(struct pci_dev *dev, int nvec)
> +{
> +	return pci_auto_enable_msi_range(dev, nvec, nvec);
> +}
> +
> +int pci_auto_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
> +			       int minvec, int maxvec);
> +static inline int
> +pci_auto_enable_msix(struct pci_dev *dev,
> +		     struct msix_entry *entries, int maxvec)
> +{
> +	return pci_auto_enable_msix_range(dev, entries, 1, maxvec);
> +}
> +static inline int
> +pci_auto_enable_msix_exact(struct pci_dev *dev,
> +			   struct msix_entry *entries, int nvec)
> +{
> +	return pci_auto_enable_msix_range(dev, entries, nvec, nvec);
> +}
>  #endif
>  
>  #ifdef CONFIG_PCIEPORTBUS
> -- 
> 1.7.7.6
> 

Re: [PATCH v4 6/9] PCI/MSI: Factor out pci_get_msi_vec_count() interface

[Bjorn Helgaas]

On Mon, Dec 16, 2013 at 09:34:59AM +0100, Alexander Gordeev wrote:
> 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..c03b815 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_vec_count
> +
> +int pci_get_msi_vec_count(struct pci_dev *dev)

What would you think of "pci_msi_vec_count()" or "pci_msi_vector_count()"
instead?  The "get" doesn't really seem necessary, and we often use "get"
to indicate acquiring a reference (though it's obvious that doesn't apply
here).

Bjorn

> +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 c0e2259..8915edb 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -834,6 +834,31 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
>  }
>  
>  /**
> + * pci_get_msi_vec_count - Return the number of MSI vectors a device can 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_vec_count(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_vec_count);
> +
> +/**
>   * pci_enable_msi_block - configure device's MSI capability structure
>   * @dev: device to configure
>   * @nvec: number of interrupts to configure
> @@ -849,13 +874,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_vec_count(dev);
> +	if (maxvec < 0)
> +		return maxvec;
>  	if (nvec > maxvec)
>  		return maxvec;
>  
> @@ -880,13 +905,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_vec_count(dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (maxvec)
>  		*maxvec = ret;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8cf7b15..997751d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1154,6 +1154,11 @@ struct msix_entry {
>  
>  
>  #ifndef CONFIG_PCI_MSI
> +static inline int pci_get_msi_vec_count(struct pci_dev *dev)
> +{
> +	return -ENOSYS;
> +}
> +
>  static inline int pci_enable_msi_block(struct pci_dev *dev, int nvec)
>  {
>  	return -ENOSYS;
> @@ -1195,6 +1200,7 @@ static inline int pci_msi_enabled(void)
>  	return 0;
>  }
>  #else
> +int pci_get_msi_vec_count(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
> 

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Alexander Gordeev]

On Tue, Dec 17, 2013 at 05:30:02PM -0700, Bjorn Helgaas wrote:

Hi Bjorn,

Thank you for the review!

Sorry for a heavy skipping - I just wanted to focus on a principal
moment in your suggestion and then go on with the original note.

> I only see five users of pci_enable_msi_block() (nvme, ath10k, wil6210,
> ipr, vfio); we can easily convert those to use pci_enable_msi_range() and
> then remove pci_enable_msi_block().

> It would be good if pci_enable_msix() could be implemented in terms of
> pci_enable_msix_range(nvec, nvec), with a little extra glue to handle the
> positive return values.

So you want to get rid of the tri-state "low-level" pci_enable_msi_block()
and pci_enable_msix(), right? I believe we can not do this, since we need
to support a non-standard hardware which (a) can not be asked any arbitrary
number of vectors within a range and (b) needs extra magic to enable MSI
operation.

I.e. below is a snippet from a real device driver Mark Lord has sent in a
previous conversation:

        xx_disable_all_irqs(dev);
        do {
            	if (nvec < 2)
                        xx_prep_for_1_msix_vector(dev);
                else if (nvec < 4)
                        xx_prep_for_2_msix_vectors(dev);
                else if (nvec < 8)
                        xx_prep_for_4_msix_vectors(dev);
                else if (nvec < 16)
                        xx_prep_for_8_msix_vectors(dev);
                else
                    	xx_prep_for_16_msix_vectors(dev);
                nvec = pci_enable_msix(dev->pdev, dev->irqs, dev->num_vectors);
       	} while (nvec > 0);

The same probably could have been done with pci_enable_msix_range(nvec, nvec)
call and checking for -ENOSPC errno, but IMO it would be less graceful and
reliable, since -ENOSPC might come from anywhere.

IOW, I believe we need to keep the door open for custom MSI-enablement (loop)
implementations.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Bjorn Helgaas]

On Wed, Dec 18, 2013 at 6:23 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Tue, Dec 17, 2013 at 05:30:02PM -0700, Bjorn Helgaas wrote:
>
> Hi Bjorn,
>
> Thank you for the review!
>
> Sorry for a heavy skipping - I just wanted to focus on a principal
> moment in your suggestion and then go on with the original note.
>
>> I only see five users of pci_enable_msi_block() (nvme, ath10k, wil6210,
>> ipr, vfio); we can easily convert those to use pci_enable_msi_range() and
>> then remove pci_enable_msi_block().
>
>> It would be good if pci_enable_msix() could be implemented in terms of
>> pci_enable_msix_range(nvec, nvec), with a little extra glue to handle the
>> positive return values.
>
> So you want to get rid of the tri-state "low-level" pci_enable_msi_block()
> and pci_enable_msix(), right? I believe we can not do this, since we need
> to support a non-standard hardware which (a) can not be asked any arbitrary
> number of vectors within a range and (b) needs extra magic to enable MSI
> operation.
>
> I.e. below is a snippet from a real device driver Mark Lord has sent in a
> previous conversation:
>
>         xx_disable_all_irqs(dev);
>         do {
>                 if (nvec < 2)
>                         xx_prep_for_1_msix_vector(dev);
>                 else if (nvec < 4)
>                         xx_prep_for_2_msix_vectors(dev);
>                 else if (nvec < 8)
>                         xx_prep_for_4_msix_vectors(dev);
>                 else if (nvec < 16)
>                         xx_prep_for_8_msix_vectors(dev);
>                 else
>                         xx_prep_for_16_msix_vectors(dev);
>                 nvec = pci_enable_msix(dev->pdev, dev->irqs, dev->num_vectors);
>         } while (nvec > 0);
>
> The same probably could have been done with pci_enable_msix_range(nvec, nvec)
> call and checking for -ENOSPC errno, but IMO it would be less graceful and
> reliable, since -ENOSPC might come from anywhere.
>
> IOW, I believe we need to keep the door open for custom MSI-enablement (loop)
> implementations.

I think this can still be done even with pci_enable_msix_range().
Here's what I'm thinking, tell me where I'm going wrong:

    rc = pci_enable_msix_range(dev->pdev, dev->irqs, 1, 16);
    if (rc < 0) { /* error */ }
    else { /* rc interrupts allocated */ }

If rc == 13 and the device can only use 8, the extra 5 would be
ignored and wasted.

If the waste is unacceptable, the driver can try this:

    rc = pci_enable_msix_range(dev->pdev, dev->irqs, 16, 16);
    if (rc < 0) {
        rc = pci_enable_msix_range(dev->pdev, dev->irqs, 8, 8);
        if (rc < 0) {
            rc = pci_enable_msix_range(dev->pdev, dev->irqs, 4, 4);
            ...
    }

    if (rc < 0) { /* error, couldn't allocate *any* interrupts */
    else { /* rc interrupts allocated (1, 2, 4, 8, or 16) */ }

Bjorn

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Alexander Gordeev]

On Wed, Dec 18, 2013 at 11:58:47AM -0700, Bjorn Helgaas wrote:
> If rc == 13 and the device can only use 8, the extra 5 would be
> ignored and wasted.
> 
> If the waste is unacceptable, the driver can try this:
> 
>     rc = pci_enable_msix_range(dev->pdev, dev->irqs, 16, 16);
>     if (rc < 0) {
>         rc = pci_enable_msix_range(dev->pdev, dev->irqs, 8, 8);
>         if (rc < 0) {
>             rc = pci_enable_msix_range(dev->pdev, dev->irqs, 4, 4);
>             ...
>     }

I have troubles with this fallback logic. On each failed step we get an
error and we do not know if this is indeed an error or an indication of
insufficient MSI resources. Even -ENOSPC would not tell much, since it
could be thrown from a lower level.

By contrast, with the tri-state return value we can distinguish and bail
out on errors right away.

So the above is bit ungraceful for me. Combined with a possible waste in
logs (if we're hitting the same error) it is quite enough for me to keep
current the interfaces, at least for a time being.

>     if (rc < 0) { /* error, couldn't allocate *any* interrupts */
>     else { /* rc interrupts allocated (1, 2, 4, 8, or 16) */ }
> 
> Bjorn

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Tejun Heo]

Hello,

On Thu, Dec 19, 2013 at 02:42:45PM +0100, Alexander Gordeev wrote:
> I have troubles with this fallback logic. On each failed step we get an
> error and we do not know if this is indeed an error or an indication of
> insufficient MSI resources. Even -ENOSPC would not tell much, since it
> could be thrown from a lower level.

Well, it's not hard to define what -ENOSPC should mean.

> By contrast, with the tri-state return value we can distinguish and bail
> out on errors right away.

I kinda like that the available options are listed explicitly on the
caller side (even if it ends up being a loop).  It decreases the
chance of the caller going "oh I didn't expect _that_" and generaly
makes things easier to follow.

> So the above is bit ungraceful for me. Combined with a possible waste in
> logs (if we're hitting the same error) it is quite enough for me to keep
> current the interfaces, at least for a time being.

FWIW, I like Bjorn's suggestion.  Given that this is mostly corner
case thing, it isn't as important as the common ones but we might as
well while we're at it.

Thanks a lot for your work in the area!  :)

-- 
tejun

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Bjorn Helgaas]

On Thu, Dec 19, 2013 at 6:42 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Wed, Dec 18, 2013 at 11:58:47AM -0700, Bjorn Helgaas wrote:
>> If rc == 13 and the device can only use 8, the extra 5 would be
>> ignored and wasted.
>>
>> If the waste is unacceptable, the driver can try this:
>>
>>     rc = pci_enable_msix_range(dev->pdev, dev->irqs, 16, 16);
>>     if (rc < 0) {
>>         rc = pci_enable_msix_range(dev->pdev, dev->irqs, 8, 8);
>>         if (rc < 0) {
>>             rc = pci_enable_msix_range(dev->pdev, dev->irqs, 4, 4);
>>             ...
>>     }
>
> I have troubles with this fallback logic. On each failed step we get an
> error and we do not know if this is indeed an error or an indication of
> insufficient MSI resources. Even -ENOSPC would not tell much, since it
> could be thrown from a lower level.
>
> By contrast, with the tri-state return value we can distinguish and bail
> out on errors right away.

I thought the main point of this was to get rid of interfaces that
were prone to misuse, and tri-state return values was a big part of
that.  All we really care about in the driver is success/failure.  I'm
not sure there's much to be gained by analyzing *why* we failed, and I
think it tends to make uncommon error paths more complicated than
necessary.  If we fail four times instead of bailing out after the
first failure, well, that doesn't sound terrible to me.  The last
failure can log the errno, which is enough for debugging.

> So the above is bit ungraceful for me. Combined with a possible waste in
> logs (if we're hitting the same error) it is quite enough for me to keep
> current the interfaces, at least for a time being.
>
>>     if (rc < 0) { /* error, couldn't allocate *any* interrupts */
>>     else { /* rc interrupts allocated (1, 2, 4, 8, or 16) */ }
>>
>> Bjorn
>
> --
> Regards,
> Alexander Gordeev
> agordeev@redhat.com

Re: [PATCH v4 0/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Bjorn Helgaas]

On Mon, Dec 16, 2013 at 09:34:53AM +0100, Alexander Gordeev wrote:
> Alexander Gordeev (9):
>   PCI/MSI/s390: Fix single MSI only check
>   PCI/MSI/s390: Remove superfluous check of MSI type
>   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

I applied the patches above to pci/msi for v3.14 to get them out of the
way.

>   PCI/MSI: Factor out pci_get_msi_vec_count() interface
>   PCI/MSI: Get rid of pci_enable_msi_block_auto() interface
>   PCI/MSI: Introduce pci_get_msix_vec_count() interface
>   PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Alexander Gordeev]

On Thu, Dec 19, 2013 at 02:37:22PM -0700, Bjorn Helgaas wrote:
> On Thu, Dec 19, 2013 at 6:42 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> > On Wed, Dec 18, 2013 at 11:58:47AM -0700, Bjorn Helgaas wrote:
> >> If rc == 13 and the device can only use 8, the extra 5 would be
> >> ignored and wasted.
> >>
> >> If the waste is unacceptable, the driver can try this:
> >>
> >>     rc = pci_enable_msix_range(dev->pdev, dev->irqs, 16, 16);
> >>     if (rc < 0) {
> >>         rc = pci_enable_msix_range(dev->pdev, dev->irqs, 8, 8);
> >>         if (rc < 0) {
> >>             rc = pci_enable_msix_range(dev->pdev, dev->irqs, 4, 4);
> >>             ...
> >>     }
> >
> > I have troubles with this fallback logic. On each failed step we get an
> > error and we do not know if this is indeed an error or an indication of
> > insufficient MSI resources. Even -ENOSPC would not tell much, since it
> > could be thrown from a lower level.
> >
> > By contrast, with the tri-state return value we can distinguish and bail
> > out on errors right away.
> 
> I thought the main point of this was to get rid of interfaces that
> were prone to misuse, and tri-state return values was a big part of
> that.  All we really care about in the driver is success/failure.  I'm
> not sure there's much to be gained by analyzing *why* we failed, and I
> think it tends to make uncommon error paths more complicated than
> necessary.  If we fail four times instead of bailing out after the
> first failure, well, that doesn't sound terrible to me.  The last
> failure can log the errno, which is enough for debugging.

Sure, the main point is to get rid of try-state interfaces. I just afraid
to throw out the baby with the bath water for unusual devices (which we do
not have in the tree).

I can only identify two downsides of the approach above - (a) repeated error
logging in a platform code (i.e. caused by -ENOMEM) and (b) repeated attempts
to enable MSI when the platform already reported a fatal error.

I think if a device needs an extra magic to enable MSI (i.e. writing to
specific registers etc.) it would be manageable with pci_enable_msix_range(),
but may be I am missing something?

So my thought is may be we deprecate the tri-state interfaces, but do not
do it immediately.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Alexander Gordeev]

On Tue, Dec 17, 2013 at 05:30:02PM -0700, Bjorn Helgaas wrote:
> After this patch, we would have:
> 
>     pci_enable_msi()				# existing (1 vector)
>     pci_enable_msi_block(nvec)			# existing
>     pci_enable_msi_block_auto(maxvec)		# existing (removed)
> 
>     pci_auto_enable_msi(maxvec)			# new	(1-maxvec)
>     pci_auto_enable_msi_range(minvec, maxvec)	# new
>     pci_auto_enable_msi_exact(nvec)		# new	(nvec-nvec)
> 
>     pci_enable_msix(nvec)			# existing
> 
>     pci_auto_enable_msix(maxvec)		# new	(1-maxvec)
>     pci_auto_enable_msix_range(minvec, maxvec)	# new
>     pci_auto_enable_msix_exact(nvec)		# new	(nvec-nvec)
> 
> That seems like a lot of interfaces to document and understand, especially
> since most of them are built on each other.  I'd prefer just these:
> 
>     pci_enable_msi()				# existing (1 vector)
>     pci_enable_msi_range(minvec, maxvec)	# new
> 
>     pci_enable_msix(nvec)			# existing
>     pci_enable_msix_range(minvec, maxvec)	# new
> 
> with examples in the documentation about how to call them with ranges like
> (1, maxvec), (nvec, nvec), etc.  I think that will be easier than
> understanding several interfaces.

I agree pci_auto_enable_msix() and pci_auto_enable_msix_exact() are worth
sacrificing for the sake of clarity. My only concern is people will start
defining their own helpers for (1, maxvec) and (nvec, nvec) cases here and
there...

> I don't think the "auto" in the names really adds anything, does it?  The
> whole point of supplying a range is that the core has the flexibility to
> choose any number of vectors within the range.

"Auto" indicates auto-retry, but I see no problem in skipping it, especially
if we deprecate or phase out the existing interfaces.

> I only see five users of pci_enable_msi_block() (nvme, ath10k, wil6210,
> ipr, vfio); we can easily convert those to use pci_enable_msi_range() and
> then remove pci_enable_msi_block().
> 
> pci_enable_msi() itself can simply be pci_enable_msi_range(1, 1).
> 
> There are nearly 80 callers of pci_enable_msix(), so that's a bit harder.
> Can we deprecate that somehow, and incrementally convert callers to use
> pci_enable_msix_range() instead?  Maybe you're already planning that; I
> know you dropped some driver patches from the series for now, and I didn't
> look to see exactly what they did.

Right, the plan is first to introduce pci_auto_* (or whatever) family into
the tree and then gradually convert all drivers to the new interfaces.

> It would be good if pci_enable_msix() could be implemented in terms of
> pci_enable_msix_range(nvec, nvec), with a little extra glue to handle the
> positive return values.

[...]

> I think it would be better to make pci_enable_msix_range() the fundamental
> implementation, with pci_enable_msix() built on top of it.  That way we
> could deprecate and eventually remove pci_enable_msix() and its tri-state
> return values.

We can reuse pci_enable_msix() name, but not before all drivers converted.

But considering the other thread you want to have only pci_enable_msi_range()
and pci_enable_msix_range() interfaces - am I getting it right?

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Tejun Heo]

Hello,

On Fri, Dec 20, 2013 at 10:04:13AM +0100, Alexander Gordeev wrote:
> I can only identify two downsides of the approach above - (a) repeated error
> logging in a platform code (i.e. caused by -ENOMEM) and (b) repeated attempts
> to enable MSI when the platform already reported a fatal error.

I don't think (a) is likely as long as only -ENOSPC is retried, which
also solves (b).

Thanks.

-- 
tejun

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Alexander Gordeev]

On Tue, Dec 17, 2013 at 05:30:02PM -0700, Bjorn Helgaas wrote:
> > +int pci_auto_enable_msi_range(struct pci_dev *dev, struct msix_entry *entries,
> > +			      int minvec, int maxvec)

[...]

> > +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).
> 
> I assume this means the return value may be larger than the "maxvec"
> requested, right?  And the driver is free to use all the vectors up to the
> return value, even those above maxvec, right?

No, the returned value may not be larger than the "maxvec" ever. This is just
paraphrasing the semantics of exisitng pci_enable_msi_block() interface - a
value written to MMC register might be larger than the returned value, but the
driver may not use the extra vectors it did not request.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v4 9/9] PCI/MSI: Introduce pci_auto_enable_msi*() family helpers

[Bjorn Helgaas]

On Mon, Dec 23, 2013 at 7:44 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Tue, Dec 17, 2013 at 05:30:02PM -0700, Bjorn Helgaas wrote:
>> > +int pci_auto_enable_msi_range(struct pci_dev *dev, struct msix_entry *entries,
>> > +                         int minvec, int maxvec)
>
> [...]
>
>> > +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).
>>
>> I assume this means the return value may be larger than the "maxvec"
>> requested, right?  And the driver is free to use all the vectors up to the
>> return value, even those above maxvec, right?
>
> No, the returned value may not be larger than the "maxvec" ever. This is just
> paraphrasing the semantics of exisitng pci_enable_msi_block() interface - a
> value written to MMC register might be larger than the returned value, but the
> driver may not use the extra vectors it did not request.

Then I think we should remove the "(it may have allocated more...)"
text.  If the driver can't use those extra vectors, they are an
internal implementation detail, and mentioning them here will only
cause confusion.  The "at least" text should also be removed.  From
the driver's point of view, it can use exactly the number of
interrupts returned.

Bjorn