Skip to contentSkip to conversation

minor nvme fixes for 4.18

PATCH New Keith Busch 6 participants 25 messages Jun 6, 2018 – Jun 7, 2018 NVMe

lore
Share & tools
lore

Keith Busch

author
#0 lore raw

cover minor nvme fixes for 4.18

A series of unrelated trivial patches. More to come later, but I think these are non-controversial ones.

Keith Busch (6):
nvme-pci: Remove unnecessary nested locking
nvme-pci: Remove unnecessary completion doorbell check
nvme-pci: Queue creation fixes
nvme-pci: Remove HMB teardown on reset
nvme-pci: Unquiesce dead controller queues
nvme-pci: Make CMB SQ mod-param read-only

 drivers/nvme/host/pci.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH 1/6 nvme-pci: Remove unnecessary nested locking

The nvme pci driver no longer handles completions under the cq lock, so the nested locking is not necesssary.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
drivers/nvme/host/pci.c +1 -7
index 4d0bb0e45401..d42a1c361162 100644
@@ -2003,13 +2003,7 @@ static void nvme_del_cq_end(struct request *req, blk_status_t error)
 	if (!error) {
 		unsigned long flags;
 
-		/*
-		 * We might be called with the AQ cq_lock held
-		 * and the I/O queue cq_lock should always
-		 * nest inside the AQ one.
-		 */
-		spin_lock_irqsave_nested(&nvmeq->cq_lock, flags,
-					SINGLE_DEPTH_NESTING);
+		spin_lock_irqsave(&nvmeq->cq_lock, flags);
 		nvme_process_cq(nvmeq, &start, &end, -1);
 		spin_unlock_irqrestore(&nvmeq->cq_lock, flags);
Signature · 1 line
2.14.3
maintainer
#2 lore raw

PATCH 1/6 nvme-pci: Remove unnecessary nested locking

Quoted 2 lines · Keith Busch

The nvme pci driver no longer handles completions under the cq lock, so the nested locking is not necesssary.

                                 ^^

necessary

Other than that:

Reviewed-by Jens Axboe <axboe at kernel.dk>
Signature · 1 line
Jens Axboe
#3 lore raw

PATCH 1/6 nvme-pci: Remove unnecessary nested locking

In reply to Keith Busch

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
author
#5 lore raw

PATCH 2/6 nvme-pci: Remove unnecessary completion doorbell check

In reply to Keith Busch

The nvme pci driver never unmaps the doorbell registers while the requests are active, so we can always safely update the completion queue head.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/nvme/host/pci.c +3 -5
index d42a1c361162..94d9abbe2d45 100644
@@ -920,11 +920,9 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
 {
 	u16 head = nvmeq->cq_head;
 
-	if (likely(nvmeq->cq_vector >= 0)) {
-		if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
-						      nvmeq->dbbuf_cq_ei))
-			writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
-	}
+	if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
+					      nvmeq->dbbuf_cq_ei))
+		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
 }
 
 static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
Signature · 1 line
2.14.3
maintainer
#6 lore raw

PATCH 2/6 nvme-pci: Remove unnecessary completion doorbell check

Quoted 2 lines · Keith Busch

The nvme pci driver never unmaps the doorbell registers while the requests are active, so we can always safely update the completion queue head.

Reviewed-by Jens Axboe <axboe at kernel.dk>
Signature · 1 line
Jens Axboe
#7 lore raw

PATCH 2/6 nvme-pci: Remove unnecessary completion doorbell check

In reply to Keith Busch

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
author
#9 lore raw

PATCH 3/6 nvme-pci: Queue creation fixes

In reply to Keith Busch

We've been ignoring NVMe error status on queue creations. Fortunately they are uncommon, but we should handle these anyway. This patch adds checks for the a positive error return value that indicates an NVMe status.

If we do see a negative return, the controller isn't usable, so this patch returns immediately in since we can't unwind that failure.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/nvme/host/pci.c +4 -3
index 94d9abbe2d45..ed9fac721b8c 100644
@@ -1466,11 +1466,13 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	 */
 	vector = dev->num_vecs == 1 ? 0 : qid;
 	result = adapter_alloc_cq(dev, qid, nvmeq, vector);
-	if (result < 0)
-		goto out;
+	if (result)
+		return result;
 
 	result = adapter_alloc_sq(dev, qid, nvmeq);
 	if (result < 0)
+		return result;
+	else if (result)
 		goto release_cq;
 
 	/*

@@ -1492,7 +1494,6 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	adapter_delete_sq(dev, qid);
 release_cq:
 	adapter_delete_cq(dev, qid);
-out:
 	return result;
 }
Signature · 1 line
2.14.3
maintainer
#10 lore raw

PATCH 3/6 nvme-pci: Queue creation fixes

Quoted 6 lines · Keith Busch

We've been ignoring NVMe error status on queue creations. Fortunately they are uncommon, but we should handle these anyway. This patch adds checks for the a positive error return value that indicates an NVMe status.

If we do see a negative return, the controller isn't usable, so this patch returns immediately in since we can't unwind that failure.

Reviewed-by Jens Axboe <axboe at kernel.dk>
Signature · 1 line
Jens Axboe
#11 lore raw

PATCH 3/6 nvme-pci: Queue creation fixes

In reply to Keith Busch

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
author
#13 lore raw

PATCH 4/6 nvme-pci: Remove HMB teardown on reset

In reply to Keith Busch

The controller is required to disable its host memory buffer use on controller reset. We don't need to submit an admin command to delete it, so this patch skips sending that command so we don't need to worry about handling a timeout.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 8 --------
 1 file changed, 8 deletions(-)
drivers/nvme/host/pci.c +0 -8
index ed9fac721b8c..3a92eb8ffb36 100644
@@ -2215,14 +2215,6 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	nvme_stop_queues(&dev->ctrl);
 
 	if (!dead && dev->ctrl.queue_count > 0) {
-		/*
-		 * If the controller is still alive tell it to stop using the
-		 * host memory buffer.  In theory the shutdown / reset should
-		 * make sure that it doesn't access the host memoery anymore,
-		 * but I'd rather be safe than sorry..
-		 */
-		if (dev->host_mem_descs)
-			nvme_set_host_mem(dev, 0);
 		nvme_disable_io_queues(dev);
 		nvme_disable_admin_queue(dev, shutdown);
 	}
