[PATCH RFC v2 00/29] Introduce pcim_enable_msi*() family helpers

[PATCH RFC v2 00/29] 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

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,
				    nvec,
				    FOO_DRIVER_MINIMUM_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.

It is fair to say the proposal in v1 ("Re-design MSI/MSI-X interrupts
enablement pattern")is scrapped. Thus, I do not clarify what are the
changes since v1  - the whole v2 series could be
considered as completely new.

Device driver updates (patches 12-29) are provided to clarify the
change and expected to get reviews in follow-up post(s) when/if
patch 12 ("PCI/MSI: Introduce pcim_enable_msi*() family helpers")
is accepted.

Patches 1,2	- ACK'ed tweaks for s390 architecture
Patches 3,4	- fixes for PowerPC pSeries platform
Patches 5-12	- fixes, tweaks and changes of the generic MSI code
Patches 12-29	- example updates of few device drivers

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

Alexander Gordeev (29):
  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: Get rid of useless count of msi_desc leftovers
  PCI/MSI: Return -ENOSYS for unimplemented interfaces, not -1
  PCI/MSI: Make pci_enable_msix() 'nvec' argument unsigned 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
  ipr: Do not call pci_disable_msi/msix() if pci_enable_msi/msix()
    failed
  ipr: Make use of pcim_enable_msi/msix() interfaces
  ixgbe: Make use of pcim_enable_msix_range() interface
  ixgbevf: Make use of pcim_enable_msix_range() interface
  megaraid: Make use of pcim_enable_msix() interface
  ntb: Fix missed call to pci_enable_msix()
  ntb: Make use of pcim_enable_msix/msix_exact() interfaces
  qib: Make use of pcim_enable_msix() and pci_msix_table_size()
    interfaces
  qla2xxx: Make use of pcim_enable_msix_range() interface
  qlge: Get rid of an redundant assignment
  qlge: Make use of pcim_enable_msix() interface
  tg3: Make use of pcim_enable_msix() interface
  vmxnet3: Return -EINVAL if number of requested MSI-Xs is not enough
  vmxnet3: Fixup a weird loop exit
  vmxnet3: Return -ENOSPC when not enough MSI-X vectors available
  vmxnet3: Limit number of rx queues to 1 if per-queue MSI-Xs failed
  vmxnet3: Make use of pcim_enable_msix_range() interface

 Documentation/PCI/MSI-HOWTO.txt                   |  169 ++++++++++++++++++---
 arch/powerpc/platforms/pseries/msi.c              |   26 +++-
 arch/s390/pci/pci.c                               |    4 +-
 drivers/ata/ahci.c                                |   56 +++++---
 drivers/infiniband/hw/qib/qib_pcie.c              |   48 +++---
 drivers/net/ethernet/broadcom/tg3.c               |    6 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c      |   16 +--
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   33 ++---
 drivers/net/ethernet/qlogic/qlge/qlge_main.c      |   16 +--
 drivers/net/vmxnet3/vmxnet3_drv.c                 |   58 +++-----
 drivers/ntb/ntb_hw.c                              |   34 ++---
 drivers/pci/msi.c                                 |  129 +++++++++-------
 drivers/pci/pcie/portdrv_core.c                   |    5 +-
 drivers/scsi/ipr.c                                |   51 +++----
 drivers/scsi/megaraid/megaraid_sas_base.c         |   16 +--
 drivers/scsi/qla2xxx/qla_isr.c                    |   28 ++--
 include/linux/pci.h                               |   79 +++++++++--
 17 files changed, 466 insertions(+), 308 deletions(-)

-- 
1.7.7.6

[PATCH RFC v2 01/29] 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 RFC v2 02/29] 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 RFC v2 03/29] 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 RFC v2 04/29] PCI/MSI/pSeries: Make quota traversing and requesting race-safe

[Alexander Gordeev]

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 RFC v2 04/29] PCI/MSI/pSeries: Make quota traversing and requesting race-safe

[Tejun Heo]

Hello,

Sorry about the long delay.  Was traveling quite a bit.

On Fri, Oct 18, 2013 at 07:12:04PM +0200, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

Please explain what the race condition in detail and how it gets fixed
by the patch.  This isn't a trivial change and non-trivial changes
especially stuff like race condition fixes definitely require detailed
explanation.

Thanks.

-- 
tejun

[PATCH RFC v2 05/29] 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>
---
 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 d5f90d6..b43f391 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -719,7 +719,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
@@ -732,10 +732,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);
@@ -746,7 +744,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
@@ -763,6 +761,7 @@ error:
 			ret = avail;
 	}
 
+out_free:
 	free_msi_irqs(dev);
 
 	return ret;
-- 
1.7.7.6

Re: [PATCH RFC v2 05/29] PCI/MSI: Fix return value when populate_msi_sysfs() failed

[Tejun Heo]

On Fri, Oct 18, 2013 at 07:12:05PM +0200, Alexander Gordeev wrote:
> 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>

Thanks.

-- 
tejun

[PATCH RFC v2 06/29] PCI/MSI: Get rid of useless count of msi_desc leftovers

[Alexander Gordeev]

If an architecture failed to allocate requested number of
MSI-Xs we should not mislead device drivers and make them
retry based on how many MSI-Xs were possibly allocated by
the architecture in the process, before the failure. It is
rude for an architecture to leave any leftovers anyway.

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

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index b43f391..bbe3d3d 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -719,7 +719,7 @@ static int msix_capability_init(struct pci_dev *dev,
 
 	ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
 	if (ret)
-		goto out_avail;
+		goto out_free;
 
 	/*
 	 * Some devices require MSI-X to be enabled before we can touch the
@@ -744,23 +744,6 @@ static int msix_capability_init(struct pci_dev *dev,
 
 	return 0;
 
-out_avail:
-	if (ret < 0) {
-		/*
-		 * If we had some success, report the number of irqs
-		 * we succeeded in setting up.
-		 */
-		struct msi_desc *entry;
-		int avail = 0;
-
-		list_for_each_entry(entry, &dev->msi_list, list) {
-			if (entry->irq != 0)
-				avail++;
-		}
-		if (avail != 0)
-			ret = avail;
-	}
-
 out_free:
 	free_msi_irqs(dev);
 
-- 
1.7.7.6

Re: [PATCH RFC v2 06/29] PCI/MSI: Get rid of useless count of msi_desc leftovers

[Tejun Heo]

Hello, Alexander.

On Fri, Oct 18, 2013 at 07:12:06PM +0200, Alexander Gordeev wrote:
> @@ -744,23 +744,6 @@ static int msix_capability_init(struct pci_dev *dev,
>  
>  	return 0;
>  
> -out_avail:
> -	if (ret < 0) {
> -		/*
> -		 * If we had some success, report the number of irqs
> -		 * we succeeded in setting up.
> -		 */
> -		struct msi_desc *entry;
> -		int avail = 0;
> -
> -		list_for_each_entry(entry, &dev->msi_list, list) {
> -			if (entry->irq != 0)
> -				avail++;
> -		}
> -		if (avail != 0)
> -			ret = avail;
> -	}

Hmmm... so, before this, the function would have returned the partial
number of irqs that arch set up successfully.  Is it okay to lose that
information?  If so, can you please elaborate a bit more on why it's
okay in the description?

Thanks.

-- 
tejun

Re: [PATCH RFC v2 06/29] PCI/MSI: Get rid of useless count of msi_desc leftovers

[Alexander Gordeev]

On Wed, Nov 20, 2013 at 11:07:16AM -0500, Tejun Heo wrote:
> Hmmm... so, before this, the function would have returned the partial
> number of irqs that arch set up successfully.  Is it okay to lose that
> information?  If so, can you please elaborate a bit more on why it's
> okay in the description?

Well, this is embarrassing. A quick check into x86 shows the partially
initialized MSI-Xs are possible - something I should have noticed.

Self-Nack.

> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH RFC v2 07/29] 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>
---
 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 d3a888a..5cfe54c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1148,13 +1148,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)
@@ -1169,7 +1169,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

Re: [PATCH RFC v2 07/29] PCI/MSI: Return -ENOSYS for unimplemented interfaces, not -1

[Tejun Heo]

On Fri, Oct 18, 2013 at 07:12:07PM +0200, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Suggested-by: Ben Hutchings <bhutchings@solarflare.com>

Yes, please.

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

Thanks.

-- 
tejun

[PATCH RFC v2 08/29] PCI/MSI: Make pci_enable_msix() 'nvec' argument unsigned int

