[PATCH 0/6] minor nvme fixes for 4.18

[PATCH 0/6] minor nvme fixes for 4.18

[Keith Busch]

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

-- 
2.14.3

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

[Keith Busch]

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4d0bb0e45401..d42a1c361162 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
 
-- 
2.14.3

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

[Jens Axboe]

On 6/6/18 8:13 AM, Keith Busch wrote:
> 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>

-- 
Jens Axboe

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

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

[Sagi Grimberg]

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

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

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d42a1c361162..94d9abbe2d45 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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)
-- 
2.14.3

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

[Jens Axboe]

On 6/6/18 8:13 AM, Keith Busch wrote:
> 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>

-- 
Jens Axboe

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

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

[Sagi Grimberg]

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

[PATCH 3/6] nvme-pci: Queue creation fixes

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 94d9abbe2d45..ed9fac721b8c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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;
 }
 
-- 
2.14.3

[PATCH 3/6] nvme-pci: Queue creation fixes

[Jens Axboe]

On 6/6/18 8:13 AM, Keith Busch wrote:
> 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>

-- 
Jens Axboe

[PATCH 3/6] nvme-pci: Queue creation fixes

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

[PATCH 3/6] nvme-pci: Queue creation fixes

[Sagi Grimberg]

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

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

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ed9fac721b8c..3a92eb8ffb36 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
 	}
-- 
2.14.3

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

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

[Sagi Grimberg]

> 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,
> 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(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index ed9fac721b8c..3a92eb8ffb36 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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);
>   	}
> 

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

[Keith Busch]

On Thu, Jun 07, 2018@11:46:53AM +0300, Sagi Grimberg wrote:
> 
> > 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".

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

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 3a92eb8ffb36..e18753f1ef19 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
-- 
2.14.3

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

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

[Sagi Grimberg]

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

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

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e18753f1ef19..c8dae140db06 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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;
-- 
2.14.3

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

[Johannes Thumshirn]

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


Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

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

[Keith Busch]

On Thu, Jun 07, 2018@09:19:58AM +0200, Johannes Thumshirn wrote:
> 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.

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

[Sagi Grimberg]

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

[PATCH 0/6] minor nvme fixes for 4.18

[Christoph Hellwig]

On Wed, Jun 06, 2018@08:13:03AM -0600, Keith Busch wrote:
> 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.