proper controller state machine and more common code

PATCH New Christoph Hellwig 3 participants 19 messages Apr 18, 2016 – Apr 20, 2016 NVMe

Share & tools

Christoph Hellwig

author maintainer

2016-04-18 12:34 UTC #0 lore raw

This series introduces a controller state machine based on code that Sagi did for the Fabrics driver, and moves the namespace scanning and AER code to nvme-core now that we have that state machine.

Christoph Hellwig

author maintainer

2016-04-18 12:34 UTC #1 lore raw

PATCH 1/5 nvme: remove the io_incapable method

It's unused since "NVMe: Move error handling to failed reset handler".

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/nvme.h | 12 ------------
 drivers/nvme/host/pci.c  |  8 --------
 2 files changed, 20 deletions(-)

drivers/nvme/host/nvme.h +0 ~~-12~~

index 8e8fae8..c6b32c5 100644
@@ -136,7 +136,6 @@ struct nvme_ctrl_ops {
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
 	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
-	bool (*io_incapable)(struct nvme_ctrl *ctrl);
 	int (*reset_ctrl)(struct nvme_ctrl *ctrl);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 };

@@ -150,17 +149,6 @@ static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl)
 	return val & NVME_CSTS_RDY;
 }
 
-static inline bool nvme_io_incapable(struct nvme_ctrl *ctrl)
-{
-	u32 val = 0;
-
-	if (ctrl->ops->io_incapable(ctrl))
-		return true;
-	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &val))
-		return true;
-	return val & NVME_CSTS_CFS;
-}
-
 static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
 {
 	if (!ctrl->subsystem)

drivers/nvme/host/pci.c +0 -8

index ff3c8d7..23998c7 100644
@@ -1941,13 +1941,6 @@ static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
 	return 0;
 }
 
-static bool nvme_pci_io_incapable(struct nvme_ctrl *ctrl)
-{
-	struct nvme_dev *dev = to_nvme_dev(ctrl);
-
-	return !dev->bar || dev->online_queues < 2;
-}
-
 static int nvme_pci_reset_ctrl(struct nvme_ctrl *ctrl)
 {
 	return nvme_reset(to_nvme_dev(ctrl));

@@ -1958,7 +1951,6 @@ static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.reg_read32		= nvme_pci_reg_read32,
 	.reg_write32		= nvme_pci_reg_write32,
 	.reg_read64		= nvme_pci_reg_read64,
-	.io_incapable		= nvme_pci_io_incapable,
 	.reset_ctrl		= nvme_pci_reset_ctrl,
 	.free_ctrl		= nvme_pci_free_ctrl,
 };

Signature · 1 line

2.1.4

Christoph Hellwig

author maintainer

2016-04-18 12:34 UTC #2 lore raw

PATCH 2/5 nvme: introduce a controller state machine

In reply to Christoph Hellwig

Replace the adhoc flags in the PCI driver with a state machine in the core code. Based on code from Sagi Grimberg for the Fabrics driver.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/core.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h | 11 +++++++++++
 drivers/nvme/host/pci.c  | 25 ++++++++++++------------
 3 files changed, 74 insertions(+), 13 deletions(-)

drivers/nvme/host/core.c +51 -0

index 4eb5759..c334fe4 100644
@@ -58,6 +58,55 @@ static DEFINE_SPINLOCK(dev_list_lock);
 
 static struct class *nvme_class;
 
+bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
+		enum nvme_ctrl_state new_state)
+{
+	enum nvme_ctrl_state old_state = ctrl->state;
+	bool changed = false;
+
+	spin_lock_irq(&ctrl->lock);
+	switch (new_state) {
+	case NVME_CTRL_LIVE:
+		switch (old_state) {
+		case NVME_CTRL_RESETTING:
+			changed = true;
+			/* FALLTHRU */
+		default:
+			break;
+		}
+		break;
+	case NVME_CTRL_RESETTING:
+		switch (old_state) {
+		case NVME_CTRL_NEW:
+		case NVME_CTRL_LIVE:
+			changed = true;
+			/* FALLTHRU */
+		default:
+			break;
+		}
+		break;
+	case NVME_CTRL_DELETING:
+		switch (old_state) {
+		case NVME_CTRL_LIVE:
+		case NVME_CTRL_RESETTING:
+			changed = true;
+			/* FALLTHRU */
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	spin_unlock_irq(&ctrl->lock);
+
+	if (changed)
+		ctrl->state = new_state;
+
+	return changed;
+}
+EXPORT_SYMBOL_GPL(nvme_change_ctrl_state);
+
 static void nvme_free_ns(struct kref *kref)
 {
 	struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);

@@ -1583,6 +1632,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 {
 	int ret;
 
+	ctrl->state = NVME_CTRL_NEW;
+	spin_lock_init(&ctrl->lock);
 	INIT_LIST_HEAD(&ctrl->namespaces);
 	mutex_init(&ctrl->namespaces_mutex);
 	kref_init(&ctrl->kref);

drivers/nvme/host/nvme.h +11 -0

index c6b32c5..2ea9f47 100644
@@ -67,7 +67,16 @@ enum nvme_quirks {
 	NVME_QUIRK_DISCARD_ZEROES		= (1 << 2),
 };
 
+enum nvme_ctrl_state {
+	NVME_CTRL_NEW,
+	NVME_CTRL_LIVE,
+	NVME_CTRL_RESETTING,
+	NVME_CTRL_DELETING,
+};
+
 struct nvme_ctrl {
+	enum nvme_ctrl_state state;
+	spinlock_t lock;
 	const struct nvme_ctrl_ops *ops;
 	struct request_queue *admin_q;
 	struct device *dev;

@@ -187,6 +196,8 @@ static inline bool nvme_req_needs_retry(struct request *req, u16 status)
 		(jiffies - req->start_time) < req->timeout;
 }
 
+bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
+		enum nvme_ctrl_state new_state);
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);

drivers/nvme/host/pci.c +12 ~~-13~~

index 23998c7..b922608 100644
@@ -102,11 +102,6 @@ struct nvme_dev {
 	dma_addr_t cmb_dma_addr;
 	u64 cmb_size;
 	u32 cmbsz;
-	unsigned long flags;
-
-#define NVME_CTRL_RESETTING    0
-#define NVME_CTRL_REMOVING     1
-
 	struct nvme_ctrl ctrl;
 	struct completion ioq_wait;
 };

@@ -277,9 +272,8 @@ static void nvme_queue_scan(struct nvme_dev *dev)
 	 * Do not queue new scan work when a controller is reset during
 	 * removal.
 	 */
-	if (test_bit(NVME_CTRL_REMOVING, &dev->flags))
-		return;
-	queue_work(nvme_workq, &dev->scan_work);
+	if (dev->ctrl.state != NVME_CTRL_DELETING)
+		queue_work(nvme_workq, &dev->scan_work);
 }
 
 static void nvme_complete_async_event(struct nvme_dev *dev,

@@ -901,7 +895,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	 * cancellation error. All outstanding requests are completed on
 	 * shutdown, so we return BLK_EH_HANDLED.
 	 */
-	if (test_bit(NVME_CTRL_RESETTING, &dev->flags)) {
+	if (dev->ctrl.state == NVME_CTRL_RESETTING) {
 		dev_warn(dev->ctrl.device,
 			 "I/O %d QID %d timeout, disable controller\n",
 			 req->tag, nvmeq->qid);

@@ -1835,7 +1829,7 @@ static void nvme_reset_work(struct work_struct *work)
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, reset_work);
 	int result = -ENODEV;
 
-	if (WARN_ON(test_bit(NVME_CTRL_RESETTING, &dev->flags)))
+	if (WARN_ON(dev->ctrl.state == NVME_CTRL_RESETTING))
 		goto out;
 
 	/*

@@ -1845,7 +1839,8 @@ static void nvme_reset_work(struct work_struct *work)
 	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
 		nvme_dev_disable(dev, false);
 
-	set_bit(NVME_CTRL_RESETTING, &dev->flags);
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
+		goto out;
 
 	result = nvme_pci_enable(dev);
 	if (result)

@@ -1893,7 +1888,10 @@ static void nvme_reset_work(struct work_struct *work)
 		nvme_dev_add(dev);
 	}
 
-	clear_bit(NVME_CTRL_RESETTING, &dev->flags);
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_LIVE)) {
+		dev_warn(dev->ctrl.device, "failed to mark controller live\n");
+		goto out;
+	}
 	return;
 
  out:

@@ -2066,7 +2064,8 @@ static void nvme_remove(struct pci_dev *pdev)
 
 	del_timer_sync(&dev->watchdog_timer);
 
-	set_bit(NVME_CTRL_REMOVING, &dev->flags);
+	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
+
 	pci_set_drvdata(pdev, NULL);
 	flush_work(&dev->async_work);
 	flush_work(&dev->scan_work);

Signature · 1 line

2.1.4

Sagi Grimberg

maintainer

2016-04-18 18:36 UTC #3 lore raw

PATCH 2/5 nvme: introduce a controller state machine

drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 1897

Quoted 13 lines

drivers/nvme/host/pci.c +4 -1

@@ -1893,7 +1888,10 @@ static void nvme_reset_work(struct work_struct *work)
  		nvme_dev_add(dev);
  	}
 
-	clear_bit(NVME_CTRL_RESETTING, &dev->flags);
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_LIVE)) {
+		dev_warn(dev->ctrl.device, "failed to mark controller live\n");
+		goto out;
+	}
  	return;
 
   out:

You're missing a state change before removing a
dead controller...

Christoph Hellwig

author maintainer

2016-04-18 20:17 UTC #4 lore raw

PATCH 2/5 nvme: introduce a controller state machine

Quoted 11 lines

drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 1897
Quoted 8 lines
drivers/nvme/host/pci.c +4 -1
-	clear_bit(NVME_CTRL_RESETTING, &dev->flags);
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_LIVE)) {
+		dev_warn(dev->ctrl.device, "failed to mark controller live\n");
+		goto out;
+	}
  	return;
 
   out:
You're missing a state change before removing a
dead controller...

why?

Sagi Grimberg

maintainer

2016-04-18 20:30 UTC #5 lore raw

PATCH 2/5 nvme: introduce a controller state machine

Quoted 8 lines

drivers/nvme/host/pci.c +4 -1

-	clear_bit(NVME_CTRL_RESETTING, &dev->flags);
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_LIVE)) {
+		dev_warn(dev->ctrl.device, "failed to mark controller live\n");
+		goto out;
+	}
   	return;

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
out:
You're missing a state change before removing a
dead controller...
why?

Just to have the state consistent with what the driver is doing. Not a must though...

Christoph Hellwig

author maintainer

2016-04-19 18:25 UTC #6 lore raw

PATCH 2/5 nvme: introduce a controller state machine

Quoted 13 lines · Sagi Grimberg

Quoted 13 lines
Quoted 12 lines
drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 1897
Quoted 9 lines
drivers/nvme/host/pci.c +4 -1
-	clear_bit(NVME_CTRL_RESETTING, &dev->flags);
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_LIVE)) {
+		dev_warn(dev->ctrl.device, "failed to mark controller live\n");
+		goto out;
+	}
   	return;
 
    out:
You're missing a state change before removing a
dead controller...

Quoted 4 lines

Quoted 1 line
why?
Just to have the state consistent with what the driver is doing. Not a must though...

Let's allocate states on an as needed basis and avoid that new state for now.

Sagi Grimberg

maintainer

2016-04-19 18:35 UTC #7 lore raw

PATCH 2/5 nvme: introduce a controller state machine

Quoted 13 lines

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 1897
Quoted 9 lines
drivers/nvme/host/pci.c +4 -1
-	clear_bit(NVME_CTRL_RESETTING, &dev->flags);
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_LIVE)) {
+		dev_warn(dev->ctrl.device, "failed to mark controller live\n");
+		goto out;
+	}
    	return;
 
     out:
You're missing a state change before removing a
dead controller...

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
why?
Just to have the state consistent with what the driver is doing. Not a must though...
Let's allocate states on an as needed basis and avoid that new state for now.

Didn't mean a new state, just a transition to NVME_CTRL_DELETING if nvme_reset_work fails...

Christoph Hellwig

author maintainer

2016-04-19 18:37 UTC #8 lore raw

PATCH 2/5 nvme: introduce a controller state machine

Quoted 5 lines · Sagi Grimberg

Quoted 2 lines
Let's allocate states on an as needed basis and avoid that new state for now.
Didn't mean a new state, just a transition to NVME_CTRL_DELETING if nvme_reset_work fails...

Oh, we can't actually do that - for PCIe deleting is only set for calls from the PCI ->remove callback, which we'll invoke a little later through the driver model from the workqueue we just kicked off.

Sagi Grimberg

maintainer

2016-04-20 17:25 UTC #9 lore raw

PATCH 2/5 nvme: introduce a controller state machine

Quoted 11 lines

Quoted 6 lines
Quoted 3 lines
Let's allocate states on an as needed basis and avoid that new state for now.
Didn't mean a new state, just a transition to NVME_CTRL_DELETING if nvme_reset_work fails...
Oh, we can't actually do that - for PCIe deleting is only set for calls from the PCI ->remove callback, which we'll invoke a little later through the driver model from the workqueue we just kicked off.

Fair enough,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Christoph Hellwig

author maintainer

2016-04-18 12:34 UTC #10 lore raw

PATCH 3/5 nvme: tighten up state check for namespace scanning

In reply to Christoph Hellwig

We only should be scanning namespaces if the controller is live. Currently we call the function just before setting it live, so fix the code up to move the call to nvme_queue_scan to just below the state change.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/pci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/nvme/host/pci.c +4 -2

index b922608..f98d77f 100644
@@ -272,7 +272,7 @@ static void nvme_queue_scan(struct nvme_dev *dev)
 	 * Do not queue new scan work when a controller is reset during
 	 * removal.
 	 */
-	if (dev->ctrl.state != NVME_CTRL_DELETING)
+	if (dev->ctrl.state == NVME_CTRL_LIVE)
 		queue_work(nvme_workq, &dev->scan_work);
 }

@@ -1659,7 +1659,6 @@ static int nvme_dev_add(struct nvme_dev *dev)
 		nvme_free_queues(dev, dev->online_queues);
 	}
 