[Alexander Gordeev]

Make pci_enable_msix() and pci_enable_msi_block() consistent
with regard to the type of 'nvec' argument. Indeed, a number
of vectors to allocate is a natural value, so make it unsigned.

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

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index a091780..845edb5 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -190,7 +190,8 @@ same number.
 
 4.3.1 pci_enable_msix
 
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
+int pci_enable_msix(struct pci_dev *dev,
+		    struct msix_entry *entries, unsigned int nvec)
 
 Calling this function asks the PCI subsystem to allocate 'nvec' MSIs.
 The 'entries' argument is a pointer to an array of msix_entry structs
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index bbe3d3d..fdae3a4 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -932,7 +932,8 @@ int pci_msix_table_size(struct pci_dev *dev)
  * of irqs or MSI-X vectors available. Driver should use the returned value to
  * re-send its request.
  **/
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
+int pci_enable_msix(struct pci_dev *dev,
+		    struct msix_entry *entries, unsigned int nvec)
 {
 	int status, nr_entries;
 	int i, j;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5cfe54c..d6832de 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1167,7 +1167,7 @@ static inline int pci_msix_table_size(struct pci_dev *dev)
 	return 0;
 }
 static inline int pci_enable_msix(struct pci_dev *dev,
-				  struct msix_entry *entries, int nvec)
+				  struct msix_entry *entries, int unsigned nvec)
 {
 	return -ENOSYS;
 }
@@ -1192,7 +1192,8 @@ int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned 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);
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec);
+int pci_enable_msix(struct pci_dev *dev,
+		    struct msix_entry *entries, unsigned int nvec);
 void pci_msix_shutdown(struct pci_dev *dev);
 void pci_disable_msix(struct pci_dev *dev);
 void msi_remove_pci_irq_vectors(struct pci_dev *dev);
-- 
1.7.7.6

Re: [PATCH RFC v2 08/29] PCI/MSI: Make pci_enable_msix() 'nvec' argument unsigned int

[Tejun Heo]

Hello,

On Fri, Oct 18, 2013 at 07:12:08PM +0200, Alexander Gordeev wrote:
> Make pci_enable_msix() and pci_enable_msi_block() consistent
> with regard to the type of 'nvec' argument. Indeed, a number
> of vectors to allocate is a natural value, so make it unsigned.

I'm personally not a big fan of using unsigneds where they're just
used as numbers unless strictly necessary.  They don't really provide
any meaningful protection and we often end up in situations where we
want to encode error conditions in the same type variable as return
value or whatever and end up with silly situations like functions
which take unsigned and returns integer or having a separate err
variable when the high bit of the orignal variable would have worked
just fine.  Also, people have different thresholds for what should be
unsigned and we end up with half things unsigned and the other signed.

I'll defer the decision to Bjorn but I'd vote for converting things to
int.

Thanks.

-- 
tejun

Re: [PATCH RFC v2 08/29] PCI/MSI: Make pci_enable_msix() 'nvec' argument unsigned int

[Alexander Gordeev]

On Wed, Nov 20, 2013 at 11:14:12AM -0500, Tejun Heo wrote:
> I'm personally not a big fan of using unsigneds where they're just
> used as numbers unless strictly necessary.  They don't really provide
> any meaningful protection and we often end up in situations where we
> want to encode error conditions in the same type variable as return
> value or whatever and end up with silly situations like functions
> which take unsigned and returns integer or having a separate err
> variable when the high bit of the orignal variable would have worked
> just fine.  Also, people have different thresholds for what should be
> unsigned and we end up with half things unsigned and the other signed.

That sounds convincing. Given that arch hooks do use signed int's 
there is one more reason to convert to signed.

> I'll defer the decision to Bjorn but I'd vote for converting things to
> int.

I will re-post with int's in next version unless Bjorn (someone) speaks
up against.

> Thanks.
> 
> -- 
> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH RFC v2 09/29] 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>
---
 Documentation/PCI/MSI-HOWTO.txt |   15 +++++++++++++++
 drivers/pci/msi.c               |   33 +++++++++++++++++++++------------
 include/linux/pci.h             |    6 ++++++
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 845edb5..e7e3b20 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 fdae3a4..14dc4d1 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -795,6 +795,21 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 	return 0;
 }
 
+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
@@ -811,13 +826,10 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
 {
 	int status, maxvec;
-	u16 msgctl;
 
-	if (!dev->msi_cap)
-		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;
 
@@ -842,13 +854,10 @@ EXPORT_SYMBOL(pci_enable_msi_block);
 int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec)
 {
 	int ret, nvec;
-	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);
+	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 d6832de..df72084 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1146,6 +1146,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, unsigned int nvec)
 {
 	return -ENOSYS;
@@ -1187,6 +1192,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, unsigned int nvec);
 int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec);
 void pci_msi_shutdown(struct pci_dev *dev);
-- 
1.7.7.6

Re: [PATCH RFC v2 09/29] PCI/MSI: Factor out pci_get_msi_cap() interface

[Tejun Heo]

Hello,

On Fri, Oct 18, 2013 at 07:12:09PM +0200, Alexander Gordeev wrote:
> +If this function returns a negative number, it indicates the device is
> +not capable of sending MSIs.

Wouldn't "errno" better describe the error return rather than
"negative number"?

> @@ -795,6 +795,21 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
>  	return 0;
>  }

We probably want a function comment here?

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

Other than that,

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

Thanks.

-- 
tejun

Re: [PATCH RFC v2 09/29] PCI/MSI: Factor out pci_get_msi_cap() interface

[Alexander Gordeev]

On Wed, Nov 20, 2013 at 11:18:08AM -0500, Tejun Heo wrote:
> > +If this function returns a negative number, it indicates the device is
> > +not capable of sending MSIs.
> 
> Wouldn't "errno" better describe the error return rather than
> "negative number"?

Term "negative number" is used throughout the whole document.
Could be updated in a separate patch if it worth it.

> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH RFC v2 10/29] 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.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 Documentation/PCI/MSI-HOWTO.txt |   30 +-------------------
 drivers/ata/ahci.c              |   56 ++++++++++++++++++++++++--------------
 drivers/pci/msi.c               |   22 ---------------
 include/linux/pci.h             |    7 -----
 4 files changed, 37 insertions(+), 78 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index e7e3b20..47118f3 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -127,33 +127,7 @@ 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, unsigned 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)
 
@@ -169,7 +143,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.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 14dc4d1..13bd901 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -851,28 +851,6 @@ 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 ret, nvec;
-
-	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 df72084..a04f5da 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1156,12 +1156,6 @@ static inline int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
 	return -ENOSYS;
 }
 
-static inline int
-pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec)
-{
-	return -ENOSYS;
-}
-
 static inline void pci_msi_shutdown(struct pci_dev *dev)
 { }
 static inline void pci_disable_msi(struct pci_dev *dev)
@@ -1194,7 +1188,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, unsigned int nvec);
-int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned 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 RFC v2 10/29] PCI/MSI: Get rid of pci_enable_msi_block_auto() interface

[Tejun Heo]

On Fri, Oct 18, 2013 at 07:12:10PM +0200, Alexander Gordeev wrote:
> 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.

It'd be nice to mention that _auto has been introduced recently and
has only single user in ahci which is updated by this patch.

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

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

Bjorn, please feel free to route this through pci branch.

Thanks.

-- 
tejun

[PATCH RFC v2 11/29] PCI/MSI: Convert pci_msix_table_size() to a public interface

[Alexander Gordeev]

Make pci_msix_table_size() return error code if the device
does not support MSI-X. This update is needed to create a
consistent MSI-X counterpart for pci_get_msi_cap() MSI
interface.

Device drivers can use this interface 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.

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

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 47118f3..fdf3ae3 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -245,6 +245,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 13bd901..96f51d0 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -898,11 +898,12 @@ 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
@@ -933,6 +934,8 @@ int pci_enable_msix(struct pci_dev *dev,
 		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 31063ac..242f9ff 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 a04f5da..bef5775 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1163,7 +1163,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 unsigned nvec)
-- 
1.7.7.6

Re: [PATCH RFC v2 11/29] PCI/MSI: Convert pci_msix_table_size() to a public interface

[Tejun Heo]

Hello,

On Fri, Oct 18, 2013 at 07:12:11PM +0200, Alexander Gordeev wrote:
> Make pci_msix_table_size() return error code if the device
> does not support MSI-X. This update is needed to create a
> consistent MSI-X counterpart for pci_get_msi_cap() MSI
> interface.
> 
> Device drivers can use this interface 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.

