[PATCH v2 0/9] Phase out pci_enable_msi_block()

[PATCH v2 0/9] Phase out pci_enable_msi_block()

[Alexander Gordeev]

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

Changes from v1 to v2:
  - added a regression fix "ahci: Fix broken fallback to single
    MSI mode" as patch 1/9;
  - the series is reordered to move the regression fix in front;
  - at Bjorn's request pci_enable_msi() is un-deprecated;
  - as result, pci_enable_msi_range(pdev, 1, 1) styled calls
    rolled back to pci_enable_msi(pdev);
  - nvme bug fix moved out as a separate patch 5/9 "nvme: Fix
    invalid call to irq_set_affinity_hint()"
  - patches changelog elaborated a bit;

Bjorn,

As the release is supposedly this weekend, do you prefer
the patches to go to your tree or to individual trees after
the release?

Thanks!

Alexander Gordeev (9):
  ahci: Fix broken fallback to single MSI mode
  ahci: Use pci_enable_msi_range()
  ipr: Get rid of superfluous call to pci_disable_msi/msix()
  ipr: Use pci_enable_msi_range() and pci_enable_msix_range()
  nvme: Fix invalid call to irq_set_affinity_hint()
  nvme: Use pci_enable_msi_range() and pci_enable_msix_range()
  vfio: Use pci_enable_msi_range() and pci_enable_msix_range()
  ath10k: Use pci_enable_msi_range()
  wil6210: Use pci_enable_msi_range()

 drivers/ata/ahci.c                          |   18 +++++-----
 drivers/block/nvme-core.c                   |   33 ++++-------------
 drivers/net/wireless/ath/ath10k/pci.c       |   20 +++++-----
 drivers/net/wireless/ath/wil6210/pcie_bus.c |   36 ++++++++++---------
 drivers/scsi/ipr.c                          |   51 +++++++++-----------------
 drivers/vfio/pci/vfio_pci_intrs.c           |   12 ++++--
 6 files changed, 72 insertions(+), 98 deletions(-)

-- 
1.7.7.6

[PATCH 10/9] PCI/MSI: Undeprecate pci_enable_msi()

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 Documentation/PCI/MSI-HOWTO.txt |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index a8d0100..3c3080e 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -82,7 +82,19 @@ Most of the hard work is done for the driver in the PCI layer.  It simply
 has to request that the PCI layer set up the MSI capability for this
 device.
 
-4.2.1 pci_enable_msi_range
+4.2.1 pci_enable_msi
+
+int pci_enable_msi(struct pci_dev *dev)
+
+A successful call allocates ONE interrupt to the device, regardless
+of how many MSIs the device supports.  The device is switched from
+pin-based interrupt mode to MSI mode.  The dev->irq number is changed
+to a new number which represents the message signaled interrupt;
+consequently, this function should be called before the driver calls
+request_irq(), because an MSI is delivered via a vector that is
+different from the vector of a pin-based interrupt.
+
+4.2.2 pci_enable_msi_range
 
 int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
 
@@ -158,7 +170,12 @@ static int foo_driver_enable_single_msi(struct pci_dev *pdev)
 	return pci_enable_msi_range(pdev, 1, 1);
 }
 
-4.2.2 pci_disable_msi
+Note, unlike pci_enable_msi() function, which could be also used to
+enable the single MSI mode, pci_enable_msi_range() returns either a
+negative errno or 1 (not negative errno or 0 - as pci_enable_msi()
+does).
+
+4.2.3 pci_disable_msi
 
 void pci_disable_msi(struct pci_dev *dev)
 
@@ -172,7 +189,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.3 pci_msi_vec_count
+4.2.4 pci_msi_vec_count
 
 int pci_msi_vec_count(struct pci_dev *dev)
 
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 10/9] PCI/MSI: Undeprecate pci_enable_msi()

[Bjorn Helgaas]

On Fri, Jan 17, 2014 at 10:26:20AM +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

I applied this to pci/msi, thanks!

> ---
>  Documentation/PCI/MSI-HOWTO.txt |   23 ++++++++++++++++++++---
>  1 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
> index a8d0100..3c3080e 100644
> --- a/Documentation/PCI/MSI-HOWTO.txt
> +++ b/Documentation/PCI/MSI-HOWTO.txt
> @@ -82,7 +82,19 @@ Most of the hard work is done for the driver in the PCI layer.  It simply
>  has to request that the PCI layer set up the MSI capability for this
>  device.
>  
> -4.2.1 pci_enable_msi_range
> +4.2.1 pci_enable_msi
> +
> +int pci_enable_msi(struct pci_dev *dev)
> +
> +A successful call allocates ONE interrupt to the device, regardless
> +of how many MSIs the device supports.  The device is switched from
> +pin-based interrupt mode to MSI mode.  The dev->irq number is changed
> +to a new number which represents the message signaled interrupt;
> +consequently, this function should be called before the driver calls
> +request_irq(), because an MSI is delivered via a vector that is
> +different from the vector of a pin-based interrupt.
> +
> +4.2.2 pci_enable_msi_range
>  
>  int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
>  
> @@ -158,7 +170,12 @@ static int foo_driver_enable_single_msi(struct pci_dev *pdev)
>  	return pci_enable_msi_range(pdev, 1, 1);
>  }
>  
> -4.2.2 pci_disable_msi
> +Note, unlike pci_enable_msi() function, which could be also used to
> +enable the single MSI mode, pci_enable_msi_range() returns either a
> +negative errno or 1 (not negative errno or 0 - as pci_enable_msi()
> +does).
> +
> +4.2.3 pci_disable_msi
>  
>  void pci_disable_msi(struct pci_dev *dev)
>  
> @@ -172,7 +189,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.3 pci_msi_vec_count
> +4.2.4 pci_msi_vec_count
>  
>  int pci_msi_vec_count(struct pci_dev *dev)
>  
> -- 
> 1.7.7.6
> 
> -- 
> Regards,
> Alexander Gordeev
> agordeev@redhat.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/9] PCI/MSI: Phase out pci_enable_msi_block()

[Alexander Gordeev]

This update obsoletes pci_enable_msi_block() function
in favor of pci_enable_msi_range().

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/pci/msi.c   |   17 +++++++++++++++--
 include/linux/pci.h |    7 ++-----
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 7a0fec6..49088d0 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -889,7 +889,7 @@ EXPORT_SYMBOL(pci_msi_vec_count);
  * updates the @dev's irq member to the lowest new interrupt number; the
  * other interrupt numbers allocated to this device are consecutive.
  */
-int pci_enable_msi_block(struct pci_dev *dev, int nvec)
+static int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 {
 	int status, maxvec;
 
@@ -918,7 +918,6 @@ int pci_enable_msi_block(struct pci_dev *dev, int nvec)
 	status = msi_capability_init(dev, nvec);
 	return status;
 }
-EXPORT_SYMBOL(pci_enable_msi_block);
 
 void pci_msi_shutdown(struct pci_dev *dev)
 {
@@ -1148,6 +1147,20 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
 EXPORT_SYMBOL(pci_enable_msi_range);
 
 /**
+ * pci_enable_msi - configure device's MSI capability structure
+ * @dev: device to configure
+ *
+ * This function initializes the single MSI mode. It returns a negative errno
+ * if an error occurs. If it succeeds, it returns 0 and updates the @dev's
+ * irq member to the new interrupt number;
+ **/
+int pci_enable_msi(struct pci_dev *dev)
+{
+	return pci_enable_msi_block(dev, 1);
+}
+EXPORT_SYMBOL(pci_enable_msi);
+
+/**
  * pci_enable_msix_range - configure device's MSI-X capability structure
  * @dev: pointer to the pci_dev data structure of MSI-X device function
  * @entries: pointer to an array of MSI-X entries
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fa959aa..5b58078 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1157,7 +1157,7 @@ struct msix_entry {
 
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
-int pci_enable_msi_block(struct pci_dev *dev, int nvec);
+int pci_enable_msi(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
 int pci_msix_vec_count(struct pci_dev *dev);
@@ -1172,8 +1172,7 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
 			  int minvec, int maxvec);
 #else
 static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
-static inline int pci_enable_msi_block(struct pci_dev *dev, int nvec)
-{ return -ENOSYS; }
+static inline int pci_enable_msi(struct pci_dev *dev) { return -ENOSYS; }
 static inline void pci_msi_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msi(struct pci_dev *dev) { }
 static inline int pci_msix_vec_count(struct pci_dev *dev) { return -ENOSYS; }
@@ -1223,8 +1222,6 @@ static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }
 static inline void pcie_ecrc_get_policy(char *str) { }
 #endif
 
-#define pci_enable_msi(pdev)	pci_enable_msi_block(pdev, 1)
-
 #ifdef CONFIG_HT_IRQ
 /* The functions a driver should call */
 int  ht_create_irq(struct pci_dev *dev, int idx);
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 11/9] PCI/MSI: Phase out pci_enable_msi_block()

[Bjorn Helgaas]