-	nvme_queue_scan(dev);
 	return 0;
 }

@@ -1892,6 +1891,9 @@ static void nvme_reset_work(struct work_struct *work)
 		dev_warn(dev->ctrl.device, "failed to mark controller live\n");
 		goto out;
 	}
+
+	if (dev->online_queues > 1)
+		nvme_queue_scan(dev);
 	return;
 
  out:

Signature · 1 line

2.1.4

Sagi Grimberg

maintainer

2016-04-18 18:40 UTC #11 lore raw

PATCH 3/5 nvme: tighten up state check for namespace scanning

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Christoph Hellwig

author maintainer

2016-04-18 12:34 UTC #12 lore raw

PATCH 4/5 nvme: move namespace scanning to core

In reply to Christoph Hellwig

Move the scan work item and surrounding code to the common code. For now we need a new finish_scan method to allow the PCI driver to set the irq affinity hints, but I have plans in the works to obsolete this as well.

Note that this moves the namespace scanning from nvme_wq to the system workqueue, but as we don't rely on namespace scanning to finish from reset or I/O this should be fine.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/core.c | 30 ++++++++++++++++++++++++++----
 drivers/nvme/host/nvme.h |  4 +++-
 drivers/nvme/host/pci.c  | 32 +++++---------------------------
 3 files changed, 34 insertions(+), 32 deletions(-)

