[PATCHv2 0/5] NVMe: Controller reset and shutdown

[PATCHv2 0/5] NVMe: Controller reset and shutdown

[Keith Busch]

More controller reset handling. Fixes a couple errors and adds more
conditions that have come up that require this kind of thing.

Again, it relies on this patch set here:
http://merlin.infradead.org/pipermail/linux-nvme/2013-July/000331.html

Keith Busch (5):
  NVMe: Reset failed controller
  NVMe: User initiated controller reset
  NVMe: Reset controller on timed out commands
  NVMe: Don't wait for delete queues to finish
  NVMe: Add shutdown pci callback

 drivers/block/nvme-core.c |   90 +++++++++++++++++++++++++++++++++++++++------
 include/linux/nvme.h      |    1 +
 2 files changed, 79 insertions(+), 12 deletions(-)

-- 
1.7.10.4

[PATCHv2 1/5] NVMe: Reset failed controller

[Keith Busch]

Polls on the controller fatal status bit and resets the controller per
the nvme spec on this condition.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
v1->v2:

Fixed clean-up on module unload to delete the work queue.

I have a question on this: should we use the predefined kernel work
queue instead of making our own? The shutdown sequence can block for a
while on which is why I have a workqueue_struct for the module.

 drivers/block/nvme-core.c |   31 ++++++++++++++++++++++++++++++-
 include/linux/nvme.h      |    1 +
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 608b0a7..5713dd2 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
@@ -1605,6 +1606,12 @@ 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) {
+				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)
@@ -2151,6 +2158,19 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	return result;
 }
 
+static void nvme_dev_reset(struct nvme_dev *dev)
+{
+	nvme_dev_shutdown(dev);
+	if (nvme_dev_start(dev))
+		nvme_free_queues(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;
@@ -2178,6 +2198,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)
 		goto release_pools;
@@ -2288,9 +2309,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;
 
@@ -2301,6 +2327,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;
@@ -2310,6 +2338,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..612e640 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -87,6 +87,7 @@ struct nvme_dev {
 	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.10.4

[PATCHv2 2/5] 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 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>
---
v1->v2:

Fix cleanup on device_create_file failure.

 drivers/block/nvme-core.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 5713dd2..c0f2533 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2171,6 +2171,17 @@ 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;
@@ -2207,6 +2218,10 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (result)
 		goto shutdown;
 
+	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;
@@ -2214,11 +2229,13 @@ 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);
 	return 0;
 
+ del_sysfs:
+	device_remove_file(&pdev->dev, &dev_attr_reset_controller);
  remove:
 	nvme_dev_remove(dev);
  shutdown:
@@ -2238,6 +2255,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.10.4

[PATCHv2 3/5] NVMe: Reset controller on timed out commands

[Keith Busch]

This fixes the race between the controller and the timeout handler. Timing
out the command previously called the completion handler with a failure
status and the completion handler frees the command's target memory. If
the controller is still active, it may use this memory for dma, which can
be bad. This patch makes a timed out command trigger a controller reset,
which will shut down the controller prior to freeing memory associated
with outstanding commands.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
I know we should send an abort command prior to going to the big
hammer. That gets complicated quickly though, so I just want to submit
something that should fix the race condition first, then tackle the
abort handling.

 drivers/block/nvme-core.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index c0f2533..c07a507 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1010,7 +1010,7 @@ int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11,
  * @queue: The queue to cancel I/Os on
  * @timeout: True to only cancel I/Os which have timed out
  */
-static void nvme_cancel_ios(struct nvme_queue *nvmeq, bool timeout)
+static int nvme_cancel_ios(struct nvme_queue *nvmeq, bool timeout)
 {
 	int depth = nvmeq->q_depth - 1;
 	struct nvme_cmd_info *info = nvme_cmd_info(nvmeq);
@@ -1028,10 +1028,14 @@ static void nvme_cancel_ios(struct nvme_queue *nvmeq, bool timeout)
 			continue;
 		if (info[cmdid].ctx == CMD_CTX_CANCELLED)
 			continue;
+		if (timeout)
+			return 1;
 		dev_warn(nvmeq->q_dmadev, "Cancelling I/O %d\n", cmdid);
 		ctx = cancel_cmdid(nvmeq, cmdid, &fn);
 		fn(nvmeq->dev, ctx, &cqe);
 	}
+
+	return 0;
 }
 
 static void nvme_free_queue(struct nvme_queue *nvmeq)
@@ -1620,7 +1624,12 @@ static int nvme_kthread(void *data)
 				if (nvmeq->q_suspended)
 					goto unlock;
 				nvme_process_cq(nvmeq);
-				nvme_cancel_ios(nvmeq, true);
+				if (nvme_cancel_ios(nvmeq, true)) {
+					dev_warn(&dev->pci_dev->dev,
+						"command time out, reset controller\n");
+					queue_work(nvme_workq, &dev->ws);
+					goto unlock;
+				}
 				nvme_resubmit_bios(nvmeq);
  unlock:
 				spin_unlock_irq(&nvmeq->q_lock);
-- 
1.7.10.4

[PATCHv2 4/5] NVMe: Don't wait for delete queues to finish

[Keith Busch]

If a controller is unresponsive, the shutdown sequence was held
up waiting on the admin command to timeout when trying to delete IO
queues. The driver previously would wait on the timeout for each queue
created making the shutdown take a long time if the controller isn't
working. This patch will have the controller wait once, then skip
sending the delete queue commands if it ever failed.

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

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index c07a507..cb64866 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1065,7 +1065,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;
@@ -1073,7 +1073,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);
@@ -1082,15 +1082,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) || adapter_delete_cq(dev, qid))
+			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,
@@ -1873,8 +1875,9 @@ 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) {
+			int del_q = 1;
 			for (--i; i > 0; i--)
-				nvme_disable_queue(dev, i);
+				del_q = nvme_disable_queue(dev, i, del_q);
 			goto free_queues;
 		}
 	}
@@ -2010,10 +2013,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.10.4

[PATCHv2 5/5] NVMe: Add shutdown pci callback

[Keith Busch]

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
A couple vendors at FMS as well as a few firmware engineers I spoke with
highly recommended we do a safe shutdown sequence for their devices.

 drivers/block/nvme-core.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index cb64866..84bb905 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2272,6 +2272,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
@@ -2325,6 +2331,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.10.4