Please mention that the return value of the function has been changed
in the description.  Also, do we want to rename it to
pci_get_msix_cap() for consistency?

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

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

Thanks.

-- 
tejun

Re: [PATCH RFC v2 11/29] PCI/MSI: Convert pci_msix_table_size() to a public interface

[Alexander Gordeev]

On Wed, Nov 20, 2013 at 11:23:24AM -0500, Tejun Heo wrote:
> On Fri, Oct 18, 2013 at 07:12:11PM +0200, Alexander Gordeev wrote:
> > Make pci_msix_table_size() return error code if the device
> > does not support MSI-X. This update is needed to create a
> > consistent MSI-X counterpart for pci_get_msi_cap() MSI
> > interface.
> > 
> > Device drivers can use this interface 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.
> 
> Please mention that the return value of the function has been changed
> in the description.  Also, do we want to rename it to
> pci_get_msix_cap() for consistency?

I do not think so. The naming explicitly hints PCI registers and
pci_get_msix_cap() is confusing at least. I would leave it as is.

> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> 
> Reviewed-by: Tejun Heo <tj@kernel.org>
> 
> Thanks.
> 
> -- 
> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH RFC v2 12/29] 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,
				    nvec,
				    FOO_DRIVER_MINIMUM_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>
---
 Documentation/PCI/MSI-HOWTO.txt |  134 +++++++++++++++++++++++++++++++++++++--
 drivers/pci/msi.c               |   46 +++++++++++++
 include/linux/pci.h             |   59 +++++++++++++++++
 3 files changed, 234 insertions(+), 5 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index fdf3ae3..f348b6f 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,
+			  unsigned int nvec, unsigned int minvec)
+
+This variation on pci_enable_msi_block() call allows a device driver to
+request any number of MSIs within specified range minvec to nvec. 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, unsigned 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, unsigned 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)
 
@@ -143,7 +198,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)
 
@@ -224,7 +279,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,
+			   unsigned int nvec, unsigned int minvec)
+
+This variation on pci_enable_msix() call allows a device driver to request
+any number of MSI-Xs within specified range minvec to nvec. 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,
+				    nvec, FOO_DRIVER_MINIMUM_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, unsigned 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, unsigned 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)
 
@@ -238,14 +362,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 96f51d0..91acd8a 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1042,3 +1042,49 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
 	if (dev->msix_cap)
 		msix_set_enable(dev, 0);
 }
+
+int pcim_enable_msi_range(struct pci_dev *dev,
+			  unsigned int nvec, unsigned int minvec)
+{
+	int rc;
+
+	if (nvec < 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);
+
+int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
+			   unsigned int nvec, unsigned int minvec)
+{
+	int rc;
+
+	if (nvec < 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 bef5775..3c18a8f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1185,6 +1185,39 @@ static inline int pci_msi_enabled(void)
 {
 	return 0;
 }
+
+int pcim_enable_msi_range(struct pci_dev *dev,
+			  unsigned int nvec, unsigned int minvec)
+{
+	return -ENOSYS;
+}
+static inline int pcim_enable_msi(struct pci_dev *dev, unsigned int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int pcim_enable_msi_exact(struct pci_dev *dev, unsigned int nvec)
+{
+	return -ENOSYS;
+}
+
+static inline int
+pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
+		       unsigned int nvec, unsigned int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int
+pcim_enable_msix(struct pci_dev *dev,
+		 struct msix_entry *entries, unsigned int maxvec)
+{
+	return -ENOSYS;
+}
+static inline int
+pcim_enable_msix_exact(struct pci_dev *dev,
+		       struct msix_entry *entries, unsigned int nvec)
+{
+	return -ENOSYS;
+}
 #else
 int pci_get_msi_cap(struct pci_dev *dev);
 int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec);
@@ -1198,6 +1231,32 @@ 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,
+			  unsigned int nvec, unsigned int minvec);
+static inline int pcim_enable_msi(struct pci_dev *dev, unsigned int maxvec)
+{
+	return pcim_enable_msi_range(dev, maxvec, 1);
+}
+static inline int pcim_enable_msi_exact(struct pci_dev *dev, unsigned int nvec)
+{
+	return pcim_enable_msi_range(dev, nvec, nvec);
+}
+
+int pcim_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
+			   unsigned int nvec, unsigned int minvec);
+static inline int
+pcim_enable_msix(struct pci_dev *dev,
+		 struct msix_entry *entries, unsigned int maxvec)
+{
+	return pcim_enable_msix_range(dev, entries, maxvec, 1);
+}
+static inline int
+pcim_enable_msix_exact(struct pci_dev *dev,
+		       struct msix_entry *entries, unsigned int nvec)
+{
+	return pcim_enable_msix_range(dev, entries, nvec, nvec);
+}
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS
-- 
1.7.7.6

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Tejun Heo]

Hello, Alexander.

On Fri, Oct 18, 2013 at 07:12:12PM +0200, Alexander Gordeev wrote:
> 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,
> 				    nvec,
> 				    FOO_DRIVER_MINIMUM_NVEC);

I haven't looked into any details but, if the above works for most use
cases, it looks really good to me.

Thanks!

-- 
tejun

RE: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[David Laight]

> > ...would turn into a single helper call....
> >
> > 	rc = pcim_enable_msix_range(adapter->pdev,
> > 				    adapter->msix_entries,
> > 				    nvec,
> > 				    FOO_DRIVER_MINIMUM_NVEC);
> 
> I haven't looked into any details but, if the above works for most use
> cases, it looks really good to me.

The one case it doesn't work is where the driver either
wants the full number or the minimum number - but not
a value in between.

Might be worth adding an extra parameter so that this
(and maybe other) odd requirements can be met.

Some static inline functions could be used for the common cases.

	David

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Tejun Heo]

Hello,

On Thu, Oct 24, 2013 at 11:57:40AM +0100, David Laight wrote:
> > > ...would turn into a single helper call....
> > >
> > > 	rc = pcim_enable_msix_range(adapter->pdev,
> > > 				    adapter->msix_entries,
> > > 				    nvec,
> > > 				    FOO_DRIVER_MINIMUM_NVEC);
> > 
> > I haven't looked into any details but, if the above works for most use
> > cases, it looks really good to me.
> 
> The one case it doesn't work is where the driver either
> wants the full number or the minimum number - but not
> a value in between.
> 
> Might be worth adding an extra parameter so that this
> (and maybe other) odd requirements can be met.
> 
> Some static inline functions could be used for the common cases.

If those are edge cases, I don't think it's a big deal no matter what
we do.

Thanks.

-- 
tejun

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Thu, Oct 24, 2013 at 11:57:40AM +0100, David Laight wrote:
> The one case it doesn't work is where the driver either
> wants the full number or the minimum number - but not
> a value in between.
> 
> Might be worth adding an extra parameter so that this
> (and maybe other) odd requirements can be met.

IMHO its not worth it, since it is not possible to generalize
all odd requirements out there. I do not think we should blow
the API in this case.

Having said that, the min-or-max interface is probably the only
worth considering. But again, I would prefer to put its semantics
to function name rather than to extra parameters, i.e.

pcim_enable_msix_min_max(struct pci_dev *dev, struct msix_entry *entries,
			 unsigned int minvec, unsigned int maxvec);

> Some static inline functions could be used for the common cases.
> 
> 	David

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Mark Lord]

On 13-10-24 07:41 AM, Alexander Gordeev wrote:
> On Thu, Oct 24, 2013 at 11:57:40AM +0100, David Laight wrote:
>> The one case it doesn't work is where the driver either
>> wants the full number or the minimum number - but not
>> a value in between.
>>
>> Might be worth adding an extra parameter so that this
>> (and maybe other) odd requirements can be met.
> 
> IMHO its not worth it, since it is not possible to generalize
> all odd requirements out there. I do not think we should blow
> the API in this case.
> 
> Having said that, the min-or-max interface is probably the only
> worth considering. But again, I would prefer to put its semantics
> to function name rather than to extra parameters, i.e.
> 
> pcim_enable_msix_min_max(struct pci_dev *dev, struct msix_entry *entries,
> 			 unsigned int minvec, unsigned int maxvec);

The hardware I have in mind here works only for powers of two.
Eg. 16, 8, 4, 2, or 1 MSI-X vector.  Not the odd values in between.