drivers/nvme/host/core.c +26 -4

index c334fe4..606008d 100644
@@ -1518,7 +1518,7 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl, unsigned nn)
 	return ret;
 }
 
-static void __nvme_scan_namespaces(struct nvme_ctrl *ctrl, unsigned nn)
+static void nvme_scan_ns_sequential(struct nvme_ctrl *ctrl, unsigned nn)
 {
 	struct nvme_ns *ns, *next;
 	unsigned i;

@@ -1534,11 +1534,16 @@ static void __nvme_scan_namespaces(struct nvme_ctrl *ctrl, unsigned nn)
 	}
 }
 
-void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
+static void nvme_scan_work(struct work_struct *work)
 {
+	struct nvme_ctrl *ctrl =
+		container_of(work, struct nvme_ctrl, scan_work);
 	struct nvme_id_ctrl *id;
 	unsigned nn;
 
+	if (ctrl->state != NVME_CTRL_LIVE)
+		return;
+
 	if (nvme_identify_ctrl(ctrl, &id))
 		return;

@@ -1549,13 +1554,26 @@ void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
 		if (!nvme_scan_ns_list(ctrl, nn))
 			goto done;
 	}
-	__nvme_scan_namespaces(ctrl, le32_to_cpup(&id->nn));
+	nvme_scan_ns_sequential(ctrl, nn);
  done:
 	list_sort(NULL, &ctrl->namespaces, ns_cmp);
 	mutex_unlock(&ctrl->namespaces_mutex);
 	kfree(id);
+
+	if (ctrl->ops->finish_scan)
+		ctrl->ops->finish_scan(ctrl);
 }
