[PATCH 0/9] NVMe: Error handling

[PATCH 0/9] NVMe: Error handling

[Keith Busch]

These are a bunch of patches to handle errors and replaces this set:

http://merlin.infradead.org/pipermail/linux-nvme/2013-August/000354.html

Keith Busch (9):
  NVMe: Merge issue on character device bring-up
  NVMe: Differentiate commands not completed
  NVMe: Fail device if unresponsive during init
  NVMe: Reset failed controller
  NVMe: Abort timed out commands
  NVMe: User initiated controller reset
  NVMe: Add shutdown pci callback
  NVMe: Set queue db only when queue is initialized
  NVMe: Don't wait for delete queues to complete

 drivers/block/nvme-core.c |  237 ++++++++++++++++++++++++++++++++++++--------
 include/linux/nvme.h      |    3 +
 include/uapi/linux/nvme.h |   11 ++
 3 files changed, 208 insertions(+), 43 deletions(-)

[PATCH 1/9] NVMe: Merge issue on character device bring-up

[Keith Busch]

A recent patch made it possible to bring up the character handle when the
device is responsive but not accepting a set-features command. Another
recent patch moved the initialization that requires we move where the
checks for this condition occur. This patch merges these two ideas so
it works much as before.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 9f2b424..da52092 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2135,10 +2135,10 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	spin_unlock(&dev_list_lock);
 
 	result = nvme_setup_io_queues(dev);
-	if (result)
+	if (result && result != -EBUSY)
 		goto disable;
 
-	return 0;
+	return result;
 
  disable:
 	spin_lock(&dev_list_lock);
@@ -2177,13 +2177,17 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto release;
 
 	result = nvme_dev_start(dev);
-	if (result)
+	if (result) {
+		if (result == -EBUSY)
+			goto create_cdev;
 		goto release_pools;
+	}
 
 	result = nvme_dev_add(dev);
-	if (result && result != -EBUSY)
+	if (result)
 		goto shutdown;
 
+ create_cdev:
 	scnprintf(dev->name, sizeof(dev->name), "nvme%d", dev->instance);
 	dev->miscdev.minor = MISC_DYNAMIC_MINOR;
 	dev->miscdev.parent = &pdev->dev;
-- 
1.7.0.4

[PATCH 1/9] NVMe: Merge issue on character device bring-up

[John Utz]

What will happen when you go to create_cdev: if the character device already exists? 

Harmless? Or Not?

Just curious.

Tnx!

johnu

-----Original Message-----
From: Linux-nvme [mailto:linux-nvme-bounces@lists.infradead.org] On Behalf Of Keith Busch
Sent: Thursday, September 5, 2013 1:45 PM
To: linux-nvme at lists.infradead.org
Cc: Keith Busch
Subject: [PATCH 1/9] NVMe: Merge issue on character device bring-up

A recent patch made it possible to bring up the character handle when the device is responsive but not accepting a set-features command. Another recent patch moved the initialization that requires we move where the checks for this condition occur. This patch merges these two ideas so it works much as before.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c index 9f2b424..da52092 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2135,10 +2135,10 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	spin_unlock(&dev_list_lock);
 
 	result = nvme_setup_io_queues(dev);
-	if (result)
+	if (result && result != -EBUSY)
 		goto disable;
 
-	return 0;
+	return result;
 
  disable:
 	spin_lock(&dev_list_lock);
@@ -2177,13 +2177,17 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto release;
 
 	result = nvme_dev_start(dev);
-	if (result)
+	if (result) {
+		if (result == -EBUSY)
+			goto create_cdev;
 		goto release_pools;
+	}
 
 	result = nvme_dev_add(dev);
-	if (result && result != -EBUSY)
+	if (result)
 		goto shutdown;
 
+ create_cdev:
 	scnprintf(dev->name, sizeof(dev->name), "nvme%d", dev->instance);
 	dev->miscdev.minor = MISC_DYNAMIC_MINOR;
 	dev->miscdev.parent = &pdev->dev;
--
1.7.0.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/9] NVMe: Merge issue on character device bring-up

[Keith Busch]

On Thu, 5 Sep 2013, John Utz wrote:
> What will happen when you go to create_cdev: if the character device already exists?
>
> Harmless? Or Not?
>
> Just curious.
>
> Tnx!
>
> johnu

Hi John,

The character device is created at the end of this pci_driver's 'probe'
routine and deleted on 'remove', so I don't think it is possible for
the character device to exist prior to the 'goto', right?

Thanks,
Keith

[PATCH 1/9] NVMe: Merge issue on character device bring-up

[John Utz]

OH HECK. I am on outlook, so I have no quoting mechanism. I gotta come up with a different MUA that speaks exchange.....