But it appears I can still just use a loop for that case,
calling the new function above instead of the old functions.

Cheers

RE: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[David Laight]

PiA+IHBjaW1fZW5hYmxlX21zaXhfbWluX21heChzdHJ1Y3QgcGNpX2RldiAqZGV2LCBzdHJ1Y3Qg
bXNpeF9lbnRyeSAqZW50cmllcywNCj4gPiAJCQkgdW5zaWduZWQgaW50IG1pbnZlYywgdW5zaWdu
ZWQgaW50IG1heHZlYyk7DQo+IA0KPiBUaGUgaGFyZHdhcmUgSSBoYXZlIGluIG1pbmQgaGVyZSB3
b3JrcyBvbmx5IGZvciBwb3dlcnMgb2YgdHdvLg0KPiBFZy4gMTYsIDgsIDQsIDIsIG9yIDEgTVNJ
LVggdmVjdG9yLiAgTm90IHRoZSBvZGQgdmFsdWVzIGluIGJldHdlZW4uDQo+IA0KPiBCdXQgaXQg
YXBwZWFycyBJIGNhbiBzdGlsbCBqdXN0IHVzZSBhIGxvb3AgZm9yIHRoYXQgY2FzZSwNCj4gY2Fs
bGluZyB0aGUgbmV3IGZ1bmN0aW9uIGFib3ZlIGluc3RlYWQgb2YgdGhlIG9sZCBmdW5jdGlvbnMu
DQoNCllvdSdkIGVpdGhlciBoYXZlIHRvIGxvb3Agd2l0aCBtaW4gYW5kIG1heCB0aGUgc2FtZSAo
c3RhcnRpbmcgYXQgMTYpLA0Kb3IgZG8gYSBzaW5nbGUgY2FsbCB3aXRoIG1pbj0xIGFuZCBtYXg9
MTYgYW5kIHRoZSByZWxlYXNlIHRoZQ0KdW53YW50ZWQgb25lcy4NCg0KVGhlIGxhdHRlciBtaWdo
dCBiZSBwcmVmZXJyZWQgYmVjYXVzZSBpdCBtaWdodCBzdG9wIGFuIGFubm95aW5nDQp0cmFjZSBh
Ym91dCB0aGUgc3lzdGVtIG5vdCBoYXZpbmcgZW5vdWdoIE1TSSBpbnRlcnJ1cHRzLg0KDQpXaGF0
IHRoaXMgZG9lc24ndCByZXNvbHZlIGlzIGEgZHJpdmVyIHJlcXVlc3RpbmcgYSBsb3Qgb2YgaW50
ZXJydXB0cw0KZWFybHkgb24gYW5kIGxlYXZpbmcgbm9uZSBmb3IgbGF0ZXIgZHJpdmVycy4NCg0K
UmVhbGx5IHRoZSBzeXN0ZW0gbmVlZHMgdG8gYWxsb2NhdGUgdGhlIG1pbmltdW0gbnVtYmVyIHRv
IGFsbA0KZHJpdmVycyBiZWZvcmUgZ2l2aW5nIG91dCBhbnkgZXh0cmEgb25lcyAtIEkndmUgTkZJ
IGhvdyB0aGlzDQp3b3VsZCBiZSBhcnJhbmdlZCENCg0KCURhdmlkDQoNCg==

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Fri, Oct 25, 2013 at 10:10:02AM +0100, David Laight wrote:
> What this doesn't resolve is a driver requesting a lot of interrupts
> early on and leaving none for later drivers.

If this problem really exists anywhere besides pSeries?

I can imagine x86 hitting lack of vectors in interrupt table when
number of CPUs exceeds hundreds, but do we have this problem now?

> Really the system needs to allocate the minimum number to all
> drivers before giving out any extra ones - I've NFI how this
> would be arranged!

Do not know. The pSeries quota approach seems more reasonable to me.

> 	David

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

intel e1000e pci driver issue on RT Linux kernel

[Karicheri, Muralidharan]

All,

I am a first timer for this mailing list. I have tried sending email to the list after subscribing successfully. I had sent an email before, but I can't see that in the mailing list as seen through the web browser. So I am re-sending. My apologies if multiple copies appear in the list.

I am working on my SMP kernel on an ARM A15 with RT patch applied against v3.10.10. I have a setup to use the e1000e pci driver against the pcie root complex driver that I have on this platform. It works well with non RT version of the kernel. I am able to do iperf on UDP and TCP traffic. But against RT kernel, I am able to do only ping. If I run iperf, it seems there is a deadlock happening. When I did suspend the core, it seems to be in the idle loop. Some time I am able to do ctrl-c and get back to shell. When I try doing ping again, it times out. If anyone has seen this issue and know how to fix, please reply.

Thanks.

Murali Karicheri

RE: intel e1000e pci driver issue on RT Linux kernel

[Karicheri, Muralidharan]

Resending as there was a format issue

I am a first timer for this mailing list. I have tried sending email to the list after subscribing
successfully. I had sent an email before, but I can't see that in the mailing list as seen
through the web browser. So I am re-sending. My apologies if multiple copies appear in the
list.

I am working on my SMP kernel on an ARM A15 with RT patch applied against v3.10.10. I
have a setup to use the e1000e pci driver against the pcie root complex driver that I have
on this platform. It works well with non RT version of the kernel. I am able to do iperf on
UDP and TCP traffic. But against RT kernel, I am able to do only ping. If I run iperf, it seems
there is a deadlock happening. When I did suspend the core, it seems to be in the idle loop.
Some time I am able to do ctrl-c and get back to shell. When I try doing ping again, it times
out. If anyone has seen this issue and know how to fix, please reply.
 
 Thanks.
 
 Murali Karicheri

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Michael Ellerman]

On Fri, 2013-10-25 at 12:01 +0200, Alexander Gordeev wrote:
> On Fri, Oct 25, 2013 at 10:10:02AM +0100, David Laight wrote:
> > What this doesn't resolve is a driver requesting a lot of interrupts
> > early on and leaving none for later drivers.
> 
> If this problem really exists anywhere besides pSeries?
> 
> I can imagine x86 hitting lack of vectors in interrupt table when
> number of CPUs exceeds hundreds, but do we have this problem now?
> 
> > Really the system needs to allocate the minimum number to all
> > drivers before giving out any extra ones - I've NFI how this
> > would be arranged!
> 
> Do not know. The pSeries quota approach seems more reasonable to me.

When the system boots each driver should get a fair share of the
available MSIs, the quota achieves this.

But ideally the sysadmin would then be able to override that, and give
more MSIs to one device, the quota doesn't allow that.

Hopefully we'll see the number of available MSIs grow faster than the
number required by devices (usually driven by NR_CPUs), and so this will
become a non-problem.

cheers

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Mark Lord]

On 13-10-25 06:01 AM, Alexander Gordeev wrote:
>
> If this problem really exists anywhere besides pSeries?
> 
> I can imagine x86 hitting lack of vectors in interrupt table when
> number of CPUs exceeds hundreds, but do we have this problem now?

An awful lot of x86 hardware has a 256 (255?) vector limit for MSI/MSI-X.
Couple that with PCIe Virtual Functions, each wanting 16 vectors (for example),
and that limit is really simple to exceed today.

But this is more a problem for a sysadmin, and I am happy with the current
and the proposed methods.

Cheers

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Thu, Oct 24, 2013 at 11:51:58AM +0100, Tejun Heo wrote:
> I haven't looked into any details but, if the above works for most use
> cases, it looks really good to me.

Well, if we reuse Michael's statistics:

 - 58 drivers call pci_enable_msix()
 - 24 try a single allocation and then fallback to MSI/LSI
 - 19 use the loop style allocation
 - 14 try an allocation, and if it fails retry once

...then I expect most of 19/58 (loop style) could be converted to
pcim_enable_msix() and pcim_enable_msix_range() and all of 14/58
(single fallback) should be converted to pcim_enable_msix() users.

> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Mark Lord]

On 13-10-24 10:31 AM, Alexander Gordeev wrote:
> On Thu, Oct 24, 2013 at 11:51:58AM +0100, Tejun Heo wrote:
>> I haven't looked into any details but, if the above works for most use
>> cases, it looks really good to me.
> 
> Well, if we reuse Michael's statistics:
> 
>  - 58 drivers call pci_enable_msix()
>  - 24 try a single allocation and then fallback to MSI/LSI
>  - 19 use the loop style allocation
>  - 14 try an allocation, and if it fails retry once
> 
> ...then I expect most of 19/58 (loop style) could be converted to
> pcim_enable_msix() and pcim_enable_msix_range() and all of 14/58
> (single fallback) should be converted to pcim_enable_msix() users.