-EXPORT_SYMBOL_GPL(nvme_scan_namespaces);
+
+void nvme_queue_scan(struct nvme_ctrl *ctrl)
+{
+	/*
+	 * Do not queue new scan work when a controller is reset during
+	 * removal.
+	 */
+	if (ctrl->state == NVME_CTRL_LIVE)
+		schedule_work(&ctrl->scan_work);
+}
+EXPORT_SYMBOL_GPL(nvme_queue_scan);
 
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 {

@@ -1597,6 +1615,9 @@ static void nvme_release_instance(struct nvme_ctrl *ctrl)
 
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
+	flush_work(&ctrl->scan_work);
+	nvme_remove_namespaces(ctrl);
+
 	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
 
 	spin_lock(&dev_list_lock);

@@ -1640,6 +1661,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->dev = dev;
 	ctrl->ops = ops;
 	ctrl->quirks = quirks;
+	INIT_WORK(&ctrl->scan_work, nvme_scan_work);
 
 	ret = nvme_set_instance(ctrl);
 	if (ret)

drivers/nvme/host/nvme.h +3 -1

index 2ea9f47..bac49e0 100644
@@ -108,6 +108,7 @@ struct nvme_ctrl {
 	u32 vs;
 	bool subsystem;
 	unsigned long quirks;
+	struct work_struct scan_work;
 };
 
 /*

@@ -147,6 +148,7 @@ struct nvme_ctrl_ops {
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
 	int (*reset_ctrl)(struct nvme_ctrl *ctrl);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
+	void (*finish_scan)(struct nvme_ctrl *ctrl);
 };
 
 static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl)

@@ -207,7 +209,7 @@ void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
 void nvme_put_ctrl(struct nvme_ctrl *ctrl);
 int nvme_init_identify(struct nvme_ctrl *ctrl);
 
-void nvme_scan_namespaces(struct nvme_ctrl *ctrl);
+void nvme_queue_scan(struct nvme_ctrl *ctrl);
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl);

drivers/nvme/host/pci.c +5 ~~-27~~

index f98d77f..52dd108 100644
@@ -92,7 +92,6 @@ struct nvme_dev {
 	struct msix_entry *entry;
 	void __iomem *bar;
 	struct work_struct reset_work;
-	struct work_struct scan_work;
 	struct work_struct remove_work;
 	struct work_struct async_work;
 	struct timer_list watchdog_timer;

@@ -266,16 +265,6 @@ static int nvme_init_request(void *data, struct request *req,
 	return 0;
 }
 
-static void nvme_queue_scan(struct nvme_dev *dev)
-{
-	/*
-	 * Do not queue new scan work when a controller is reset during
-	 * removal.
-	 */
-	if (dev->ctrl.state == NVME_CTRL_LIVE)
-		queue_work(nvme_workq, &dev->scan_work);
-}
-
 static void nvme_complete_async_event(struct nvme_dev *dev,
 		struct nvme_completion *cqe)
 {

@@ -293,7 +282,7 @@ static void nvme_complete_async_event(struct nvme_dev *dev,
 	switch (result & 0xff07) {
 	case NVME_AER_NOTICE_NS_CHANGED:
 		dev_info(dev->ctrl.device, "rescanning\n");
-		nvme_queue_scan(dev);
+		nvme_queue_scan(&dev->ctrl);
 	default:
 		dev_warn(dev->ctrl.device, "async event result %08x\n", result);
 	}

@@ -1520,8 +1509,9 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	return result;
 }
 
-static void nvme_set_irq_hints(struct nvme_dev *dev)
+static void nvme_pci_finish_scan(struct nvme_ctrl *ctrl)
 {
+	struct nvme_dev *dev = to_nvme_dev(ctrl);
 	struct nvme_queue *nvmeq;
 	int i;

@@ -1536,16 +1526,6 @@ static void nvme_set_irq_hints(struct nvme_dev *dev)
 	}
 }
 
-static void nvme_dev_scan(struct work_struct *work)
-{
-	struct nvme_dev *dev = container_of(work, struct nvme_dev, scan_work);
-
-	if (!dev->tagset.tags)
-		return;
-	nvme_scan_namespaces(&dev->ctrl);
-	nvme_set_irq_hints(dev);
-}
-
 static void nvme_del_queue_end(struct request *req, int error)
 {
 	struct nvme_queue *nvmeq = req->end_io_data;

@@ -1893,7 +1873,7 @@ static void nvme_reset_work(struct work_struct *work)
 	}
 
 	if (dev->online_queues > 1)
