[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 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 at 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

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

[Bjorn Helgaas]

On Fri, Jan 17, 2014@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 at 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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@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 at 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;
>>                 }
>>         }

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

[Alexander Gordeev]

On Fri, Jan 17, 2014@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 at 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 at 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 at redhat.com

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

[Alexander Gordeev]

On Mon, Jan 20, 2014@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 at redhat.com

[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 at redhat.com>

Reviewed-by: Keith Busch <keith.busch at 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 at 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 at redhat.com

[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 at 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);
> --

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

[Alexander Gordeev]

On Mon, Jan 20, 2014@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 at 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 at 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 at 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 at redhat.com

[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 at 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 at 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 at redhat.com

[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 at 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 at 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 0/9] Phase out pci_enable_msi_block()

[Bjorn Helgaas]

On Fri, Jan 17, 2014@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

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

[Alexander Gordeev]

On Fri, Jan 17, 2014@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 at redhat.com

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

[Bjorn Helgaas]

On Sat, Jan 18, 2014@12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Fri, Jan 17, 2014@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

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

[Tejun Heo]

On Sat, Jan 18, 2014@07:38:55AM -0700, Bjorn Helgaas wrote:
> On Sat, Jan 18, 2014@12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> > On Fri, Jan 17, 2014@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 at kernel.org>

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

Thanks!

-- 
tejun

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

[Bjorn Helgaas]

On Sat, Jan 18, 2014@09:59:40AM -0500, Tejun Heo wrote:
> On Sat, Jan 18, 2014@07:38:55AM -0700, Bjorn Helgaas wrote:
> > On Sat, Jan 18, 2014@12:15 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> > > On Fri, Jan 17, 2014@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 at 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

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

[Alexander Gordeev]

On Fri, Jan 17, 2014@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 at redhat.com