NVMe: Controller reset and shutdown

PATCH Superseded Keith Busch 1 participant 6 messages Aug 16, 2013

Subsystems (2)

Block layer NVMe

lore

Share & tools

lore

Keith Busch

author

2013-08-16 22:00 UTC #0 lore raw

v2 cover NVMe: Controller reset and shutdown

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(-)

Signature · 1 line

1.7.10.4

Keith Busch

author

2013-08-16 22:00 UTC #1 lore raw

PATCH v2 1/5 NVMe: Reset failed controller

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(-)

drivers/block/nvme-core.c +30 -1

index 608b0a7..5713dd2 100644
@@ -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);
 }

include/linux/nvme.h +1 -0

index 26ebcf4..612e640 100644
@@ -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];

Signature · 1 line

1.7.10.4

Keith Busch

author

2013-08-16 22:00 UTC #2 lore raw

PATCH v2 2/5 NVMe: User initiated controller reset

In reply to 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(-)

drivers/block/nvme-core.c +19 -1

index 5713dd2..c0f2533 100644
@@ -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);
 }

Signature · 1 line

1.7.10.4

Keith Busch

author

2013-08-16 22:00 UTC #3 lore raw

PATCH v2 3/5 NVMe: Reset controller on timed out commands

In reply to 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(-)

drivers/block/nvme-core.c +11 -2

index c0f2533..c07a507 100644
@@ -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);

Signature · 1 line

1.7.10.4

Keith Busch

author

2013-08-16 22:00 UTC #4 lore raw

PATCH v2 4/5 NVMe: Don't wait for delete queues to finish

In reply to 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(-)

drivers/block/nvme-core.c +11 -8

index c07a507..cb64866 100644
@@ -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);

Signature · 1 line

1.7.10.4

Keith Busch

author

2013-08-16 22:00 UTC #5 lore raw

PATCH v2 5/5 NVMe: Add shutdown pci callback

In reply to 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(+)

drivers/block/nvme-core.c +7 -0

index cb64866..84bb905 100644
@@ -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,
 	},

Signature · 1 line

1.7.10.4