-		nvme_queue_scan(dev);
+		nvme_queue_scan(&dev->ctrl);
 	return;
 
  out:

@@ -1953,6 +1933,7 @@ static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.reg_read64		= nvme_pci_reg_read64,
 	.reset_ctrl		= nvme_pci_reset_ctrl,
 	.free_ctrl		= nvme_pci_free_ctrl,
+	.finish_scan		= nvme_pci_finish_scan,
 };
 
 static int nvme_dev_map(struct nvme_dev *dev)

@@ -2004,7 +1985,6 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (result)
 		goto free;
 
-	INIT_WORK(&dev->scan_work, nvme_dev_scan);
 	INIT_WORK(&dev->reset_work, nvme_reset_work);
 	INIT_WORK(&dev->remove_work, nvme_remove_dead_ctrl_work);
 	INIT_WORK(&dev->async_work, nvme_async_event_work);

@@ -2070,8 +2050,6 @@ static void nvme_remove(struct pci_dev *pdev)
 
 	pci_set_drvdata(pdev, NULL);
 	flush_work(&dev->async_work);
-	flush_work(&dev->scan_work);
-	nvme_remove_namespaces(&dev->ctrl);
 	nvme_uninit_ctrl(&dev->ctrl);
 	nvme_dev_disable(dev, true);
 	flush_work(&dev->reset_work);

Signature · 1 line

2.1.4

Sagi Grimberg

maintainer

2016-04-18 18:46 UTC #13 lore raw

PATCH 4/5 nvme: move namespace scanning to core

Quoted 4 lines

drivers/nvme/host/core.c +0 -0

@@ -1549,13 +1554,26 @@ void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
  		if (!nvme_scan_ns_list(ctrl, nn))
  			goto done;

drivers/nvme/host/core.c void nvme_scan_namespaces(struct nvme_ctrl *ctrl) · line 1564

Quoted 10 lines

drivers/nvme/host/core.c +4 -1

  	}
-	__nvme_scan_namespaces(ctrl, le32_to_cpup(&id->nn));
+	nvme_scan_ns_sequential(ctrl, nn);
   done:
  	list_sort(NULL, &ctrl->namespaces, ns_cmp);
  	mutex_unlock(&ctrl->namespaces_mutex);
  	kfree(id);
+
+	if (ctrl->ops->finish_scan)
+		ctrl->ops->finish_scan(ctrl);

finish_scan suggest this is a ns-scan related activity, but that does not seem to be the case (pci just sets affinity hints). Why does the affinity hints setting needs to happen after the ns scan?

Otherwise looks good,

Reviewed-by Sagi Grimberg <sagi at rimberg.me>

Keith Busch

2016-04-18 18:49 UTC #14 lore raw

PATCH 4/5 nvme: move namespace scanning to core

Quoted 2 lines · Sagi Grimberg

Why does the affinity hints setting needs to happen after the ns scan?

The driver has no idea what queues are associated to which CPU(s) until after the namespaces are registered with blk-mq. We need to know that mapping before providing affinity hints.

Sagi Grimberg

maintainer

2016-04-18 19:41 UTC #15 lore raw

PATCH 4/5 nvme: move namespace scanning to core

Quoted 7 lines

Quoted 3 lines
Why does the affinity hints setting needs to happen after the ns scan?
The driver has no idea what queues are associated to which CPU(s) until after the namespaces are registered with blk-mq. We need to know that mapping before providing affinity hints.

So the name made me think at first that this callout is related to ns-scanning (like the driver needs something to finish the scan), is post_scan a better name?

Christoph Hellwig

author maintainer

2016-04-18 20:17 UTC #16 lore raw

PATCH 4/5 nvme: move namespace scanning to core

Quoted 11 lines · Sagi Grimberg

Quoted 7 lines
Quoted 3 lines
Why does the affinity hints setting needs to happen after the ns scan?
The driver has no idea what queues are associated to which CPU(s) until after the namespaces are registered with blk-mq. We need to know that mapping before providing affinity hints.
So the name made me think at first that this callout is related to ns-scanning (like the driver needs something to finish the scan), is post_scan a better name?

Fine with me.

Christoph Hellwig

author maintainer