Grrrr.....

Comments crudely inserted inline

-----Original Message-----
From: Keith Busch [mailto:keith.busch@intel.com] 
Sent: Thursday, September 5, 2013 3:21 PM
To: John Utz
Cc: Keith Busch; linux-nvme at lists.infradead.org
Subject: RE: [PATCH 1/9] NVMe: Merge issue on character device bring-up

On Thu, 5 Sep 2013, John Utz wrote:
> What will happen when you go to create_cdev: if the character device already exists?
>
> Harmless? Or Not?
>
> Just curious.
>
> Tnx!
>
> johnu

Hi John,

The character device is created at the end of this pci_driver's 'probe'
routine and deleted on 'remove', so I don't think it is possible for the character device to exist prior to the 'goto', right?

JLUIII-> If the character device has been created and then the driver dies prior to the remove executing, you would be faced with an existing chardev when you restarted the driver.

Thanks,
Keith

[PATCH 1/9] NVMe: Merge issue on character device bring-up

[Keith Busch]

On Thu, 5 Sep 2013, John Utz wrote:
> OH HECK. I am on outlook, so I have no quoting mechanism. I gotta come up with a different MUA that speaks exchange.....
>
> Grrrr.....
>
> Comments crudely inserted inline

Sometimes I have no choice but to use outlook so I feel your pain. :)

Depending on which version you're using, there should be something like
"Options" -> "Mail", and you set "When replying to a message" with
"Prefix each line of original message with". I set that option to '>'
like how my linux email client does replies.

> On Thu, 5 Sep 2013, John Utz wrote:
>> What will happen when you go to create_cdev: if the character device already exists?
>>
>> Harmless? Or Not?
>>
>> Just curious.
>>
>> Tnx!
>>
>> johnu
>
> Hi John,
>
> The character device is created at the end of this pci_driver's 'probe'
> routine and deleted on 'remove', so I don't think it is possible for the
> character device to exist prior to the 'goto', right?
>
> JLUIII-> If the character device has been created and then the driver dies
> prior to the remove executing, you would be faced with an existing chardev
> when you restarted the driver.

I'm not sure how to make that kind of situation happen without creating
a more catastrophic problem. Can a pci driver be unloaded without all
the devices the driver claims be removed and still have a sane system?
The nvme_dev structure encapsulating the misc_dev it is associated with no
longer exists, right? Accessing the old character device handle probably
causes a page fault or worse.

Anyway, I tried to force the situation by skipping the misc_deregister
in the 'remove' function; it's not a good idea to leave the misc_dev up
and then reload the driver. You'll get "sysfs: cannot create duplicate
filename" and "kobject_add_internal failed with -EEXIST" warnings. Opening
the character device causes a "BUG: unable to handle kernel paging
request at ..." in this scenario.

[PATCH 2/9] NVMe: Differentiate commands not completed

[Keith Busch]