Those are just the in-kernel drivers.
There are many, many more out-of-kernel drivers for embedded platforms,
hardware in-development, etc..

Let's not be overly presumptive about the size of the user base.

Cheers

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Thu, Oct 24, 2013 at 11:51:58AM +0100, Tejun Heo wrote:

Hi Tejun,

> I haven't looked into any details but, if the above works for most use
> cases, it looks really good to me.

Could you please take a look (at least) at patches 10 and 12?

> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Tejun Heo]

Hello,

On Fri, Oct 18, 2013 at 07:12:12PM +0200, 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.
> 
> 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,
> 				    nvec,
> 				    FOO_DRIVER_MINIMUM_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>

The use of @nvec and @maxvec is a bit inconsistent.  Maybe it'd be
better to make them uniform?  Also, can you please add function
comments to the new public functions?  People are much more likely to
check them than the documentation.  Other than that,

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

Thanks a lot!

-- 
tejun

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Wed, Nov 20, 2013 at 12:15:26PM -0500, Tejun Heo wrote:
> The use of @nvec and @maxvec is a bit inconsistent.  Maybe it'd be
> better to make them uniform?

With @maxvec I tried to stress an implication there could be values
less than @maxvec. While @nvec is more like an exact number.
Perfectly makes sense to me, but this is personal :)

> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Tejun Heo]

On Fri, Nov 22, 2013 at 07:44:30PM +0100, Alexander Gordeev wrote:
> On Wed, Nov 20, 2013 at 12:15:26PM -0500, Tejun Heo wrote:
> > The use of @nvec and @maxvec is a bit inconsistent.  Maybe it'd be
> > better to make them uniform?
> 
> With @maxvec I tried to stress an implication there could be values
> less than @maxvec. While @nvec is more like an exact number.
> Perfectly makes sense to me, but this is personal :)

Oh yeah, I agree but saw a place where @nvec is used for max.  Maybe I
was confused.  Looking again...

+int pcim_enable_msi_range(struct pci_dev *dev, struct msix_entry *entries,
+                         unsigned int nvec, unsigned int minvec)
+
+This variation on pci_enable_msi_block() call allows a device driver to
+request any number of MSIs within specified range minvec to nvec. Whenever
+possible device drivers are encouraged to use this function rather than
+explicit request loop calling pci_enable_msi_block().

e.g. shouldn't that @nvec be @maxvec?

Thanks.

-- 
tejun

Re: [PATCH RFC v2 12/29] PCI/MSI: Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Fri, Nov 22, 2013 at 01:44:47PM -0500, Tejun Heo wrote:
> +int pcim_enable_msi_range(struct pci_dev *dev, struct msix_entry *entries,
> +                         unsigned int nvec, unsigned int minvec)
> +
> +This variation on pci_enable_msi_block() call allows a device driver to
> +request any number of MSIs within specified range minvec to nvec. Whenever
> +possible device drivers are encouraged to use this function rather than
> +explicit request loop calling pci_enable_msi_block().
> 
> e.g. shouldn't that @nvec be @maxvec?

Right, it should. May be even in different order.

> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH RFC v2 13/29] ipr: Do not call pci_disable_msi/msix() if pci_enable_msi/msix() failed

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/scsi/ipr.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 36ac1c3..fb57e21 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -9255,10 +9255,8 @@ static int ipr_enable_msix(struct ipr_ioa_cfg *ioa_cfg)
 	while ((err = pci_enable_msix(ioa_cfg->pdev, entries, vectors)) > 0)
 			vectors = err;
 