2016-04-18 12:34 UTC #17 lore raw

PATCH 5/5 nvme: move AER handling to common code

In reply to Christoph Hellwig

The transport driver still needs to do the actual submission, but all the higher level code can be shared.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/core.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  7 +++++++
 drivers/nvme/host/pci.c  | 49 +++++++++--------------------------------------
 3 files changed, 66 insertions(+), 40 deletions(-)

drivers/nvme/host/core.c +50 -0

index 606008d..53a93e7 100644
@@ -1584,6 +1584,54 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
 
+static void nvme_async_event_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl =
+		container_of(work, struct nvme_ctrl, async_event_work);
+
+	spin_lock_irq(&ctrl->lock);
+	while (ctrl->event_limit > 0) {
+		int aer_idx = --ctrl->event_limit;
+
+		spin_unlock_irq(&ctrl->lock);
+		ctrl->ops->submit_async_event(ctrl, aer_idx);
+		spin_lock_irq(&ctrl->lock);
+	}
+	spin_unlock_irq(&ctrl->lock);
+}
+
+void nvme_complete_async_event(struct nvme_ctrl *ctrl,
+		struct nvme_completion *cqe)
+{
+	u16 status = le16_to_cpu(cqe->status) >> 1;
+	u32 result = le32_to_cpu(cqe->result);
+
+	if (status == NVME_SC_SUCCESS || status == NVME_SC_ABORT_REQ) {
+		++ctrl->event_limit;
+		schedule_work(&ctrl->async_event_work);
+	}
+
+	if (status != NVME_SC_SUCCESS)
+		return;
+
+	switch (result & 0xff07) {
+	case NVME_AER_NOTICE_NS_CHANGED:
+		dev_info(ctrl->device, "rescanning\n");
+		nvme_queue_scan(ctrl);
+		break;
+	default:
+		dev_warn(ctrl->device, "async event result %08x\n", result);
+	}
+}
+EXPORT_SYMBOL_GPL(nvme_complete_async_event);
+
+void nvme_queue_async_events(struct nvme_ctrl *ctrl)
+{
+	ctrl->event_limit = NVME_NR_AERS;
+	schedule_work(&ctrl->async_event_work);
+}
+EXPORT_SYMBOL_GPL(nvme_queue_async_events);
+
 static DEFINE_IDA(nvme_instance_ida);
 
 static int nvme_set_instance(struct nvme_ctrl *ctrl)

@@ -1615,6 +1663,7 @@ static void nvme_release_instance(struct nvme_ctrl *ctrl)
 
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
+	flush_work(&ctrl->async_event_work);
 	flush_work(&ctrl->scan_work);
 	nvme_remove_namespaces(ctrl);

@@ -1662,6 +1711,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->ops = ops;
 	ctrl->quirks = quirks;
 	INIT_WORK(&ctrl->scan_work, nvme_scan_work);
+	INIT_WORK(&ctrl->async_event_work, nvme_async_event_work);
 
 	ret = nvme_set_instance(ctrl);
 	if (ret)

drivers/nvme/host/nvme.h +7 -0

index bac49e0..bb77f31 100644
@@ -109,6 +109,7 @@ struct nvme_ctrl {
 	bool subsystem;
 	unsigned long quirks;
 	struct work_struct scan_work;
+	struct work_struct async_event_work;
 };
 
 /*

@@ -149,6 +150,7 @@ struct nvme_ctrl_ops {
 	int (*reset_ctrl)(struct nvme_ctrl *ctrl);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 	void (*finish_scan)(struct nvme_ctrl *ctrl);
+	void (*submit_async_event)(struct nvme_ctrl *ctrl, int aer_idx);
 };
 
 static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl)

@@ -212,6 +214,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl);
 void nvme_queue_scan(struct nvme_ctrl *ctrl);
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
 
+#define NVME_NR_AERS	1
+void nvme_complete_async_event(struct nvme_ctrl *ctrl,
+		struct nvme_completion *cqe);
+void nvme_queue_async_events(struct nvme_ctrl *ctrl);
+
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_kill_queues(struct nvme_ctrl *ctrl);

drivers/nvme/host/pci.c +9 ~~-40~~

111 lines not on this page · raw message

Signature · 1 line

2.1.4

Sagi Grimberg

maintainer

2016-04-20 17:26 UTC #18 lore raw

PATCH 5/5 nvme: move AER handling to common code

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>