On Fri, Jan 17, 2014 at 10:27:38AM +0100, Alexander Gordeev wrote:
> This update obsoletes pci_enable_msi_block() function
> in favor of pci_enable_msi_range().
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/pci/msi.c   |   17 +++++++++++++++--
>  include/linux/pci.h |    7 ++-----
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 7a0fec6..49088d0 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -889,7 +889,7 @@ EXPORT_SYMBOL(pci_msi_vec_count);
>   * updates the @dev's irq member to the lowest new interrupt number; the
>   * other interrupt numbers allocated to this device are consecutive.
>   */
> -int pci_enable_msi_block(struct pci_dev *dev, int nvec)
> +static int pci_enable_msi_block(struct pci_dev *dev, int nvec)
>  {
>  	int status, maxvec;
>  
> @@ -918,7 +918,6 @@ int pci_enable_msi_block(struct pci_dev *dev, int nvec)
>  	status = msi_capability_init(dev, nvec);
>  	return status;
>  }
> -EXPORT_SYMBOL(pci_enable_msi_block);
>  
>  void pci_msi_shutdown(struct pci_dev *dev)
>  {
> @@ -1148,6 +1147,20 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
>  EXPORT_SYMBOL(pci_enable_msi_range);
>  
>  /**
> + * pci_enable_msi - configure device's MSI capability structure
> + * @dev: device to configure
> + *
> + * This function initializes the single MSI mode. It returns a negative errno
> + * if an error occurs. If it succeeds, it returns 0 and updates the @dev's
> + * irq member to the new interrupt number;
> + **/
> +int pci_enable_msi(struct pci_dev *dev)
> +{
> +	return pci_enable_msi_block(dev, 1);

I like un-exporting pci_enable_msi_block(), but I don't like the fact that
we keep it internally.  I'd rather fold it into pci_enable_msi_range()
directly, and then make pci_enable_msi() a wrapper around *that*.

pci_enable_msi_block() has some one-time checks (vector count, already
enabled) that should not be inside the loop in pci_enable_msi_range().
So I think some rearrangement could lead to cleaner code here.

> +}
> +EXPORT_SYMBOL(pci_enable_msi);
> +
> +/**
>   * pci_enable_msix_range - configure device's MSI-X capability structure
>   * @dev: pointer to the pci_dev data structure of MSI-X device function
>   * @entries: pointer to an array of MSI-X entries
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index fa959aa..5b58078 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1157,7 +1157,7 @@ struct msix_entry {
>  
>  #ifdef CONFIG_PCI_MSI
>  int pci_msi_vec_count(struct pci_dev *dev);
> -int pci_enable_msi_block(struct pci_dev *dev, int nvec);
> +int pci_enable_msi(struct pci_dev *dev);
>  void pci_msi_shutdown(struct pci_dev *dev);
>  void pci_disable_msi(struct pci_dev *dev);
>  int pci_msix_vec_count(struct pci_dev *dev);
> @@ -1172,8 +1172,7 @@ int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
>  			  int minvec, int maxvec);
>  #else
>  static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
> -static inline int pci_enable_msi_block(struct pci_dev *dev, int nvec)
> -{ return -ENOSYS; }
> +static inline int pci_enable_msi(struct pci_dev *dev) { return -ENOSYS; }
>  static inline void pci_msi_shutdown(struct pci_dev *dev) { }
>  static inline void pci_disable_msi(struct pci_dev *dev) { }
>  static inline int pci_msix_vec_count(struct pci_dev *dev) { return -ENOSYS; }
> @@ -1223,8 +1222,6 @@ static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }
>  static inline void pcie_ecrc_get_policy(char *str) { }
>  #endif
>  
> -#define pci_enable_msi(pdev)	pci_enable_msi_block(pdev, 1)
> -
>  #ifdef CONFIG_HT_IRQ
>  /* The functions a driver should call */
>  int  ht_create_irq(struct pci_dev *dev, int idx);
> -- 
> 1.7.7.6
> 
> -- 
> Regards,
> Alexander Gordeev
> agordeev@redhat.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/9] ahci: Fix broken fallback to single MSI mode

[Alexander Gordeev]

Commit 7b92b4f ("PCI/MSI: Remove pci_enable_msi_block_auto()")
introduced a regression - in case multiple MSIs initialization
fails the code falls back to INTx rather than to single MSI.

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

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 8516f4d..df24eef4 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1117,8 +1117,10 @@ int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 
 	nvec = rc;
 	rc = pci_enable_msi_block(pdev, nvec);
-	if (rc)
+	if (rc < 0)
 		goto intx;
+	else if (rc > 0)
+		goto single_msi;
 
 	return nvec;
 
-- 
1.7.7.6

[PATCH v2 2/9] ahci: Use pci_enable_msi_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

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

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

[PATCH v2 3/9] ipr: Get rid of superfluous call to pci_disable_msi/msix()

[Alexander Gordeev]

There is no need to call pci_disable_msi() or pci_disable_msix()
in case the call to pci_enable_msi() or pci_enable_msix() failed.

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 v2 4/9] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

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..3841298 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)
 		entries[i].entry = i;
 
-	vectors = ipr_number_of_msix;
+	vectors = pci_enable_msix_range(ioa_cfg->pdev, entries,
+					1, ipr_number_of_msix);
+	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 = pci_enable_msi_range(ioa_cfg->pdev, 1, 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)
@@ -9350,7 +9339,7 @@ static irqreturn_t ipr_test_intr(int irq, void *devp)
  * ipr_test_msi - Test for Message Signaled Interrupt (MSI) support.
  * @pdev:		PCI device struct
  *
- * Description: The return value from pci_enable_msi() can not always be
+ * Description: The return value from pci_enable_msi_range() can not always be
  * trusted.  This routine sets up and initiates a test interrupt to determine
  * if the interrupt is received via the ipr_test_intr() service routine.
  * If the tests fails, the driver will fall back to LSI.
-- 
1.7.7.6

Re: [PATCH v2 4/9] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

On Fri, Jan 17, 2014 at 05:02:18PM +0100, Alexander Gordeev wrote:
> As result deprecation of MSI-X/MSI enablement functions
> pci_enable_msix() and pci_enable_msi_block() all drivers
> using these two interfaces need to be updated to use the
> new pci_enable_msi_range() and pci_enable_msix_range()
> interfaces.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

Brian, James,

Could you please review patches 3 and 4?

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 4/9] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Brian King]

On 01/28/2014 03:54 AM, Alexander Gordeev wrote:
> On Fri, Jan 17, 2014 at 05:02:18PM +0100, Alexander Gordeev wrote:
>> As result deprecation of MSI-X/MSI enablement functions
>> pci_enable_msix() and pci_enable_msi_block() all drivers
>> using these two interfaces need to be updated to use the
>> new pci_enable_msi_range() and pci_enable_msix_range()
>> interfaces.
>>
>> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> 
> Brian, James,
> 
> Could you please review patches 3 and 4?

These look like they will conflict with a recent patch to
the ipr driver:

http://www.spinics.net/lists/linux-scsi/msg71644.html

Do you want me to rediff your patches on top of this one,
or do you want to keep the entire MSI series together
and do the rediff? Otherwise the patches seem fine.

Thanks,

Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH v2 4/9] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

On Tue, Jan 28, 2014 at 09:28:45AM -0600, Brian King wrote:

Thanks for the review, Brian!

> These look like they will conflict with a recent patch to
> the ipr driver:
> 
> http://www.spinics.net/lists/linux-scsi/msg71644.html

BTW, this one does not appear ACKed.

> Do you want me to rediff your patches on top of this one,
> or do you want to keep the entire MSI series together
> and do the rediff? Otherwise the patches seem fine.

I would prefer the former.

@Bjorn, please let us know if you prefer otherwise.

> Thanks,
> 
> Brian
> 
> 
> -- 
> Brian King
> Power Linux I/O
> IBM Linux Technology Center
> 
> 

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 4/9] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

On Wed, Jan 29, 2014 at 02:26:52PM +0100, Alexander Gordeev wrote:
> > Do you want me to rediff your patches on top of this one,
> > or do you want to keep the entire MSI series together
> > and do the rediff? Otherwise the patches seem fine.
> 
> I would prefer the former.

Hi Brian,

I am sending the refreshed patches on top of "ipr: Handle early EEH".


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v2 1/2] ipr: Get rid of superfluous call to pci_disable_msi/msix()

[Alexander Gordeev]

There is no need to call pci_disable_msi() or pci_disable_msix()
in case the call to pci_enable_msi() or pci_enable_msix() failed.

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

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 96c10ce..48d0cfc 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -9329,7 +9329,6 @@ static int ipr_enable_msix(struct ipr_ioa_cfg *ioa_cfg)
 
 	if (err < 0) {
 		ipr_wait_for_pci_err_recovery(ioa_cfg);
-		pci_disable_msix(ioa_cfg->pdev);
 		return err;
 	}
 
@@ -9353,7 +9352,6 @@ static int ipr_enable_msi(struct ipr_ioa_cfg *ioa_cfg)
 
 	if (err < 0) {
 		ipr_wait_for_pci_err_recovery(ioa_cfg);
-		pci_disable_msi(ioa_cfg->pdev);
 		return err;
 	}
 
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 1/2] ipr: Get rid of superfluous call to pci_disable_msi/msix()

[Brian King]

Acked-by: Brian King <brking@linux.vnet.ibm.com>


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[PATCH v2 2/2] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

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 48d0cfc..70f40ca 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -9317,51 +9317,40 @@ static void ipr_wait_for_pci_err_recovery(struct ipr_ioa_cfg *ioa_cfg)
 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)
 		entries[i].entry = i;
 
-	vectors = ipr_number_of_msix;
-
-	while ((err = pci_enable_msix(ioa_cfg->pdev, entries, vectors)) > 0)
-			vectors = err;
-
-	if (err < 0) {
+	vectors = pci_enable_msix_range(ioa_cfg->pdev,
+					entries, 1, ipr_number_of_msix);
+	if (vectors < 0) {
 		ipr_wait_for_pci_err_recovery(ioa_cfg);
-		return err;
+		return vectors;
 	}
 
-	if (!err) {
-		for (i = 0; i < vectors; i++)
-			ioa_cfg->vectors_info[i].vec = entries[i].vector;
-		ioa_cfg->nvectors = vectors;
-	}
+	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;
-
-	while ((err = pci_enable_msi_block(ioa_cfg->pdev, vectors)) > 0)
-			vectors = err;
-
-	if (err < 0) {
+	vectors = pci_enable_msi_range(ioa_cfg->pdev, 1, ipr_number_of_msix);
+	if (vectors < 0) {
 		ipr_wait_for_pci_err_recovery(ioa_cfg);
-		return err;
+		return vectors;
 	}
 
-	if (!err) {
-		for (i = 0; i < vectors; i++)
-			ioa_cfg->vectors_info[i].vec = ioa_cfg->pdev->irq + i;
-		ioa_cfg->nvectors = vectors;
-	}
+	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)
@@ -9426,7 +9415,7 @@ static irqreturn_t ipr_test_intr(int irq, void *devp)
  * ipr_test_msi - Test for Message Signaled Interrupt (MSI) support.
  * @pdev:		PCI device struct
  *
- * Description: The return value from pci_enable_msi() can not always be
+ * Description: The return value from pci_enable_msi_range() can not always be
  * trusted.  This routine sets up and initiates a test interrupt to determine
  * if the interrupt is received via the ipr_test_intr() service routine.
  * If the tests fails, the driver will fall back to LSI.
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 2/2] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Brian King]