Signature · 1 line
2.14.3
#14 lore raw

PATCH 4/6 nvme-pci: Remove HMB teardown on reset

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
maintainer
#15 lore raw

PATCH 4/6 nvme-pci: Remove HMB teardown on reset

In reply to Keith Busch
Quoted 3 lines

The controller is required to disable its host memory buffer use on controller reset.

You mean clearing controller enable?

We don't need to submit an admin command to delete it,

Quoted 28 lines

so this patch skips sending that command so we don't need to worry about handling a timeout.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
  drivers/nvme/host/pci.c | 8 --------
  1 file changed, 8 deletions(-)
drivers/nvme/host/pci.c +0 -8
index ed9fac721b8c..3a92eb8ffb36 100644
@@ -2215,14 +2215,6 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
  	nvme_stop_queues(&dev->ctrl);
 
  	if (!dead && dev->ctrl.queue_count > 0) {
-		/*
-		 * If the controller is still alive tell it to stop using the
-		 * host memory buffer.  In theory the shutdown / reset should
-		 * make sure that it doesn't access the host memoery anymore,
-		 * but I'd rather be safe than sorry..
-		 */
-		if (dev->host_mem_descs)
-			nvme_set_host_mem(dev, 0);
  		nvme_disable_io_queues(dev);
  		nvme_disable_admin_queue(dev, shutdown);
  	}
#16 lore raw

PATCH 4/6 nvme-pci: Remove HMB teardown on reset

Quoted 5 lines · Sagi Grimberg
Quoted 3 lines

The controller is required to disable its host memory buffer use on controller reset.

You mean clearing controller enable?

Right, clearing CC.EN in nvme spec is a "Controller Reset".

author
#17 lore raw

PATCH 5/6 nvme-pci: Unquiesce dead controller queues

In reply to Keith Busch

This patch ensures the nvme namsepace request queues are not quiesced on a surprise removal. It's possible the queues were previously killed in a failed reset, so the queues need to be unquiesced to ensure all requests are flushed to completion.

Signed-off-by Keith Busch <keith.busch at intel.com>
Reviewed-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
index 3a92eb8ffb36..e18753f1ef19 100644
@@ -2590,7 +2590,7 @@ static void nvme_remove(struct pci_dev *pdev)
 
 	if (!pci_device_is_present(pdev)) {
 		nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DEAD);
-		nvme_dev_disable(dev, false);
+		nvme_dev_disable(dev, true);
 	}
 
 	flush_work(&dev->ctrl.reset_work);
Signature · 1 line
2.14.3
#18 lore raw

PATCH 5/6 nvme-pci: Unquiesce dead controller queues

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
author
#20 lore raw

PATCH 6/6 nvme-pci: Make CMB SQ mod-param read-only

In reply to Keith Busch

A controller reset after a run time change of the CMB module parameter breaks the driver. An 'on -> off' will have the driver use NULL for the host memory queue, and 'off -> on' will use mismatched queue depth between the device and the host.

We could fix both, but there isn't really a good reason to change this at run time anyway, compared to at module load time, so this patch makes parameter read-only after after modprobe.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
index e18753f1ef19..c8dae140db06 100644
@@ -42,7 +42,7 @@ static int use_threaded_interrupts;
 module_param(use_threaded_interrupts, int, 0);
 
 static bool use_cmb_sqes = true;
-module_param(use_cmb_sqes, bool, 0644);
+module_param(use_cmb_sqes, bool, 0444);
 MODULE_PARM_DESC(use_cmb_sqes, "use controller's memory buffer for I/O SQes");
 
 static unsigned int max_host_mem_size_mb = 128;
Signature · 1 line
2.14.3
#21 lore raw

PATCH 6/6 nvme-pci: Make CMB SQ mod-param read-only

This could evnet be a stable candidate, couldn't it?

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
#22 lore raw

PATCH 6/6 nvme-pci: Make CMB SQ mod-param read-only

Quoted 1 line · Johannes Thumshirn

This could evnet be a stable candidate, couldn't it?

Yep, I intend to combine this with a back-port of 815c6704bf9f1 ("nvme-pci: Remap CMB SQ entries on every controller reset") for stable.

maintainer
#24 lore raw

cover minor nvme fixes for 4.18

In reply to Keith Busch
Quoted 2 lines · Keith Busch

A series of unrelated trivial patches. More to come later, but I think these are non-controversial ones.

Looks fine to me. I'll wait a bit more for some feedback before applying.