-	if (err < 0) {
-		pci_disable_msix(ioa_cfg->pdev);
+	if (err < 0)
 		return err;
-	}
 
 	if (!err) {
 		for (i = 0; i < vectors; i++)
@@ -9278,10 +9276,8 @@ static int ipr_enable_msi(struct ipr_ioa_cfg *ioa_cfg)
 	while ((err = pci_enable_msi_block(ioa_cfg->pdev, vectors)) > 0)
 			vectors = err;
 
-	if (err < 0) {
-		pci_disable_msi(ioa_cfg->pdev);
+	if (err < 0)
 		return err;
-	}
 
 	if (!err) {
 		for (i = 0; i < vectors; i++)
-- 
1.7.7.6

[PATCH RFC v2 14/29] ipr: Make use of pcim_enable_msi/msix() interfaces

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/scsi/ipr.c |   47 ++++++++++++++++++-----------------------------
 1 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index fb57e21..d7c94d5 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -9245,47 +9245,36 @@ ipr_get_chip_info(const struct pci_device_id *dev_id)
 static int ipr_enable_msix(struct ipr_ioa_cfg *ioa_cfg)
 {
 	struct msix_entry entries[IPR_MAX_MSIX_VECTORS];
-	int i, err, vectors;
+	int i, vectors;
 
-	for (i = 0; i < ARRAY_SIZE(entries); ++i)
+	vectors = min_t(int, ARRAY_SIZE(entries), ipr_number_of_msix);
+	for (i = 0; i < vectors; ++i)
 		entries[i].entry = i;
 
-	vectors = ipr_number_of_msix;
+	vectors = pcim_enable_msix(ioa_cfg->pdev, entries, vectors);
+	if (vectors < 0)
+		return vectors;
 
-	while ((err = pci_enable_msix(ioa_cfg->pdev, entries, vectors)) > 0)
-			vectors = err;
+	for (i = 0; i < vectors; i++)
+		ioa_cfg->vectors_info[i].vec = entries[i].vector;
+	ioa_cfg->nvectors = vectors;
 
-	if (err < 0)
-		return err;
-
-	if (!err) {
-		for (i = 0; i < vectors; i++)
-			ioa_cfg->vectors_info[i].vec = entries[i].vector;
-		ioa_cfg->nvectors = vectors;
-	}
-
-	return err;
+	return 0;
 }
 
 static int ipr_enable_msi(struct ipr_ioa_cfg *ioa_cfg)
 {
-	int i, err, vectors;
+	int i, vectors;
 
-	vectors = ipr_number_of_msix;
+	vectors = pcim_enable_msi(ioa_cfg->pdev, ipr_number_of_msix);
+	if (vectors < 0)
+		return vectors;
 
-	while ((err = pci_enable_msi_block(ioa_cfg->pdev, vectors)) > 0)
-			vectors = err;
+	for (i = 0; i < vectors; i++)
+		ioa_cfg->vectors_info[i].vec = ioa_cfg->pdev->irq + i;
+	ioa_cfg->nvectors = vectors;
 
-	if (err < 0)
-		return err;
-
-	if (!err) {
-		for (i = 0; i < vectors; i++)
-			ioa_cfg->vectors_info[i].vec = ioa_cfg->pdev->irq + i;
-		ioa_cfg->nvectors = vectors;
-	}
-
-	return err;
+	return 0;
 }
 
 static void name_msi_vectors(struct ipr_ioa_cfg *ioa_cfg)
-- 
1.7.7.6

[PATCH RFC v2 15/29] ixgbe: Make use of pcim_enable_msix_range() interface

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
index 90b4e10..9954b7a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
@@ -697,7 +697,7 @@ static void ixgbe_set_num_queues(struct ixgbe_adapter *adapter)
 static void ixgbe_acquire_msix_vectors(struct ixgbe_adapter *adapter,
 				       int vectors)
 {
-	int err, vector_threshold;
+	int vector_threshold;
 
 	/* We'll want at least 2 (vector_threshold):
 	 * 1) TxQ[0] + RxQ[0] handler
@@ -711,18 +711,10 @@ static void ixgbe_acquire_msix_vectors(struct ixgbe_adapter *adapter,
 	 * Right now, we simply care about how many we'll get; we'll
 	 * set them up later while requesting irq's.
 	 */
-	while (vectors >= vector_threshold) {
-		err = pci_enable_msix(adapter->pdev, adapter->msix_entries,
-				      vectors);
-		if (!err) /* Success in acquiring all requested vectors. */
-			break;
-		else if (err < 0)
-			vectors = 0; /* Nasty failure, quit now */
-		else /* err == number of vectors we should try again with */
-			vectors = err;
-	}
+	vectors = pcim_enable_msix_range(adapter->pdev, adapter->msix_entries,
+					 vectors, vector_threshold);
 
-	if (vectors < vector_threshold) {
+	if (vectors < 0) {
 		/* Can't allocate enough MSI-X interrupts?  Oh well.
 		 * This just means we'll go with either a single MSI
 		 * vector or fall back to legacy interrupts.
-- 
1.7.7.6

[PATCH RFC v2 16/29] ixgbevf: Make use of pcim_enable_msix_range() interface

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   33 +++++++-------------
 1 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 59a62bb..ced3b24 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -1749,7 +1749,6 @@ void ixgbevf_reset(struct ixgbevf_adapter *adapter)
 static int ixgbevf_acquire_msix_vectors(struct ixgbevf_adapter *adapter,
 					int vectors)
 {
-	int err = 0;
 	int vector_threshold;
 
 	/* We'll want at least 2 (vector_threshold):
@@ -1763,33 +1762,25 @@ static int ixgbevf_acquire_msix_vectors(struct ixgbevf_adapter *adapter,
 	 * Right now, we simply care about how many we'll get; we'll
 	 * set them up later while requesting irq's.
 	 */
-	while (vectors >= vector_threshold) {
-		err = pci_enable_msix(adapter->pdev, adapter->msix_entries,
-				      vectors);
-		if (!err || err < 0) /* Success or a nasty failure. */
-			break;
-		else /* err == number of vectors we should try again with */
-			vectors = err;
-	}
+	vectors = pcim_enable_msix_range(adapter->pdev, adapter->msix_entries,
+					 vectors, vector_threshold);
 
-	if (vectors < vector_threshold)
-		err = -ENOMEM;
-
-	if (err) {
+	if (vectors < 0) {
 		dev_err(&adapter->pdev->dev,
 			"Unable to allocate MSI-X interrupts\n");
 		kfree(adapter->msix_entries);
 		adapter->msix_entries = NULL;
-	} else {
-		/*
-		 * Adjust for only the vectors we'll use, which is minimum
-		 * of max_msix_q_vectors + NON_Q_VECTORS, or the number of
-		 * vectors we were allocated.
-		 */
-		adapter->num_msix_vectors = vectors;
+		return vectors;
 	}
 
-	return err;
+	/*
+	 * Adjust for only the vectors we'll use, which is minimum
+	 * of max_msix_q_vectors + NON_Q_VECTORS, or the number of
+	 * vectors we were allocated.
+	 */
+	adapter->num_msix_vectors = vectors;
+
+	return 0;
 }
 
 /**
-- 
1.7.7.6

[PATCH RFC v2 17/29] megaraid: Make use of pcim_enable_msix() interface

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/scsi/megaraid/megaraid_sas_base.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 3020921..e54c2e7 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -3727,18 +3727,12 @@ static int megasas_init_fw(struct megasas_instance *instance)
 					     (unsigned int)num_online_cpus());
 		for (i = 0; i < instance->msix_vectors; i++)
 			instance->msixentry[i].entry = i;
-		i = pci_enable_msix(instance->pdev, instance->msixentry,
-				    instance->msix_vectors);
-		if (i >= 0) {
-			if (i) {
-				if (!pci_enable_msix(instance->pdev,
-						     instance->msixentry, i))
-					instance->msix_vectors = i;
-				else
-					instance->msix_vectors = 0;
-			}
-		} else
+		i = pcim_enable_msix(instance->pdev, instance->msixentry,
+				     instance->msix_vectors);
+		if (i < 0)
 			instance->msix_vectors = 0;
+		else
+			instance->msix_vectors = i;
 
 		dev_info(&instance->pdev->dev, "[scsi%d]: FW supports"
 			"<%d> MSIX vector,Online CPUs: <%d>,"
-- 
1.7.7.6

[PATCH RFC v2 18/29] ntb: Fix missed call to pci_enable_msix()

[Alexander Gordeev]

Current MSI-X enablement code assumes MSI-Xs were successfully
allocated in case less than requested vectors were available.
That assumption is wrong, since MSI-Xs should be enabled with
a repeated call to pci_enable_msix(). This update fixes this.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Acked-by: Jon Mason <jon.mason@intel.com>
---
 drivers/ntb/ntb_hw.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index 1cb6e51..de2062c 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1075,6 +1075,10 @@ static int ntb_setup_msix(struct ntb_device *ndev)
 			 "Only %d MSI-X vectors.  Limiting the number of queues to that number.\n",
 			 rc);
 		msix_entries = rc;
+
+		rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
+		if (rc)
+			goto err1;
 	}
 
 	for (i = 0; i < msix_entries; i++) {
-- 
1.7.7.6

[PATCH RFC v2 19/29] ntb: Make use of pcim_enable_msix/msix_exact() interfaces

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/ntb/ntb_hw.c |   38 +++++++++++++-------------------------
 1 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index de2062c..b7d6eed 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1032,22 +1032,15 @@ static int ntb_setup_msix(struct ntb_device *ndev)
 	struct msix_entry *msix;
 	int msix_entries;
 	int rc, i;
-	u16 val;
 
-	if (!pdev->msix_cap) {
-		rc = -EIO;
-		goto err;
-	}
-
-	rc = pci_read_config_word(pdev, pdev->msix_cap + PCI_MSIX_FLAGS, &val);
-	if (rc)
+	rc = pci_msix_table_size(pdev);
+	if (rc < 0)
 		goto err;
-
-	msix_entries = msix_table_size(val);
-	if (msix_entries > ndev->limits.msix_cnt) {
+	else if (rc > ndev->limits.msix_cnt) {
 		rc = -EINVAL;
 		goto err;
-	}
+	} else
+		msix_entries = rc;
 
 	ndev->msix_entries = kmalloc(sizeof(struct msix_entry) * msix_entries,
 				     GFP_KERNEL);
@@ -1059,26 +1052,21 @@ static int ntb_setup_msix(struct ntb_device *ndev)
 	for (i = 0; i < msix_entries; i++)
 		ndev->msix_entries[i].entry = i;
 
-	rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
-	if (rc < 0)
-		goto err1;
-	if (rc > 0) {
+	if (ndev->hw_type != BWD_HW)
 		/* On SNB, the link interrupt is always tied to 4th vector.  If
 		 * we can't get all 4, then we can't use MSI-X.
 		 */
-		if (ndev->hw_type != BWD_HW) {
-			rc = -EIO;
-			goto err1;
-		}
-
+		rc = pcim_enable_msix_exact(pdev,
+					    ndev->msix_entries, msix_entries);
+	else
+		rc = pcim_enable_msix(pdev, ndev->msix_entries, msix_entries);
+	if (rc < 0)
+		goto err1;
+	else if (rc < msix_entries) {
 		dev_warn(&pdev->dev,
 			 "Only %d MSI-X vectors.  Limiting the number of queues to that number.\n",
 			 rc);
 		msix_entries = rc;
-
-		rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
-		if (rc)
-			goto err1;
 	}
 
 	for (i = 0; i < msix_entries; i++) {
-- 
1.7.7.6

[PATCH RFC v2 20/29] qib: Make use of pcim_enable_msix() and pci_msix_table_size() interfaces

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/infiniband/hw/qib/qib_pcie.c |   48 +++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_pcie.c b/drivers/infiniband/hw/qib/qib_pcie.c
index 3f14009..bad0ca3 100644
--- a/drivers/infiniband/hw/qib/qib_pcie.c
+++ b/drivers/infiniband/hw/qib/qib_pcie.c
@@ -197,46 +197,46 @@ static void qib_msix_setup(struct qib_devdata *dd, int pos, u32 *msixcnt,
 			   struct qib_msix_entry *qib_msix_entry)
 {
 	int ret;
-	u32 tabsize = 0;
-	u16 msix_flags;
+	int tabsize = *msixcnt;
 	struct msix_entry *msix_entry;
 	int i;
 
+	ret = pci_msix_table_size(dd->pcidev);
+	if (ret < 0)
+		goto err;
+
+	tabsize = min(tabsize, ret);
+
 	/* We can't pass qib_msix_entry array to qib_msix_setup
 	 * so use a dummy msix_entry array and copy the allocated
 	 * irq back to the qib_msix_entry array. */
-	msix_entry = kmalloc(*msixcnt * sizeof(*msix_entry), GFP_KERNEL);
+	msix_entry = kmalloc(tabsize * sizeof(*msix_entry), GFP_KERNEL);
 	if (!msix_entry) {
 		ret = -ENOMEM;
-		goto do_intx;
+		goto err;
 	}
-	for (i = 0; i < *msixcnt; i++)
+	for (i = 0; i < tabsize; i++)
 		msix_entry[i] = qib_msix_entry[i].msix;
 
-	pci_read_config_word(dd->pcidev, pos + PCI_MSIX_FLAGS, &msix_flags);
-	tabsize = 1 + (msix_flags & PCI_MSIX_FLAGS_QSIZE);
-	if (tabsize > *msixcnt)
-		tabsize = *msixcnt;
-	ret = pci_enable_msix(dd->pcidev, msix_entry, tabsize);
-	if (ret > 0) {
+	ret = pcim_enable_msix(dd->pcidev, msix_entry, tabsize);
+	if (ret < 0)
+		goto err;
+	else
 		tabsize = ret;
-		ret = pci_enable_msix(dd->pcidev, msix_entry, tabsize);
-	}
-do_intx:
-	if (ret) {
-		qib_dev_err(dd,
-			"pci_enable_msix %d vectors failed: %d, falling back to INTx\n",
-			tabsize, ret);
-		tabsize = 0;
-	}
+
 	for (i = 0; i < tabsize; i++)
 		qib_msix_entry[i].msix = msix_entry[i];
+
 	kfree(msix_entry);
 	*msixcnt = tabsize;
-
-	if (ret)
-		qib_enable_intx(dd->pcidev);
-
+	return;
+
+err:
+	qib_dev_err(dd,
+		"pci_enable_msix %d vectors failed: %d, falling back to INTx\n",
+		tabsize, ret);
+	*msixcnt = 0;
+	qib_enable_intx(dd->pcidev);
 }
 
 /**
-- 
1.7.7.6

[PATCH RFC v2 21/29] qla2xxx: Make use of pcim_enable_msix_range() interface

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/scsi/qla2xxx/qla_isr.c |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index df1b30b..3238a0ac 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -2836,27 +2836,23 @@ qla24xx_enable_msix(struct qla_hw_data *ha, struct rsp_que *rsp)
 	for (i = 0; i < ha->msix_count; i++)
 		entries[i].entry = i;
 
-	ret = pci_enable_msix(ha->pdev, entries, ha->msix_count);
-	if (ret) {
-		if (ret < MIN_MSIX_COUNT)
-			goto msix_failed;
-
+	ret = pcim_enable_msix_range(ha->pdev,
+				     entries, ha->msix_count, MIN_MSIX_COUNT);
+	if (ret < 0) {
+		ql_log(ql_log_fatal, vha, 0x00c7,
+		    "MSI-X: Failed to enable support, "
+		    "giving   up -- %d/%d.\n",
+		    ha->msix_count, ret);
+		goto msix_out;
+	}
+	if (ret < ha->msix_count) {
 		ql_log(ql_log_warn, vha, 0x00c6,
 		    "MSI-X: Failed to enable support "
 		    "-- %d/%d\n Retry with %d vectors.\n",
 		    ha->msix_count, ret, ret);
-		ha->msix_count = ret;
-		ret = pci_enable_msix(ha->pdev, entries, ha->msix_count);
-		if (ret) {
-msix_failed:
-			ql_log(ql_log_fatal, vha, 0x00c7,
-			    "MSI-X: Failed to enable support, "
-			    "giving   up -- %d/%d.\n",
-			    ha->msix_count, ret);
-			goto msix_out;
-		}
-		ha->max_rsp_queues = ha->msix_count - 1;
 	}
+	ha->msix_count = ret;
+	ha->max_rsp_queues = ha->msix_count - 1;
 	ha->msix_entries = kzalloc(sizeof(struct qla_msix_entry) *
 				ha->msix_count, GFP_KERNEL);
 	if (!ha->msix_entries) {
-- 
1.7.7.6

[PATCH RFC v2 22/29] qlge: Get rid of an redundant assignment

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge_main.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 2553cf4..ac54cb0 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -3287,7 +3287,6 @@ static void ql_enable_msix(struct ql_adapter *qdev)
 			qdev->msi_x_entry = NULL;
 			netif_warn(qdev, ifup, qdev->ndev,
 				   "MSI-X Enable failed, trying MSI.\n");
-			qdev->intr_count = 1;
 			qlge_irq_type = MSI_IRQ;
 		} else if (err == 0) {
 			set_bit(QL_MSIX_ENABLED, &qdev->flags);
-- 
1.7.7.6

[PATCH RFC v2 23/29] qlge: Make use of pcim_enable_msix() interface

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge_main.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index ac54cb0..b142902 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -3272,24 +3272,17 @@ static void ql_enable_msix(struct ql_adapter *qdev)
 		for (i = 0; i < qdev->intr_count; i++)
 			qdev->msi_x_entry[i].entry = i;
 
-		/* Loop to get our vectors.  We start with
-		 * what we want and settle for what we get.
-		 */
-		do {
-			err = pci_enable_msix(qdev->pdev,
-				qdev->msi_x_entry, qdev->intr_count);
-			if (err > 0)
-				qdev->intr_count = err;
-		} while (err > 0);
-
+		err = pcim_enable_msix(qdev->pdev,
+				       qdev->msi_x_entry, qdev->intr_count);
 		if (err < 0) {
 			kfree(qdev->msi_x_entry);
 			qdev->msi_x_entry = NULL;
 			netif_warn(qdev, ifup, qdev->ndev,
 				   "MSI-X Enable failed, trying MSI.\n");
 			qlge_irq_type = MSI_IRQ;
-		} else if (err == 0) {
+		} else {
 			set_bit(QL_MSIX_ENABLED, &qdev->flags);
+			qdev->intr_count = err;
 			netif_info(qdev, ifup, qdev->ndev,
 				   "MSI-X Enabled, got %d vectors.\n",
 				   qdev->intr_count);
-- 
1.7.7.6

[PATCH RFC v2 24/29] tg3: Make use of pcim_enable_msix() interface

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/ethernet/broadcom/tg3.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 12d961c..026c371 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -11262,12 +11262,10 @@ static bool tg3_enable_msix(struct tg3 *tp)
 		msix_ent[i].vector = 0;
 	}
 
-	rc = pci_enable_msix(tp->pdev, msix_ent, tp->irq_cnt);
+	rc = pcim_enable_msix(tp->pdev, msix_ent, tp->irq_cnt);
 	if (rc < 0) {
 		return false;
-	} else if (rc != 0) {
-		if (pci_enable_msix(tp->pdev, msix_ent, rc))
-			return false;
+	} else if (rc < tp->irq_cnt) {
 		netdev_notice(tp->dev, "Requested %d MSI-X vectors, received %d\n",
 			      tp->irq_cnt, rc);
 		tp->irq_cnt = rc;
-- 
1.7.7.6

[PATCH RFC v2 25/29] vmxnet3: Return -EINVAL if number of requested MSI-Xs is not enough

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 7e2788c..5b8ea71 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2738,7 +2738,7 @@ static int
 vmxnet3_acquire_msix_vectors(struct vmxnet3_adapter *adapter,
 			     int vectors)
 {
-	int err = 0, vector_threshold;
+	int err = -EINVAL, vector_threshold;
 	vector_threshold = VMXNET3_LINUX_MIN_MSIX_VECT;
 
 	while (vectors >= vector_threshold) {
-- 
1.7.7.6

[PATCH RFC v2 26/29] vmxnet3: Fixup a weird loop exit

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 5b8ea71..3518173 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2750,7 +2750,7 @@ vmxnet3_acquire_msix_vectors(struct vmxnet3_adapter *adapter,
 		} else if (err < 0) {
 			dev_err(&adapter->netdev->dev,
 				   "Failed to enable MSI-X, error: %d\n", err);
-			vectors = 0;
+			return err;
 		} else if (err < vector_threshold) {
 			break;
 		} else {
-- 
1.7.7.6

[PATCH RFC v2 27/29] vmxnet3: Return -ENOSPC when not enough MSI-X vectors available

[Alexander Gordeev]

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

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 3518173..3df7f32 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2752,7 +2752,10 @@ vmxnet3_acquire_msix_vectors(struct vmxnet3_adapter *adapter,
 				   "Failed to enable MSI-X, error: %d\n", err);
 			return err;
 		} else if (err < vector_threshold) {
-			break;
+			dev_info(&adapter->pdev->dev,
+				 "Number of MSI-Xs which can be allocated "
+				 "is lower than min threshold required.\n");
+			return -ENOSPC;
 		} else {
 			/* If fails to enable required number of MSI-x vectors
 			 * try enabling minimum number of vectors required.
@@ -2764,9 +2767,6 @@ vmxnet3_acquire_msix_vectors(struct vmxnet3_adapter *adapter,
 		}
 	}
 
-	dev_info(&adapter->pdev->dev,
-		 "Number of MSI-X interrupts which can be allocated "
-		 "is lower than min threshold required.\n");
 	return err;
 }
 
-- 
1.7.7.6

[PATCH RFC v2 28/29] vmxnet3: Limit number of rx queues to 1 if per-queue MSI-Xs failed

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 3df7f32..d33802c 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2814,23 +2814,21 @@ vmxnet3_alloc_intr_resources(struct vmxnet3_adapter *adapter)
 
 		err = vmxnet3_acquire_msix_vectors(adapter,
 						   adapter->intr.num_intrs);
-		/* If we cannot allocate one MSIx vector per queue
-		 * then limit the number of rx queues to 1
-		 */
-		if (err == VMXNET3_LINUX_MIN_MSIX_VECT) {
-			if (adapter->share_intr != VMXNET3_INTR_BUDDYSHARE
-			    || adapter->num_rx_queues != 1) {
+		if (!err) {
+			/* If we cannot allocate one MSIx vector per queue
+			 * then limit the number of rx queues to 1
+			 */
+			if ((adapter->intr.num_intrs ==
+			     VMXNET3_LINUX_MIN_MSIX_VECT) &&
+			    ((adapter->share_intr != VMXNET3_INTR_BUDDYSHARE) ||
+			     (adapter->num_rx_queues != 1))) {
 				adapter->share_intr = VMXNET3_INTR_TXSHARE;
 				netdev_err(adapter->netdev,
 					   "Number of rx queues : 1\n");
 				adapter->num_rx_queues = 1;
-				adapter->intr.num_intrs =
-						VMXNET3_LINUX_MIN_MSIX_VECT;
 			}
 			return;
 		}
-		if (!err)
-			return;
 
 		/* If we cannot allocate MSIx vectors use only one rx queue */
 		dev_info(&adapter->pdev->dev,
-- 
1.7.7.6

[PATCH RFC v2 29/29] vmxnet3: Make use of pcim_enable_msix_range() interface

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c |   40 +++++++++---------------------------
 1 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index d33802c..e552d2b 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2735,39 +2735,19 @@ vmxnet3_read_mac_addr(struct vmxnet3_adapter *adapter, u8 *mac)
  */
 
 static int
-vmxnet3_acquire_msix_vectors(struct vmxnet3_adapter *adapter,
-			     int vectors)
+vmxnet3_acquire_msix_vectors(struct vmxnet3_adapter *adapter, int vectors)
 {
-	int err = -EINVAL, vector_threshold;
-	vector_threshold = VMXNET3_LINUX_MIN_MSIX_VECT;
-
-	while (vectors >= vector_threshold) {
-		err = pci_enable_msix(adapter->pdev, adapter->intr.msix_entries,
-				      vectors);
-		if (!err) {
-			adapter->intr.num_intrs = vectors;
-			return 0;
-		} else if (err < 0) {
-			dev_err(&adapter->netdev->dev,
-				   "Failed to enable MSI-X, error: %d\n", err);
-			return err;
-		} else if (err < vector_threshold) {
-			dev_info(&adapter->pdev->dev,
-				 "Number of MSI-Xs which can be allocated "
-				 "is lower than min threshold required.\n");
-			return -ENOSPC;
-		} else {
-			/* If fails to enable required number of MSI-x vectors
-			 * try enabling minimum number of vectors required.
-			 */
-			dev_err(&adapter->netdev->dev,
-				"Failed to enable %d MSI-X, trying %d instead\n",
-				    vectors, vector_threshold);
-			vectors = vector_threshold;
-		}
+	vectors = pcim_enable_msix_range(adapter->pdev,
+					 adapter->intr.msix_entries, vectors,
+					 VMXNET3_LINUX_MIN_MSIX_VECT);
+	if (vectors < 0) {
+		dev_err(&adapter->netdev->dev,
+			"Failed to enable MSI-X, error: %d\n", vectors);
+		return vectors;
 	}
 
-	return err;
+	adapter->intr.num_intrs = vectors;
+	return 0;
 }
 
 
-- 
1.7.7.6

RE: [PATCH RFC v2 29/29] vmxnet3: Make use of pcim_enable_msix_range() interface

[David Laight]

> Subject: [PATCH RFC v2 29/29] vmxnet3: Make use of pcim_enable_msix_range() interface
...
> diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
> index d33802c..e552d2b 100644
> --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> @@ -2735,39 +2735,19 @@ vmxnet3_read_mac_addr(struct vmxnet3_adapter *adapter, u8 *mac)
>   */
> 
>  static int
> -vmxnet3_acquire_msix_vectors(struct vmxnet3_adapter *adapter,
> -			     int vectors)
> +vmxnet3_acquire_msix_vectors(struct vmxnet3_adapter *adapter, int vectors)
>  {
> -	int err = -EINVAL, vector_threshold;
> -	vector_threshold = VMXNET3_LINUX_MIN_MSIX_VECT;
> -
> -	while (vectors >= vector_threshold) {
> -		err = pci_enable_msix(adapter->pdev, adapter->intr.msix_entries,
> -				      vectors);
> -		if (!err) {
> -			adapter->intr.num_intrs = vectors;
> -			return 0;
> -		} else if (err < 0) {
> -			dev_err(&adapter->netdev->dev,
> -				   "Failed to enable MSI-X, error: %d\n", err);
> -			return err;
> -		} else if (err < vector_threshold) {
> -			dev_info(&adapter->pdev->dev,
> -				 "Number of MSI-Xs which can be allocated "
> -				 "is lower than min threshold required.\n");
> -			return -ENOSPC;
> -		} else {
> -			/* If fails to enable required number of MSI-x vectors
> -			 * try enabling minimum number of vectors required.
> -			 */
> -			dev_err(&adapter->netdev->dev,
> -				"Failed to enable %d MSI-X, trying %d instead\n",
> -				    vectors, vector_threshold);
> -			vectors = vector_threshold;
> -		}
> +	vectors = pcim_enable_msix_range(adapter->pdev,
> +					 adapter->intr.msix_entries, vectors,
> +					 VMXNET3_LINUX_MIN_MSIX_VECT);
> +	if (vectors < 0) {
> +		dev_err(&adapter->netdev->dev,
> +			"Failed to enable MSI-X, error: %d\n", vectors);
> +		return vectors;
>  	}
> 
> -	return err;
> +	adapter->intr.num_intrs = vectors;
> +	return 0;
>  }

AFAICT the old code either used the requested number or the minimum number.
The new code seems to claim an intermediate number of interrupts - but probably
only uses the minimum number.
This wastes the last few MSI-X interrupts.
The code (especially the calling code) would be easier to read if the 'vectors'
value wasn't explicitly passed.

	David

Re: [PATCH RFC v2 29/29] vmxnet3: Make use of pcim_enable_msix_range() interface

[Alexander Gordeev]

On Mon, Oct 21, 2013 at 09:51:32AM +0100, David Laight wrote:
> > Subject: [PATCH RFC v2 29/29] vmxnet3: Make use of pcim_enable_msix_range() interface
> AFAICT the old code either used the requested number or the minimum number.
> The new code seems to claim an intermediate number of interrupts - but probably
> only uses the minimum number.

Yes, you are right. I missed the vectors = vector_threshold assignment.
This driver is inapplicable as an example.


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RFC v2 00/29] Introduce pcim_enable_msi*() family helpers

[Alexander Gordeev]

On Fri, Oct 18, 2013 at 07:12:00PM +0200, Alexander Gordeev wrote:
> Patches 1,2	- ACK'ed tweaks for s390 architecture
> Patches 3,4	- fixes for PowerPC pSeries platform
> Patches 5-12	- fixes, tweaks and changes of the generic MSI code
> Patches 12-29	- example updates of few device drivers

Hi Bjorn,

There was not negative feedback so far - can we go ahead with patches
1,2 and 5-12? Would be nice to have patches 3,4 reviewed, but they are
not really a blocker.

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RFC v2 00/29] Introduce pcim_enable_msi*() family helpers

[Tejun Heo]

Hello,

Sorry about the long delay.  Except for the several nits that I
mentioned, things generally look good to me.

Thanks a lot for your patience.

-- 
tejun