Acked-by: Brian King <brking@linux.vnet.ibm.com>

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH v2 4/9] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

On Thu, Jan 30, 2014 at 03:06:30PM +0100, Alexander Gordeev wrote:
> On Wed, Jan 29, 2014 at 02:26:52PM +0100, Alexander Gordeev wrote:
> > > Do you want me to rediff your patches on top of this one,
> > > or do you want to keep the entire MSI series together
> > > and do the rediff? Otherwise the patches seem fine.
> > 
> > I would prefer the former.
> 
> Hi Brian,
> 
> I am sending the refreshed patches on top of "ipr: Handle early EEH".

Hi Brian,

What three the two patches you ACKed are against?
Should I ask James to take them?

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 4/9] ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

[Brian King]

On 02/24/2014 02:12 AM, Alexander Gordeev wrote:
> On Thu, Jan 30, 2014 at 03:06:30PM +0100, Alexander Gordeev wrote:
>> On Wed, Jan 29, 2014 at 02:26:52PM +0100, Alexander Gordeev wrote:
>>>> Do you want me to rediff your patches on top of this one,
>>>> or do you want to keep the entire MSI series together
>>>> and do the rediff? Otherwise the patches seem fine.
>>>
>>> I would prefer the former.
>>
>> Hi Brian,
>>
>> I am sending the refreshed patches on top of "ipr: Handle early EEH".
> 
> Hi Brian,
> 
> What three the two patches you ACKed are against?
> Should I ask James to take them?

Wendy - can you pull in these two patches from Alexander and rebase on
top of the latest patches you've sent upstream?

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[PATCH v2 5/9] nvme: Fix invalid call to irq_set_affinity_hint()

[Alexander Gordeev]

In case MSI-X and MSI initialization failed the function
irq_set_affinity_hint() is called with uninitialized value
in dev->entry[0].vector. This update fixes the issue.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/block/nvme-core.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 26d03fa..e292450 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1790,15 +1790,15 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 			vecs = 32;
 		for (;;) {
 			result = pci_enable_msi_block(pdev, vecs);
-			if (result == 0) {
-				for (i = 0; i < vecs; i++)
-					dev->entry[i].vector = i + pdev->irq;
-				break;
+			if (result > 0) {
+				vecs = result;
+				continue;
 			} else if (result < 0) {
 				vecs = 1;
-				break;
 			}
-			vecs = result;
+			for (i = 0; i < vecs; i++)
+				dev->entry[i].vector = i + pdev->irq;
+			break;
 		}
 	}
 
-- 
1.7.7.6

Re: [PATCH v2 5/9] nvme: Fix invalid call to irq_set_affinity_hint()

[Bjorn Helgaas]

On Fri, Jan 17, 2014 at 9:02 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> In case MSI-X and MSI initialization failed the function
> irq_set_affinity_hint() is called with uninitialized value
> in dev->entry[0].vector. This update fixes the issue.

dev->entry[0].vector is initialized in nvme_dev_map(), and it's used
for free_irq() above the area of your patch, so I don't think this is
actually a bug, though it might be somewhat confusing.

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/block/nvme-core.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index 26d03fa..e292450 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -1790,15 +1790,15 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
>                         vecs = 32;
>                 for (;;) {
>                         result = pci_enable_msi_block(pdev, vecs);
> -                       if (result == 0) {
> -                               for (i = 0; i < vecs; i++)
> -                                       dev->entry[i].vector = i + pdev->irq;
> -                               break;
> +                       if (result > 0) {
> +                               vecs = result;
> +                               continue;
>                         } else if (result < 0) {
>                                 vecs = 1;
> -                               break;
>                         }
> -                       vecs = result;
> +                       for (i = 0; i < vecs; i++)
> +                               dev->entry[i].vector = i + pdev->irq;
> +                       break;
>                 }
>         }
>
> --
> 1.7.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/9] nvme: Fix invalid call to irq_set_affinity_hint()

[Keith Busch]

On Fri, 17 Jan 2014, Bjorn Helgaas wrote:

> On Fri, Jan 17, 2014 at 9:02 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
>> In case MSI-X and MSI initialization failed the function
>> irq_set_affinity_hint() is called with uninitialized value
>> in dev->entry[0].vector. This update fixes the issue.
>
> dev->entry[0].vector is initialized in nvme_dev_map(), and it's used
> for free_irq() above the area of your patch, so I don't think this is
> actually a bug, though it might be somewhat confusing.

It is confusing, but there's a reason. :)

We send a single command using legacy irq to discover how many msix
vectors we want. The legacy entry needs to be set some time before calling
request_irq in nvme_configure_admin_queue, but also within nvme_dev_start
(for power-management). I don't think there's a place to set it that
won't look odd when looking at nvme_setup_io_queues. I settled on
'nvme_dev_map' was because 'nvme_dev_unmap' invalidates the entries,
so this seemed to provide some amount of symmetry.

>> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
>> ---
>>  drivers/block/nvme-core.c |   12 ++++++------
>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
>> index 26d03fa..e292450 100644
>> --- a/drivers/block/nvme-core.c
>> +++ b/drivers/block/nvme-core.c
>> @@ -1790,15 +1790,15 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
>>                         vecs = 32;
>>                 for (;;) {
>>                         result = pci_enable_msi_block(pdev, vecs);
>> -                       if (result == 0) {
>> -                               for (i = 0; i < vecs; i++)
>> -                                       dev->entry[i].vector = i + pdev->irq;
>> -                               break;
>> +                       if (result > 0) {
>> +                               vecs = result;
>> +                               continue;
>>                         } else if (result < 0) {
>>                                 vecs = 1;
>> -                               break;
>>                         }
>> -                       vecs = result;
>> +                       for (i = 0; i < vecs; i++)
>> +                               dev->entry[i].vector = i + pdev->irq;
>> +                       break;
>>                 }
>>         }

Re: [PATCH v2 5/9] nvme: Fix invalid call to irq_set_affinity_hint()

[Alexander Gordeev]

On Fri, Jan 17, 2014 at 03:01:10PM -0700, Keith Busch wrote:
> >dev->entry[0].vector is initialized in nvme_dev_map(), and it's used
> >for free_irq() above the area of your patch, so I don't think this is
> >actually a bug, though it might be somewhat confusing.
> 
> It is confusing, but there's a reason. :)
> 
> We send a single command using legacy irq to discover how many msix
> vectors we want. The legacy entry needs to be set some time before calling
> request_irq in nvme_configure_admin_queue, but also within nvme_dev_start
> (for power-management). I don't think there's a place to set it that
> won't look odd when looking at nvme_setup_io_queues. I settled on
> 'nvme_dev_map' was because 'nvme_dev_unmap' invalidates the entries,
> so this seemed to provide some amount of symmetry.

I am sending v3 of the patch which concerns Bjorn's comment.

I am also sending two follow-up patches an attempt to make nvme_dev_start()
more readable and fix couple of (what I suspect are) issues.

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v3 6/9] nvme: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/block/nvme-core.c |   33 +++++++++------------------------
 1 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 26d03fa..3e1ae55 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1774,31 +1774,16 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	/* Deregister the admin queue's interrupt */
 	free_irq(dev->entry[0].vector, dev->queues[0]);
 
-	vecs = nr_io_queues;
-	for (i = 0; i < vecs; i++)
+	for (i = 0; i < nr_io_queues; i++)
 		dev->entry[i].entry = i;
-	for (;;) {
-		result = pci_enable_msix(pdev, dev->entry, vecs);
-		if (result <= 0)
-			break;
-		vecs = result;
-	}
-
-	if (result < 0) {
-		vecs = nr_io_queues;
-		if (vecs > 32)
-			vecs = 32;
-		for (;;) {
-			result = pci_enable_msi_block(pdev, vecs);
-			if (result == 0) {
-				for (i = 0; i < vecs; i++)
-					dev->entry[i].vector = i + pdev->irq;
-				break;
-			} else if (result < 0) {
-				vecs = 1;
-				break;
-			}
-			vecs = result;
+	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
+	if (vecs < 0) {
+		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
+		if (vecs < 0) {
+			vecs = 1;
+		} else {
+			for (i = 0; i < vecs; i++)
+				dev->entry[i].vector = i + pdev->irq;
 		}
 	}
 
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v3 6/9] nvme: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

On Mon, Jan 20, 2014 at 09:40:46AM +0100, Alexander Gordeev wrote:
> As result deprecation of MSI-X/MSI enablement functions
> pci_enable_msix() and pci_enable_msi_block() all drivers
> using these two interfaces need to be updated to use the
> new pci_enable_msi_range() and pci_enable_msix_range()
> interfaces.

Hi Keith,

What about letting this change only for now and considering
the series "NVMe: Cleanup device initialization" afterwards?

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v3 6/9] nvme: Use pci_enable_msi_range() and pci_enable_msix_range()

[Keith Busch]

On Mon, 20 Jan 2014, Alexander Gordeev wrote:
> As result deprecation of MSI-X/MSI enablement functions
> pci_enable_msix() and pci_enable_msi_block() all drivers
> using these two interfaces need to be updated to use the
> new pci_enable_msi_range() and pci_enable_msix_range()
> interfaces.
>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>

Reviewed-by: Keith Busch <keith.busch@intel.com>

[PATCH] nvme: Cleanup nvme_dev_start() and fix IRQ leak

[Alexander Gordeev]

This is an attempt to make handling of admin queue in a
single scope. This update also fixes a IRQ leak in case
nvme_setup_io_queues() failed to allocate enough iomem
and bailed out with -ENOMEM errno.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/block/nvme-core.c |   44 +++++++++++++++++++++++---------------------
 1 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 3e1ae55..e1e4ad4 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1287,6 +1287,7 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	if (result)
 		return result;
 
+	dev->entry[0].vector = pdev->pci_dev->irq;
 	result = queue_request_irq(dev, nvmeq, "nvme admin");
 	if (result)
 		return result;
