From f39ae4719b1c33d048aa4d3c284d82ecf252742b Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Thu, 31 May 2018 18:23:48 +0200 Subject: nvmet: return all zeroed buffer when we can't find an active namespace Quote from Figure 106 in NVMe 1.3a: The Identify Namespace data structure is returned to the host for the namespace specified in the Namespace Identifier (CDW1.NSID) field if it is an active NSID. If the specified namespace is not an active NSID, then the controller returns a zero filled data structure. Signed-off-by: Christoph Hellwig Reviewed-by: Sagi Grimberg Reviewed-by: Max Gurtovoy Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/target/admin-cmd.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c index ead8fbe6922e..962532842769 100644 --- a/drivers/nvme/target/admin-cmd.c +++ b/drivers/nvme/target/admin-cmd.c @@ -270,8 +270,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req) struct nvme_id_ns *id; u16 status = 0; - ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid); - if (!ns) { + if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) { status = NVME_SC_INVALID_NS | NVME_SC_DNR; goto out; } @@ -279,9 +278,14 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req) id = kzalloc(sizeof(*id), GFP_KERNEL); if (!id) { status = NVME_SC_INTERNAL; - goto out_put_ns; + goto out; } + /* return an all zeroed buffer if we can't find an active namespace */ + ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid); + if (!ns) + goto done; + /* * nuse = ncap = nsze isn't always true, but we have no way to find * that out from the underlying device. @@ -306,11 +310,10 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req) id->lbaf[0].ds = ns->blksize_shift; + nvmet_put_namespace(ns); +done: status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id)); - kfree(id); -out_put_ns: - nvmet_put_namespace(ns); out: nvmet_req_complete(req, status); } -- cgit v1.2.3-59-g8ed1b From 12a0b662210702c6b0ce9f66f0c177ff1dea99cb Mon Sep 17 00:00:00 2001 From: Johannes Thumshirn Date: Fri, 1 Jun 2018 09:11:20 +0200 Subject: nvme: don't hold nvmf_transports_rwsem for more than transport lookups Only take nvmf_transports_rwsem when doing a lookup of registered transports, so that a blocking ->create_ctrl doesn't prevent other actions on /dev/nvme-fabrics. Signed-off-by: Johannes Thumshirn [hch: increased lock hold time a bit to be safe, added a comment and updated the changelog] Signed-off-by: Christoph Hellwig Reviewed-by: Sagi Grimberg Reviewed-by: Hannes Reinecke Signed-off-by: Jens Axboe --- drivers/nvme/host/fabrics.c | 3 ++- drivers/nvme/host/fabrics.h | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index 5f5f7067c41d..fa32c1216409 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -952,6 +952,7 @@ nvmf_create_ctrl(struct device *dev, const char *buf, size_t count) ret = -EBUSY; goto out_unlock; } + up_read(&nvmf_transports_rwsem); ret = nvmf_check_required_opts(opts, ops->required_opts); if (ret) @@ -968,11 +969,11 @@ nvmf_create_ctrl(struct device *dev, const char *buf, size_t count) } module_put(ops->module); - up_read(&nvmf_transports_rwsem); return ctrl; out_module_put: module_put(ops->module); + goto out_free_opts; out_unlock: up_read(&nvmf_transports_rwsem); out_free_opts: diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h index 0cf0460a5c92..7491a0bbf711 100644 --- a/drivers/nvme/host/fabrics.h +++ b/drivers/nvme/host/fabrics.h @@ -124,6 +124,9 @@ struct nvmf_ctrl_options { * 1. At minimum, 'required_opts' and 'allowed_opts' should * be set to the same enum parsing options defined earlier. * 2. create_ctrl() must be defined (even if it does nothing) + * 3. struct nvmf_transport_ops must be statically allocated in the + * modules .bss section so that a pure module_get on @module + * prevents the memory from beeing freed. */ struct nvmf_transport_ops { struct list_head entry; -- cgit v1.2.3-59-g8ed1b From d4c68c7a8e2aa060d3c245052c76e74dd20b141b Mon Sep 17 00:00:00 2001 From: Steve Wise Date: Tue, 5 Jun 2018 10:16:41 -0700 Subject: nvme-rdma: correctly check for target keyed sgl support The code was checking bit 20 instead of bit 2. Also fixed the log entry. Reviewed-by: Sagi Grimberg Signed-off-by: Steve Wise Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/rdma.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index 7b3f08410430..2aba03876d84 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -1951,8 +1951,9 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev, } /* sanity check keyed sgls */ - if (!(ctrl->ctrl.sgls & (1 << 20))) { - dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not support\n"); + if (!(ctrl->ctrl.sgls & (1 << 2))) { + dev_err(ctrl->ctrl.device, + "Mandatory keyed sgls are not supported!\n"); ret = -EINVAL; goto out_remove_admin_queue; } -- cgit v1.2.3-59-g8ed1b From 9ba2a5cb88969c847015905db7f1627ae3c82f73 Mon Sep 17 00:00:00 2001 From: Sagi Grimberg Date: Wed, 6 Jun 2018 15:27:48 +0300 Subject: nvmet: filter newlines from user input We should avoid consuming the newlines in traddr, trsvcid and device_path. Add minimal processing to make sure they are gone. Reviewed-by: Johannes Thumshirn Signed-off-by: Sagi Grimberg Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/target/configfs.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c index ad9ff27234b5..d3f3b3ec4d1a 100644 --- a/drivers/nvme/target/configfs.c +++ b/drivers/nvme/target/configfs.c @@ -137,8 +137,10 @@ static ssize_t nvmet_addr_traddr_store(struct config_item *item, pr_err("Disable the address before modifying\n"); return -EACCES; } - return snprintf(port->disc_addr.traddr, - sizeof(port->disc_addr.traddr), "%s", page); + + if (sscanf(page, "%s\n", port->disc_addr.traddr) != 1) + return -EINVAL; + return count; } CONFIGFS_ATTR(nvmet_, addr_traddr); @@ -208,8 +210,10 @@ static ssize_t nvmet_addr_trsvcid_store(struct config_item *item, pr_err("Disable the address before modifying\n"); return -EACCES; } - return snprintf(port->disc_addr.trsvcid, - sizeof(port->disc_addr.trsvcid), "%s", page); + + if (sscanf(page, "%s\n", port->disc_addr.trsvcid) != 1) + return -EINVAL; + return count; } CONFIGFS_ATTR(nvmet_, addr_trsvcid); @@ -288,7 +292,7 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item, kfree(ns->device_path); ret = -ENOMEM; - ns->device_path = kstrdup(page, GFP_KERNEL); + ns->device_path = kstrndup(page, strcspn(page, "\n"), GFP_KERNEL); if (!ns->device_path) goto out_unlock; -- cgit v1.2.3-59-g8ed1b From 0bc88192033a6e652e3fb1adfd6d1b66be33951e Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:04 -0600 Subject: nvme-pci: remove unnecessary nested locking The nvme pci driver no longer handles completions under the cq lock, so the nested locking is not necessary. Signed-off-by: Keith Busch Reviewed-by: Jens Axboe Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index e526437bacbf..a7bed8dccd61 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2012,13 +2012,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); -- cgit v1.2.3-59-g8ed1b From 397c699fb096e7a822990990c17c6b43e829cfc4 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:05 -0600 Subject: nvme-pci: remove unnecessary completion doorbell check 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 Reviewed-by: Jens Axboe Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index a7bed8dccd61..4963a407e728 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) -- cgit v1.2.3-59-g8ed1b From ded45505dbfdcaf1e49ae0349e5dafb59c9efbe5 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:06 -0600 Subject: nvme-pci: queue creation fixes 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 Reviewed-by: Jens Axboe Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 4963a407e728..7a42ccad3864 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1475,11 +1475,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; /* @@ -1501,7 +1503,6 @@ release_sq: adapter_delete_sq(dev, qid); release_cq: adapter_delete_cq(dev, qid); -out: return result; } -- cgit v1.2.3-59-g8ed1b From fe76fcfb91a97eccf20cec17ff05a27b8d5b0801 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:07 -0600 Subject: nvme-pci: remove HMB teardown on reset 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 Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 8 -------- 1 file changed, 8 deletions(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 7a42ccad3864..7f8b1bd03db4 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2224,14 +2224,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); } -- cgit v1.2.3-59-g8ed1b From 1d39e6928cbd0eb737c51545210b5186d5551ba1 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:08 -0600 Subject: nvme-pci: unquiesce dead controller queues 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 Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 7f8b1bd03db4..d935aba0288f 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2599,7 +2599,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); -- cgit v1.2.3-59-g8ed1b From 69f4eb9ff79556c1a3daf5af5573594c196f30cc Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:09 -0600 Subject: nvme-pci: make CMB SQ mod-param read-only 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 Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index d935aba0288f..cd7aec58a301 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; -- cgit v1.2.3-59-g8ed1b From 77016199f11eacd7b23e2faeb4d0f36166e3530b Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Thu, 7 Jun 2018 11:27:41 +0300 Subject: nvme: cleanup double shift issue The problem here is that set_bit() and test_bit() take a bit number so we should be passing 0 but instead we're passing (1 << 0) which leads to a double shift. It doesn't cause a runtime bug in the current code because it's done consistently and we only set that one bit. I decided to just re-use NVME_AER_NOTICE_NS_CHANGED instead of introducing a new define for this. Signed-off-by: Dan Carpenter Reviewed-by: Sagi Grimberg Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/core.c | 4 ++-- drivers/nvme/host/nvme.h | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/nvme') diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 04a20da76786..e2dcd14012b1 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3245,7 +3245,7 @@ static void nvme_scan_work(struct work_struct *work) WARN_ON_ONCE(!ctrl->tagset); - if (test_and_clear_bit(EVENT_NS_CHANGED, &ctrl->events)) { + if (test_and_clear_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events)) { if (nvme_scan_changed_ns_log(ctrl)) goto out_sort_namespaces; dev_info(ctrl->device, "rescanning namespaces.\n"); @@ -3386,7 +3386,7 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result) { switch ((result & 0xff00) >> 8) { case NVME_AER_NOTICE_NS_CHANGED: - set_bit(EVENT_NS_CHANGED, &ctrl->events); + set_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events); nvme_queue_scan(ctrl); break; case NVME_AER_NOTICE_FW_ACT_STARTING: diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index de24fe77c80b..34df07d44f80 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -194,7 +194,6 @@ struct nvme_ctrl { struct delayed_work ka_work; struct nvme_command ka_cmd; struct work_struct fw_act_work; -#define EVENT_NS_CHANGED (1 << 0) unsigned long events; /* Power saving configuration */ -- cgit v1.2.3-59-g8ed1b