If we interally abort a command the device has not returned to us, do not
create a fake completetion queue entry so the callback can know the device
is not responsive. The only real use of this is the sync completion which
may use the return status to know if the device should be used or not.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index da52092..8b22068 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -181,13 +181,15 @@ static void special_completion(struct nvme_dev *dev, void *ctx,
 	if (ctx == CMD_CTX_FLUSH)
 		return;
 	if (ctx == CMD_CTX_COMPLETED) {
-		dev_warn(&dev->pci_dev->dev,
+		if (cqe)
+			dev_warn(&dev->pci_dev->dev,
 				"completed id %d twice on queue %d\n",
 				cqe->command_id, le16_to_cpup(&cqe->sq_id));
 		return;
 	}
 	if (ctx == CMD_CTX_INVALID) {
-		dev_warn(&dev->pci_dev->dev,
+		if (cqe)
+			dev_warn(&dev->pci_dev->dev,
 				"invalid id %d completed on queue %d\n",
 				cqe->command_id, le16_to_cpup(&cqe->sq_id));
 		return;
@@ -346,7 +348,7 @@ static void bio_completion(struct nvme_dev *dev, void *ctx,
 {
 	struct nvme_iod *iod = ctx;
 	struct bio *bio = iod->private;
-	u16 status = le16_to_cpup(&cqe->status) >> 1;
+	u16 status = cqe ? le16_to_cpup(&cqe->status) >> 1 : -EIO;
 
 	if (iod->nents) {
 		dma_unmap_sg(&dev->pci_dev->dev, iod->sg, iod->nents,
@@ -847,8 +849,10 @@ static void sync_completion(struct nvme_dev *dev, void *ctx,
 						struct nvme_completion *cqe)
 {
 	struct sync_cmd_info *cmdinfo = ctx;
-	cmdinfo->result = le32_to_cpup(&cqe->result);
-	cmdinfo->status = le16_to_cpup(&cqe->status) >> 1;
+	if (cqe) {
+		cmdinfo->result = le32_to_cpup(&cqe->result);
+		cmdinfo->status = le16_to_cpup(&cqe->status) >> 1;
+	}
 	wake_up_process(cmdinfo->task);
 }
 
@@ -1016,9 +1020,6 @@ static void nvme_cancel_ios(struct nvme_queue *nvmeq, bool timeout)
 	for_each_set_bit(cmdid, nvmeq->cmdid_data, depth) {
 		void *ctx;
 		nvme_completion_fn fn;
-		static struct nvme_completion cqe = {
-			.status = cpu_to_le16(NVME_SC_ABORT_REQ << 1),
-		};
 
 		if (timeout && !time_after(now, info[cmdid].timeout))
 			continue;
@@ -1026,7 +1027,7 @@ static void nvme_cancel_ios(struct nvme_queue *nvmeq, bool timeout)
 			continue;
 		dev_warn(nvmeq->q_dmadev, "Cancelling I/O %d\n", cmdid);
 		ctx = cancel_cmdid(nvmeq, cmdid, &fn);
-		fn(nvmeq->dev, ctx, &cqe);
+		fn(nvmeq->dev, ctx, NULL);
 	}
 }
 
-- 
1.7.0.4

[PATCH 3/9] NVMe: Fail device if unresponsive during init

[Keith Busch]

This handles identifying namespace errors differently depending on
the error. If the controller is unresponsive during this point of
initialization, the namespaces are freed and the pci probe is failed.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
Yes, part of this undoes a patch I submitted removing 'dead code',
but that code wasn't reachable before! :)

 drivers/block/nvme-core.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 8b22068..db15c3d 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1879,7 +1879,7 @@ static int nvme_dev_add(struct nvme_dev *dev)
 {
 	int res;
 	unsigned nn, i;
-	struct nvme_ns *ns;
+	struct nvme_ns *ns, *next;
 	struct nvme_id_ctrl *ctrl;
 	struct nvme_id_ns *id_ns;
 	void *mem;
@@ -1912,16 +1912,22 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	id_ns = mem;
 	for (i = 1; i <= nn; i++) {
 		res = nvme_identify(dev, i, 0, dma_addr);
-		if (res)
+		if (res) {
+			if (res < 0)
+				goto out_free;
 			continue;
+		}
 
 		if (id_ns->ncap == 0)
 			continue;
 
 		res = nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
 							dma_addr + 4096, NULL);
-		if (res)
+		if (res) {
+			if (res < 0)
+				goto out_free;
 			memset(mem + 4096, 0, 4096);
+		}
 
 		ns = nvme_alloc_ns(dev, i, mem, mem + 4096);
 		if (ns)
@@ -1930,7 +1936,13 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	list_for_each_entry(ns, &dev->namespaces, list)
 		add_disk(ns->disk);
 	res = 0;
+	goto out;
 
+ out_free:
+	list_for_each_entry_safe(ns, next, &dev->namespaces, list) {
+		list_del(&ns->list);
+		nvme_ns_free(ns);
+	}
  out:
 	dma_free_coherent(&dev->pci_dev->dev, 8192, mem, dma_addr);
 	return res;
-- 
1.7.0.4

[PATCH 3/9] NVMe: Fail device if unresponsive during init

[Matthew Wilcox]

On Thu, Sep 05, 2013@02:45:09PM -0600, Keith Busch wrote:
> This handles identifying namespace errors differently depending on
> the error. If the controller is unresponsive during this point of
> initialization, the namespaces are freed and the pci probe is failed.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>  		res = nvme_identify(dev, i, 0, dma_addr);
> -		if (res)
> +		if (res) {
> +			if (res < 0)
> +				goto out_free;
>  			continue;
> +		}

Feels a little klunky.  How about:

		res = nvme_identify(dev, i, 0, dma_addr);
-		if (res)
+		if (res < 0)
+			goto out_free;
+		else if (res)
			continue;

>  		res = nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
>  							dma_addr + 4096, NULL);
> -		if (res)
> +		if (res) {
> +			if (res < 0)
> +				goto out_free;
>  			memset(mem + 4096, 0, 4096);
> +		}

I don't know if we need to do this.  Consider a hypothetical device that
has a broken get_features, but everything else works fine.  We can still
use that device just fine.  Contrariwise, if the device happens to break
between issuing the identify and get_features, we'll still error out
really soon afterwards.

> @@ -1930,7 +1936,13 @@ static int nvme_dev_add(struct nvme_dev *dev)
>  	list_for_each_entry(ns, &dev->namespaces, list)
>  		add_disk(ns->disk);
>  	res = 0;
> +	goto out;
>  
> + out_free:
> +	list_for_each_entry_safe(ns, next, &dev->namespaces, list) {
> +		list_del(&ns->list);
> +		nvme_ns_free(ns);
> +	}
>   out:
>  	dma_free_coherent(&dev->pci_dev->dev, 8192, mem, dma_addr);
>  	return res;

I tend to prefer the error path flow to look like this:

	res = 0;
 out:
	dma_free_coherent(&dev->pci_dev->dev, 8192, mem, dma_addr);
	return res;
 out_free:
	list_for_each_entry_safe(ns, next, &dev->namespaces, list) {
		list_del(&ns->list);
		nvme_ns_free(ns);
	}
	goto out;
}

[PATCH 3/9] NVMe: Fail device if unresponsive during init

[Keith Busch]

On Thu, 19 Sep 2013, Matthew Wilcox wrote:
> On Thu, Sep 05, 2013@02:45:09PM -0600, Keith Busch wrote:
>> -		if (res)
>> +		if (res) {
>> +			if (res < 0)
>> +				goto out_free;
>>  			continue;
>> +		}
> Feels a little klunky.  How about:
>
> 		res = nvme_identify(dev, i, 0, dma_addr);
> -		if (res)
> +		if (res < 0)
> +			goto out_free;
> +		else if (res)
> 			continue;

Aha, that is much more pleasent.

>>  		res = nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
>>  							dma_addr + 4096, NULL);
>> -		if (res)
>> +		if (res) {
>> +			if (res < 0)
>> +				goto out_free;
>>  			memset(mem + 4096, 0, 4096);
>> +		}
>
> I don't know if we need to do this.  Consider a hypothetical device that
> has a broken get_features, but everything else works fine.  We can still
> use that device just fine.  Contrariwise, if the device happens to break
> between issuing the identify and get_features, we'll still error out
> really soon afterwards.

The fault is really on the hardware in such a scenario (get features
support is mandatory), but I'm not sure we want to do what you're
suggesting. It will take 1 minute per namespace before completing probe so
a user could be waiting a very long time for a driver to load. This would
also leak command id's and we only have 64 of them; if the device has
more than 64 namespaces, modprobe blocks forever on alloc_cmdid_killable
until a user kills the task which may not be possible if this is happening
on boot.

[PATCH 4/9] NVMe: Reset failed controller

[Keith Busch]

Polls on the controller fatal status bit and resets the controller per
the nvme spec on this condition. If the device probe has not completed,
commands may be timed out in the previous way as resetting the controller
would cause the probe to fail, which will conflict with the the work
task that is resetting the controller.

If the controller fails to start after attempting to reset it, the pci
driver will be removed since the device would appear to be dead. I think
that would work on a surprise removal where the driver's remove function
isn't automatically called.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   67 +++++++++++++++++++++++++++++++++++++++------
 include/linux/nvme.h      |    2 +
 2 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index db15c3d..18bb04e 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -58,6 +58,7 @@ module_param(use_threaded_interrupts, int, 0);
 static DEFINE_SPINLOCK(dev_list_lock);
 static LIST_HEAD(dev_list);
 static struct task_struct *nvme_thread;
+static struct workqueue_struct *nvme_workq;
 
 /*
  * An NVM Express queue.  Each device has at least two (one for admin
@@ -1604,6 +1605,14 @@ static int nvme_kthread(void *data)
 		spin_lock(&dev_list_lock);
 		list_for_each_entry(dev, &dev_list, node) {
 			int i;
+			if (readl(&dev->bar->csts) & NVME_CSTS_CFS) {
+				if (dev->is_initialised) {
+					dev_warn(&dev->pci_dev->dev,
+						"failed status, reset controller\n");
+					queue_work(nvme_workq, &dev->ws);
+					continue;
+				}
+			}
 			for (i = 0; i < dev->queue_count; i++) {
 				struct nvme_queue *nvmeq = dev->queues[i];
 				if (!nvmeq)
@@ -1996,9 +2005,8 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
 	if (dev->bar) {
 		iounmap(dev->bar);
 		dev->bar = NULL;
+		pci_release_regions(dev->pci_dev);
 	}
-
-	pci_release_regions(dev->pci_dev);
 	if (pci_is_enabled(dev->pci_dev))
 		pci_disable_device(dev->pci_dev);
 }
@@ -2162,6 +2170,41 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	return result;
 }
 
+static int nvme_remove_dead_ctrl(void *arg)
+{
+	struct nvme_dev *dev = (struct nvme_dev *)arg;
+	struct pci_dev *pdev;
+
+	if ((dev == NULL))
+		return -1;
+
+	pdev = dev->pci_dev;
+	if ((pdev == NULL))
+		return -1;
+	pci_stop_and_remove_bus_device(pdev);
+	return 0;
+}
+
+static void nvme_dev_resume(struct nvme_dev *dev)
+{
+	int ret = nvme_dev_start(dev);
+	if (ret)
+		kthread_run(nvme_remove_dead_ctrl, dev,
+					"nvme%d", dev->instance);
+}
+
+static void nvme_dev_reset(struct nvme_dev *dev)
+{
+	nvme_dev_shutdown(dev);
+	nvme_dev_resume(dev);
+}
+
+static void nvme_reset_failed_dev(struct work_struct *ws)
+{
+	struct nvme_dev *dev = container_of(ws, struct nvme_dev, ws);
+	nvme_dev_reset(dev);
+}
+
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	int result = -ENOMEM;
@@ -2189,6 +2232,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (result)
 		goto release;
 
+	INIT_WORK(&dev->ws, nvme_reset_failed_dev);
 	result = nvme_dev_start(dev);
 	if (result) {
 		if (result == -EBUSY)
@@ -2211,6 +2255,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto remove;
 
 	kref_init(&dev->kref);
+	dev->is_initialised = 1;
 	return 0;
 
  remove:
@@ -2256,13 +2301,9 @@ static int nvme_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct nvme_dev *ndev = pci_get_drvdata(pdev);
-	int ret;
 
-	ret = nvme_dev_start(ndev);
-	/* XXX: should remove gendisks if resume fails */
-	if (ret)
-		nvme_free_queues(ndev);
-	return ret;
+	nvme_dev_resume(ndev);
+	return 0;
 }
 
 static SIMPLE_DEV_PM_OPS(nvme_dev_pm_ops, nvme_suspend, nvme_resume);
@@ -2303,9 +2344,14 @@ static int __init nvme_init(void)
 	if (IS_ERR(nvme_thread))
 		return PTR_ERR(nvme_thread);
 
+	result = -ENOMEM;
+	nvme_workq = create_workqueue("nvme");
+	if (!nvme_workq)
+		goto kill_kthread;
+
 	result = register_blkdev(nvme_major, "nvme");
 	if (result < 0)
-		goto kill_kthread;
+		goto kill_workq;
 	else if (result > 0)
 		nvme_major = result;
 
@@ -2316,6 +2362,8 @@ static int __init nvme_init(void)
 
  unregister_blkdev:
 	unregister_blkdev(nvme_major, "nvme");
+ kill_workq:
+	destroy_workqueue(nvme_workq);
  kill_kthread:
 	kthread_stop(nvme_thread);
 	return result;
@@ -2325,6 +2373,7 @@ static void __exit nvme_exit(void)
 {
 	pci_unregister_driver(&nvme_driver);
 	unregister_blkdev(nvme_major, "nvme");
+	destroy_workqueue(nvme_workq);
 	kthread_stop(nvme_thread);
 }
 
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 26ebcf4..a25bba2 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -81,12 +81,14 @@ struct nvme_dev {
 	int instance;
 	int queue_count;
 	int db_stride;
+	int is_initialised;
 	u32 ctrl_config;
 	struct msix_entry *entry;
 	struct nvme_bar __iomem *bar;
 	struct list_head namespaces;
 	struct kref kref;
 	struct miscdevice miscdev;
+	struct work_struct ws;
 	char name[12];
 	char serial[20];
 	char model[40];
-- 
1.7.0.4

[PATCH 5/9] NVMe: Abort timed out commands

[Keith Busch]

Send an abort request if the command times out. If the command times
out yet again, or if it is an admin command that is timed out, schedule
the controller to be reset. If the device hasn't completed probe, cancel
the command as normal.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   64 ++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/nvme.h      |    1 +
 include/uapi/linux/nvme.h |   11 +++++++
 3 files changed, 75 insertions(+), 1 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 18bb04e..e179614 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -81,6 +81,7 @@ struct nvme_queue {
 	u16 sq_head;
 	u16 sq_tail;
 	u16 cq_head;
+	u16 qid;
 	u8 cq_phase;
 	u8 cqe_seen;
 	u8 q_suspended;
@@ -98,6 +99,7 @@ static inline void _nvme_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_delete_queue) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_features) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_format_cmd) != 64);
+	BUILD_BUG_ON(sizeof(struct nvme_abort_cmd) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl) != 4096);
 	BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
@@ -112,6 +114,7 @@ struct nvme_cmd_info {
 	nvme_completion_fn fn;
 	void *ctx;
 	unsigned long timeout;
+	int aborted;
 };
 
 static struct nvme_cmd_info *nvme_cmd_info(struct nvme_queue *nvmeq)
@@ -155,6 +158,7 @@ static int alloc_cmdid(struct nvme_queue *nvmeq, void *ctx,
 	info[cmdid].fn = handler;
 	info[cmdid].ctx = ctx;
 	info[cmdid].timeout = jiffies + timeout;
+	info[cmdid].aborted = 0;
 	return cmdid;
 }
 
@@ -173,6 +177,7 @@ static int alloc_cmdid_killable(struct nvme_queue *nvmeq, void *ctx,
 #define CMD_CTX_COMPLETED	(0x310 + CMD_CTX_BASE)
 #define CMD_CTX_INVALID		(0x314 + CMD_CTX_BASE)
 #define CMD_CTX_FLUSH		(0x318 + CMD_CTX_BASE)
+#define CMD_CTX_ABORT		(0x31C + CMD_CTX_BASE)
 
 static void special_completion(struct nvme_dev *dev, void *ctx,
 						struct nvme_completion *cqe)
@@ -181,6 +186,10 @@ static void special_completion(struct nvme_dev *dev, void *ctx,
 		return;
 	if (ctx == CMD_CTX_FLUSH)
 		return;
+	if (ctx == CMD_CTX_ABORT) {
+		++dev->abort_limit;
+		return;
+	}
 	if (ctx == CMD_CTX_COMPLETED) {
 		if (cqe)
 			dev_warn(&dev->pci_dev->dev,
@@ -1007,6 +1016,52 @@ int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11,
 }
 
 /**
+ * nvme_abort_cmd - Attempt aborting a command
+ * @cmdid: Command id of a timed out IO
+ * @queue: The queue with timed out IO
+ *
+ * Schedule controller reset if the command was already aborted once before and
+ * still hasn't been returned to the driver, or if this is the admin queue.
+ */
+static void nvme_abort_cmd(int cmdid, struct nvme_queue *nvmeq)
+{
+	int a_cmdid;
+	struct nvme_command cmd;
+	struct nvme_dev *dev = nvmeq->dev;
+	struct nvme_cmd_info *info = nvme_cmd_info(nvmeq);
+
+	if (!nvmeq->qid || info[cmdid].aborted) {
+		dev_warn(&dev->pci_dev->dev,
+			"I/O:%d queue:%d timeout, reset controller\n", cmdid,
+								nvmeq->qid);
+		queue_work(nvme_workq, &dev->ws);
+		return;
+	}
+
+	if (!dev->abort_limit)
+		return;
+
+	a_cmdid = alloc_cmdid(dev->queues[0], CMD_CTX_ABORT, special_completion,
+								ADMIN_TIMEOUT);
+	if (a_cmdid < 0)
+		return;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.abort.opcode = nvme_admin_abort_cmd;
+	cmd.abort.cid = cmdid;
+	cmd.abort.sqid = nvmeq->qid;
+	cmd.abort.command_id = a_cmdid;
+
+	--dev->abort_limit;
+	info[cmdid].aborted = 1;
+	info[cmdid].timeout = jiffies + ADMIN_TIMEOUT;
+
+	dev_warn(nvmeq->q_dmadev, "Aborting I/O %d QID %d\n", cmdid,
+							nvmeq->qid);
+	nvme_submit_cmd(dev->queues[0], &cmd);
+}
+
+/**
  * nvme_cancel_ios - Cancel outstanding I/Os
  * @queue: The queue to cancel I/Os on
  * @timeout: True to only cancel I/Os which have timed out
@@ -1026,7 +1081,12 @@ static void nvme_cancel_ios(struct nvme_queue *nvmeq, bool timeout)
 			continue;
 		if (info[cmdid].ctx == CMD_CTX_CANCELLED)
 			continue;
-		dev_warn(nvmeq->q_dmadev, "Cancelling I/O %d\n", cmdid);
+		if (timeout && nvmeq->dev->is_initialised) {
+			nvme_abort_cmd(cmdid, nvmeq);
+			continue;
+		}
+		dev_warn(nvmeq->q_dmadev, "Cancelling I/O %d QID:%d\n", cmdid,
+								nvmeq->qid);
 		ctx = cancel_cmdid(nvmeq, cmdid, &fn);
 		fn(nvmeq->dev, ctx, NULL);
 	}
@@ -1118,6 +1178,7 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
 	nvmeq->q_db = &dev->dbs[qid << (dev->db_stride + 1)];
 	nvmeq->q_depth = depth;
 	nvmeq->cq_vector = vector;
+	nvmeq->qid = qid;
 	nvmeq->q_suspended = 1;
 	dev->queue_count++;
 
@@ -1909,6 +1970,7 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	ctrl = mem;
 	nn = le32_to_cpup(&ctrl->nn);
 	dev->oncs = le16_to_cpup(&ctrl->oncs);
+	dev->abort_limit = ctrl->acl + 1;
 	memcpy(dev->serial, ctrl->sn, sizeof(ctrl->sn));
 	memcpy(dev->model, ctrl->mn, sizeof(ctrl->mn));
 	memcpy(dev->firmware_rev, ctrl->fr, sizeof(ctrl->fr));
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index a25bba2..a5d169b 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -96,6 +96,7 @@ struct nvme_dev {
 	u32 max_hw_sectors;
 	u32 stripe_size;
 	u16 oncs;
+	u16 abort_limit;
 };
 
 /*
diff --git a/include/uapi/linux/nvme.h b/include/uapi/linux/nvme.h
index 989c04e..3423b52 100644
--- a/include/uapi/linux/nvme.h
+++ b/include/uapi/linux/nvme.h
@@ -350,6 +350,16 @@ struct nvme_delete_queue {
 	__u32			rsvd11[5];
 };
 
+struct nvme_abort_cmd {
+	__u8			opcode;
+	__u8			flags;
+	__u16			command_id;
+	__u32			rsvd1[9];
+	__le16			sqid;
+	__le16			cid;
+	__u32			rsvd11[5];
+};
+
 struct nvme_download_firmware {
 	__u8			opcode;
 	__u8			flags;
@@ -381,6 +391,7 @@ struct nvme_command {
 		struct nvme_create_cq create_cq;
 		struct nvme_create_sq create_sq;
 		struct nvme_delete_queue delete_queue;
+		struct nvme_abort_cmd abort;
 		struct nvme_download_firmware dlfw;
 		struct nvme_format_cmd format;
 		struct nvme_dsm_cmd dsm;
-- 
1.7.0.4

[PATCH 6/9] NVMe: User initiated controller reset

[Keith Busch]

Creates a sysfs entry for each nvme controller that when written to
initiates a controller reset. This may be done by a root user if they
need to reset the controller for any reason. For example, it may be
required as part of an firmware activate procedure.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index e179614..c400c0a 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2267,6 +2267,16 @@ static void nvme_reset_failed_dev(struct work_struct *ws)
 	nvme_dev_reset(dev);
 }
 
+static ssize_t nvme_reset(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct pci_dev  *pdev = container_of(dev, struct pci_dev, dev);
+	struct nvme_dev *ndev = pci_get_drvdata(pdev);
+	nvme_dev_reset(ndev);
+	return count;
+}
+static DEVICE_ATTR(reset_controller, S_IWUSR, NULL, nvme_reset);
+
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	int result = -ENOMEM;
@@ -2307,6 +2317,10 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto shutdown;
 
  create_cdev:
+	result = device_create_file(&pdev->dev, &dev_attr_reset_controller);
+	if (result)
+		goto remove;
+
 	scnprintf(dev->name, sizeof(dev->name), "nvme%d", dev->instance);
 	dev->miscdev.minor = MISC_DYNAMIC_MINOR;
 	dev->miscdev.parent = &pdev->dev;
@@ -2314,12 +2328,14 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	dev->miscdev.fops = &nvme_dev_fops;
 	result = misc_register(&dev->miscdev);
 	if (result)
-		goto remove;
+		goto del_sysfs;
 
 	kref_init(&dev->kref);
 	dev->is_initialised = 1;
 	return 0;
 
+ del_sysfs:
+	device_remove_file(&pdev->dev, &dev_attr_reset_controller);
  remove:
 	nvme_dev_remove(dev);
  shutdown:
@@ -2339,6 +2355,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 static void nvme_remove(struct pci_dev *pdev)
 {
 	struct nvme_dev *dev = pci_get_drvdata(pdev);
+	device_remove_file(&pdev->dev, &dev_attr_reset_controller);
 	misc_deregister(&dev->miscdev);
 	kref_put(&dev->kref, nvme_free_dev);
 }
-- 
1.7.0.4

[PATCH 7/9] NVMe: Add shutdown pci callback

[Keith Busch]

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index c400c0a..d09ef43 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2360,6 +2360,12 @@ static void nvme_remove(struct pci_dev *pdev)
 	kref_put(&dev->kref, nvme_free_dev);
 }
 
+static void nvme_shutdown(struct pci_dev *pdev)
+{
+	struct nvme_dev *dev = pci_get_drvdata(pdev);
+	nvme_dev_shutdown(dev);
+}
+
 /* These functions are yet to be implemented */
 #define nvme_error_detected NULL
 #define nvme_dump_registers NULL
@@ -2409,6 +2415,7 @@ static struct pci_driver nvme_driver = {
 	.id_table	= nvme_id_table,
 	.probe		= nvme_probe,
 	.remove		= nvme_remove,
+	.shutdown	= nvme_shutdown,
 	.driver		= {
 		.pm	= &nvme_dev_pm_ops,
 	},
-- 
1.7.0.4

[PATCH 8/9] NVMe: Set queue db only when queue is initialized

[Keith Busch]

There is no need to set it when the queue is allocated as it can't be
used until it is created in the controller. We may initialized a queue
multiple times, but allocate it only once, so we have to set the queue
doorbell address each time it is initialized.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index d09ef43..2c99e17 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1175,7 +1175,6 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
 	init_waitqueue_head(&nvmeq->sq_full);
 	init_waitqueue_entry(&nvmeq->sq_cong_wait, nvme_thread);
 	bio_list_init(&nvmeq->sq_cong);
-	nvmeq->q_db = &dev->dbs[qid << (dev->db_stride + 1)];
 	nvmeq->q_depth = depth;
 	nvmeq->cq_vector = vector;
 	nvmeq->qid = qid;
-- 
1.7.0.4

[PATCH 9/9] NVMe: Don't wait for delete queues to complete

[Keith Busch]

Skip sending the delete queue commands if the controller is unresponsive
so the driver does not hold up a shutdown sequence. Previously it would
take 2 minutes per IO queue on a broken device.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 2c99e17..5ee9f61 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -908,17 +908,13 @@ int nvme_submit_admin_cmd(struct nvme_dev *dev, struct nvme_command *cmd,
 
 static int adapter_delete_queue(struct nvme_dev *dev, u8 opcode, u16 id)
 {
-	int status;
 	struct nvme_command c;
 
 	memset(&c, 0, sizeof(c));
 	c.delete_queue.opcode = opcode;
 	c.delete_queue.qid = cpu_to_le16(id);
 
-	status = nvme_submit_admin_cmd(dev, &c, NULL);
-	if (status)
-		return -EIO;
-	return 0;
+	return nvme_submit_admin_cmd(dev, &c, NULL);
 }
 
 static int adapter_alloc_cq(struct nvme_dev *dev, u16 qid,
@@ -1119,7 +1115,7 @@ static void nvme_free_queues(struct nvme_dev *dev)
 	}
 }
 
-static void nvme_disable_queue(struct nvme_dev *dev, int qid)
+static int nvme_disable_queue(struct nvme_dev *dev, int qid, int del_q)
 {
 	struct nvme_queue *nvmeq = dev->queues[qid];
 	int vector = dev->entry[nvmeq->cq_vector].vector;
@@ -1127,7 +1123,7 @@ static void nvme_disable_queue(struct nvme_dev *dev, int qid)
 	spin_lock_irq(&nvmeq->q_lock);
 	if (nvmeq->q_suspended) {
 		spin_unlock_irq(&nvmeq->q_lock);
-		return;
+		return del_q;
 	}
 	nvmeq->q_suspended = 1;
 	spin_unlock_irq(&nvmeq->q_lock);
@@ -1136,15 +1132,17 @@ static void nvme_disable_queue(struct nvme_dev *dev, int qid)
 	free_irq(vector, nvmeq);
 
 	/* Don't tell the adapter to delete the admin queue */
-	if (qid) {
-		adapter_delete_sq(dev, qid);
-		adapter_delete_cq(dev, qid);
-	}
+	if (qid && del_q)
+		if (adapter_delete_sq(dev, qid) < 0 ||
+					adapter_delete_cq(dev, qid) < 0)
+			del_q = 0;
 
 	spin_lock_irq(&nvmeq->q_lock);
 	nvme_process_cq(nvmeq);
 	nvme_cancel_ios(nvmeq, false);
 	spin_unlock_irq(&nvmeq->q_lock);
+
+	return del_q;
 }
 
 static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
@@ -1925,8 +1923,10 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	for (i = 1; i < dev->queue_count; i++) {
 		result = nvme_create_queue(dev->queues[i], i);
 		if (result) {
-			for (--i; i > 0; i--)
-				nvme_disable_queue(dev, i);
+			int del_q = 1;
+
+			for (--i; i >= 0; i--)
+				del_q = nvme_disable_queue(dev, i, del_q);
 			goto free_queues;
 		}
 	}
@@ -2074,10 +2074,10 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
 
 static void nvme_dev_shutdown(struct nvme_dev *dev)
 {
-	int i;
+	int i, del_q = 1;
 
 	for (i = dev->queue_count - 1; i >= 0; i--)
-		nvme_disable_queue(dev, i);
+		del_q = nvme_disable_queue(dev, i, del_q);
 
 	spin_lock(&dev_list_lock);
 	list_del_init(&dev->node);
-- 
1.7.0.4