@@ -1297,6 +1298,11 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	return result;
 }
 
+static int nvme_teardown_admin_queue(struct nvme_dev *dev)
+{
+	free_irq(dev->entry[0].vector, dev->queues[0]);
+}
+
 struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 				unsigned long addr, unsigned length)
 {
@@ -1744,17 +1750,10 @@ static size_t db_bar_size(struct nvme_dev *dev, unsigned nr_io_queues)
 	return 4096 + ((nr_io_queues + 1) << (dev->db_stride + 3));
 }
 
-static int nvme_setup_io_queues(struct nvme_dev *dev)
+static int nvme_setup_io_queues(struct nvme_dev *dev, int nr_io_queues)
 {
 	struct pci_dev *pdev = dev->pci_dev;
-	int result, cpu, i, vecs, nr_io_queues, size, q_depth;
-
-	nr_io_queues = num_online_cpus();
-	result = set_queue_count(dev, nr_io_queues);
-	if (result < 0)
-		return result;
-	if (result < nr_io_queues)
-		nr_io_queues = result;
+	int result, cpu, i, vecs, size, q_depth;
 
 	size = db_bar_size(dev, nr_io_queues);
 	if (size > 8192) {
@@ -1771,20 +1770,15 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 		dev->queues[0]->q_db = dev->dbs;
 	}
 
-	/* Deregister the admin queue's interrupt */
-	free_irq(dev->entry[0].vector, dev->queues[0]);
-
 	for (i = 0; i < nr_io_queues; i++)
 		dev->entry[i].entry = i;
 	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
 	if (vecs < 0) {
 		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
-		if (vecs < 0) {
+		if (vecs < 0)
 			vecs = 1;
-		} else {
-			for (i = 0; i < vecs; i++)
-				dev->entry[i].vector = i + pdev->irq;
-		}
+		for (i = 0; i < vecs; i++)
+			dev->entry[i].vector = i + pdev->irq;
 	}
 
 	/*
@@ -1928,7 +1922,6 @@ static int nvme_dev_map(struct nvme_dev *dev)
 	if (pci_enable_device_mem(pdev))
 		return result;
 
-	dev->entry[0].vector = pdev->irq;
 	pci_set_master(pdev);
 	bars = pci_select_bars(pdev, IORESOURCE_MEM);
 	if (pci_request_selected_regions(pdev, bars, "nvme"))
@@ -2116,11 +2109,20 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	list_add(&dev->node, &dev_list);
 	spin_unlock(&dev_list_lock);
 
-	result = nvme_setup_io_queues(dev);
-	if (result && result != -EBUSY)
+	result = set_queue_count(dev, num_online_cpus());
+	if (result == -EBUSY)
+		return -EBUSY;
+
+	nvme_teardown_admin_queue(dev);
+
+	if (result)
 		goto disable;
 
-	return result;
+	result = nvme_setup_io_queues(dev, result);
+	if (result)
+		goto disable;
+
+	return 0;
 
  disable:
 	spin_lock(&dev_list_lock);
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] nvme: Cleanup nvme_dev_start() and fix IRQ leak

[Keith Busch]

On Mon, 20 Jan 2014, Alexander Gordeev wrote:
> This is an attempt to make handling of admin queue in a
> single scope. This update also fixes a IRQ leak in case
> nvme_setup_io_queues() failed to allocate enough iomem
> and bailed out with -ENOMEM errno.

This definitely seems to improve the code flow, but this leak was already
fixed in the latest linux-nvme tree with this commit:

http://git.infradead.org/users/willy/linux-nvme.git/commit/c5dc9192d52a4a3a479f701e97386347d454af84

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
> drivers/block/nvme-core.c |   44 +++++++++++++++++++++++---------------------
> 1 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index 3e1ae55..e1e4ad4 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -1287,6 +1287,7 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
> 	if (result)
> 		return result;
>
> +	dev->entry[0].vector = pdev->pci_dev->irq;
> 	result = queue_request_irq(dev, nvmeq, "nvme admin");
> 	if (result)
> 		return result;
> @@ -1297,6 +1298,11 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
> 	return result;
> }
>
> +static int nvme_teardown_admin_queue(struct nvme_dev *dev)
> +{
> +	free_irq(dev->entry[0].vector, dev->queues[0]);
> +}
> +
> struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
> 				unsigned long addr, unsigned length)
> {
> @@ -1744,17 +1750,10 @@ static size_t db_bar_size(struct nvme_dev *dev, unsigned nr_io_queues)
> 	return 4096 + ((nr_io_queues + 1) << (dev->db_stride + 3));
> }
>
> -static int nvme_setup_io_queues(struct nvme_dev *dev)
> +static int nvme_setup_io_queues(struct nvme_dev *dev, int nr_io_queues)
> {
> 	struct pci_dev *pdev = dev->pci_dev;
> -	int result, cpu, i, vecs, nr_io_queues, size, q_depth;
> -
> -	nr_io_queues = num_online_cpus();
> -	result = set_queue_count(dev, nr_io_queues);
> -	if (result < 0)
> -		return result;
> -	if (result < nr_io_queues)
> -		nr_io_queues = result;
> +	int result, cpu, i, vecs, size, q_depth;
>
> 	size = db_bar_size(dev, nr_io_queues);
> 	if (size > 8192) {
> @@ -1771,20 +1770,15 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
> 		dev->queues[0]->q_db = dev->dbs;
> 	}
>
> -	/* Deregister the admin queue's interrupt */
> -	free_irq(dev->entry[0].vector, dev->queues[0]);
> -
> 	for (i = 0; i < nr_io_queues; i++)
> 		dev->entry[i].entry = i;
> 	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
> 	if (vecs < 0) {
> 		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
> -		if (vecs < 0) {
> +		if (vecs < 0)
> 			vecs = 1;
> -		} else {
> -			for (i = 0; i < vecs; i++)
> -				dev->entry[i].vector = i + pdev->irq;
> -		}
> +		for (i = 0; i < vecs; i++)
> +			dev->entry[i].vector = i + pdev->irq;
> 	}
>
> 	/*
> @@ -1928,7 +1922,6 @@ static int nvme_dev_map(struct nvme_dev *dev)
> 	if (pci_enable_device_mem(pdev))
> 		return result;
>
> -	dev->entry[0].vector = pdev->irq;
> 	pci_set_master(pdev);
> 	bars = pci_select_bars(pdev, IORESOURCE_MEM);
> 	if (pci_request_selected_regions(pdev, bars, "nvme"))
> @@ -2116,11 +2109,20 @@ static int nvme_dev_start(struct nvme_dev *dev)
> 	list_add(&dev->node, &dev_list);
> 	spin_unlock(&dev_list_lock);
>
> -	result = nvme_setup_io_queues(dev);
> -	if (result && result != -EBUSY)
> +	result = set_queue_count(dev, num_online_cpus());
> +	if (result == -EBUSY)
> +		return -EBUSY;
> +
> +	nvme_teardown_admin_queue(dev);
> +
> +	if (result)
> 		goto disable;
>
> -	return result;
> +	result = nvme_setup_io_queues(dev, result);
> +	if (result)
> +		goto disable;
> +
> +	return 0;
>
>  disable:
> 	spin_lock(&dev_list_lock);
> --

Re: [PATCH] nvme: Cleanup nvme_dev_start() and fix IRQ leak

[Alexander Gordeev]

On Mon, Jan 20, 2014 at 03:48:56PM -0700, Keith Busch wrote:
> On Mon, 20 Jan 2014, Alexander Gordeev wrote:
> >This is an attempt to make handling of admin queue in a
> >single scope. This update also fixes a IRQ leak in case
> >nvme_setup_io_queues() failed to allocate enough iomem
> >and bailed out with -ENOMEM errno.
> 
> This definitely seems to improve the code flow, but this leak was already
> fixed in the latest linux-nvme tree with this commit:
> 
> http://git.infradead.org/users/willy/linux-nvme.git/commit/c5dc9192d52a4a3a479f701e97386347d454af84

Oh, that one seems to complicate things even further to me.
Whould you consider the revert?
Sending an improved version against your tree.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH 1/2] Revert "NVMe: Disable admin queue on init failure"

[Alexander Gordeev]

This reverts commit c5dc9192d52a4a3a479f701e97386347d454af84.
---
 drivers/block/nvme-core.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index b59a93a..c790679 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1151,11 +1151,11 @@ static void nvme_free_queue(struct nvme_queue *nvmeq)
 	kfree(nvmeq);
 }
 
-static void nvme_free_queues(struct nvme_dev *dev, int lowest)
+static void nvme_free_queues(struct nvme_dev *dev)
 {
 	int i;
 
-	for (i = dev->queue_count - 1; i >= lowest; i--) {
+	for (i = dev->queue_count - 1; i >= 0; i--) {
 		nvme_free_queue(dev->queues[i]);
 		dev->queue_count--;
 		dev->queues[i] = NULL;
@@ -1989,7 +1989,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	return 0;
 
  free_queues:
-	nvme_free_queues(dev, 1);
+	nvme_free_queues(dev);
 	return result;
 }
 
@@ -2409,7 +2409,6 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	return result;
 
  disable:
-	nvme_disable_queue(dev, 0);
 	spin_lock(&dev_list_lock);
 	list_del_init(&dev->node);
 	spin_unlock(&dev_list_lock);
@@ -2541,7 +2540,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  shutdown:
 	nvme_dev_shutdown(dev);
  release_pools:
-	nvme_free_queues(dev, 0);
+	nvme_free_queues(dev);
 	nvme_release_prp_pools(dev);
  release:
 	nvme_release_instance(dev);
@@ -2565,7 +2564,7 @@ static void nvme_remove(struct pci_dev *pdev)
 	misc_deregister(&dev->miscdev);
 	nvme_dev_remove(dev);
 	nvme_dev_shutdown(dev);
-	nvme_free_queues(dev, 0);
+	nvme_free_queues(dev);
 	nvme_release_instance(dev);
 	nvme_release_prp_pools(dev);
 	kref_put(&dev->kref, nvme_free_dev);
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH 2/2] nvme: Cleanup nvme_dev_start() and fix IRQ leak

[Alexander Gordeev]

This is an attempt to make handling of admin queue in a
single scope. This update also fixes a IRQ leak in case
nvme_setup_io_queues() failed to allocate enough iomem
and bailed out with -ENOMEM errno.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/block/nvme-core.c |   37 +++++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index c790679..010a789 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1414,6 +1414,7 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	if (result)
 		return result;
 
+	dev->entry[0].vector = dev->pci_dev->irq;
 	result = queue_request_irq(dev, nvmeq, "nvme admin");
 	if (result)
 		return result;
@@ -1424,6 +1425,12 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	return result;
 }
 
+static void nvme_teardown_admin_queue(struct nvme_dev *dev)
+{
+	nvme_disable_queue(dev, 0);
+	nvme_free_queue(dev->queues[0]);
+}
+
 struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 				unsigned long addr, unsigned length)
 {
@@ -1869,17 +1876,10 @@ static size_t db_bar_size(struct nvme_dev *dev, unsigned nr_io_queues)
 	return 4096 + ((nr_io_queues + 1) * 8 * dev->db_stride);
 }
 
-static int nvme_setup_io_queues(struct nvme_dev *dev)
+static int nvme_setup_io_queues(struct nvme_dev *dev, int nr_io_queues)
 {
 	struct pci_dev *pdev = dev->pci_dev;
-	int result, cpu, i, vecs, nr_io_queues, size, q_depth;
-
-	nr_io_queues = num_online_cpus();
-	result = set_queue_count(dev, nr_io_queues);
-	if (result < 0)
-		return result;
-	if (result < nr_io_queues)
-		nr_io_queues = result;
+	int result, cpu, i, vecs, size, q_depth;
 
 	size = db_bar_size(dev, nr_io_queues);
 	if (size > 8192) {
@@ -1896,9 +1896,6 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 		dev->queues[0]->q_db = dev->dbs;
 	}
 
-	/* Deregister the admin queue's interrupt */
-	free_irq(dev->entry[0].vector, dev->queues[0]);
-
 	vecs = nr_io_queues;
 	for (i = 0; i < vecs; i++)
 		dev->entry[i].entry = i;
@@ -2069,7 +2066,6 @@ static int nvme_dev_map(struct nvme_dev *dev)
 	if (pci_enable_device_mem(pdev))
 		return result;
 
-	dev->entry[0].vector = pdev->irq;
 	pci_set_master(pdev);
 	bars = pci_select_bars(pdev, IORESOURCE_MEM);
 	if (pci_request_selected_regions(pdev, bars, "nvme"))
@@ -2402,11 +2398,20 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	list_add(&dev->node, &dev_list);
 	spin_unlock(&dev_list_lock);
 
-	result = nvme_setup_io_queues(dev);
-	if (result && result != -EBUSY)
+	result = set_queue_count(dev, num_online_cpus());
+	if (result == -EBUSY)
+		return -EBUSY;
+
+	nvme_teardown_admin_queue(dev);
+
+	if (result)
 		goto disable;
 
-	return result;
+	result = nvme_setup_io_queues(dev, result);
+	if (result)
+		goto disable;
+
+	return 0;
 
  disable:
 	spin_lock(&dev_list_lock);
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 2/2] nvme: Cleanup nvme_dev_start() and fix IRQ leak

[Keith Busch]

On Tue, 21 Jan 2014, Alexander Gordeev wrote:
> This is an attempt to make handling of admin queue in a
> single scope. This update also fixes a IRQ leak in case
> nvme_setup_io_queues() failed to allocate enough iomem
> and bailed out with -ENOMEM errno.
>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---

> +static void nvme_teardown_admin_queue(struct nvme_dev *dev)
> +{
> +	nvme_disable_queue(dev, 0);
> +	nvme_free_queue(dev->queues[0]);
> +}

> @@ -2402,11 +2398,20 @@ static int nvme_dev_start(struct nvme_dev *dev)
> 	list_add(&dev->node, &dev_list);
> 	spin_unlock(&dev_list_lock);
>
> -	result = nvme_setup_io_queues(dev);
> -	if (result && result != -EBUSY)
> +	result = set_queue_count(dev, num_online_cpus());
> +	if (result == -EBUSY)
> +		return -EBUSY;
> +
> +	nvme_teardown_admin_queue(dev);

Oh no! Your new teardown function is freeing the admin queue, but it
would be used immediatly after that in nvme_setup_io_queues ...

> +
> +	if (result)
> 		goto disable;

... but you'll never actually get to setup io queues because the 'result'
here is non-zero if we were successful, and is the number of queues the
controller can allocate. I think you meant to do this instead:

+	if (result < 0)

>
> -	return result;
> +	result = nvme_setup_io_queues(dev, result);
> +	if (result)
> +		goto disable;
> +
> +	return 0;

[PATCH] nvme: Cleanup nvme_dev_start()

[Alexander Gordeev]

This update fixes an oddity when a device is first added
and then removed from dev_list in case of initialization
failure, instead of just being added in case of success.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/block/nvme-core.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index e1e4ad4..e4e12be 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2105,29 +2105,26 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	if (result)
 		goto unmap;
 
-	spin_lock(&dev_list_lock);
-	list_add(&dev->node, &dev_list);
-	spin_unlock(&dev_list_lock);
-
 	result = set_queue_count(dev, num_online_cpus());
 	if (result == -EBUSY)
-		return -EBUSY;
+		goto adddev;
 
 	nvme_teardown_admin_queue(dev);
 
 	if (result)
-		goto disable;
+		goto unmap;
 
 	result = nvme_setup_io_queues(dev, result);
 	if (result)
-		goto disable;
-
-	return 0;
+		goto unmap;
 
- disable:
+ adddev:
 	spin_lock(&dev_list_lock);
-	list_del_init(&dev->node);
+	list_add(&dev->node, &dev_list);
 	spin_unlock(&dev_list_lock);
+
+	return result;
+
  unmap:
 	nvme_dev_unmap(dev);
 	return result;
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] nvme: Cleanup nvme_dev_start()

[Keith Busch]

On Mon, 20 Jan 2014, Alexander Gordeev wrote:
> This update fixes an oddity when a device is first added
> and then removed from dev_list in case of initialization
> failure, instead of just being added in case of success.
>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
> drivers/block/nvme-core.c |   19 ++++++++-----------
> 1 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index e1e4ad4..e4e12be 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -2105,29 +2105,26 @@ static int nvme_dev_start(struct nvme_dev *dev)
> 	if (result)
> 		goto unmap;
>
> -	spin_lock(&dev_list_lock);
> -	list_add(&dev->node, &dev_list);
> -	spin_unlock(&dev_list_lock);
> -
> 	result = set_queue_count(dev, num_online_cpus());
> 	if (result == -EBUSY)

For whatever reason, some of these devices unfortunetly don't support
legacy interrupts. We expect an interrupt when the completion is posted
for setting the queue count, but failing that, we rely on the polling
thread to invoke the completion, so the device needs to be in the dev_list
before calling set_queue_count.

[PATCH v2 6/9] nvme: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/block/nvme-core.c |   33 ++++++++-------------------------
 1 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index e292450..adf26c2 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1774,32 +1774,15 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	/* Deregister the admin queue's interrupt */
 	free_irq(dev->entry[0].vector, dev->queues[0]);
 
-	vecs = nr_io_queues;
-	for (i = 0; i < vecs; i++)
+	for (i = 0; i < nr_io_queues; i++)
 		dev->entry[i].entry = i;
-	for (;;) {
-		result = pci_enable_msix(pdev, dev->entry, vecs);
-		if (result <= 0)
-			break;
-		vecs = result;
-	}
-
-	if (result < 0) {
-		vecs = nr_io_queues;
-		if (vecs > 32)
-			vecs = 32;
-		for (;;) {
-			result = pci_enable_msi_block(pdev, vecs);
-			if (result > 0) {
-				vecs = result;
-				continue;
-			} else if (result < 0) {
-				vecs = 1;
-			}
-			for (i = 0; i < vecs; i++)
-				dev->entry[i].vector = i + pdev->irq;
-			break;
-		}
+	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
+	if (vecs < 0) {
+		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
+		if (vecs < 0)
+			vecs = 1;
+		for (i = 0; i < vecs; i++)
+			dev->entry[i].vector = i + pdev->irq;
 	}
 
 	/*
-- 
1.7.7.6

[PATCH v2 7/9] vfio: Use pci_enable_msi_range() and pci_enable_msix_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

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

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

Re: [PATCH v2 7/9] vfio: Use pci_enable_msi_range() and pci_enable_msix_range()

[Bjorn Helgaas]

On Fri, Jan 17, 2014 at 05:02:21PM +0100, Alexander Gordeev wrote:
> As result deprecation of MSI-X/MSI enablement functions
> pci_enable_msix() and pci_enable_msi_block() all drivers
> using these two interfaces need to be updated to use the
> new pci_enable_msi_range() and pci_enable_msix_range()
> interfaces.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Acked-by: Alex Williamson <alex.williamson@redhat.com>

Applied to my pci/msi branch, thanks!

> ---
>  drivers/vfio/pci/vfio_pci_intrs.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 641bc87..4a9db1d 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -482,15 +482,19 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
>  		for (i = 0; i < nvec; i++)
>  			vdev->msix[i].entry = i;
>  
> -		ret = pci_enable_msix(pdev, vdev->msix, nvec);
> -		if (ret) {
> +		ret = pci_enable_msix_range(pdev, vdev->msix, 1, nvec);
> +		if (ret < nvec) {
> +			if (ret > 0)
> +				pci_disable_msix(pdev);
>  			kfree(vdev->msix);
>  			kfree(vdev->ctx);
>  			return ret;
>  		}
>  	} else {
> -		ret = pci_enable_msi_block(pdev, nvec);
> -		if (ret) {
> +		ret = pci_enable_msi_range(pdev, 1, nvec);
> +		if (ret < nvec) {
> +			if (ret > 0)
> +				pci_disable_msi(pdev);
>  			kfree(vdev->ctx);
>  			return ret;
>  		}
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 8/9] ath10k: Use pci_enable_msi_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

This update also fixes a stylistic (naming and messaging only)
confusion of MSI-X vs multiple MSIs which are not the same.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 9e86a81..873f50c 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2073,14 +2073,14 @@ static void ath10k_pci_tasklet(unsigned long data)
 	}
 }
 
-static int ath10k_pci_start_intr_msix(struct ath10k *ar, int num)
+static int ath10k_pci_start_intr_multi_msi(struct ath10k *ar, int num)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 	int ret;
 	int i;
 
-	ret = pci_enable_msi_block(ar_pci->pdev, num);
-	if (ret)
+	ret = pci_enable_msi_range(ar_pci->pdev, num, num);
+	if (ret < 0)
 		return ret;
 
 	ret = request_irq(ar_pci->pdev->irq + MSI_ASSIGN_FW,
@@ -2111,11 +2111,11 @@ static int ath10k_pci_start_intr_msix(struct ath10k *ar, int num)
 		}
 	}
 
-	ath10k_info("MSI-X interrupt handling (%d intrs)\n", num);
+	ath10k_info("Multi MSI interrupt handling (%d intrs)\n", num);
 	return 0;
 }
 
-static int ath10k_pci_start_intr_msi(struct ath10k *ar)
+static int ath10k_pci_start_intr_single_msi(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 	int ret;
@@ -2132,7 +2132,7 @@ static int ath10k_pci_start_intr_msi(struct ath10k *ar)
 		return ret;
 	}
 
-	ath10k_info("MSI interrupt handling\n");
+	ath10k_info("Single MSI interrupt handling\n");
 	return 0;
 }
 
@@ -2199,20 +2199,20 @@ static int ath10k_pci_start_intr(struct ath10k *ar)
 		num = 1;
 
 	if (num > 1) {
-		ret = ath10k_pci_start_intr_msix(ar, num);
+		ret = ath10k_pci_start_intr_multi_msi(ar, num);
 		if (ret == 0)
 			goto exit;
 
-		ath10k_warn("MSI-X didn't succeed (%d), trying MSI\n", ret);
+		ath10k_warn("Multi MSI failed (%d), trying single MSI\n", ret);
 		num = 1;
 	}
 
 	if (num == 1) {
-		ret = ath10k_pci_start_intr_msi(ar);
+		ret = ath10k_pci_start_intr_single_msi(ar);
 		if (ret == 0)
 			goto exit;
 
-		ath10k_warn("MSI didn't succeed (%d), trying legacy INTR\n",
+		ath10k_warn("Single MSI failed (%d), trying legacy INTR\n",
 			    ret);
 		num = 0;
 	}
-- 
1.7.7.6

[PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

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

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 29fd197..6525e1f 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2414,8 +2414,6 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
 		ret = pci_enable_msi_block(ar_pci->pdev, ar_pci->num_msi_intrs);
 		if (ret == 0)
 			return 0;
-		if (ret > 0)
-			pci_disable_msi(ar_pci->pdev);
 
 		/* fall-through */
 	}
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Alexander Gordeev <agordeev@redhat.com> writes:

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/net/wireless/ath/ath10k/pci.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index 29fd197..6525e1f 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2414,8 +2414,6 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
>  		ret = pci_enable_msi_block(ar_pci->pdev, ar_pci->num_msi_intrs);
>  		if (ret == 0)
>  			return 0;
> -		if (ret > 0)
> -			pci_disable_msi(ar_pci->pdev);

I don't understand how this is superfluous. When I read the
documentation for pci_enable_msi_block() it states that if it can't
allocate all requests, it will return the number requests it could
allocate. And in that case we want to fall back other modes.

Am I missing something?

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

On Tue, Feb 04, 2014 at 08:32:12PM +0200, Kalle Valo wrote:
> Alexander Gordeev <agordeev@redhat.com> writes:
> 
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  drivers/net/wireless/ath/ath10k/pci.c |    2 --
> >  1 files changed, 0 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > index 29fd197..6525e1f 100644
> > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > @@ -2414,8 +2414,6 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
> >  		ret = pci_enable_msi_block(ar_pci->pdev, ar_pci->num_msi_intrs);
> >  		if (ret == 0)
> >  			return 0;
> > -		if (ret > 0)
> > -			pci_disable_msi(ar_pci->pdev);
> 
> I don't understand how this is superfluous. When I read the
> documentation for pci_enable_msi_block() it states that if it can't
> allocate all requests, it will return the number requests it could
> allocate. And in that case we want to fall back other modes.
> 
> Am I missing something?

Yep. The documentation states 'could have been allocated', not 'could
allocate'. IOW, MSIs are *not* enabled if a positive value returned.
The code I changed tries to disable MSIs in such case, although it is
not necessary, nor required. Just superfluous.

HTH.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Alexander Gordeev <agordeev@redhat.com> writes:

> On Tue, Feb 04, 2014 at 08:32:12PM +0200, Kalle Valo wrote:
>> Alexander Gordeev <agordeev@redhat.com> writes:
>> 
>> I don't understand how this is superfluous. When I read the
>> documentation for pci_enable_msi_block() it states that if it can't
>> allocate all requests, it will return the number requests it could
>> allocate. And in that case we want to fall back other modes.
>> 
>> Am I missing something?
>
> Yep. The documentation states 'could have been allocated', not 'could
> allocate'. IOW, MSIs are *not* enabled if a positive value returned.
> The code I changed tries to disable MSIs in such case, although it is
> not necessary, nor required. Just superfluous.

Ah, thanks for explaining that. I added this to the commit log (I hate
empty commit logs anyway):

    ath10k: Get rid of superfluous call to pci_disable_msi()
    
    The documentation states that pci_enable_msi_block() returns the number of
    requests 'could have been allocated', not 'could allocate'. IOW, MSIs are *not*
    enabled if a positive value returned.
    
    kvalo: add commit log based on Alexander's email
    
    Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
    Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>

Is it ok for me to take these patches to my ath.git tree or would you
prefer to route them some other way?

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

On Wed, Feb 05, 2014 at 10:21:28AM +0200, Kalle Valo wrote:
> Is it ok for me to take these patches to my ath.git tree or would you
> prefer to route them some other way?

Yeah, Bjorn has indicated he would pull it to his tree.
I get it you are fine with 2/3 and 3/3?

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Alexander Gordeev <agordeev@redhat.com> writes:

> On Wed, Feb 05, 2014 at 10:21:28AM +0200, Kalle Valo wrote:
>> Is it ok for me to take these patches to my ath.git tree or would you
>> prefer to route them some other way?
>
> Yeah, Bjorn has indicated he would pull it to his tree.

Ok, I'll drop these from my pending branch then. I just think it would
have been easier if I would take these, smaller chance of conflicts that
way.

> I get it you are fine with 2/3 and 3/3?

Yes, with the addition of the commit log to 1/3 I'll give:

Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Bjorn Helgaas]

On Wed, Feb 05, 2014 at 10:54:37AM +0200, Kalle Valo wrote:
> Alexander Gordeev <agordeev@redhat.com> writes:
> 
> > On Wed, Feb 05, 2014 at 10:21:28AM +0200, Kalle Valo wrote:
> >> Is it ok for me to take these patches to my ath.git tree or would you
> >> prefer to route them some other way?
> >
> > Yeah, Bjorn has indicated he would pull it to his tree.
> 
> Ok, I'll drop these from my pending branch then. I just think it would
> have been easier if I would take these, smaller chance of conflicts that
> way.
> 
> > I get it you are fine with 2/3 and 3/3?
> 
> Yes, with the addition of the commit log to 1/3 I'll give:
> 
> Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>

I haven't put these in my branch, so you can take them.

Alexander has a whole batch of network driver updates that I think David
Miller is going to apply; would it make sense to include these in that
batch?

There's also the wil6210 patch for
drivers/net/wireless/ath/wil6210/pcie_bus.c, which seems like it maybe
could be in that batch, too.

Bjorn

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

On Tue, Feb 11, 2014 at 05:31:43PM -0700, Bjorn Helgaas wrote:
> I haven't put these in my branch, so you can take them.
> 
> Alexander has a whole batch of network driver updates that I think David
> Miller is going to apply; would it make sense to include these in that
> batch?
> 
> There's also the wil6210 patch for
> drivers/net/wireless/ath/wil6210/pcie_bus.c, which seems like it maybe
> could be in that batch, too.

Well, as this series is small I thought it could quickly go thru your
tree. But since ipr had conflicts, there is no point routing all patches
altogether, so up to you guys. The wil6210 patch is already in your pci/msi
branch though.

> Bjorn

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Bjorn Helgaas]

On Wed, Feb 12, 2014 at 6:38 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Tue, Feb 11, 2014 at 05:31:43PM -0700, Bjorn Helgaas wrote:
>> I haven't put these in my branch, so you can take them.
>>
>> Alexander has a whole batch of network driver updates that I think David
>> Miller is going to apply; would it make sense to include these in that
>> batch?
>>
>> There's also the wil6210 patch for
>> drivers/net/wireless/ath/wil6210/pcie_bus.c, which seems like it maybe
>> could be in that batch, too.
>
> Well, as this series is small I thought it could quickly go thru your
> tree. But since ipr had conflicts, there is no point routing all patches
> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
> branch though.

It's in pci/msi, but that's not in my -next branch yet, so I can
easily drop it.  Do drivers/net/wireless patches normally follow a
different path than the other drivers/net patches?  The wil6210 and
ath10k patches look just like the others in the 34-patch series (bnx2,
bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
to include them there.

I think I need to put the ahci regression fix in v3.14, so I'll move
that to my for-linus branch and just keep the other odds and ends
(ahci and vfio) for v3.15.

Bjorn

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Bjorn Helgaas <bhelgaas@google.com> writes:

>> Well, as this series is small I thought it could quickly go thru your
>> tree. But since ipr had conflicts, there is no point routing all patches
>> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
>> branch though.
>
> It's in pci/msi, but that's not in my -next branch yet, so I can
> easily drop it.  Do drivers/net/wireless patches normally follow a
> different path than the other drivers/net patches?  The wil6210 and
> ath10k patches look just like the others in the 34-patch series (bnx2,
> bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
> to include them there.

ath10k patches normally go through my ath.git tree to Linville and then
to David Miller. To avoid conflicts I would prefer to take ath10k
patches to my tree whenever possible.

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Bjorn Helgaas]

On Wed, Feb 12, 2014 at 2:30 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
>
>>> Well, as this series is small I thought it could quickly go thru your
>>> tree. But since ipr had conflicts, there is no point routing all patches
>>> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
>>> branch though.
>>
>> It's in pci/msi, but that's not in my -next branch yet, so I can
>> easily drop it.  Do drivers/net/wireless patches normally follow a
>> different path than the other drivers/net patches?  The wil6210 and
>> ath10k patches look just like the others in the 34-patch series (bnx2,
>> bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
>> to include them there.
>
> ath10k patches normally go through my ath.git tree to Linville and then
> to David Miller. To avoid conflicts I would prefer to take ath10k
> patches to my tree whenever possible.

OK, I won't do anything with ath10k (I haven't applied it anywhere).
And if Alexander re-posts the networking series (I think he might, to
add a pci_enable_msix_exact() interface), maybe he can include wil6210
with that series.

Bjorn

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Bjorn Helgaas <bhelgaas@google.com> writes:

> On Wed, Feb 12, 2014 at 2:30 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Bjorn Helgaas <bhelgaas@google.com> writes:
>>
>>>> Well, as this series is small I thought it could quickly go thru your
>>>> tree. But since ipr had conflicts, there is no point routing all patches
>>>> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
>>>> branch though.
>>>
>>> It's in pci/msi, but that's not in my -next branch yet, so I can
>>> easily drop it.  Do drivers/net/wireless patches normally follow a
>>> different path than the other drivers/net patches?  The wil6210 and
>>> ath10k patches look just like the others in the 34-patch series (bnx2,
>>> bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
>>> to include them there.
>>
>> ath10k patches normally go through my ath.git tree to Linville and then
>> to David Miller. To avoid conflicts I would prefer to take ath10k
>> patches to my tree whenever possible.
>
> OK, I won't do anything with ath10k (I haven't applied it anywhere).

I have now taken the ath10k patches to my pending branch, will apply
them soon.

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Alexander Gordeev]

On Wed, Feb 12, 2014 at 11:30:44PM +0200, Kalle Valo wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
> 
> >> Well, as this series is small I thought it could quickly go thru your
> >> tree. But since ipr had conflicts, there is no point routing all patches
> >> altogether, so up to you guys. The wil6210 patch is already in your pci/msi
> >> branch though.
> >
> > It's in pci/msi, but that's not in my -next branch yet, so I can
> > easily drop it.  Do drivers/net/wireless patches normally follow a
> > different path than the other drivers/net patches?  The wil6210 and
> > ath10k patches look just like the others in the 34-patch series (bnx2,
> > bnx2x, tg3, bna, cxgb3, etc.), so I thought it would make more sense
> > to include them there.
> 
> ath10k patches normally go through my ath.git tree to Linville and then
> to David Miller. To avoid conflicts I would prefer to take ath10k
> patches to my tree whenever possible.

CC'ing Vladimir, in case he decides to do the same with wil6210.

> -- 
> Kalle Valo

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/3] ath10k: Get rid of superfluous call to pci_disable_msi()

[Kalle Valo]

Alexander Gordeev <agordeev@redhat.com> writes:

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

Thanks, all three patches applied to my ath.git tree.

-- 
Kalle Valo

[PATCH 2/3] ath10k: Disable MSI in case IRQ configuration is unknown

[Alexander Gordeev]

In case IRQ configuration is unknown possibly enabled MSIs
are left enabled in ath10k_pci_deinit_irq(). This update
fixes the described misbehaviour.

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

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 6525e1f..563ce77 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2480,6 +2480,8 @@ static int ath10k_pci_deinit_irq(struct ath10k *ar)
 	case MSI_NUM_REQUEST:
 		pci_disable_msi(ar_pci->pdev);
 		return 0;
+	default:
+		pci_disable_msi(ar_pci->pdev);
 	}
 
 	ath10k_warn("unknown irq configuration upon deinit\n");
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v3 3/3] ath10k: Use pci_enable_msi_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 563ce77..ad2a6cf 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2411,8 +2411,9 @@ static int ath10k_pci_init_irq(struct ath10k *ar)
 	/* Try MSI-X */
 	if (ath10k_pci_irq_mode == ATH10K_PCI_IRQ_AUTO && msix_supported) {
 		ar_pci->num_msi_intrs = MSI_NUM_REQUEST;
-		ret = pci_enable_msi_block(ar_pci->pdev, ar_pci->num_msi_intrs);
-		if (ret == 0)
+		ret = pci_enable_msi_range(ar_pci->pdev, ar_pci->num_msi_intrs,
+							 ar_pci->num_msi_intrs);
+		if (ret > 0)
 			return 0;
 
 		/* fall-through */
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 8/9] ath10k: Use pci_enable_msi_range()

[Alexander Gordeev]

Hi Kalle,

Could you please review the three updated patches?

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v2 9/9] wil6210: Use pci_enable_msi_range()

[Alexander Gordeev]

As result deprecation of MSI-X/MSI enablement functions
pci_enable_msix() and pci_enable_msi_block() all drivers
using these two interfaces need to be updated to use the
new pci_enable_msi_range() and pci_enable_msix_range()
interfaces.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Acked-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>
---
 drivers/net/wireless/ath/wil6210/pcie_bus.c |   36 ++++++++++++++------------
 1 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index eeceab3..4e1bf54 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -41,30 +41,32 @@ static int wil_if_pcie_enable(struct wil6210_priv *wil)
 	switch (use_msi) {
 	case 3:
 	case 1:
+		wil_dbg_misc(wil, "Setup %d MSI interrupts\n", use_msi);
+		break;
 	case 0:
+		wil_dbg_misc(wil, "MSI interrupts disabled, use INTx\n");
 		break;
 	default:
-		wil_err(wil, "Invalid use_msi=%d, default to 1\n",
-			use_msi);
+		wil_err(wil, "Invalid use_msi=%d, default to 1\n", use_msi);
 		use_msi = 1;
 	}
-	wil->n_msi = use_msi;
-	if (wil->n_msi) {
-		wil_dbg_misc(wil, "Setup %d MSI interrupts\n", use_msi);
-		rc = pci_enable_msi_block(pdev, wil->n_msi);
-		if (rc && (wil->n_msi == 3)) {
-			wil_err(wil, "3 MSI mode failed, try 1 MSI\n");
-			wil->n_msi = 1;
-			rc = pci_enable_msi_block(pdev, wil->n_msi);
-		}
-		if (rc) {
-			wil_err(wil, "pci_enable_msi failed, use INTx\n");
-			wil->n_msi = 0;
-		}
-	} else {
-		wil_dbg_misc(wil, "MSI interrupts disabled, use INTx\n");
+
+	switch (use_msi) {
+	case 3:
+		if (pci_enable_msi_range(pdev, 3, 3) > 0)
+			break;
+		wil_err(wil, "3 MSI mode failed, try 1 MSI\n");
+		use_msi = 1;
+		/* fallthrough */
+	case 1:
+		if (!pci_enable_msi(pdev))
+			break;
+		wil_err(wil, "pci_enable_msi failed, use INTx\n");
+		use_msi = 0;
 	}
 
+	wil->n_msi = use_msi;
+
 	rc = wil6210_init_irq(wil, pdev->irq);
 	if (rc)
 		goto stop_master;
-- 
1.7.7.6

Re: [PATCH v2 9/9] wil6210: Use pci_enable_msi_range()

[Bjorn Helgaas]

On Fri, Jan 17, 2014 at 05:02:23PM +0100, Alexander Gordeev wrote:
> As result deprecation of MSI-X/MSI enablement functions
> pci_enable_msix() and pci_enable_msi_block() all drivers
> using these two interfaces need to be updated to use the
> new pci_enable_msi_range() and pci_enable_msix_range()
> interfaces.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Acked-by: Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>

Applied to my pci/msi branch, thanks!

> ---
>  drivers/net/wireless/ath/wil6210/pcie_bus.c |   36 ++++++++++++++------------
>  1 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
> index eeceab3..4e1bf54 100644
> --- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
> +++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
> @@ -41,30 +41,32 @@ static int wil_if_pcie_enable(struct wil6210_priv *wil)
>  	switch (use_msi) {
>  	case 3:
>  	case 1:
> +		wil_dbg_misc(wil, "Setup %d MSI interrupts\n", use_msi);
> +		break;
>  	case 0:
> +		wil_dbg_misc(wil, "MSI interrupts disabled, use INTx\n");
>  		break;
>  	default:
> -		wil_err(wil, "Invalid use_msi=%d, default to 1\n",
> -			use_msi);
> +		wil_err(wil, "Invalid use_msi=%d, default to 1\n", use_msi);
>  		use_msi = 1;
>  	}
> -	wil->n_msi = use_msi;
> -	if (wil->n_msi) {
> -		wil_dbg_misc(wil, "Setup %d MSI interrupts\n", use_msi);
> -		rc = pci_enable_msi_block(pdev, wil->n_msi);
> -		if (rc && (wil->n_msi == 3)) {
> -			wil_err(wil, "3 MSI mode failed, try 1 MSI\n");
> -			wil->n_msi = 1;
> -			rc = pci_enable_msi_block(pdev, wil->n_msi);
> -		}
> -		if (rc) {
> -			wil_err(wil, "pci_enable_msi failed, use INTx\n");
> -			wil->n_msi = 0;
> -		}
> -	} else {
> -		wil_dbg_misc(wil, "MSI interrupts disabled, use INTx\n");
> +
> +	switch (use_msi) {
> +	case 3:
> +		if (pci_enable_msi_range(pdev, 3, 3) > 0)
> +			break;
> +		wil_err(wil, "3 MSI mode failed, try 1 MSI\n");
> +		use_msi = 1;
> +		/* fallthrough */
> +	case 1:
> +		if (!pci_enable_msi(pdev))
> +			break;
> +		wil_err(wil, "pci_enable_msi failed, use INTx\n");
> +		use_msi = 0;
>  	}
>  
> +	wil->n_msi = use_msi;
> +
>  	rc = wil6210_init_irq(wil, pdev->irq);
>  	if (rc)
>  		goto stop_master;
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] wil6210: Fix switch operator "missing break?" warning

[Alexander Gordeev]

This update fixes a warning introduced with commit bc977ba1
("wil6210: Use pci_enable_msi_range() instead of pci_enable_msi_block()")

drivers/net/wireless/ath/wil6210/pcie_bus.c:65 wil_if_pcie_enable()
warn: missing break? reassigning 'use_msi'

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

diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c
index 4e1bf54..e1c8cc4 100644
--- a/drivers/net/wireless/ath/wil6210/pcie_bus.c
+++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
@@ -51,16 +51,12 @@ static int wil_if_pcie_enable(struct wil6210_priv *wil)
 		use_msi = 1;
 	}
 
-	switch (use_msi) {
-	case 3:
-		if (pci_enable_msi_range(pdev, 3, 3) > 0)
-			break;
+	if (use_msi == 3 && pci_enable_msi_range(pdev, 3, 3) < 0) {
 		wil_err(wil, "3 MSI mode failed, try 1 MSI\n");
 		use_msi = 1;
-		/* fallthrough */
-	case 1:
-		if (!pci_enable_msi(pdev))
-			break;
+	}
+
+	if (use_msi == 1 && pci_enable_msi(pdev)) {
 		wil_err(wil, "pci_enable_msi failed, use INTx\n");
 		use_msi = 0;
 	}
-- 
1.7.7.6

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] wil6210: Fix switch operator "missing break?" warning

[Vladimir Kondratiev]

On Friday, February 07, 2014 03:46:40 PM Alexander Gordeev wrote:
> This update fixes a warning introduced with commit bc977ba1
> ("wil6210: Use pci_enable_msi_range() instead of pci_enable_msi_block()")
> 
> drivers/net/wireless/ath/wil6210/pcie_bus.c:65 wil_if_pcie_enable()
> warn: missing break? reassigning 'use_msi'
> 

I can't reproduce this warning. What tools used to get it?
Neither gcc (I have 4.8.1) nor sparse report it.

Anyway, I am fine with both 'switch' and 'if'.

Thanks, Vladimir.

Re: [PATCH] wil6210: Fix switch operator "missing break?" warning

[Alexander Gordeev]

On Mon, Feb 10, 2014 at 12:54:13PM +0200, Vladimir Kondratiev wrote:
> I can't reproduce this warning. What tools used to get it?

Fengguang?

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] wil6210: Fix switch operator "missing break?" warning

[Fengguang Wu]

Hi Alexander,

On Mon, Feb 10, 2014 at 01:16:38PM +0100, Alexander Gordeev wrote:
> On Mon, Feb 10, 2014 at 12:54:13PM +0200, Vladimir Kondratiev wrote:
> > I can't reproduce this warning. What tools used to get it?
> 
> Fengguang?

This is a smatch warning:

        http://smatch.sourceforge.net/

Thanks,
Fengguang

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Bjorn Helgaas]

On Fri, Jan 17, 2014 at 9:02 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> This series is against "next" branch in Bjorn's repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
>
> Changes from v1 to v2:
>   - added a regression fix "ahci: Fix broken fallback to single
>     MSI mode" as patch 1/9;
>   - the series is reordered to move the regression fix in front;
>   - at Bjorn's request pci_enable_msi() is un-deprecated;
>   - as result, pci_enable_msi_range(pdev, 1, 1) styled calls
>     rolled back to pci_enable_msi(pdev);
>   - nvme bug fix moved out as a separate patch 5/9 "nvme: Fix
>     invalid call to irq_set_affinity_hint()"
>   - patches changelog elaborated a bit;
>
> Bjorn,
>
> As the release is supposedly this weekend, do you prefer
> the patches to go to your tree or to individual trees after
> the release?

I'd be happy to merge them, except for the fact that they probably
wouldn't have any time in -next before I ask Linus to pull them.  So
how about if we wait until after the release, ask the area maintainers
to take them, and if they don't take them, I'll put them in my tree
for v3.15?

Bjorn

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Alexander Gordeev]

On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
> > As the release is supposedly this weekend, do you prefer
> > the patches to go to your tree or to individual trees after
> > the release?
> 
> I'd be happy to merge them, except for the fact that they probably
> wouldn't have any time in -next before I ask Linus to pull them.  So
> how about if we wait until after the release, ask the area maintainers
> to take them, and if they don't take them, I'll put them in my tree
> for v3.15?

Patch 11 depends on patches 1-10, so I am not sure how to better handle it.
Whatever works for you ;)

I am only concerned with a regression fix "ahci: Fix broken fallback to
single MSI mode" which would be nice to have in 3.14. But it seems pretty
much too late.

> Bjorn

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Bjorn Helgaas]

On Sat, Jan 18, 2014 at 12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
>> > As the release is supposedly this weekend, do you prefer
>> > the patches to go to your tree or to individual trees after
>> > the release?
>>
>> I'd be happy to merge them, except for the fact that they probably
>> wouldn't have any time in -next before I ask Linus to pull them.  So
>> how about if we wait until after the release, ask the area maintainers
>> to take them, and if they don't take them, I'll put them in my tree
>> for v3.15?
>
> Patch 11 depends on patches 1-10, so I am not sure how to better handle it.
> Whatever works for you ;)
>
> I am only concerned with a regression fix "ahci: Fix broken fallback to
> single MSI mode" which would be nice to have in 3.14. But it seems pretty
> much too late.

Tejun, if you want to ack that one, I can put it in either the first
3.14 pull request or a subsequent one.  Either way, since it's a
regression fix, we should be able to get it in 3.14.

Bjorn

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Tejun Heo]

On Sat, Jan 18, 2014 at 07:38:55AM -0700, Bjorn Helgaas wrote:
> On Sat, Jan 18, 2014 at 12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> > On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
> >> > As the release is supposedly this weekend, do you prefer
> >> > the patches to go to your tree or to individual trees after
> >> > the release?
> >>
> >> I'd be happy to merge them, except for the fact that they probably
> >> wouldn't have any time in -next before I ask Linus to pull them.  So
> >> how about if we wait until after the release, ask the area maintainers
> >> to take them, and if they don't take them, I'll put them in my tree
> >> for v3.15?
> >
> > Patch 11 depends on patches 1-10, so I am not sure how to better handle it.
> > Whatever works for you ;)
> >
> > I am only concerned with a regression fix "ahci: Fix broken fallback to
> > single MSI mode" which would be nice to have in 3.14. But it seems pretty
> > much too late.
> 
> Tejun, if you want to ack that one, I can put it in either the first
> 3.14 pull request or a subsequent one.  Either way, since it's a
> regression fix, we should be able to get it in 3.14.

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

Please feel free to route it any way you see fit.

Thanks!

-- 
tejun

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Bjorn Helgaas]

On Sat, Jan 18, 2014 at 09:59:40AM -0500, Tejun Heo wrote:
> On Sat, Jan 18, 2014 at 07:38:55AM -0700, Bjorn Helgaas wrote:
> > On Sat, Jan 18, 2014 at 12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> > > On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
> > >> > As the release is supposedly this weekend, do you prefer
> > >> > the patches to go to your tree or to individual trees after
> > >> > the release?
> > >>
> > >> I'd be happy to merge them, except for the fact that they probably
> > >> wouldn't have any time in -next before I ask Linus to pull them.  So
> > >> how about if we wait until after the release, ask the area maintainers
> > >> to take them, and if they don't take them, I'll put them in my tree
> > >> for v3.15?
> > >
> > > Patch 11 depends on patches 1-10, so I am not sure how to better handle it.
> > > Whatever works for you ;)
> > >
> > > I am only concerned with a regression fix "ahci: Fix broken fallback to
> > > single MSI mode" which would be nice to have in 3.14. But it seems pretty
> > > much too late.
> > 
> > Tejun, if you want to ack that one, I can put it in either the first
> > 3.14 pull request or a subsequent one.  Either way, since it's a
> > regression fix, we should be able to get it in 3.14.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Please feel free to route it any way you see fit.

I applied the following to my pci/msi branch, since they had acks from
maintainers (Tejun, I assumed your ack applies to both ahci patches):

    ahci: Fix broken fallback to single MSI mode
    ahci: Use pci_enable_msi_range()
    vfio: Use pci_enable_msi_range() and pci_enable_msix_range()
    wil6210: Use pci_enable_msi_range()

I didn't do anything with these:

    ipr: Get rid of superfluous call to pci_disable_msi/msix()
    ipr: Use pci_enable_msi_range() and pci_enable_msix_range()

	The conflict with "ipr: Handle early EEH" needs to get resolved
	first.  Either Alexander's patches need to go via the same tree as
	the EEH change, or the EEH change needs to be in some published
	tree so I can cherry-pick it.

    nvme: Fix invalid call to irq_set_affinity_hint()
    nvme: Use pci_enable_msi_range() and pci_enable_msix_range()

	These don't seem fully baked yet.  If/when Keith acks them, I (or
	he) can merge them.

    ath10k: Use pci_enable_msi_range()

	This has been acked, but no longer applies to mainline (I'm
	currently at 0e47c969c65e).

Bjorn

Re: [PATCH v2 0/9] Phase out pci_enable_msi_block()

[Alexander Gordeev]

On Fri, Jan 17, 2014 at 02:00:32PM -0700, Bjorn Helgaas wrote:
> > Bjorn,
> >
> > As the release is supposedly this weekend, do you prefer
> > the patches to go to your tree or to individual trees after
> > the release?
> 
> I'd be happy to merge them, except for the fact that they probably
> wouldn't have any time in -next before I ask Linus to pull them.  So
> how about if we wait until after the release, ask the area maintainers
> to take them, and if they don't take them, I'll put them in my tree
> for v3.15?

Hi Gentleman,

As the prerequisite commit 302a252 ("PCI/MSI: Add pci_enable_msi_range()
and pci_enable_msix_range()") is in mainline now, could you please take
the ACKed patches to your trees?

Thanks!

> Bjorn

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com