Skip to contentSkip to conversation

nvme driver split V2

PATCH New Christoph Hellwig 6 participants 59 messages Oct 16, 2015 – Oct 23, 2015

Subsystems (2)
lore
Share & tools
lore

Christoph Hellwig

author maintainer
#0 lore raw

This series contains most of the work of splitting the nvme driver into a core driver, a PCIe frontend for it and the possibility of future other frontends like virtio or Fabrics.

This creates a new core.c with functions sitting on top of the block layer, splits and split a new struct nvme_ctrl out of struct nvme_dev so that the SCSI translation layer is decoupled from the PCI driver internals. A new struct nvme_ctrl_ops is introduced to call into the low level driver, and this is subsequently used to move all the block device, char device and sysfs interface code as well as the controller identification and namespace scanning into the common code. I've ported both my nvme-loop and the fabrics driver over to this scheme to validate that it works fine, and Ming ported over virtio_nvme,

Once this this series is done there are a few more items remaining:

- abort rewrite to sit on top of struct request infrastructure (in progress)
- AEN rewrite, including making a small part of it common
- a proper state machine for probing, scanning and resets (will take a while)

To make testing easier I've pushed out a git tree with this and all dependencies:

git://git.infradead.org/users/hch/block.git nvme-split.5

or in gitweb:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-split.5

Chances since V1:
- fixed a section mismatch
- fixed missing byte swap
- fixed metadata passthrough (thanks Keith!)
- merged the previously separate series into a single one
- one whitespace fix
- added two additional patches to move trivial I/O path helpers to nvme.h

author maintainer
#1 lore raw

PATCH 1/18 nvme: add missing unmaps in nvme_queue_rq

When we fail various metadata related operations in nvme_queue_rq we need to unmap the data SGL.

Cc stable at vger.kernel.org
Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/nvme/host/pci.c +12 -3
index 22d8375..2f05292 100644
@@ -906,19 +906,28 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 			goto retry_cmd;
 		}
 		if (blk_integrity_rq(req)) {
-			if (blk_rq_count_integrity_sg(req->q, req->bio) != 1)
+			if (blk_rq_count_integrity_sg(req->q, req->bio) != 1) {
+				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
+						dma_dir);
 				goto error_cmd;
+			}
 
 			sg_init_table(iod->meta_sg, 1);
 			if (blk_rq_map_integrity_sg(
-					req->q, req->bio, iod->meta_sg) != 1)
+					req->q, req->bio, iod->meta_sg) != 1) {
+				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
+						dma_dir);
 				goto error_cmd;
+			}
 
 			if (rq_data_dir(req))
 				nvme_dif_remap(req, nvme_dif_prep);
 
-			if (!dma_map_sg(nvmeq->q_dmadev, iod->meta_sg, 1, dma_dir))
+			if (!dma_map_sg(nvmeq->q_dmadev, iod->meta_sg, 1, dma_dir)) {
+				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
+						dma_dir);
 				goto error_cmd;
+			}
 		}
 	}
 
Signature · 1 line
1.9.1
#2 lore raw

PATCH 1/18 nvme: add missing unmaps in nvme_queue_rq

Quoted 22 lines · Christoph Hellwig

When we fail various metadata related operations in nvme_queue_rq we need to unmap the data SGL.

Cc stable at vger.kernel.org
Signed-off-by Christoph Hellwig <hch at lst.de>
---
  drivers/nvme/host/pci.c | 15 ++++++++++++---
  1 file changed, 12 insertions(+), 3 deletions(-)
drivers/nvme/host/pci.c +3 -1
index 22d8375..2f05292 100644
@@ -906,19 +906,28 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
  			goto retry_cmd;
  		}
  		if (blk_integrity_rq(req)) {
-			if (blk_rq_count_integrity_sg(req->q, req->bio) != 1)
+			if (blk_rq_count_integrity_sg(req->q, req->bio) != 1) {
+				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
+						dma_dir);
  				goto error_cmd;

drivers/nvme/host/pci.c static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 921

Quoted 10 lines
drivers/nvme/host/pci.c +5 -1
+			}
 
  			sg_init_table(iod->meta_sg, 1);
  			if (blk_rq_map_integrity_sg(
-					req->q, req->bio, iod->meta_sg) != 1)
+					req->q, req->bio, iod->meta_sg) != 1) {
+				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
+						dma_dir);
  				goto error_cmd;
+			}

This is not related to the patch itself. But this condition seems bogus to me. We passed meta_sg that consists of a single entry. If we happened to map more than a single entry we're already in trouble as we overrun meta_sg (modified the iod->sg pointer).

I think a WARN_ON_ONCE statement is more suitable here (which should probably come as a separate patch).

Other than that, looks good to me:

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
#3 lore raw

PATCH 1/18 nvme: add missing unmaps in nvme_queue_rq

Quoted 15 lines · Sagi Grimberg

drivers/nvme/host/pci.c static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 921

Quoted 8 lines · Christoph Hellwig
drivers/nvme/host/pci.c +4 -1
 			sg_init_table(iod->meta_sg, 1);
 			if (blk_rq_map_integrity_sg(
-					req->q, req->bio, iod->meta_sg) != 1)
+					req->q, req->bio, iod->meta_sg) != 1) {
+				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
+						dma_dir);
 				goto error_cmd;
+			}

This is not related to the patch itself. But this condition seems bogus to me. We passed meta_sg that consists of a single entry. If we happened to map more than a single entry we're already in trouble as we overrun meta_sg (modified the iod->sg pointer).

Quoted 2 lines

I think a WARN_ON_ONCE statement is more suitable here (which should probably come as a separate patch).

We are in trouble if it maps more than 1, but I think the condition here is intended to guard against 0 rather than > 1. We should already be ensured it won't be > 1 from a previous check.

Based on the implementation of blk_rq_map_integrity_sg and the functions earlier setup, I don't think we can ever see 0 here either.

author maintainer
#4 lore raw

PATCH 2/18 nvme: move struct nvme_iod to pci.c

In reply to Christoph Hellwig

This structure is specific to the PCIe driver internals and should be moved to pci.c.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/nvme.h | 17 -----------------
 drivers/nvme/host/pci.c  | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 17 deletions(-)
drivers/nvme/host/nvme.h +0 -17
index c1f41bf..835941b 100644
@@ -88,23 +88,6 @@ struct nvme_ns {
 	u32 mode_select_block_len;
 };
 
-/*
- * The nvme_iod describes the data in an I/O, including the list of PRP
- * entries.  You can't see it in this data structure because C doesn't let
- * me express that.  Use nvme_alloc_iod to ensure there's enough space
- * allocated to store the PRP list.
- */
-struct nvme_iod {
-	unsigned long private;	/* For the use of the submitter of the I/O */
-	int npages;		/* In the PRP list. 0 means small pool in use */
-	int offset;		/* Of PRP list */
-	int nents;		/* Used in scatterlist */
-	int length;		/* Of data, in bytes */
-	dma_addr_t first_dma;
-	struct scatterlist meta_sg[1]; /* metadata requires single contiguous buffer */
-	struct scatterlist sg[0];
-};
-
 static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector)
 {
 	return (sector >> (ns->lba_shift - 9));
drivers/nvme/host/pci.c +17 -0
index 2f05292..7e1b438 100644
@@ -128,6 +128,23 @@ struct nvme_queue {
 };
 
 /*
+ * The nvme_iod describes the data in an I/O, including the list of PRP
+ * entries.  You can't see it in this data structure because C doesn't let
+ * me express that.  Use nvme_alloc_iod to ensure there's enough space
+ * allocated to store the PRP list.
+ */
+struct nvme_iod {
+	unsigned long private;	/* For the use of the submitter of the I/O */
+	int npages;		/* In the PRP list. 0 means small pool in use */
+	int offset;		/* Of PRP list */
+	int nents;		/* Used in scatterlist */
+	int length;		/* Of data, in bytes */
+	dma_addr_t first_dma;
+	struct scatterlist meta_sg[1]; /* metadata requires single contiguous buffer */
+	struct scatterlist sg[0];
+};
+
+/*
  * Check we didin't inadvertently grow the command struct
  */
 static inline void _nvme_check_size(void)
Signature · 1 line
1.9.1
#5 lore raw

PATCH 2/18 nvme: move struct nvme_iod to pci.c

Quoted 5 lines · Christoph Hellwig

This structure is specific to the PCIe driver internals and should be moved to pci.c.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>

looks good to me:

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
author maintainer
#6 lore raw

PATCH 3/18 nvme: split command submission helpers out of pci.c

In reply to Christoph Hellwig

Create a new core.c and start by adding the command submission helpers to it, which are already abstracted away from the actual hardware queues by the block layer.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/Makefile |   2 +-
 drivers/nvme/host/core.c   | 172 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h   |   3 +
 drivers/nvme/host/pci.c    | 153 +---------------------------------------
 4 files changed, 177 insertions(+), 153 deletions(-)
 create mode 100644 drivers/nvme/host/core.c
drivers/nvme/host/Makefile +1 -1
index cfb6679..336b4ea 100644
@@ -1,4 +1,4 @@
 
 obj-$(CONFIG_BLK_DEV_NVME)     += nvme.o
 
-nvme-y		+= pci.o scsi.o
+nvme-y		+= core.o pci.o scsi.o
drivers/nvme/host/core.c new file +172 -0

172 lines not on this page · raw message

drivers/nvme/host/nvme.h +3 -0
index 835941b..0633a7b 100644
@@ -22,6 +22,9 @@
 extern unsigned char nvme_io_timeout;
 #define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
+extern unsigned char admin_timeout;
+#define ADMIN_TIMEOUT	(admin_timeout * HZ)
+
 /*
  * Represents an NVM Express device.  Each nvme_dev is a PCI function.
  */
drivers/nvme/host/pci.c +1 -152

173 lines not on this page · raw message

Signature · 1 line
1.9.1
#7 lore raw

PATCH 3/18 nvme: split command submission helpers out of pci.c

Quoted 6 lines · Christoph Hellwig

Create a new core.c and start by adding the command submission helpers to it, which are already abstracted away from the actual hardware queues by the block layer.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>

looks good to me,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
#8 lore raw

PATCH 3/18 nvme: split command submission helpers out of pci.c

In reply to Christoph Hellwig
Quoted 248 lines · Christoph Hellwig

Create a new core.c and start by adding the command submission helpers
to it, which are already abstracted away from the actual hardware queues
by the block layer.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/Makefile |   2 +-
 drivers/nvme/host/core.c   | 172 
+++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h   |   3 +
 drivers/nvme/host/pci.c    | 153 +----------------------------------
-----
 4 files changed, 177 insertions(+), 153 deletions(-)
 create mode 100644 drivers/nvme/host/core.c
drivers/nvme/host/Makefile +1 -1
index cfb6679..336b4ea 100644
@@ -1,4 +1,4 @@
 
 obj-$(CONFIG_BLK_DEV_NVME)     += nvme.o
 
-nvme-y		+= pci.o scsi.o
+nvme-y		+= core.o pci.o scsi.o
drivers/nvme/host/core.c new file +5 -0
new file mode 100644
index 0000000..dfb528d
@@ -0,0 +1,172 @@
+/*
+ * NVM Express device driver
+ * Copyright (c) 2011-2014, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or 

modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */

drivers/nvme/host/core.c new file +12 -0
+
+#include <linux/blkdev.h>
+#include <linux/blk-mq.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "nvme.h"
+
+/*
+ * Returns 0 on success.  If the result is negative, it's a Linux 

error code;
+ * if the result is positive, it's an NVM Express status code
+ */
+int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,

drivers/nvme/host/core.c new file +1 -0
+		void *buffer, void __user *ubuffer, unsigned 

bufflen,

drivers/nvme/host/core.c new file +25 -0
+		u32 *result, unsigned timeout)
+{
+	bool write = cmd->common.opcode & 1;
+	struct bio *bio = NULL;
+	struct request *req;
+	int ret;
+
+	req = blk_mq_alloc_request(q, write, GFP_KERNEL, false);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	req->cmd_type = REQ_TYPE_DRV_PRIV;
+	req->cmd_flags |= REQ_FAILFAST_DRIVER;
+	req->__data_len = 0;
+	req->__sector = (sector_t) -1;
+	req->bio = req->biotail = NULL;
+
+	req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+
+	req->cmd = (unsigned char *)cmd;
+	req->cmd_len = sizeof(struct nvme_command);
+	req->special = (void *)0;
+
+	if (buffer && bufflen) {
+		ret = blk_rq_map_kern(q, req, buffer, bufflen, 

__GFP_WAIT);

drivers/nvme/host/core.c new file +4 -0
+		if (ret)
+			goto out;
+	} else if (ubuffer && bufflen) {
+		ret = blk_rq_map_user(q, req, NULL, ubuffer, 

bufflen, __GFP_WAIT);

drivers/nvme/host/core.c new file +17 -0
+		if (ret)
+			goto out;
+		bio = req->bio;
+	}
+
+	blk_execute_rq(req->q, NULL, req, 0);
+	if (bio)
+		blk_rq_unmap_user(bio);
+	if (result)
+		*result = (u32)(uintptr_t)req->special;
+	ret = req->errors;
+ out:
+	blk_mq_free_request(req);
+	return ret;
+}
+
+int nvme_submit_sync_cmd(struct request_queue *q, struct 

nvme_command *cmd,

drivers/nvme/host/core.c new file +3 -0
+		void *buffer, unsigned bufflen)
+{
+	return __nvme_submit_sync_cmd(q, cmd, buffer, NULL, bufflen, 

NULL, 0);

drivers/nvme/host/core.c new file +3 -0
+}
+
+int nvme_identify_ctrl(struct nvme_dev *dev, struct nvme_id_ctrl 

**id)

drivers/nvme/host/core.c new file +5 -0
+{
+	struct nvme_command c = { };
+	int error;
+
+	/* gcc-4.4.4 (at least) has issues with initializers and 

anon unions */

drivers/nvme/host/core.c new file +21 -0
+	c.identify.opcode = nvme_admin_identify;
+	c.identify.cns = cpu_to_le32(1);
+
+	*id = kmalloc(sizeof(struct nvme_id_ctrl), GFP_KERNEL);
+	if (!*id)
+		return -ENOMEM;
+
+	error = nvme_submit_sync_cmd(dev->admin_q, &c, *id,
+			sizeof(struct nvme_id_ctrl));
+	if (error)
+		kfree(*id);
+	return error;
+}
+
+int nvme_identify_ns(struct nvme_dev *dev, unsigned nsid,
+		struct nvme_id_ns **id)
+{
+	struct nvme_command c = { };
+	int error;
+
+	/* gcc-4.4.4 (at least) has issues with initializers and 

anon unions */

drivers/nvme/host/core.c new file +15 -0
+	c.identify.opcode = nvme_admin_identify,
+	c.identify.nsid = cpu_to_le32(nsid),
+
+	*id = kmalloc(sizeof(struct nvme_id_ns), GFP_KERNEL);
+	if (!*id)
+		return -ENOMEM;
+
+	error = nvme_submit_sync_cmd(dev->admin_q, &c, *id,
+			sizeof(struct nvme_id_ns));
+	if (error)
+		kfree(*id);
+	return error;
+}
+
+int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned 

nsid,

drivers/nvme/host/core.c new file +1 -0
+					dma_addr_t dma_addr, u32 

*result)

drivers/nvme/host/core.c new file +10 -0
+{
+	struct nvme_command c;
+
+	memset(&c, 0, sizeof(c));
+	c.features.opcode = nvme_admin_get_features;
+	c.features.nsid = cpu_to_le32(nsid);
+	c.features.prp1 = cpu_to_le64(dma_addr);
+	c.features.fid = cpu_to_le32(fid);
+
+	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, NULL, 

0,

drivers/nvme/host/core.c new file +4 -0
+			result, 0);
+}
+
+int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned 

dword11,

drivers/nvme/host/core.c new file +1 -0
+					dma_addr_t dma_addr, u32 

*result)

drivers/nvme/host/core.c new file +10 -0
+{
+	struct nvme_command c;
+
+	memset(&c, 0, sizeof(c));
+	c.features.opcode = nvme_admin_set_features;
+	c.features.prp1 = cpu_to_le64(dma_addr);
+	c.features.fid = cpu_to_le32(fid);
+	c.features.dword11 = cpu_to_le32(dword11);
+
+	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, NULL, 

0,

drivers/nvme/host/core.c new file +4 -0
+			result, 0);
+}
+
+int nvme_get_log_page(struct nvme_dev *dev, struct nvme_smart_log 

**log)

drivers/nvme/host/core.c new file +8 -0
+{
+	struct nvme_command c = { };
+	int error;
+
+	c.common.opcode = nvme_admin_get_log_page,
+	c.common.nsid = cpu_to_le32(0xFFFFFFFF),
+	c.common.cdw10[0] = cpu_to_le32(
+			(((sizeof(struct nvme_smart_log) / 4) - 1) 

<< 16) |

drivers/nvme/host/core.c new file +12 -0
+			 NVME_LOG_SMART),
+
+	*log = kmalloc(sizeof(struct nvme_smart_log), GFP_KERNEL);
+	if (!*log)
+		return -ENOMEM;
+
+	error = nvme_submit_sync_cmd(dev->admin_q, &c, *log,
+			sizeof(struct nvme_smart_log));
+	if (error)
+		kfree(*log);
+	return error;
+}
drivers/nvme/host/nvme.h +3 -0
index 835941b..0633a7b 100644
@@ -22,6 +22,9 @@
 extern unsigned char nvme_io_timeout;
 #define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
+extern unsigned char admin_timeout;
+#define ADMIN_TIMEOUT	(admin_timeout * HZ)
+
 /*
  * Represents an NVM Express device.  Each nvme_dev is a PCI 

function.
*/

drivers/nvme/host/pci.c +0 -0
index 7e1b438..628c572 100644

drivers/nvme/host/pci.c line 55

Quoted 7 lines
drivers/nvme/host/pci.c +0 -1
@@ -50,10 +50,9 @@
 #define NVME_AQ_DEPTH		256
 #define SQ_SIZE(depth)		(depth * sizeof(struct 
 nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct 
 nvme_completion))
-#define ADMIN_TIMEOUT		(admin_timeout * HZ)
 #define SHUTDOWN_TIMEOUT	(shutdown_timeout * HZ)

I think some or all of these #defines should be moved out of the .c file and stuck into a .h file as well? Why would any of the queue depths or the shutdown timeout be PCIe specific?

Quoted 194 lines
drivers/nvme/host/pci.c +1 -1
-static unsigned char admin_timeout = 60;
+unsigned char admin_timeout = 60;
 module_param(admin_timeout, byte, 0644);
 MODULE_PARM_DESC(admin_timeout, "timeout in seconds for admin 

commands");

drivers/nvme/host/pci.c +0 -6
@@ -1031,63 +1030,6 @@ static irqreturn_t nvme_irq_check(int irq, 
 void *data)
 	return IRQ_WAKE_THREAD;
 }
 
-/*
- * Returns 0 on success.  If the result is negative, it's a Linux 
 error code;
- * if the result is positive, it's an NVM Express status code
- */
-int __nvme_submit_sync_cmd(struct request_queue *q, struct 
 nvme_command *cmd,
-		void *buffer, void __user *ubuffer, unsigned 

bufflen,

drivers/nvme/host/pci.c +0 -25
-		u32 *result, unsigned timeout)
-{
-	bool write = cmd->common.opcode & 1;
-	struct bio *bio = NULL;
-	struct request *req;
-	int ret;
-
-	req = blk_mq_alloc_request(q, write, GFP_KERNEL, false);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
-
-	req->cmd_type = REQ_TYPE_DRV_PRIV;
-	req->cmd_flags |= REQ_FAILFAST_DRIVER;
-	req->__data_len = 0;
-	req->__sector = (sector_t) -1;
-	req->bio = req->biotail = NULL;
-
-	req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
-
-	req->cmd = (unsigned char *)cmd;
-	req->cmd_len = sizeof(struct nvme_command);
-	req->special = (void *)0;
-
-	if (buffer && bufflen) {
-		ret = blk_rq_map_kern(q, req, buffer, bufflen, 

__GFP_WAIT);

drivers/nvme/host/pci.c +0 -4
-		if (ret)
-			goto out;
-	} else if (ubuffer && bufflen) {
-		ret = blk_rq_map_user(q, req, NULL, ubuffer, 

bufflen, __GFP_WAIT);

drivers/nvme/host/pci.c +0 -17
-		if (ret)
-			goto out;
-		bio = req->bio;
-	}
-
-	blk_execute_rq(req->q, NULL, req, 0);
-	if (bio)
-		blk_rq_unmap_user(bio);
-	if (result)
-		*result = (u32)(uintptr_t)req->special;
-	ret = req->errors;
- out:
-	blk_mq_free_request(req);
-	return ret;
-}
-
-int nvme_submit_sync_cmd(struct request_queue *q, struct 

nvme_command *cmd,

drivers/nvme/host/pci.c +0 -3
-		void *buffer, unsigned bufflen)
-{
-	return __nvme_submit_sync_cmd(q, cmd, buffer, NULL, bufflen, 

NULL, 0);

drivers/nvme/host/pci.c +0 -8
-}
-
 static int nvme_submit_async_admin_req(struct nvme_dev *dev)
 {
 	struct nvme_queue *nvmeq = dev->queues[0];
@@ -1199,99 +1141,6 @@ static int adapter_delete_sq(struct nvme_dev 
 *dev, u16 sqid)
 	return adapter_delete_queue(dev, nvme_admin_delete_sq, 
 sqid);
 }
 
-int nvme_identify_ctrl(struct nvme_dev *dev, struct nvme_id_ctrl 
 **id)
-{
-	struct nvme_command c = { };
-	int error;
-
-	/* gcc-4.4.4 (at least) has issues with initializers and 

anon unions */

drivers/nvme/host/pci.c +0 -21
-	c.identify.opcode = nvme_admin_identify;
-	c.identify.cns = cpu_to_le32(1);
-
-	*id = kmalloc(sizeof(struct nvme_id_ctrl), GFP_KERNEL);
-	if (!*id)
-		return -ENOMEM;
-
-	error = nvme_submit_sync_cmd(dev->admin_q, &c, *id,
-			sizeof(struct nvme_id_ctrl));
-	if (error)
-		kfree(*id);
-	return error;
-}
-
-int nvme_identify_ns(struct nvme_dev *dev, unsigned nsid,
-		struct nvme_id_ns **id)
-{
-	struct nvme_command c = { };
-	int error;
-
-	/* gcc-4.4.4 (at least) has issues with initializers and 

anon unions */

drivers/nvme/host/pci.c +0 -15
-	c.identify.opcode = nvme_admin_identify,
-	c.identify.nsid = cpu_to_le32(nsid),
-
-	*id = kmalloc(sizeof(struct nvme_id_ns), GFP_KERNEL);
-	if (!*id)
-		return -ENOMEM;
-
-	error = nvme_submit_sync_cmd(dev->admin_q, &c, *id,
-			sizeof(struct nvme_id_ns));
-	if (error)
-		kfree(*id);
-	return error;
-}
-
-int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned 

nsid,

drivers/nvme/host/pci.c +0 -1
-					dma_addr_t dma_addr, u32 

*result)

drivers/nvme/host/pci.c +0 -10
-{
-	struct nvme_command c;
-
-	memset(&c, 0, sizeof(c));
-	c.features.opcode = nvme_admin_get_features;
-	c.features.nsid = cpu_to_le32(nsid);
-	c.features.prp1 = cpu_to_le64(dma_addr);
-	c.features.fid = cpu_to_le32(fid);
-
-	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, NULL, 

0,

drivers/nvme/host/pci.c +0 -4
-			result, 0);
-}
-
-int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned 

dword11,

drivers/nvme/host/pci.c +0 -1
-					dma_addr_t dma_addr, u32 

*result)

drivers/nvme/host/pci.c +0 -10
-{
-	struct nvme_command c;
-
-	memset(&c, 0, sizeof(c));
-	c.features.opcode = nvme_admin_set_features;
-	c.features.prp1 = cpu_to_le64(dma_addr);
-	c.features.fid = cpu_to_le32(fid);
-	c.features.dword11 = cpu_to_le32(dword11);
-
-	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, NULL, 

0,

drivers/nvme/host/pci.c +0 -4
-			result, 0);
-}
-
-int nvme_get_log_page(struct nvme_dev *dev, struct nvme_smart_log 

**log)

drivers/nvme/host/pci.c +0 -8
-{
-	struct nvme_command c = { };
-	int error;
-
-	c.common.opcode = nvme_admin_get_log_page,
-	c.common.nsid = cpu_to_le32(0xFFFFFFFF),
-	c.common.cdw10[0] = cpu_to_le32(
-			(((sizeof(struct nvme_smart_log) / 4) - 1) 

<< 16) |

drivers/nvme/host/pci.c +0 -13
-			 NVME_LOG_SMART),
-
-	*log = kmalloc(sizeof(struct nvme_smart_log), GFP_KERNEL);
-	if (!*log)
-		return -ENOMEM;
-
-	error = nvme_submit_sync_cmd(dev->admin_q, &c, *log,
-			sizeof(struct nvme_smart_log));
-	if (error)
-		kfree(*log);
-	return error;
-}
-
 /**
  * nvme_abort_req - Attempt aborting a request
  *
author maintainer
#9 lore raw

PATCH 4/18 nvme: add a vendor field to struct nvme_dev

In reply to Christoph Hellwig

The SCSI translation layer currently has to poke into the PCI device structure to find a vendor ID for the device identification fallback. We won't nessecarily have a PCI device behind the device structure in the future, so add a new vendor field that can be filled out by the PCIe driver instead.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/nvme.h | 1 +
 drivers/nvme/host/pci.c  | 3 +++
 drivers/nvme/host/scsi.c | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)
drivers/nvme/host/nvme.h +1 -0
index 0633a7b..706f678 100644
@@ -69,6 +69,7 @@ struct nvme_dev {
 	u16 abort_limit;
 	u8 event_limit;
 	u8 vwc;
+	u16 vendor;
 };
 
 /*
drivers/nvme/host/pci.c +3 -0
index 628c572..cd731f5 100644
@@ -3025,6 +3025,9 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	INIT_WORK(&dev->reset_work, nvme_reset_work);
 	dev->dev = get_device(&pdev->dev);
 	pci_set_drvdata(pdev, dev);
+
+	dev->vendor = pdev->vendor;
+
 	result = nvme_set_instance(dev);
 	if (result)
 		goto put_pci;
drivers/nvme/host/scsi.c +1 -1
index c3d8d38..8f2d2c5 100644
@@ -657,7 +657,7 @@ static int nvme_trans_device_id_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		inq_response[6] = 0x00;    /* Rsvd */
 		inq_response[7] = 0x44;    /* Designator Length */
 
-		sprintf(&inq_response[8], "%04x", to_pci_dev(dev->dev)->vendor);
+		sprintf(&inq_response[8], "%04x", dev->vendor);
 		memcpy(&inq_response[12], dev->model, sizeof(dev->model));
 		sprintf(&inq_response[52], "%04x", tmp_id);
 		memcpy(&inq_response[56], dev->serial, sizeof(dev->serial));
Signature · 1 line
1.9.1
#10 lore raw

PATCH 4/18 nvme: add a vendor field to struct nvme_dev

Quoted 8 lines · Christoph Hellwig

The SCSI translation layer currently has to poke into the PCI device structure to find a vendor ID for the device identification fallback. We won't nessecarily have a PCI device behind the device structure in the future, so add a new vendor field that can be filled out by the PCIe driver instead.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>

looks good to me,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
#11 lore raw

PATCH 4/18 nvme: add a vendor field to struct nvme_dev

In reply to Christoph Hellwig
Quoted 46 lines · Christoph Hellwig

The SCSI translation layer currently has to poke into the PCI device structure to find a vendor ID for the device identification fallback. We won't nessecarily have a PCI device behind the device structure in the future, so add a new vendor field that can be filled out by the PCIe driver instead.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/nvme.h | 1 +
 drivers/nvme/host/pci.c  | 3 +++
 drivers/nvme/host/scsi.c | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)
drivers/nvme/host/nvme.h +1 -0
index 0633a7b..706f678 100644
@@ -69,6 +69,7 @@ struct nvme_dev {
 	u16 abort_limit;
 	u8 event_limit;
 	u8 vwc;
+	u16 vendor;
 };
 
 /*
drivers/nvme/host/pci.c +3 -0
index 628c572..cd731f5 100644
@@ -3025,6 +3025,9 @@ static int nvme_probe(struct pci_dev *pdev, 
 const struct pci_device_id *id)
 	INIT_WORK(&dev->reset_work, nvme_reset_work);
 	dev->dev = get_device(&pdev->dev);
 	pci_set_drvdata(pdev, dev);
+
+	dev->vendor = pdev->vendor;
+
 	result = nvme_set_instance(dev);
 	if (result)
drivers/nvme/host/pci.c +0 -0
 		goto put_pci;
drivers/nvme/host/scsi.c +0 -0
index c3d8d38..8f2d2c5 100644

drivers/nvme/host/scsi.c static int nvme_trans_device_id_page(struct · line 661

Quoted 7 lines
drivers/nvme/host/scsi.c +1 -2
@@ -657,7 +657,7 @@ static int nvme_trans_device_id_page(struct 
 nvme_ns *ns, struct sg_io_hdr *hdr,
 		inq_response[6] = 0x00;    /* Rsvd */
 		inq_response[7] = 0x44;    /* Designator Length */
 
-		sprintf(&inq_response[8], "%04x", to_pci_dev(dev
->dev)->vendor);
+		sprintf(&inq_response[8], "%04x", dev->vendor);

I'm ok with this patch, but I wanted to ask the question for my own benefit, what is the Linux kernel open-source practice of using sprintf() and string settings? I typically try to use snprintf().

Quoted 5 lines
drivers/nvme/host/scsi.c +0 -2
 		memcpy(&inq_response[12], dev->model, sizeof(dev
->model));
 		sprintf(&inq_response[52], "%04x", tmp_id);
 		memcpy(&inq_response[56], dev->serial, sizeof(dev
->serial));
#12 lore raw

PATCH 4/18 nvme: add a vendor field to struct nvme_dev

Quoted 8 lines · J Freyensee

Review

Quoted 3 lines · Christoph Hellwig
(quoted diff) +1 -2
-		sprintf(&inq_response[8], "%04x", to_pci_dev(dev
->dev)->vendor);
+		sprintf(&inq_response[8], "%04x", dev->vendor);

I'm ok with this patch, but I wanted to ask the question for my own benefit, what is the Linux kernel open-source practice of using sprintf() and string settings? I typically try to use snprintf().

Generally yes, snprintf is preferred, though the usage in this specific example is more similar to a memcpy. We know the buffer size and the length being copied into it is fixed; sprintf is just convenient.

author maintainer
#13 lore raw

PATCH 5/18 nvme: use offset instead of a struct for registers

In reply to Christoph Hellwig

This makes life easier for future non-PCI drivers where access to the registers might be more complicated. Note that Linux drivers are pretty evenly split between the two versions, and in fact the NVMe driver already uses offsets for the doorbells.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/nvme.h |  2 +-
 drivers/nvme/host/pci.c  | 58 +++++++++++++++++++++++++-----------------------
 drivers/nvme/host/scsi.c |  6 ++---
 include/linux/nvme.h     | 27 +++++++++++-----------
 4 files changed, 47 insertions(+), 46 deletions(-)
drivers/nvme/host/nvme.h +1 -1
index 706f678..370aa5b 100644
@@ -46,7 +46,7 @@ struct nvme_dev {
 	u32 db_stride;
 	u32 ctrl_config;
 	struct msix_entry *entry;
-	struct nvme_bar __iomem *bar;
+	void __iomem *bar;
 	struct list_head namespaces;
 	struct kref kref;
 	struct device *device;
drivers/nvme/host/pci.c +30 -28

181 lines not on this page · raw message

drivers/nvme/host/scsi.c +3 -3
index 8f2d2c5..a5f6af1 100644
@@ -611,7 +611,7 @@ static int nvme_trans_device_id_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 
 	memset(inq_response, 0, alloc_len);
 	inq_response[1] = INQ_DEVICE_IDENTIFICATION_PAGE;    /* Page Code */
-	if (readl(&dev->bar->vs) >= NVME_VS(1, 1)) {
+	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 1)) {
 		struct nvme_id_ns *id_ns;
 		void *eui;
 		int len;
@@ -623,7 +623,7 @@ static int nvme_trans_device_id_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 
 		eui = id_ns->eui64;
 		len = sizeof(id_ns->eui64);
-		if (readl(&dev->bar->vs) >= NVME_VS(1, 2)) {
+		if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2)) {
 			if (bitmap_empty(eui, len * 8)) {
 				eui = id_ns->nguid;
 				len = sizeof(id_ns->nguid);
@@ -2297,7 +2297,7 @@ static int nvme_trans_test_unit_ready(struct nvme_ns *ns,
 {
 	struct nvme_dev *dev = ns->dev;
 
-	if (!(readl(&dev->bar->csts) & NVME_CSTS_RDY))
+	if (!(readl(dev->bar + NVME_REG_CSTS) & NVME_CSTS_RDY))
 		return nvme_trans_completion(hdr, SAM_STAT_CHECK_CONDITION,
 					    NOT_READY, SCSI_ASC_LUN_NOT_READY,
 					    SCSI_ASCQ_CAUSE_NOT_REPORTABLE);
include/linux/nvme.h +13 -14
index 3af5f45..4d5e513 100644
@@ -17,20 +17,19 @@
 
 #include <linux/types.h>
 
-struct nvme_bar {
-	__u64			cap;	/* Controller Capabilities */
-	__u32			vs;	/* Version */
-	__u32			intms;	/* Interrupt Mask Set */
-	__u32			intmc;	/* Interrupt Mask Clear */
-	__u32			cc;	/* Controller Configuration */
-	__u32			rsvd1;	/* Reserved */
-	__u32			csts;	/* Controller Status */
-	__u32			nssr;	/* Subsystem Reset */
-	__u32			aqa;	/* Admin Queue Attributes */
-	__u64			asq;	/* Admin SQ Base Address */
-	__u64			acq;	/* Admin CQ Base Address */
-	__u32			cmbloc; /* Controller Memory Buffer Location */
-	__u32			cmbsz;  /* Controller Memory Buffer Size */
+enum {
+	NVME_REG_CAP	= 0x0000,	/* Controller Capabilities */
+	NVME_REG_VS	= 0x0008,	/* Version */
+	NVME_REG_INTMS	= 0x000c,	/* Interrupt Mask Set */
+	NVME_REG_INTMC	= 0x0010,	/* Interrupt Mask Set */
+	NVME_REG_CC	= 0x0014,	/* Controller Configuration */
+	NVME_REG_CSTS	= 0x001c,	/* Controller Status */
+	NVME_REG_NSSR	= 0x0020,	/* NVM Subsystem Reset */
+	NVME_REG_AQA	= 0x0024,	/* Admin Queue Attributes */
+	NVME_REG_ASQ	= 0x0028,	/* Admin SQ Base Address */
+	NVME_REG_ACQ	= 0x0030,	/* Admin SQ Base Address */
+	NVME_REG_CMBLOC = 0x0038,	/* Controller Memory Buffer Location */
+	NVME_REG_CMBSZ	= 0x0040,	/* Controller Memory Buffer Size */
 };
 
 #define NVME_CAP_MQES(cap)	((cap) & 0xffff)
Signature · 1 line
1.9.1
#14 lore raw

PATCH 5/18 nvme: use offset instead of a struct for registers

Quoted 9 lines · Christoph Hellwig

This makes life easier for future non-PCI drivers where access to the registers might be more complicated. Note that Linux drivers are pretty evenly split between the two versions, and in fact the NVMe driver already uses offsets for the doorbells.

include/linux/nvme.h +4 -0
+enum {
+	NVME_REG_CAP	= 0x0000,	/* Controller Capabilities */
+	NVME_REG_VS	= 0x0008,	/* Version */
+	NVME_REG_INTMS	= 0x000c,	/* Interrupt Mask Set */

include/linux/nvme.h line 33

Quoted 10 lines
include/linux/nvme.h +9 -0
+	NVME_REG_INTMC	= 0x0010,	/* Interrupt Mask Set */
+	NVME_REG_CC	= 0x0014,	/* Controller Configuration */
+	NVME_REG_CSTS	= 0x001c,	/* Controller Status */
+	NVME_REG_NSSR	= 0x0020,	/* NVM Subsystem Reset */
+	NVME_REG_AQA	= 0x0024,	/* Admin Queue Attributes */
+	NVME_REG_ASQ	= 0x0028,	/* Admin SQ Base Address */
+	NVME_REG_ACQ	= 0x0030,	/* Admin SQ Base Address */
+	NVME_REG_CMBLOC = 0x0038,	/* Controller Memory Buffer Location */
+	NVME_REG_CMBSZ	= 0x0040,	/* Controller Memory Buffer Size */
 };

Darn, CMBSZ is offset 0x3c. Missed that in the first review; controllers with this capability haven't made it to my sanity test machines yet...

#15 lore raw

PATCH 5/18 nvme: use offset instead of a struct for registers

In reply to Christoph Hellwig
Quoted 134 lines · Christoph Hellwig

This makes life easier for future non-PCI drivers where access to the registers might be more complicated. Note that Linux drivers are pretty evenly split between the two versions, and in fact the NVMe driver already uses offsets for the doorbells.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/nvme.h |  2 +-
 drivers/nvme/host/pci.c  | 58 +++++++++++++++++++++++++-------------
----------
 drivers/nvme/host/scsi.c |  6 ++---
 include/linux/nvme.h     | 27 +++++++++++-----------
 4 files changed, 47 insertions(+), 46 deletions(-)
drivers/nvme/host/nvme.h +1 -1
index 706f678..370aa5b 100644
@@ -46,7 +46,7 @@ struct nvme_dev {
 	u32 db_stride;
 	u32 ctrl_config;
 	struct msix_entry *entry;
-	struct nvme_bar __iomem *bar;
+	void __iomem *bar;
 	struct list_head namespaces;
 	struct kref kref;
 	struct device *device;
drivers/nvme/host/pci.c +1 -1
index cd731f5..6b0dcb6 100644
@@ -1307,7 +1307,7 @@ static void nvme_disable_queue(struct nvme_dev 
 *dev, int qid)
 
 	/* Don't tell the adapter to delete the admin queue.
 	 * Don't tell a removed adapter to delete IO queues. */
-	if (qid && readl(&dev->bar->csts) != -1) {
+	if (qid && readl(dev->bar + NVME_REG_CSTS) != -1) {
 		adapter_delete_sq(dev, qid);
 		adapter_delete_cq(dev, qid);
drivers/nvme/host/pci.c +1 -1
 	}
@@ -1460,7 +1460,7 @@ static int nvme_wait_ready(struct nvme_dev 
 *dev, u64 cap, bool enabled)
 
 	timeout = ((NVME_CAP_TIMEOUT(cap) + 1) * HZ / 2) + jiffies;
 
-	while ((readl(&dev->bar->csts) & NVME_CSTS_RDY) != bit) {
+	while ((readl(dev->bar + NVME_REG_CSTS) & NVME_CSTS_RDY) != 
 bit) {
 		msleep(100);
drivers/nvme/host/pci.c +1 -1
 		if (fatal_signal_pending(current))
 			return -EINTR;
@@ -1485,7 +1485,7 @@ static int nvme_disable_ctrl(struct nvme_dev 
 *dev, u64 cap)
 {
 	dev->ctrl_config &= ~NVME_CC_SHN_MASK;
 	dev->ctrl_config &= ~NVME_CC_ENABLE;
-	writel(dev->ctrl_config, &dev->bar->cc);
+	writel(dev->ctrl_config, dev->bar + NVME_REG_CC);
 
 	return nvme_wait_ready(dev, cap, false);

}

drivers/nvme/host/pci.c +1 -1
@@ -1494,7 +1494,7 @@ static int nvme_enable_ctrl(struct nvme_dev 
 *dev, u64 cap)
 {
 	dev->ctrl_config &= ~NVME_CC_SHN_MASK;
 	dev->ctrl_config |= NVME_CC_ENABLE;
-	writel(dev->ctrl_config, &dev->bar->cc);
+	writel(dev->ctrl_config, dev->bar + NVME_REG_CC);
 
 	return nvme_wait_ready(dev, cap, true);

}

drivers/nvme/host/pci.c +2 -2
@@ -1506,10 +1506,10 @@ static int nvme_shutdown_ctrl(struct nvme_dev 
 *dev)
 	dev->ctrl_config &= ~NVME_CC_SHN_MASK;
 	dev->ctrl_config |= NVME_CC_SHN_NORMAL;
 
-	writel(dev->ctrl_config, &dev->bar->cc);
+	writel(dev->ctrl_config, dev->bar + NVME_REG_CC);
 
 	timeout = SHUTDOWN_TIMEOUT + jiffies;
-	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
+	while ((readl(dev->bar + NVME_REG_CSTS) & 

NVME_CSTS_SHST_MASK) !=
NVME_CSTS_SH
ST_CMPLT) {

drivers/nvme/host/pci.c +1 -1
 		msleep(100);
 		if (fatal_signal_pending(current))
@@ -1584,7 +1584,7 @@ static int nvme_configure_admin_queue(struct 
 nvme_dev *dev)
 {
 	int result;
 	u32 aqa;
-	u64 cap = readq(&dev->bar->cap);
+	u64 cap = readq(dev->bar + NVME_REG_CAP);
 	struct nvme_queue *nvmeq;
 	unsigned page_shift = PAGE_SHIFT;
drivers/nvme/host/pci.c +1 -1
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
@@ -1605,11 +1605,12 @@ static int nvme_configure_admin_queue(struct 
 nvme_dev *dev)
 		page_shift = dev_page_max;
 	}
 
-	dev->subsystem = readl(&dev->bar->vs) >= NVME_VS(1, 1) ?
+	dev->subsystem = readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 

1) ?
NVME_CAP_NSSRC(cap)
: 0;

drivers/nvme/host/pci.c +0 -1
-	if (dev->subsystem && (readl(&dev->bar->csts) & 

NVME_CSTS_NSSRO))

drivers/nvme/host/pci.c +6 -4
-		writel(NVME_CSTS_NSSRO, &dev->bar->csts);
+	if (dev->subsystem &&
+	    (readl(dev->bar + NVME_REG_CSTS) & NVME_CSTS_NSSRO))
+		writel(NVME_CSTS_NSSRO, dev->bar + NVME_REG_CSTS);
 
 	result = nvme_disable_ctrl(dev, cap);
 	if (result < 0)
@@ -1632,9 +1633,9 @@ static int nvme_configure_admin_queue(struct 
 nvme_dev *dev)
 	dev->ctrl_config |= NVME_CC_ARB_RR | NVME_CC_SHN_NONE;
 	dev->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
 
-	writel(aqa, &dev->bar->aqa);
-	writeq(nvmeq->sq_dma_addr, &dev->bar->asq);
-	writeq(nvmeq->cq_dma_addr, &dev->bar->acq);
+	writel(aqa, dev->bar + NVME_REG_AQA);
+	writeq(nvmeq->sq_dma_addr, dev->bar + NVME_REG_ASQ);
+	writeq(nvmeq->cq_dma_addr, dev->bar + NVME_REG_ACQ);
 
 	result = nvme_enable_ctrl(dev, cap);
drivers/nvme/host/pci.c +0 -0
 	if (result)

drivers/nvme/host/pci.c static int nvme_subsys_reset(struct nvme_dev · line 1781

Quoted 6 lines
drivers/nvme/host/pci.c +1 -1
@@ -1776,7 +1777,7 @@ static int nvme_subsys_reset(struct nvme_dev 
 *dev)
 	if (!dev->subsystem)
 		return -ENOTTY;
 
-	writel(0x4E564D65, &dev->bar->nssr); /* "NVMe" */
+	writel(0x4E564D65, dev->bar + NVME_REG_NSSR); /* "NVMe" */

It would be nice if this value was a macro in a .h file as this is not necessarily specific to PCIe.

author maintainer
#16 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

In reply to Christoph Hellwig

The new struct nvme_ctrl will be used by the common NVMe code that sits on top of struct request_queue and the new nvme_ctrl_ops abstraction. It only contains the bare minimum required, which consists of values sampled during controller probe, the admin queue pointer and a second struct device pointer at the moment, but more will follow later. Only values that are not used in the I/O fast path should be moved to struct nvme_ctrl so that drivers can optimize their cache line usage easily. That's also the reason why we have two device pointers as the struct device is used for DMA mapping purposes.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c |  10 +--
 drivers/nvme/host/nvme.h |  61 ++++++---------
 drivers/nvme/host/pci.c  | 190 +++++++++++++++++++++++++++++++----------------
 drivers/nvme/host/scsi.c |  85 ++++++++++-----------
 4 files changed, 190 insertions(+), 156 deletions(-)
drivers/nvme/host/core.c +5 -5
index dfb528d..e2e8818 100644
@@ -78,7 +78,7 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 	return __nvme_submit_sync_cmd(q, cmd, buffer, NULL, bufflen, NULL, 0);
 }
 
-int nvme_identify_ctrl(struct nvme_dev *dev, struct nvme_id_ctrl **id)
+int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 {
 	struct nvme_command c = { };
 	int error;
@@ -98,7 +98,7 @@ int nvme_identify_ctrl(struct nvme_dev *dev, struct nvme_id_ctrl **id)
 	return error;
 }
 
-int nvme_identify_ns(struct nvme_dev *dev, unsigned nsid,
+int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,
 		struct nvme_id_ns **id)
 {
 	struct nvme_command c = { };
@@ -119,7 +119,7 @@ int nvme_identify_ns(struct nvme_dev *dev, unsigned nsid,
 	return error;
 }
 
-int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid,
+int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
 					dma_addr_t dma_addr, u32 *result)
 {
 	struct nvme_command c;
@@ -134,7 +134,7 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid,
 			result, 0);
 }
 
-int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11,
+int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 					dma_addr_t dma_addr, u32 *result)
 {
 	struct nvme_command c;
@@ -149,7 +149,7 @@ int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11,
 			result, 0);
 }
 
-int nvme_get_log_page(struct nvme_dev *dev, struct nvme_smart_log **log)
+int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log)
 {
 	struct nvme_command c = { };
 	int error;
drivers/nvme/host/nvme.h +22 -39

94 lines not on this page · raw message

drivers/nvme/host/pci.c +125 -65

498 lines not on this page · raw message

drivers/nvme/host/scsi.c +38 -47

320 lines not on this page · raw message

Signature · 1 line
1.9.1
#17 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

Quoted 5 lines · Christoph Hellwig

The new struct nvme_ctrl will be used by the common NVMe code that sits on top of struct request_queue and the new nvme_ctrl_ops abstraction. It only contains the bare minimum required, which consists of values sampled during controller probe, the admin queue pointer and a second struct device pointer at the moment,

Hi Christoph,

Can you explain why nvme_ctrl needs an additional struct device pointer? (I understand it will go away from your statement "at the moment" haven't read the rest of the patch set yet).

author maintainer
#18 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

Quoted 3 lines · Sagi Grimberg

Can you explain why nvme_ctrl needs an additional struct device pointer? (I understand it will go away from your statement "at the moment" haven't read the rest of the patch set yet).

No, it won't go away for the foreseeable future. I don't want to move any members used in the hot path to sturct nvme_ctrl so that drivers can be optimized in this path without impacting the generic code. Both for PCIe and Fabrics we need to squeeze the last bit of performance out and I don't want to have to cacheline optimize nvme_ctrl or even worse interactions between it and the containing structures.

#19 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Sagi Grimberg

Can you explain why nvme_ctrl needs an additional struct device pointer? (I understand it will go away from your statement "at the moment" haven't read the rest of the patch set yet).

Quoted 7 lines

No, it won't go away for the foreseeable future. I don't want to move any members used in the hot path to sturct nvme_ctrl so that drivers can be optimized in this path without impacting the generic code. Both for PCIe and Fabrics we need to squeeze the last bit of performance out and I don't want to have to cacheline optimize nvme_ctrl or even worse interactions between it and the containing structures.

I wasn't suggesting moving it to nvme_ctrl, I was suggesting to keep it in nvme_dev and maybe add a helper have nvme_ctrl_to_dev(ctrl)->dev used in slow paths. It would allow to remove the copy pointer in nvme_ctrl.

It's just a nit so I don't really mind keeping the copy.

author maintainer
#20 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

Quoted 3 lines · Sagi Grimberg

I wasn't suggesting moving it to nvme_ctrl, I was suggesting to keep it in nvme_dev and maybe add a helper have nvme_ctrl_to_dev(ctrl)->dev used in slow paths. It would allow to remove the copy pointer in nvme_ctrl.

But it would require adding a function pointer to get the location from the different transport drivers. A pointer seems much simpler than that.

#21 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · Sagi Grimberg

I wasn't suggesting moving it to nvme_ctrl, I was suggesting to keep it in nvme_dev and maybe add a helper have nvme_ctrl_to_dev(ctrl)->dev used in slow paths. It would allow to remove the copy pointer in nvme_ctrl.

But it would require adding a function pointer to get the location from the different transport drivers. A pointer seems much simpler than that.

Yea, I'm fine with that...

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
#22 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

In reply to Christoph Hellwig
Quoted 22 lines · Christoph Hellwig

The new struct nvme_ctrl will be used by the common NVMe code that sits
on top of struct request_queue and the new nvme_ctrl_ops abstraction. It only contains the bare minimum required, which consists of values sampled during controller probe, the admin queue pointer and a second struct device pointer at the moment, but more will follow later.
Only
values that are not used in the I/O fast path should be moved to struct nvme_ctrl so that drivers can optimize their cache line usage easily. That's also the reason why we have two device pointers as the struct device is used for DMA mapping purposes.

Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c |  10 +--
 drivers/nvme/host/nvme.h |  61 ++++++---------
 drivers/nvme/host/pci.c  | 190 +++++++++++++++++++++++++++++++------
----------
 drivers/nvme/host/scsi.c |  85 ++++++++++-----------
 4 files changed, 190 insertions(+), 156 deletions(-)

<snipped>

Quoted 12 lines
drivers/nvme/host/nvme.h +0 -2
index 370aa5b..3e409fa 100644
@@ -25,46 +25,16 @@ extern unsigned char nvme_io_timeout;
 extern unsigned char admin_timeout;
 #define ADMIN_TIMEOUT	(admin_timeout * HZ)
 
-/*
- * Represents an NVM Express device.  Each nvme_dev is a PCI 

function.
- */

drivers/nvme/host/nvme.h extern unsigned char nvme_io_timeout; · line 28

Quoted 4 lines
drivers/nvme/host/nvme.h +1 -3
-struct nvme_dev {
-	struct list_head node;
-	struct nvme_queue **queues;
+struct nvme_ctrl {

Whether it is a PCIe NVMe device with multiple controllers or something beyond PCIe, I think an instance of struct nvme_ctrl is going to need to know its cntlid. How does this struct know its cntlid? I'm not initially seeing it. I think it would make more sense to have struct nvme_ctrl have a member that stores its cntlid value. It would basically be the "name" of the specific nvme_ctrl instance allocated.

Quoted 19 lines
drivers/nvme/host/nvme.h +1 -15
+	const struct nvme_ctrl_ops *ops;
 	struct request_queue *admin_q;
-	struct blk_mq_tag_set tagset;
-	struct blk_mq_tag_set admin_tagset;
-	u32 __iomem *dbs;
 	struct device *dev;
-	struct dma_pool *prp_page_pool;
-	struct dma_pool *prp_small_pool;
 	int instance;
-	unsigned queue_count;
-	unsigned online_queues;
-	unsigned max_qid;
-	int q_depth;
-	u32 db_stride;
-	u32 ctrl_config;
-	struct msix_entry *entry;
-	void __iomem *bar;
-	struct list_head namespaces;
-	struct kref kref;

drivers/nvme/host/nvme.h extern unsigned char nvme_io_timeout; · line 60

Quoted 10 lines
drivers/nvme/host/nvme.h +1 -5
-	struct device *device;
-	struct work_struct reset_work;
-	struct work_struct probe_work;
-	struct work_struct scan_work;
+
 	char name[12];
 	char serial[20];
 	char model[40];
 	char firmware_rev[8];
-	bool subsystem;

Also, this is something probably a bit more far visioned, but I think struct nvme_ctrl would need a mechanism to know what NVMe subsystem it sits in. Even if 'subsystem' stayed in the struct, I'm not sure how a bool would work for this.

Quoted 11 lines
drivers/nvme/host/nvme.h +0 -7
-	u32 max_hw_sectors;
-	u32 stripe_size;
-	u32 page_size;
-	void __iomem *cmb;
-	dma_addr_t cmb_dma_addr;
-	u64 cmb_size;
-	u32 cmbsz;
 	u16 oncs;
 	u16 abort_limit;
 	u8 event_limit;

drivers/nvme/host/nvme.h struct nvme_dev { · line 51

Quoted 5 lines
drivers/nvme/host/nvme.h +1 -1
@@ -78,7 +48,7 @@ struct nvme_dev {
 struct nvme_ns {
 	struct list_head list;
 
-	struct nvme_dev *dev;
+	struct nvme_ctrl *ctrl;

This seems a bit backwards to me. It's the controller (cntlid) that is going to tell the host how many namespaces are associated with it via the NVMe Identify commands. Thus, I would have thought that a list of struct nvme_ns instances would be in a struct nvme_ctrl definition, not vice-versa. Unless '*ctrl' is going to be used as a back pointer? But then in 'struct nvme_ctrl' I didn't see initially see anything that associates itself to the namespaces attached to it.

Quoted 7 lines
drivers/nvme/host/nvme.h +0 -0
 	struct request_queue *queue;
 	struct gendisk *disk;
 	struct kref kref;
@@ -92,6 +62,19 @@ struct nvme_ns {
 	u32 mode_select_block_len;
 };
 
#23 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

Quoted 7 lines · J Freyensee

drivers/nvme/host/nvme.h extern unsigned char nvme_io_timeout; · line 60

Quoted 2 lines
drivers/nvme/host/nvme.h +0 -1
 	char firmware_rev[8];
-	bool subsystem;

Also, this is something probably a bit more far visioned, but I think struct nvme_ctrl would need a mechanism to know what NVMe subsystem it sits in. Even if 'subsystem' stayed in the struct, I'm not sure how a bool would work for this.

The bool is not to identify a specific subsystem. It's only to notify the driver that this controller is subsystem capable. In other words, please periodically check the subsystem reset notification since that can happen at anytime externally from host connected to the controller, and we want to know when such events occur.

Quoted 4 lines
Quoted 4 lines
drivers/nvme/host/nvme.h +0 -0
@@ -78,7 +48,7 @@ struct nvme_dev {
 struct nvme_ns {
 	struct list_head list;
 
Quoted 10 lines

drivers/nvme/host/nvme.h struct nvme_dev { · line 51

Quoted 2 lines
drivers/nvme/host/nvme.h +1 -1
-	struct nvme_dev *dev;
+	struct nvme_ctrl *ctrl;

This seems a bit backwards to me. It's the controller (cntlid) that is going to tell the host how many namespaces are associated with it via the NVMe Identify commands. Thus, I would have thought that a list of struct nvme_ns instances would be in a struct nvme_ctrl definition, not vice-versa. Unless '*ctrl' is going to be used as a back pointer? But then in 'struct nvme_ctrl' I didn't see initially see anything that associates itself to the namespaces attached to it.

The 'ctrl' pointer is a back pointer to the owning controller.

The controller itself contains a list_head appropriately called 'namespaces' to hold a reference to all its namespaces. The nvme_ns list_head 'list' is simply the entry item for that list.

#24 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

Quoted 39 lines · Keith
Quoted 10 lines · J Freyensee

drivers/nvme/host/nvme.h extern unsigned char nvme_io_timeout; · line 60

Quoted 2 lines
drivers/nvme/host/nvme.h +0 -1
 	char firmware_rev[8];
-	bool subsystem;

Also, this is something probably a bit more far visioned, but I think
struct nvme_ctrl would need a mechanism to know what NVMe subsystem it
sits in. Even if 'subsystem' stayed in the struct, I'm not sure how a
bool would work for this.

The bool is not to identify a specific subsystem. It's only to notify the driver that this controller is subsystem capable. In other words, please periodically check the subsystem reset notification since that can happen at anytime externally from host connected to the controller,
and we want to know when such events occur.

Quoted 4 lines
Quoted 4 lines
drivers/nvme/host/nvme.h +0 -0
@@ -78,7 +48,7 @@ struct nvme_dev {
 struct nvme_ns {
 	struct list_head list;
 
Quoted 15 lines

drivers/nvme/host/nvme.h struct nvme_dev { · line 51

Quoted 2 lines
drivers/nvme/host/nvme.h +1 -1
-	struct nvme_dev *dev;
+	struct nvme_ctrl *ctrl;

This seems a bit backwards to me. It's the controller (cntlid) that is
going to tell the host how many namespaces are associated with it via
the NVMe Identify commands. Thus, I would have thought that a list of
struct nvme_ns instances would be in a struct nvme_ctrl definition, not
vice-versa. Unless '*ctrl' is going to be used as a back pointer?
But
then in 'struct nvme_ctrl' I didn't see initially see anything that associates itself to the namespaces attached to it.

Quoted 5 lines

The 'ctrl' pointer is a back pointer to the owning controller.

The controller itself contains a list_head appropriately called 'namespaces' to hold a reference to all its namespaces. The nvme_ns list_head 'list' is simply the entry item for that list.

Yah, I saw that in a later patch in the 18 patch series and it made a lot more sense. Thanks!

author maintainer
#25 lore raw

PATCH 6/18 nvme: split a new struct nvme_ctrl out of struct nvme_dev

In reply to J Freyensee
Quoted 11 lines · J Freyensee

drivers/nvme/host/nvme.h extern unsigned char nvme_io_timeout; · line 28

Quoted 4 lines
drivers/nvme/host/nvme.h +1 -3
-struct nvme_dev {
-	struct list_head node;
-	struct nvme_queue **queues;
+struct nvme_ctrl {

Whether it is a PCIe NVMe device with multiple controllers or something beyond PCIe, I think an instance of struct nvme_ctrl is going to need to know its cntlid. How does this struct know its cntlid? I'm not initially seeing it. I think it would make more sense to have struct nvme_ctrl have a member that stores its cntlid value. It would basically be the "name" of the specific nvme_ctrl instance allocated.

Right now nothing in the Linux PCIe driver knows the cntlid. At least for PCIe it's also fairly uninteresting for the driver. Remeber this is just a simple split for now, additional functionality will be built on top of it.

Quoted 6 lines

drivers/nvme/host/nvme.h extern unsigned char nvme_io_timeout; · line 60

Quoted 1 line
drivers/nvme/host/nvme.h +0 -1
-	bool subsystem;

Also, this is something probably a bit more far visioned, but I think struct nvme_ctrl would need a mechanism to know what NVMe subsystem it sits in. Even if 'subsystem' stayed in the struct, I'm not sure how a bool would work for this.

See the reply for Keith on what this field does. But at least for PCIe the containing subsystem isn't all that interesting for the driver. Maybe as an additional safety check for the Namespae GUIDs in a multipathing setup, but that's about it.

Quoted 4 lines
Quoted 4 lines
drivers/nvme/host/nvme.h +0 -0
@@ -78,7 +48,7 @@ struct nvme_dev {
 struct nvme_ns {
 	struct list_head list;
 
Quoted 10 lines

drivers/nvme/host/nvme.h struct nvme_dev { · line 51

Quoted 2 lines
drivers/nvme/host/nvme.h +1 -1
-	struct nvme_dev *dev;
+	struct nvme_ctrl *ctrl;

This seems a bit backwards to me. It's the controller (cntlid) that is going to tell the host how many namespaces are associated with it via the NVMe Identify commands. Thus, I would have thought that a list of struct nvme_ns instances would be in a struct nvme_ctrl definition, not vice-versa. Unless '*ctrl' is going to be used as a back pointer? But then in 'struct nvme_ctrl' I didn't see initially see anything that associates itself to the namespaces attached to it.

The namespace lists moves to nvme_ctrl in this list, and the dev field has always been a backpointer that's now replaced with a backpointer to the nvme_ctrl structure.

author maintainer
#26 lore raw

PATCH 7/18 nvme: simplify nvme_setup_prps calling convention

In reply to Christoph Hellwig

Pass back a true/false value instead of the length which needs a compare with the bytes in the request and drop the pointless gfp_t argument.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
drivers/nvme/host/pci.c +10 -12
index 8b0ba11..9dbb0ec 100644
@@ -719,9 +719,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	blk_mq_complete_request(req, error);
 }
 
-/* length is in bytes.  gfp flags indicates whether we may sleep. */
-static int nvme_setup_prps(struct nvme_dev *dev, struct nvme_iod *iod,
-		int total_len, gfp_t gfp)
+static bool nvme_setup_prps(struct nvme_dev *dev, struct nvme_iod *iod,
+		int total_len)
 {
 	struct dma_pool *pool;
 	int length = total_len;
@@ -737,7 +736,7 @@ static int nvme_setup_prps(struct nvme_dev *dev, struct nvme_iod *iod,
 
 	length -= (page_size - offset);
 	if (length <= 0)
-		return total_len;
+		return true;
 
 	dma_len -= (page_size - offset);
 	if (dma_len) {
@@ -750,7 +749,7 @@ static int nvme_setup_prps(struct nvme_dev *dev, struct nvme_iod *iod,
 
 	if (length <= page_size) {
 		iod->first_dma = dma_addr;
-		return total_len;
+		return true;
 	}
 
 	nprps = DIV_ROUND_UP(length, page_size);
@@ -762,11 +761,11 @@ static int nvme_setup_prps(struct nvme_dev *dev, struct nvme_iod *iod,
 		iod->npages = 1;
 	}
 
-	prp_list = dma_pool_alloc(pool, gfp, &prp_dma);
+	prp_list = dma_pool_alloc(pool, GFP_ATOMIC, &prp_dma);
 	if (!prp_list) {
 		iod->first_dma = dma_addr;
 		iod->npages = -1;
-		return (total_len - length) + page_size;
+		return false;
 	}
 	list[0] = prp_list;
 	iod->first_dma = prp_dma;
@@ -774,9 +773,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, struct nvme_iod *iod,
 	for (;;) {
 		if (i == page_size >> 3) {
 			__le64 *old_prp_list = prp_list;
-			prp_list = dma_pool_alloc(pool, gfp, &prp_dma);
+			prp_list = dma_pool_alloc(pool, GFP_ATOMIC, &prp_dma);
 			if (!prp_list)
-				return total_len - length;
+				return false;
 			list[iod->npages++] = prp_list;
 			prp_list[0] = old_prp_list[i - 1];
 			old_prp_list[i - 1] = cpu_to_le64(prp_dma);
@@ -796,7 +795,7 @@ static int nvme_setup_prps(struct nvme_dev *dev, struct nvme_iod *iod,
 		dma_len = sg_dma_len(sg);
 	}
 
-	return total_len;
+	return true;
 }
 
 static void nvme_submit_priv(struct nvme_queue *nvmeq, struct request *req,
@@ -962,8 +961,7 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 		if (!dma_map_sg(nvmeq->q_dmadev, iod->sg, iod->nents, dma_dir))
 			goto retry_cmd;
 
-		if (blk_rq_bytes(req) !=
-                    nvme_setup_prps(dev, iod, blk_rq_bytes(req), GFP_ATOMIC)) {
+		if (!nvme_setup_prps(dev, iod, blk_rq_bytes(req))) {
 			dma_unmap_sg(dev->dev, iod->sg, iod->nents, dma_dir);
 			goto retry_cmd;
 		}
Signature · 1 line
1.9.1
#27 lore raw

PATCH 7/18 nvme: simplify nvme_setup_prps calling convention

Quoted 4 lines · Christoph Hellwig

Pass back a true/false value instead of the length which needs a compare with the bytes in the request and drop the pointless gfp_t argument.

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

Looks good to me,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
author maintainer
#28 lore raw

PATCH 8/18 nvme: refactor nvme_queue_rq

In reply to Christoph Hellwig

This "backports" the structure I've used for the fabrics driver. It mostly started out as a cleanup so that I could actually understand the code, but I think it also qualifies as a micro-optimization due to the reduced time we hold q_lock and disable interrupts.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 219 +++++++++++++++++++++---------------------------
 1 file changed, 97 insertions(+), 122 deletions(-)
drivers/nvme/host/pci.c +97 -122
index 9dbb0ec..dfe914e 100644
@@ -798,19 +798,53 @@ static bool nvme_setup_prps(struct nvme_dev *dev, struct nvme_iod *iod,
 	return true;
 }
 
-static void nvme_submit_priv(struct nvme_queue *nvmeq, struct request *req,
-		struct nvme_iod *iod)
+static int nvme_map_data(struct nvme_dev *dev, struct nvme_iod *iod,
+		struct nvme_command *cmnd)
 {
-	struct nvme_command cmnd;
+	struct request *req = iod_get_private(iod);
+	struct request_queue *q = req->q;
+	enum dma_data_direction dma_dir = rq_data_dir(req) ?
+			DMA_TO_DEVICE : DMA_FROM_DEVICE;
+	int ret = BLK_MQ_RQ_QUEUE_ERROR;
+
+	sg_init_table(iod->sg, req->nr_phys_segments);
+	iod->nents = blk_rq_map_sg(q, req, iod->sg);
+	if (!iod->nents)
+		goto out;
+
+	ret = BLK_MQ_RQ_QUEUE_BUSY;
+	if (!dma_map_sg(dev->dev, iod->sg, iod->nents, dma_dir))
+		goto out;
+
+	if (!nvme_setup_prps(dev, iod, blk_rq_bytes(req)))
+		goto out_unmap;
+
+	ret = BLK_MQ_RQ_QUEUE_ERROR;
+	if (blk_integrity_rq(req)) {
+		if (blk_rq_count_integrity_sg(q, req->bio) != 1)
+			goto out_unmap;
+
+		sg_init_table(iod->meta_sg, 1);
+		if (blk_rq_map_integrity_sg(q, req->bio, iod->meta_sg) != 1)
+			goto out_unmap;
 
-	memcpy(&cmnd, req->cmd, sizeof(cmnd));
-	cmnd.rw.command_id = req->tag;
-	if (req->nr_phys_segments) {
-		cmnd.rw.prp1 = cpu_to_le64(sg_dma_address(iod->sg));
-		cmnd.rw.prp2 = cpu_to_le64(iod->first_dma);
+		if (rq_data_dir(req))
+			nvme_dif_remap(req, nvme_dif_prep);
+
+		if (!dma_map_sg(dev->dev, iod->meta_sg, 1, dma_dir))
+			goto out_unmap;
 	}
 
-	__nvme_submit_cmd(nvmeq, &cmnd);
+	cmnd->rw.prp1 = cpu_to_le64(sg_dma_address(iod->sg));
+	cmnd->rw.prp2 = cpu_to_le64(iod->first_dma);
+	if (blk_integrity_rq(req))
+		cmnd->rw.metadata = cpu_to_le64(sg_dma_address(iod->meta_sg));
+	return BLK_MQ_RQ_QUEUE_OK;
+
+out_unmap:
+	dma_unmap_sg(dev->dev, iod->sg, iod->nents, dma_dir);
+out:
+	return ret;
 }
 
 /*
@@ -818,46 +852,42 @@ static void nvme_submit_priv(struct nvme_queue *nvmeq, struct request *req,
  * worth having a special pool for these or additional cases to handle freeing
  * the iod.
  */
-static void nvme_submit_discard(struct nvme_queue *nvmeq, struct nvme_ns *ns,
-		struct request *req, struct nvme_iod *iod)
+static int nvme_setup_discard(struct nvme_queue *nvmeq, struct nvme_ns *ns,
+		struct nvme_iod *iod, struct nvme_command *cmnd)
 {
-	struct nvme_dsm_range *range =
-				(struct nvme_dsm_range *)iod_list(iod)[0];
-	struct nvme_command cmnd;
+	struct request *req = iod_get_private(iod);
+	struct nvme_dsm_range *range;
+
+	range = dma_pool_alloc(nvmeq->dev->prp_small_pool, GFP_ATOMIC,
+						&iod->first_dma);
+	if (!range)
+		return BLK_MQ_RQ_QUEUE_BUSY;
+	iod_list(iod)[0] = (__le64 *)range;
+	iod->npages = 0;
 
 	range->cattr = cpu_to_le32(0);
 	range->nlb = cpu_to_le32(blk_rq_bytes(req) >> ns->lba_shift);
 	range->slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
 
-	memset(&cmnd, 0, sizeof(cmnd));
-	cmnd.dsm.opcode = nvme_cmd_dsm;
-	cmnd.dsm.command_id = req->tag;
-	cmnd.dsm.nsid = cpu_to_le32(ns->ns_id);
-	cmnd.dsm.prp1 = cpu_to_le64(iod->first_dma);
-	cmnd.dsm.nr = 0;
-	cmnd.dsm.attributes = cpu_to_le32(NVME_DSMGMT_AD);
-
-	__nvme_submit_cmd(nvmeq, &cmnd);
+	memset(cmnd, 0, sizeof(*cmnd));
+	cmnd->dsm.opcode = nvme_cmd_dsm;
+	cmnd->dsm.nsid = cpu_to_le32(ns->ns_id);
+	cmnd->dsm.prp1 = cpu_to_le64(iod->first_dma);
+	cmnd->dsm.nr = 0;
+	cmnd->dsm.attributes = cpu_to_le32(NVME_DSMGMT_AD);
+	return BLK_MQ_RQ_QUEUE_OK;
 }
 
-static void nvme_submit_flush(struct nvme_queue *nvmeq, struct nvme_ns *ns,
-								int cmdid)
+static void nvme_setup_flush(struct nvme_ns *ns, struct nvme_command *cmnd)
 {
-	struct nvme_command cmnd;
-
-	memset(&cmnd, 0, sizeof(cmnd));
-	cmnd.common.opcode = nvme_cmd_flush;
-	cmnd.common.command_id = cmdid;
-	cmnd.common.nsid = cpu_to_le32(ns->ns_id);
-
-	__nvme_submit_cmd(nvmeq, &cmnd);
+	memset(cmnd, 0, sizeof(*cmnd));
+	cmnd->common.opcode = nvme_cmd_flush;
+	cmnd->common.nsid = cpu_to_le32(ns->ns_id);
 }
 
-static int nvme_submit_iod(struct nvme_queue *nvmeq, struct nvme_iod *iod,
-							struct nvme_ns *ns)
+static void nvme_setup_rw(struct nvme_ns *ns, struct request *req,
+		struct nvme_command *cmnd)
 {
-	struct request *req = iod_get_private(iod);
-	struct nvme_command cmnd;
 	u16 control = 0;
 	u32 dsmgmt = 0;
 
@@ -869,14 +899,12 @@ static int nvme_submit_iod(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 	if (req->cmd_flags & REQ_RAHEAD)
 		dsmgmt |= NVME_RW_DSM_FREQ_PREFETCH;
 
-	memset(&cmnd, 0, sizeof(cmnd));
-	cmnd.rw.opcode = (rq_data_dir(req) ? nvme_cmd_write : nvme_cmd_read);
-	cmnd.rw.command_id = req->tag;
-	cmnd.rw.nsid = cpu_to_le32(ns->ns_id);
-	cmnd.rw.prp1 = cpu_to_le64(sg_dma_address(iod->sg));
-	cmnd.rw.prp2 = cpu_to_le64(iod->first_dma);
-	cmnd.rw.slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
-	cmnd.rw.length = cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
+	memset(cmnd, 0, sizeof(*cmnd));
+	cmnd->rw.opcode = (rq_data_dir(req) ? nvme_cmd_write : nvme_cmd_read);
+	cmnd->rw.command_id = req->tag;
+	cmnd->rw.nsid = cpu_to_le32(ns->ns_id);
+	cmnd->rw.slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
+	cmnd->rw.length = cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
 
 	if (ns->ms) {
 		switch (ns->pi_type) {
@@ -887,23 +915,16 @@ static int nvme_submit_iod(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 		case NVME_NS_DPS_PI_TYPE2:
 			control |= NVME_RW_PRINFO_PRCHK_GUARD |
 					NVME_RW_PRINFO_PRCHK_REF;
-			cmnd.rw.reftag = cpu_to_le32(
+			cmnd->rw.reftag = cpu_to_le32(
 					nvme_block_nr(ns, blk_rq_pos(req)));
 			break;
 		}
-		if (blk_integrity_rq(req))
-			cmnd.rw.metadata =
-				cpu_to_le64(sg_dma_address(iod->meta_sg));
-		else
+		if (!blk_integrity_rq(req))
 			control |= NVME_RW_PRINFO_PRACT;
 	}
 
-	cmnd.rw.control = cpu_to_le16(control);
-	cmnd.rw.dsmgmt = cpu_to_le32(dsmgmt);
-
-	__nvme_submit_cmd(nvmeq, &cmnd);
-
-	return 0;
+	cmnd->rw.control = cpu_to_le16(control);
+	cmnd->rw.dsmgmt = cpu_to_le32(dsmgmt);
 }
 
 /*
@@ -918,7 +939,8 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct request *req = bd->rq;
 	struct nvme_cmd_info *cmd = blk_mq_rq_to_pdu(req);
 	struct nvme_iod *iod;
-	enum dma_data_direction dma_dir;
+	struct nvme_command cmnd;
+	int ret = BLK_MQ_RQ_QUEUE_OK;
 
 	/*
 	 * If formated with metadata, require the block layer provide a buffer
@@ -938,80 +960,33 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 		return BLK_MQ_RQ_QUEUE_BUSY;
 
 	if (req->cmd_flags & REQ_DISCARD) {
-		void *range;
-		/*
-		 * We reuse the small pool to allocate the 16-byte range here
-		 * as it is not worth having a special pool for these or
-		 * additional cases to handle freeing the iod.
-		 */
-		range = dma_pool_alloc(dev->prp_small_pool, GFP_ATOMIC,
-						&iod->first_dma);
-		if (!range)
-			goto retry_cmd;
-		iod_list(iod)[0] = (__le64 *)range;
-		iod->npages = 0;
-	} else if (req->nr_phys_segments) {
-		dma_dir = rq_data_dir(req) ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
-
-		sg_init_table(iod->sg, req->nr_phys_segments);
-		iod->nents = blk_rq_map_sg(req->q, req, iod->sg);
-		if (!iod->nents)
-			goto error_cmd;
-
-		if (!dma_map_sg(nvmeq->q_dmadev, iod->sg, iod->nents, dma_dir))
-			goto retry_cmd;
-
-		if (!nvme_setup_prps(dev, iod, blk_rq_bytes(req))) {
-			dma_unmap_sg(dev->dev, iod->sg, iod->nents, dma_dir);
-			goto retry_cmd;
-		}
-		if (blk_integrity_rq(req)) {
-			if (blk_rq_count_integrity_sg(req->q, req->bio) != 1) {
-				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
-						dma_dir);
-				goto error_cmd;
-			}
-
-			sg_init_table(iod->meta_sg, 1);
-			if (blk_rq_map_integrity_sg(
-					req->q, req->bio, iod->meta_sg) != 1) {
-				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
-						dma_dir);
-				goto error_cmd;
-			}
-
-			if (rq_data_dir(req))
-				nvme_dif_remap(req, nvme_dif_prep);
+		ret = nvme_setup_discard(nvmeq, ns, iod, &cmnd);
+	} else {
+		if (req->cmd_type == REQ_TYPE_DRV_PRIV)
+			memcpy(&cmnd, req->cmd, sizeof(cmnd));
+		else if (req->cmd_flags & REQ_FLUSH)
+			nvme_setup_flush(ns, &cmnd);
+		else
+			nvme_setup_rw(ns, req, &cmnd);
 
-			if (!dma_map_sg(nvmeq->q_dmadev, iod->meta_sg, 1, dma_dir)) {
-				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
-						dma_dir);
-				goto error_cmd;
-			}
-		}
+		if (req->nr_phys_segments)
+			ret = nvme_map_data(dev, iod, &cmnd);
 	}
 
+	if (ret)
+		goto out;
+
+	cmnd.common.command_id = req->tag;
 	nvme_set_info(cmd, iod, req_completion);
-	spin_lock_irq(&nvmeq->q_lock);
-	if (req->cmd_type == REQ_TYPE_DRV_PRIV)
-		nvme_submit_priv(nvmeq, req, iod);
-	else if (req->cmd_flags & REQ_DISCARD)
-		nvme_submit_discard(nvmeq, ns, req, iod);
-	else if (req->cmd_flags & REQ_FLUSH)
-		nvme_submit_flush(nvmeq, ns, req->tag);
-	else
-		nvme_submit_iod(nvmeq, iod, ns);
 
+	spin_lock_irq(&nvmeq->q_lock);
+	__nvme_submit_cmd(nvmeq, &cmnd);
 	nvme_process_cq(nvmeq);
 	spin_unlock_irq(&nvmeq->q_lock);
 	return BLK_MQ_RQ_QUEUE_OK;
-
- error_cmd:
-	nvme_free_iod(dev, iod);
-	return BLK_MQ_RQ_QUEUE_ERROR;
- retry_cmd:
+out:
 	nvme_free_iod(dev, iod);
-	return BLK_MQ_RQ_QUEUE_BUSY;
+	return ret;
 }
 
 static int nvme_process_cq(struct nvme_queue *nvmeq)
Signature · 1 line
1.9.1
author maintainer
#29 lore raw

PATCH 9/18 nvme: move nvme_error_status to common code

In reply to Christoph Hellwig

And mark it inline so that we don't slow down the completion path by having to turn it into a forced out of line call.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/nvme.h | 12 ++++++++++++
 drivers/nvme/host/pci.c  | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)
drivers/nvme/host/nvme.h +12 -0
index 3e409fa..a4f2f2c 100644
@@ -80,6 +80,18 @@ static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector)
 	return (sector >> (ns->lba_shift - 9));
 }
 
+static inline int nvme_error_status(u16 status)
+{
+	switch (status & 0x7ff) {
+	case NVME_SC_SUCCESS:
+		return 0;
+	case NVME_SC_CAP_EXCEEDED:
+		return -ENOSPC;
+	default:
+		return -EIO;
+	}
+}
+
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void *buf, unsigned bufflen);
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
drivers/nvme/host/pci.c +0 -12
index dfe914e..7ed7dcb 100644
@@ -543,18 +543,6 @@ static void nvme_free_iod(struct nvme_dev *dev, struct nvme_iod *iod)
 		kfree(iod);
 }
 
-static int nvme_error_status(u16 status)
-{
-	switch (status & 0x7ff) {
-	case NVME_SC_SUCCESS:
-		return 0;
-	case NVME_SC_CAP_EXCEEDED:
-		return -ENOSPC;
-	default:
-		return -EIO;
-	}
-}
-
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 static void nvme_dif_prep(u32 p, u32 v, struct t10_pi_tuple *pi)
 {
Signature · 1 line
1.9.1
#30 lore raw

PATCH 9/18 nvme: move nvme_error_status to common code

Quoted 4 lines · Christoph Hellwig

And mark it inline so that we don't slow down the completion path by having to turn it into a forced out of line call.

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

Looks good,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
author maintainer
#31 lore raw

PATCH 10/18 nvme: move nvme_setup_flush and nvme_setup_rw to common code

In reply to Christoph Hellwig

And mark them inline so that we don't slow down the I/O submission path by having to turn it into a forced out of line call.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/nvme.h | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/pci.c  | 49 ----------------------------------------------
 2 files changed, 51 insertions(+), 49 deletions(-)
drivers/nvme/host/nvme.h +51 -0
index a4f2f2c..6c77db7 100644
@@ -80,6 +80,57 @@ static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector)
 	return (sector >> (ns->lba_shift - 9));
 }
 
+static inline void nvme_setup_flush(struct nvme_ns *ns,
+		struct nvme_command *cmnd)
+{
+	memset(cmnd, 0, sizeof(*cmnd));
+	cmnd->common.opcode = nvme_cmd_flush;
+	cmnd->common.nsid = cpu_to_le32(ns->ns_id);
+}
+
+static inline void nvme_setup_rw(struct nvme_ns *ns, struct request *req,
+		struct nvme_command *cmnd)
+{
+	u16 control = 0;
+	u32 dsmgmt = 0;
+
+	if (req->cmd_flags & REQ_FUA)
+		control |= NVME_RW_FUA;
+	if (req->cmd_flags & (REQ_FAILFAST_DEV | REQ_RAHEAD))
+		control |= NVME_RW_LR;
+
+	if (req->cmd_flags & REQ_RAHEAD)
+		dsmgmt |= NVME_RW_DSM_FREQ_PREFETCH;
+
+	memset(cmnd, 0, sizeof(*cmnd));
+	cmnd->rw.opcode = (rq_data_dir(req) ? nvme_cmd_write : nvme_cmd_read);
+	cmnd->rw.command_id = req->tag;
+	cmnd->rw.nsid = cpu_to_le32(ns->ns_id);
+	cmnd->rw.slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
+	cmnd->rw.length = cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
+
+	if (ns->ms) {
+		switch (ns->pi_type) {
+		case NVME_NS_DPS_PI_TYPE3:
+			control |= NVME_RW_PRINFO_PRCHK_GUARD;
+			break;
+		case NVME_NS_DPS_PI_TYPE1:
+		case NVME_NS_DPS_PI_TYPE2:
+			control |= NVME_RW_PRINFO_PRCHK_GUARD |
+					NVME_RW_PRINFO_PRCHK_REF;
+			cmnd->rw.reftag = cpu_to_le32(
+					nvme_block_nr(ns, blk_rq_pos(req)));
+			break;
+		}
+		if (!blk_integrity_rq(req))
+			control |= NVME_RW_PRINFO_PRACT;
+	}
+
+	cmnd->rw.control = cpu_to_le16(control);
+	cmnd->rw.dsmgmt = cpu_to_le32(dsmgmt);
+}
+
+
 static inline int nvme_error_status(u16 status)
 {
 	switch (status & 0x7ff) {
drivers/nvme/host/pci.c +0 -49

55 lines not on this page · raw message

Signature · 1 line
1.9.1
#32 lore raw

PATCH 10/18 nvme: move nvme_setup_flush and nvme_setup_rw to common code

Quoted 58 lines · Christoph Hellwig

And mark them inline so that we don't slow down the I/O submission path by having to turn it into a forced out of line call.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
  drivers/nvme/host/nvme.h | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
  drivers/nvme/host/pci.c  | 49 ----------------------------------------------
  2 files changed, 51 insertions(+), 49 deletions(-)
drivers/nvme/host/nvme.h +41 -0
index a4f2f2c..6c77db7 100644
@@ -80,6 +80,57 @@ static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector)
  	return (sector >> (ns->lba_shift - 9));
  }
 
+static inline void nvme_setup_flush(struct nvme_ns *ns,
+		struct nvme_command *cmnd)
+{
+	memset(cmnd, 0, sizeof(*cmnd));
+	cmnd->common.opcode = nvme_cmd_flush;
+	cmnd->common.nsid = cpu_to_le32(ns->ns_id);
+}
+
+static inline void nvme_setup_rw(struct nvme_ns *ns, struct request *req,
+		struct nvme_command *cmnd)
+{
+	u16 control = 0;
+	u32 dsmgmt = 0;
+
+	if (req->cmd_flags & REQ_FUA)
+		control |= NVME_RW_FUA;
+	if (req->cmd_flags & (REQ_FAILFAST_DEV | REQ_RAHEAD))
+		control |= NVME_RW_LR;
+
+	if (req->cmd_flags & REQ_RAHEAD)
+		dsmgmt |= NVME_RW_DSM_FREQ_PREFETCH;
+
+	memset(cmnd, 0, sizeof(*cmnd));
+	cmnd->rw.opcode = (rq_data_dir(req) ? nvme_cmd_write : nvme_cmd_read);
+	cmnd->rw.command_id = req->tag;
+	cmnd->rw.nsid = cpu_to_le32(ns->ns_id);
+	cmnd->rw.slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
+	cmnd->rw.length = cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
+
+	if (ns->ms) {
+		switch (ns->pi_type) {
+		case NVME_NS_DPS_PI_TYPE3:
+			control |= NVME_RW_PRINFO_PRCHK_GUARD;
+			break;
+		case NVME_NS_DPS_PI_TYPE1:
+		case NVME_NS_DPS_PI_TYPE2:
+			control |= NVME_RW_PRINFO_PRCHK_GUARD |
+					NVME_RW_PRINFO_PRCHK_REF;
+			cmnd->rw.reftag = cpu_to_le32(
+					nvme_block_nr(ns, blk_rq_pos(req)));
+			break;

drivers/nvme/host/nvme.h static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector) · line 133

Quoted 10 lines
drivers/nvme/host/nvme.h +10 -0
+		}
+		if (!blk_integrity_rq(req))
+			control |= NVME_RW_PRINFO_PRACT;
+	}
+
+	cmnd->rw.control = cpu_to_le16(control);
+	cmnd->rw.dsmgmt = cpu_to_le32(dsmgmt);
+}
+
+

Hi Christoph,

I do agree that making these static inline can speed things up here, but the coding style documentation asks to avoid inline'ing functions longer than a few lines of code (See Documentation/CodingStyle Chapter 15: "The inline disease").

Do you think this case qualifies as an exception?

author maintainer
#33 lore raw

PATCH 10/18 nvme: move nvme_setup_flush and nvme_setup_rw to common code

Quoted 6 lines · Sagi Grimberg

I do agree that making these static inline can speed things up here, but the coding style documentation asks to avoid inline'ing functions longer than a few lines of code (See Documentation/CodingStyle Chapter 15: "The inline disease").

Do you think this case qualifies as an exception?

Yes. The inline is only used once per NVMe transport driver and it's used in the absolute fast path in a place where being able to optimize the assignments inside and outside the function call will become useful. Also all the code is trivial assignments and simple conditionals so it's actually pretty small in terms of generated code.

#34 lore raw

PATCH 10/18 nvme: move nvme_setup_flush and nvme_setup_rw to common code

Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Sagi Grimberg

I do agree that making these static inline can speed things up here, but the coding style documentation asks to avoid inline'ing functions longer than a few lines of code (See Documentation/CodingStyle Chapter 15: "The inline disease").

Quoted 8 lines
Quoted 1 line

Do you think this case qualifies as an exception?

Yes. The inline is only used once per NVMe transport driver and it's used in the absolute fast path in a place where being able to optimize the assignments inside and outside the function call will become useful. Also all the code is trivial assignments and simple conditionals so it's actually pretty small in terms of generated code.

I agree it is better to have them inline...

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
author maintainer
#35 lore raw

PATCH 11/18 nvme: split __nvme_submit_sync_cmd

In reply to Christoph Hellwig

Add a separate nvme_submit_user_cmd for commands that directly DMA to or from userspace. We'll add metadata support to that soon and the common version would become too messy.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 82 ++++++++++++++++++++++++++++++++++--------------
 drivers/nvme/host/nvme.h |  6 ++--
 drivers/nvme/host/pci.c  |  6 ++--
 drivers/nvme/host/scsi.c |  4 +--
 4 files changed, 67 insertions(+), 31 deletions(-)
drivers/nvme/host/core.c +58 -24
index e2e8818..9d05df0 100644
@@ -21,22 +21,15 @@
 
 #include "nvme.h"
 
-/*
- * Returns 0 on success.  If the result is negative, it's a Linux error code;
- * if the result is positive, it's an NVM Express status code
- */
-int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
-		void *buffer, void __user *ubuffer, unsigned bufflen,
-		u32 *result, unsigned timeout)
+static struct request *nvme_alloc_request(struct request_queue *q,
+		struct nvme_command *cmd)
 {
 	bool write = cmd->common.opcode & 1;
-	struct bio *bio = NULL;
 	struct request *req;
-	int ret;
 
 	req = blk_mq_alloc_request(q, write, GFP_KERNEL, false);
 	if (IS_ERR(req))
-		return PTR_ERR(req);
+		return req;
 
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 	req->cmd_flags |= REQ_FAILFAST_DRIVER;
@@ -44,26 +37,36 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 	req->__sector = (sector_t) -1;
 	req->bio = req->biotail = NULL;
 
-	req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
-
 	req->cmd = (unsigned char *)cmd;
 	req->cmd_len = sizeof(struct nvme_command);
 	req->special = (void *)0;
 
+	return req;
+}
+
+/*
+ * Returns 0 on success.  If the result is negative, it's a Linux error code;
+ * if the result is positive, it's an NVM Express status code
+ */
+int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
+		void *buffer, unsigned bufflen, u32 *result, unsigned timeout)
+{
+	struct request *req;
+	int ret;
+
+	req = nvme_alloc_request(q, cmd);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+
 	if (buffer && bufflen) {
 		ret = blk_rq_map_kern(q, req, buffer, bufflen, __GFP_WAIT);
 		if (ret)
 			goto out;
-	} else if (ubuffer && bufflen) {
-		ret = blk_rq_map_user(q, req, NULL, ubuffer, bufflen, __GFP_WAIT);
-		if (ret)
-			goto out;
-		bio = req->bio;
 	}
 
 	blk_execute_rq(req->q, NULL, req, 0);
-	if (bio)
-		blk_rq_unmap_user(bio);
 	if (result)
 		*result = (u32)(uintptr_t)req->special;
 	ret = req->errors;
@@ -75,7 +78,40 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void *buffer, unsigned bufflen)
 {
-	return __nvme_submit_sync_cmd(q, cmd, buffer, NULL, bufflen, NULL, 0);
+	return __nvme_submit_sync_cmd(q, cmd, buffer, bufflen, NULL, 0);
+}
+
+int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
+		void __user *ubuffer, unsigned bufflen, u32 *result,
+		unsigned timeout)
+{
+	struct bio *bio = NULL;
+	struct request *req;
+	int ret;
+
+	req = nvme_alloc_request(q, cmd);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+
+	if (ubuffer && bufflen) {
+		ret = blk_rq_map_user(q, req, NULL, ubuffer, bufflen,
+				__GFP_WAIT);
+		if (ret)
+			goto out;
+		bio = req->bio;
+	}
+
+	blk_execute_rq(req->q, NULL, req, 0);
+	if (bio)
+		blk_rq_unmap_user(bio);
+	if (result)
+		*result = (u32)(uintptr_t)req->special;
+	ret = req->errors;
+ out:
+	blk_mq_free_request(req);
+	return ret;
 }
 
 int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
@@ -130,8 +166,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
 	c.features.prp1 = cpu_to_le64(dma_addr);
 	c.features.fid = cpu_to_le32(fid);
 
-	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, NULL, 0,
-			result, 0);
+	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, 0, result, 0);
 }
 
 int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
@@ -145,8 +180,7 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 	c.features.fid = cpu_to_le32(fid);
 	c.features.dword11 = cpu_to_le32(dword11);
 
-	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, NULL, 0,
-			result, 0);
+	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, 0, result, 0);
 }
 
 int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log)
drivers/nvme/host/nvme.h +4 -2
index 6c77db7..faa10f0 100644
@@ -146,8 +146,10 @@ static inline int nvme_error_status(u16 status)
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void *buf, unsigned bufflen);
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
-		void *buffer, void __user *ubuffer, unsigned bufflen,
-		u32 *result, unsigned timeout);
+		void *buffer, unsigned bufflen,  u32 *result, unsigned timeout);
+int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
+		void __user *ubuffer, unsigned bufflen, u32 *result,
+		unsigned timeout);
 int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id);
 int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,
 		struct nvme_id_ns **id);
drivers/nvme/host/pci.c +3 -3
index d742efe..be0ae02 100644
@@ -1677,7 +1677,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	c.rw.appmask = cpu_to_le16(io.appmask);
 	c.rw.metadata = cpu_to_le64(meta_dma);
 
-	status = __nvme_submit_sync_cmd(ns->queue, &c, NULL,
+	status = nvme_submit_user_cmd(ns->queue, &c,
 			(void __user *)(uintptr_t)io.addr, length, NULL, 0);
  unmap:
 	if (meta) {
@@ -1719,8 +1719,8 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
-	status = __nvme_submit_sync_cmd(ns ? ns->queue : ctrl->admin_q, &c,
-			NULL, (void __user *)(uintptr_t)cmd.addr, cmd.data_len,
+	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
+			(void __user *)(uintptr_t)cmd.addr, cmd.data_len,
 			&cmd.result, timeout);
 	if (status >= 0) {
 		if (put_user(cmd.result, &ucmd->result))
drivers/nvme/host/scsi.c +2 -2
index 00d0bdd..b673fe4 100644
@@ -1295,7 +1295,7 @@ static int nvme_trans_send_download_fw_cmd(struct nvme_ns *ns, struct sg_io_hdr
 	c.dlfw.numd = cpu_to_le32((tot_len/BYTES_TO_DWORDS) - 1);
 	c.dlfw.offset = cpu_to_le32(offset/BYTES_TO_DWORDS);
 
-	nvme_sc = __nvme_submit_sync_cmd(ns->ctrl->admin_q, &c, NULL,
+	nvme_sc = nvme_submit_user_cmd(ns->ctrl->admin_q, &c,
 			hdr->dxferp, tot_len, NULL, 0);
 	return nvme_trans_status_code(hdr, nvme_sc);
 }
@@ -1699,7 +1699,7 @@ static int nvme_trans_do_nvme_io(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 			nvme_sc = NVME_SC_LBA_RANGE;
 			break;
 		}
-		nvme_sc = __nvme_submit_sync_cmd(ns->queue, &c, NULL,
+		nvme_sc = nvme_submit_user_cmd(ns->queue, &c,
 				next_mapping_addr, unit_len, NULL, 0);
 		if (nvme_sc)
 			break;
Signature · 1 line
1.9.1
author maintainer
#36 lore raw

PATCH 12/18 nvme: use the block layer for userspace passthrough metadata

In reply to Christoph Hellwig

Use the integrity API to pass through metadata from userspace. For PI enabled devices this means that we now validate the reftag, which seems like an unintentional ommission in the old code.

Thanks to Keith Busch for testing and fixes.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 78 +++++++++++++++++++++++++++++++++++++++++++-----
 drivers/nvme/host/nvme.h |  4 +++
 drivers/nvme/host/pci.c  | 39 ++++--------------------
 3 files changed, 79 insertions(+), 42 deletions(-)
drivers/nvme/host/core.c +70 -8
index 9d05df0..6d738b6 100644
@@ -81,12 +81,16 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 	return __nvme_submit_sync_cmd(q, cmd, buffer, bufflen, NULL, 0);
 }
 
-int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
-		void __user *ubuffer, unsigned bufflen, u32 *result,
-		unsigned timeout)
+int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
+		void __user *ubuffer, unsigned bufflen,
+		void __user *meta_buffer, unsigned meta_len, u32 meta_seed,
+		u32 *result, unsigned timeout)
 {
-	struct bio *bio = NULL;
+	bool write = cmd->common.opcode & 1;
+	struct nvme_ns *ns = q->queuedata;
 	struct request *req;
+	struct bio *bio = NULL;
+	void *meta = NULL;
 	int ret;
 
 	req = nvme_alloc_request(q, cmd);
@@ -101,19 +105,77 @@ int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 		if (ret)
 			goto out;
 		bio = req->bio;
+		bio->bi_bdev = bdget_disk(ns->disk, 0);
+		if (!bio->bi_bdev) {
+			ret = -ENODEV;
+			goto out_unmap;
+		}
+
+		if (meta_buffer) {
+			struct bio_integrity_payload *bip;
+
+			meta = kmalloc(meta_len, GFP_KERNEL);
+			if (!meta) {
+				ret = -ENOMEM;
+				goto out_unmap;
+			}
+
+			if (write) {
+				if (copy_from_user(meta, meta_buffer,
+						meta_len)) {
+					ret = -EFAULT;
+					goto out_free_meta;
+				}
+			}
+
+			bip = bio_integrity_alloc(bio, GFP_KERNEL, 1);
+			if (!bip) {
+				ret = -ENOMEM;
+				goto out_free_meta;
+			}
+
+			bip->bip_iter.bi_size = meta_len;
+			bip->bip_iter.bi_sector = meta_seed;
+
+			ret = bio_integrity_add_page(bio, virt_to_page(meta),
+					meta_len, offset_in_page(meta));
+			if (ret != meta_len) {
+				ret = -ENOMEM;
+				goto out_free_meta;
+			}
+		}
 	}
 
-	blk_execute_rq(req->q, NULL, req, 0);
-	if (bio)
-		blk_rq_unmap_user(bio);
+	blk_execute_rq(req->q, ns->disk, req, 0);
+	ret = req->errors;
 	if (result)
 		*result = (u32)(uintptr_t)req->special;
-	ret = req->errors;
+	if (meta && !ret && !write) {
+		if (copy_to_user(meta_buffer, meta, meta_len))
+			ret = -EFAULT;
+	}
+
+ out_free_meta:
+	kfree(meta);
+ out_unmap:
+	if (bio) {
+		if (bio->bi_bdev)
+			bdput(bio->bi_bdev);
+		blk_rq_unmap_user(bio);
+	}
  out:
 	blk_mq_free_request(req);
 	return ret;
 }
 
+int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
+		void __user *ubuffer, unsigned bufflen, u32 *result,
+		unsigned timeout)
+{
+	return __nvme_submit_user_cmd(q, cmd, ubuffer, bufflen, NULL, 0, 0,
+			result, timeout);
+}
+
 int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 {
 	struct nvme_command c = { };
drivers/nvme/host/nvme.h +4 -0
index faa10f0..fefcdff 100644
@@ -150,6 +150,10 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void __user *ubuffer, unsigned bufflen, u32 *result,
 		unsigned timeout);
+int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
+		void __user *ubuffer, unsigned bufflen,
+		void __user *meta_buffer, unsigned meta_len, u32 meta_seed,
+		u32 *result, unsigned timeout);
 int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id);
 int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,
 		struct nvme_id_ns **id);
drivers/nvme/host/pci.c +5 -34
index be0ae02..c972213 100644
@@ -1615,13 +1615,9 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 
 static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 {
-	struct nvme_dev *dev = to_nvme_dev(ns->ctrl);
 	struct nvme_user_io io;
 	struct nvme_command c;
 	unsigned length, meta_len;
-	int status, write;
-	dma_addr_t meta_dma = 0;
-	void *meta = NULL;
 	void __user *metadata;
 
 	if (copy_from_user(&io, uio, sizeof(io)))
@@ -1639,29 +1635,13 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	length = (io.nblocks + 1) << ns->lba_shift;
 	meta_len = (io.nblocks + 1) * ns->ms;
 	metadata = (void __user *)(uintptr_t)io.metadata;
-	write = io.opcode & 1;
 
 	if (ns->ext) {
 		length += meta_len;
 		meta_len = 0;
-	}
-	if (meta_len) {
-		if (((io.metadata & 3) || !io.metadata) && !ns->ext)
+	} else if (meta_len) {
+		if ((io.metadata & 3) || !io.metadata)
 			return -EINVAL;
-
-		meta = dma_alloc_coherent(dev->dev, meta_len,
-						&meta_dma, GFP_KERNEL);
-
-		if (!meta) {
-			status = -ENOMEM;
-			goto unmap;
-		}
-		if (write) {
-			if (copy_from_user(meta, metadata, meta_len)) {
-				status = -EFAULT;
-				goto unmap;
-			}
-		}
 	}
 
 	memset(&c, 0, sizeof(c));
@@ -1675,19 +1655,10 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	c.rw.reftag = cpu_to_le32(io.reftag);
 	c.rw.apptag = cpu_to_le16(io.apptag);
 	c.rw.appmask = cpu_to_le16(io.appmask);
-	c.rw.metadata = cpu_to_le64(meta_dma);
 
-	status = nvme_submit_user_cmd(ns->queue, &c,
-			(void __user *)(uintptr_t)io.addr, length, NULL, 0);
- unmap:
-	if (meta) {
-		if (status == NVME_SC_SUCCESS && !write) {
-			if (copy_to_user(metadata, meta, meta_len))
-				status = -EFAULT;
-		}
-		dma_free_coherent(dev->dev, meta_len, meta, meta_dma);
-	}
-	return status;
+	return __nvme_submit_user_cmd(ns->queue, &c,
+			(void __user *)(uintptr_t)io.addr, length,
+			metadata, meta_len, io.slba, NULL, 0);
 }
 
 static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
Signature · 1 line
1.9.1
#37 lore raw

PATCH 12/18 nvme: use the block layer for userspace passthrough metadata

Quoted 3 lines · Christoph Hellwig

Use the integrity API to pass through metadata from userspace. For PI enabled devices this means that we now validate the reftag, which seems like an unintentional ommission in the old code.

More trouble. The below works for IO but not admin. There is no "namespace" associated with the queue's queuedata, so there's no way to send passthrough commands to the admin queue with metadata. I don't think anyone cares as there is no admin command that carries such a payload.

Since the admin queuedata is NULL, we can just check for NULL before proceeding with metadata setup, but need to be aware of this if the admin queue ever does define queuedata.

My fault for not regression testing before posting the previous fix. The diff to correct this new issue is at the end of this email.

Quoted 60 lines
(quoted diff) +48 -4
@@ -101,19 +105,77 @@ int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 		if (ret)
 			goto out;
 		bio = req->bio;
+		bio->bi_bdev = bdget_disk(ns->disk, 0);
+		if (!bio->bi_bdev) {
+			ret = -ENODEV;
+			goto out_unmap;
+		}
+
+		if (meta_buffer) {
+			struct bio_integrity_payload *bip;
+
+			meta = kmalloc(meta_len, GFP_KERNEL);
+			if (!meta) {
+				ret = -ENOMEM;
+				goto out_unmap;
+			}
+
+			if (write) {
+				if (copy_from_user(meta, meta_buffer,
+						meta_len)) {
+					ret = -EFAULT;
+					goto out_free_meta;
+				}
+			}
+
+			bip = bio_integrity_alloc(bio, GFP_KERNEL, 1);
+			if (!bip) {
+				ret = -ENOMEM;
+				goto out_free_meta;
+			}
+
+			bip->bip_iter.bi_size = meta_len;
+			bip->bip_iter.bi_sector = meta_seed;
+
+			ret = bio_integrity_add_page(bio, virt_to_page(meta),
+					meta_len, offset_in_page(meta));
+			if (ret != meta_len) {
+				ret = -ENOMEM;
+				goto out_free_meta;
+			}
+		}
 	}
 
-	blk_execute_rq(req->q, NULL, req, 0);
-	if (bio)
-		blk_rq_unmap_user(bio);
+	blk_execute_rq(req->q, ns->disk, req, 0);
+	ret = req->errors;
 	if (result)
 		*result = (u32)(uintptr_t)req->special;
-	ret = req->errors;
+	if (meta && !ret && !write) {
+		if (copy_to_user(meta_buffer, meta, meta_len))
+			ret = -EFAULT;
+	}
+
+ out_free_meta:
+	kfree(meta);

Review int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd, · line 169

Quoted 10 lines
(quoted diff) +6 -0
+ out_unmap:
+	if (bio) {
+		if (bio->bi_bdev)
+			bdput(bio->bi_bdev);
+		blk_rq_unmap_user(bio);
+	}
  out:
 	blk_mq_free_request(req);
 	return ret;
 }

---

drivers/nvme/host/core.c +6 -2
index b8c72d2..c8f9a71 100644
@@ -156,6 +156,10 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 		if (ret)
 			goto out;
 		bio = req->bio;
+
+		if (!ns)
+			goto submit;
+
 		bio->bi_bdev = bdget_disk(ns->disk, 0);
 		if (!bio->bi_bdev) {
 			ret = -ENODEV;
@@ -196,8 +200,8 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 			}
 		}
 	}
-
-	blk_execute_rq(req->q, ns->disk, req, 0);
+ submit:
+	blk_execute_rq(req->q, ns ? ns->disk : NULL, req, 0);
 	ret = req->errors;
 	if (result)
 		*result = (u32)(uintptr_t)req->special;

--

author maintainer
#38 lore raw

PATCH 12/18 nvme: use the block layer for userspace passthrough metadata

Thanks Keith, this looks good to me.

author maintainer
#39 lore raw

PATCH 13/18 nvme: move block_device_operations and ns/ctrl freeing to common code

In reply to Christoph Hellwig

This moves the block_device_operations over to common code mostly as-is. The only change is that the ns and ctrl refcounting got some small refcounting to have wrappers around the kref_put operations.

A new free_ctrl operation is added to allow the PCI driver to free it's ressources on the final drop.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 320 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  14 +++
 drivers/nvme/host/pci.c  | 319 ++--------------------------------------------
 3 files changed, 346 insertions(+), 307 deletions(-)
drivers/nvme/host/core.c +320 -0
index 6d738b6..1b27fd9 100644
@@ -15,12 +15,50 @@
 #include <linux/blkdev.h>
 #include <linux/blk-mq.h>
 #include <linux/errno.h>
+#include <linux/hdreg.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/ptrace.h>
+#include <linux/nvme_ioctl.h>
+#include <linux/t10-pi.h>
+#include <scsi/sg.h>
 
 #include "nvme.h"
 
+DEFINE_SPINLOCK(dev_list_lock);
+
+static void nvme_free_ns(struct kref *kref)
+{
+	struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);
+
+	spin_lock(&dev_list_lock);
+	ns->disk->private_data = NULL;
+	spin_unlock(&dev_list_lock);
+
+	nvme_put_ctrl(ns->ctrl);
+	put_disk(ns->disk);
+	kfree(ns);
+}
+
+void nvme_put_ns(struct nvme_ns *ns)
+{
+	kref_put(&ns->kref, nvme_free_ns);
+}
+
+static struct nvme_ns *nvme_get_ns_from_disk(struct gendisk *disk)
+{
+	struct nvme_ns *ns;
+
+	spin_lock(&dev_list_lock);
+	ns = disk->private_data;
+	if (ns && !kref_get_unless_zero(&ns->kref))
+		ns = NULL;
+	spin_unlock(&dev_list_lock);
+
+	return ns;
+}
+
 static struct request *nvme_alloc_request(struct request_queue *q,
 		struct nvme_command *cmd)
 {
@@ -266,3 +304,285 @@ int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log)
 		kfree(*log);
 	return error;
 }
+
+static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
+{
+	struct nvme_user_io io;
+	struct nvme_command c;
+	unsigned length, meta_len;
+	void __user *metadata;
+
+	if (copy_from_user(&io, uio, sizeof(io)))
+		return -EFAULT;
+
+	switch (io.opcode) {
+	case nvme_cmd_write:
+	case nvme_cmd_read:
+	case nvme_cmd_compare:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	length = (io.nblocks + 1) << ns->lba_shift;
+	meta_len = (io.nblocks + 1) * ns->ms;
+	metadata = (void __user *)(uintptr_t)io.metadata;
+
+	if (ns->ext) {
+		length += meta_len;
+		meta_len = 0;
+	} else if (meta_len) {
+		if ((io.metadata & 3) || !io.metadata)
+			return -EINVAL;
+	}
+
+	memset(&c, 0, sizeof(c));
+	c.rw.opcode = io.opcode;
+	c.rw.flags = io.flags;
+	c.rw.nsid = cpu_to_le32(ns->ns_id);
+	c.rw.slba = cpu_to_le64(io.slba);
+	c.rw.length = cpu_to_le16(io.nblocks);
+	c.rw.control = cpu_to_le16(io.control);
+	c.rw.dsmgmt = cpu_to_le32(io.dsmgmt);
+	c.rw.reftag = cpu_to_le32(io.reftag);
+	c.rw.apptag = cpu_to_le16(io.apptag);
+	c.rw.appmask = cpu_to_le16(io.appmask);
+
+	return __nvme_submit_user_cmd(ns->queue, &c,
+			(void __user *)(uintptr_t)io.addr, length,
+			metadata, meta_len, io.slba, NULL, 0);
+}
+
+int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+			struct nvme_passthru_cmd __user *ucmd)
+{
+	struct nvme_passthru_cmd cmd;
+	struct nvme_command c;
+	unsigned timeout = 0;
+	int status;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+	if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
+		return -EFAULT;
+
+	memset(&c, 0, sizeof(c));
+	c.common.opcode = cmd.opcode;
+	c.common.flags = cmd.flags;
+	c.common.nsid = cpu_to_le32(cmd.nsid);
+	c.common.cdw2[0] = cpu_to_le32(cmd.cdw2);
+	c.common.cdw2[1] = cpu_to_le32(cmd.cdw3);
+	c.common.cdw10[0] = cpu_to_le32(cmd.cdw10);
+	c.common.cdw10[1] = cpu_to_le32(cmd.cdw11);
+	c.common.cdw10[2] = cpu_to_le32(cmd.cdw12);
+	c.common.cdw10[3] = cpu_to_le32(cmd.cdw13);
+	c.common.cdw10[4] = cpu_to_le32(cmd.cdw14);
+	c.common.cdw10[5] = cpu_to_le32(cmd.cdw15);
+
+	if (cmd.timeout_ms)
+		timeout = msecs_to_jiffies(cmd.timeout_ms);
+
+	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
+			(void __user *)cmd.addr, cmd.data_len,
+			&cmd.result, timeout);
+	if (status >= 0) {
+		if (put_user(cmd.result, &ucmd->result))
+			return -EFAULT;
+	}
+
+	return status;
+}
+
+static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
+		unsigned int cmd, unsigned long arg)
+{
+	struct nvme_ns *ns = bdev->bd_disk->private_data;
+
+	switch (cmd) {
+	case NVME_IOCTL_ID:
+		force_successful_syscall_return();
+		return ns->ns_id;
+	case NVME_IOCTL_ADMIN_CMD:
+		return nvme_user_cmd(ns->ctrl, NULL, (void __user *)arg);
+	case NVME_IOCTL_IO_CMD:
+		return nvme_user_cmd(ns->ctrl, ns, (void __user *)arg);
+	case NVME_IOCTL_SUBMIT_IO:
+		return nvme_submit_io(ns, (void __user *)arg);
+	case SG_GET_VERSION_NUM:
+		return nvme_sg_get_version_num((void __user *)arg);
+	case SG_IO:
+		return nvme_sg_io(ns, (void __user *)arg);
+	default:
+		return -ENOTTY;
+	}
+}
+
+#ifdef CONFIG_COMPAT
+static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
+			unsigned int cmd, unsigned long arg)
+{
+	switch (cmd) {
+	case SG_IO:
+		return -ENOIOCTLCMD;
+	}
+	return nvme_ioctl(bdev, mode, cmd, arg);
+}
+#else
+#define nvme_compat_ioctl	NULL
+#endif
+
+static int nvme_open(struct block_device *bdev, fmode_t mode)
+{
+	return nvme_get_ns_from_disk(bdev->bd_disk) ? 0 : -ENXIO;
+}
+
+static void nvme_release(struct gendisk *disk, fmode_t mode)
+{
+	nvme_put_ns(disk->private_data);
+}
+
+static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
+{
+	/* some standard values */
+	geo->heads = 1 << 6;
+	geo->sectors = 1 << 5;
+	geo->cylinders = get_capacity(bdev->bd_disk) >> 11;
+	return 0;
+}
+
+#ifdef CONFIG_BLK_DEV_INTEGRITY
+static int nvme_noop_verify(struct blk_integrity_iter *iter)
+{
+	return 0;
+}
+
+static int nvme_noop_generate(struct blk_integrity_iter *iter)
+{
+	return 0;
+}
+
+static struct blk_integrity nvme_meta_noop = {
+	.name			= "NVME_META_NOOP",
+	.generate_fn		= nvme_noop_generate,
+	.verify_fn		= nvme_noop_verify,
+};
+
+static void nvme_init_integrity(struct nvme_ns *ns)
+{
+	struct blk_integrity integrity;
+
+	switch (ns->pi_type) {
+	case NVME_NS_DPS_PI_TYPE3:
+		integrity = t10_pi_type3_crc;
+		break;
+	case NVME_NS_DPS_PI_TYPE1:
+	case NVME_NS_DPS_PI_TYPE2:
+		integrity = t10_pi_type1_crc;
+		break;
+	default:
+		integrity = nvme_meta_noop;
+		break;
+	}
+	integrity.tuple_size = ns->ms;
+	blk_integrity_register(ns->disk, &integrity);
+	blk_queue_max_integrity_segments(ns->queue, 1);
+}
+#else
+static void nvme_init_integrity(struct nvme_ns *ns)
+{
+}
+#endif /* CONFIG_BLK_DEV_INTEGRITY */
+
+static void nvme_config_discard(struct nvme_ns *ns)
+{
+	u32 logical_block_size = queue_logical_block_size(ns->queue);
+	ns->queue->limits.discard_zeroes_data = 0;
+	ns->queue->limits.discard_alignment = logical_block_size;
+	ns->queue->limits.discard_granularity = logical_block_size;
+	blk_queue_max_discard_sectors(ns->queue, 0xffffffff);
+	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
+}
+
+int nvme_revalidate_disk(struct gendisk *disk)
+{
+	struct nvme_ns *ns = disk->private_data;
+	struct nvme_id_ns *id;
+	u8 lbaf, pi_type;
+	u16 old_ms;
+	unsigned short bs;
+
+	if (nvme_identify_ns(ns->ctrl, ns->ns_id, &id)) {
+		dev_warn(ns->ctrl->dev, "%s: Identify failure nvme%dn%d\n",
+				__func__, ns->ctrl->instance, ns->ns_id);
+		return -ENODEV;
+	}
+	if (id->ncap == 0) {
+		kfree(id);
+		return -ENODEV;
+	}
+
+	old_ms = ns->ms;
+	lbaf = id->flbas & NVME_NS_FLBAS_LBA_MASK;
+	ns->lba_shift = id->lbaf[lbaf].ds;
+	ns->ms = le16_to_cpu(id->lbaf[lbaf].ms);
+	ns->ext = ns->ms && (id->flbas & NVME_NS_FLBAS_META_EXT);
+
+	/*
+	 * If identify namespace failed, use default 512 byte block size so
+	 * block layer can use before failing read/write for 0 capacity.
+	 */
+	if (ns->lba_shift == 0)
+		ns->lba_shift = 9;
+	bs = 1 << ns->lba_shift;
+
+	/* XXX: PI implementation requires metadata equal t10 pi tuple size */
+	pi_type = ns->ms == sizeof(struct t10_pi_tuple) ?
+					id->dps & NVME_NS_DPS_PI_MASK : 0;
+
+	if (blk_get_integrity(disk) && (ns->pi_type != pi_type ||
+				ns->ms != old_ms ||
+				bs != queue_logical_block_size(disk->queue) ||
+				(ns->ms && ns->ext)))
+		blk_integrity_unregister(disk);
+
+	ns->pi_type = pi_type;
+	blk_queue_logical_block_size(ns->queue, bs);
+
+	if (ns->ms && !blk_get_integrity(disk) && (disk->flags & GENHD_FL_UP) &&
+								!ns->ext)
+		nvme_init_integrity(ns);
+
+	if (ns->ms && !(ns->ms == 8 && ns->pi_type) && !blk_get_integrity(disk))
+		set_capacity(disk, 0);
+	else
+		set_capacity(disk, le64_to_cpup(&id->nsze) << (ns->lba_shift - 9));
+
+	if (ns->ctrl->oncs & NVME_CTRL_ONCS_DSM)
+		nvme_config_discard(ns);
+
+	kfree(id);
+	return 0;
+}
+
+const struct block_device_operations nvme_fops = {
+	.owner		= THIS_MODULE,
+	.ioctl		= nvme_ioctl,
+	.compat_ioctl	= nvme_compat_ioctl,
+	.open		= nvme_open,
+	.release	= nvme_release,
+	.getgeo		= nvme_getgeo,
+	.revalidate_disk= nvme_revalidate_disk,
+};
+
+static void nvme_free_ctrl(struct kref *kref)
+{
+	struct nvme_ctrl *ctrl = container_of(kref, struct nvme_ctrl, kref);
+
+	ctrl->ops->free_ctrl(ctrl);
+}
+
+void nvme_put_ctrl(struct nvme_ctrl *ctrl)
+{
+	kref_put(&ctrl->kref, nvme_free_ctrl);
+}
+
drivers/nvme/host/nvme.h +14 -0
index fefcdff..d3a6c97 100644
@@ -19,6 +19,8 @@
 #include <linux/kref.h>
 #include <linux/blk-mq.h>
 
+struct nvme_passthru_cmd;
+
 extern unsigned char nvme_io_timeout;
 #define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
@@ -29,6 +31,7 @@ struct nvme_ctrl {
 	const struct nvme_ctrl_ops *ops;
 	struct request_queue *admin_q;
 	struct device *dev;
+	struct kref kref;
 	int instance;
 
 	char name[12];
@@ -64,6 +67,7 @@ struct nvme_ns {
 
 struct nvme_ctrl_ops {
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
+	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 };
 
 static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl)
@@ -143,6 +147,9 @@ static inline int nvme_error_status(u16 status)
 	}
 }
 
+void nvme_put_ctrl(struct nvme_ctrl *ctrl);
+void nvme_put_ns(struct nvme_ns *ns);
+
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void *buf, unsigned bufflen);
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
@@ -163,6 +170,13 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
 int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 			dma_addr_t dma_addr, u32 *result);
 
+extern const struct block_device_operations nvme_fops;
+extern spinlock_t dev_list_lock;
+
+int nvme_revalidate_disk(struct gendisk *disk);
+int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+			struct nvme_passthru_cmd __user *ucmd);
+
 struct sg_io_hdr;
 
 int nvme_sg_io(struct nvme_ns *ns, struct sg_io_hdr __user *u_hdr);
drivers/nvme/host/pci.c +12 -307

420 lines not on this page · raw message

Signature · 1 line
1.9.1
author maintainer
#40 lore raw

PATCH 14/18 nvme: add explicit quirk handling

In reply to Christoph Hellwig

Add an enum for all workarounds not in the spec and identify the affected controllers at probe time.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/nvme.h | 13 +++++++++++++
 drivers/nvme/host/pci.c  |  8 +++++---
 2 files changed, 18 insertions(+), 3 deletions(-)
drivers/nvme/host/nvme.h +13 -0
index d3a6c97..1ebd0da 100644
@@ -27,6 +27,18 @@ extern unsigned char nvme_io_timeout;
 extern unsigned char admin_timeout;
 #define ADMIN_TIMEOUT	(admin_timeout * HZ)
 
+/*
+ * List of workarounds for devices that required behavior not specified in
+ * the standard.
+ */
+enum nvme_quirks {
+	/*
+	 * Prefers I/O aligned to a stripe size specified in a vendor
+	 * specific Identify field.
+	 */
+	NVME_QUIRK_STRIPE_SIZE			= (1 << 0),
+};
+
 struct nvme_ctrl {
 	const struct nvme_ctrl_ops *ops;
 	struct request_queue *admin_q;
@@ -43,6 +55,7 @@ struct nvme_ctrl {
 	u8 event_limit;
 	u8 vwc;
 	u16 vendor;
+	unsigned long quirks;
 };
 
 /*
drivers/nvme/host/pci.c +5 -3
index e6b3992..2c13d7a 100644
@@ -1991,7 +1991,6 @@ static void nvme_dev_scan(struct work_struct *work)
  */
 static int nvme_dev_add(struct nvme_dev *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	int res;
 	struct nvme_id_ctrl *ctrl;
 	int shift = NVME_CAP_MPSMIN(readq(dev->bar + NVME_REG_CAP)) + 12;
@@ -2010,8 +2009,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	memcpy(dev->ctrl.firmware_rev, ctrl->fr, sizeof(ctrl->fr));
 	if (ctrl->mdts)
 		dev->max_hw_sectors = 1 << (ctrl->mdts + shift - 9);
-	if ((pdev->vendor == PCI_VENDOR_ID_INTEL) &&
-			(pdev->device == 0x0953) && ctrl->vs[3]) {
+
+	if ((dev->ctrl.quirks & NVME_QUIRK_STRIPE_SIZE) && ctrl->vs[3]) {
 		unsigned int max_hw_sectors;
 
 		dev->stripe_size = 1 << (ctrl->vs[3] + shift);
@@ -2677,6 +2676,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	dev->ctrl.vendor = pdev->vendor;
 	dev->ctrl.ops = &nvme_pci_ctrl_ops;
 	dev->ctrl.dev = dev->dev;
+	dev->ctrl.quirks = id->driver_data;
 
 	result = nvme_set_instance(dev);
 	if (result)
@@ -2804,6 +2804,8 @@ static const struct pci_error_handlers nvme_err_handler = {
 #define PCI_CLASS_STORAGE_EXPRESS	0x010802
 
 static const struct pci_device_id nvme_id_table[] = {
+	{ PCI_VDEVICE(INTEL, 0x0953),
+		.driver_data = NVME_QUIRK_STRIPE_SIZE, },
 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
 	{ 0, }
 };
Signature · 1 line
1.9.1
author maintainer
#41 lore raw

PATCH 15/18 nvme: add a common helper to read Identify Controller data

In reply to Christoph Hellwig

And add the 64-bit register read operation for it.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  4 ++++
 drivers/nvme/host/pci.c  | 53 +++++++++++++++---------------------------------
 3 files changed, 70 insertions(+), 37 deletions(-)
drivers/nvme/host/core.c +50 -0
index 1b27fd9..3e88901 100644
@@ -574,6 +574,56 @@ const struct block_device_operations nvme_fops = {
 	.revalidate_disk= nvme_revalidate_disk,
 };
 
+/*
+ * Initialize the cached copies of the Identify data and various controller
+ * register in our nvme_ctrl structure.  This should be called as soon as
+ * the admin queue is fully up and running.
+ */
+int nvme_init_identify(struct nvme_ctrl *ctrl)
+{
+	struct nvme_id_ctrl *id;
+	u64 cap;
+	int ret, page_shift;
+
+	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &cap);
+	if (ret) {
+		dev_err(ctrl->dev, "Reading CAP failed (%d)\n", ret);
+		return ret;
+	}
+	page_shift = NVME_CAP_MPSMIN(cap) + 12;
+
+	ret = nvme_identify_ctrl(ctrl, &id);
+	if (ret) {
+		dev_err(ctrl->dev, "Identify Controller failed (%d)\n", ret);
+		return -EIO;
+	}
+
+	ctrl->oncs = le16_to_cpup(&id->oncs);
+	ctrl->abort_limit = id->acl + 1;
+	ctrl->vwc = id->vwc;
+	memcpy(ctrl->serial, id->sn, sizeof(id->sn));
+	memcpy(ctrl->model, id->mn, sizeof(id->mn));
+	memcpy(ctrl->firmware_rev, id->fr, sizeof(id->fr));
+	if (id->mdts)
+		ctrl->max_hw_sectors = 1 << (id->mdts + page_shift - 9);
+
+	if ((ctrl->quirks & NVME_QUIRK_STRIPE_SIZE) && id->vs[3]) {
+		unsigned int max_hw_sectors;
+
+		ctrl->stripe_size = 1 << (id->vs[3] + page_shift);
+		max_hw_sectors = ctrl->stripe_size >> (page_shift - 9);
+		if (ctrl->max_hw_sectors) {
+			ctrl->max_hw_sectors = min(max_hw_sectors,
+							ctrl->max_hw_sectors);
+		} else {
+			ctrl->max_hw_sectors = max_hw_sectors;
+		}
+	}
+
+	kfree(id);
+	return 0;
+}
+
 static void nvme_free_ctrl(struct kref *kref)
 {
 	struct nvme_ctrl *ctrl = container_of(kref, struct nvme_ctrl, kref);
drivers/nvme/host/nvme.h +4 -0
index 1ebd0da..48b221a 100644
@@ -50,6 +50,8 @@ struct nvme_ctrl {
 	char serial[20];
 	char model[40];
 	char firmware_rev[8];
+	u32 max_hw_sectors;
+	u32 stripe_size;
 	u16 oncs;
 	u16 abort_limit;
 	u8 event_limit;
@@ -80,6 +82,7 @@ struct nvme_ns {
 
 struct nvme_ctrl_ops {
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
+	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 };
 
@@ -161,6 +164,7 @@ static inline int nvme_error_status(u16 status)
 }
 
 void nvme_put_ctrl(struct nvme_ctrl *ctrl);
+int nvme_init_identify(struct nvme_ctrl *ctrl);
 void nvme_put_ns(struct nvme_ns *ns);
 
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
drivers/nvme/host/pci.c +16 -37

81 lines not on this page · raw message

Signature · 1 line
1.9.1
#42 lore raw

PATCH 15/18 nvme: add a common helper to read Identify Controller data

Quoted 1 line · Christoph Hellwig

And add the 64-bit register read operation for it.

I apologize, but I am getting tripped up by the subject line of this patch and then the following description.

To me, this patch sounds and looks like it is doing two distinct, separate things, thereby two separate patches:

1. Add a helper function to read and cache Identify Controller data
2. Add a 64-bit read register function pointer to nvme_ctrl_ops

I think it was PATCH 6 of this series that introduced "int (*reg_read32)()". If it is easier, it would also make sense to also add "int (*reg_read64)()" there, instead of its own separate patch.

Quoted 215 lines
Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 50 
+++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  4 ++++
 drivers/nvme/host/pci.c  | 53 +++++++++++++++-----------------------
----------
 3 files changed, 70 insertions(+), 37 deletions(-)
drivers/nvme/host/core.c +2 -0
index 1b27fd9..3e88901 100644
@@ -574,6 +574,56 @@ const struct block_device_operations nvme_fops = 
 {
 	.revalidate_disk= nvme_revalidate_disk,
 };
 
+/*
+ * Initialize the cached copies of the Identify data and various 

controller
+ * register in our nvme_ctrl structure. This should be called as soon as
+ * the admin queue is fully up and running.
+ */

drivers/nvme/host/core.c +9 -0
+int nvme_init_identify(struct nvme_ctrl *ctrl)
+{
+	struct nvme_id_ctrl *id;
+	u64 cap;
+	int ret, page_shift;
+
+	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &cap);
+	if (ret) {
+		dev_err(ctrl->dev, "Reading CAP failed (%d)\n", 

ret);

drivers/nvme/host/core.c +7 -0
+		return ret;
+	}
+	page_shift = NVME_CAP_MPSMIN(cap) + 12;
+
+	ret = nvme_identify_ctrl(ctrl, &id);
+	if (ret) {
+		dev_err(ctrl->dev, "Identify Controller failed 

(%d)\n", ret);

drivers/nvme/host/core.c +11 -0
+		return -EIO;
+	}
+
+	ctrl->oncs = le16_to_cpup(&id->oncs);
+	ctrl->abort_limit = id->acl + 1;
+	ctrl->vwc = id->vwc;
+	memcpy(ctrl->serial, id->sn, sizeof(id->sn));
+	memcpy(ctrl->model, id->mn, sizeof(id->mn));
+	memcpy(ctrl->firmware_rev, id->fr, sizeof(id->fr));
+	if (id->mdts)
+		ctrl->max_hw_sectors = 1 << (id->mdts + page_shift - 

9);

drivers/nvme/host/core.c +6 -0
+
+	if ((ctrl->quirks & NVME_QUIRK_STRIPE_SIZE) && id->vs[3]) {
+		unsigned int max_hw_sectors;
+
+		ctrl->stripe_size = 1 << (id->vs[3] + page_shift);
+		max_hw_sectors = ctrl->stripe_size >> (page_shift - 

9);

drivers/nvme/host/core.c +12 -1
+		if (ctrl->max_hw_sectors) {
+			ctrl->max_hw_sectors = min(max_hw_sectors,
+							ctrl
->max_hw_sectors);
+		} else {
+			ctrl->max_hw_sectors = max_hw_sectors;
+		}
+	}
+
+	kfree(id);
+	return 0;
+}
+
 static void nvme_free_ctrl(struct kref *kref)
 {
 	struct nvme_ctrl *ctrl = container_of(kref, struct 

nvme_ctrl, kref);

drivers/nvme/host/nvme.h +3 -0
index 1ebd0da..48b221a 100644
@@ -50,6 +50,8 @@ struct nvme_ctrl {
 	char serial[20];
 	char model[40];
 	char firmware_rev[8];
+	u32 max_hw_sectors;
+	u32 stripe_size;
 	u16 oncs;
 	u16 abort_limit;
 	u8 event_limit;
@@ -80,6 +82,7 @@ struct nvme_ns {
 
 struct nvme_ctrl_ops {
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 
 *val);
+	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 
 *val);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);

};

drivers/nvme/host/nvme.h +1 -0
@@ -161,6 +164,7 @@ static inline int nvme_error_status(u16 status)
 }
 
 void nvme_put_ctrl(struct nvme_ctrl *ctrl);
+int nvme_init_identify(struct nvme_ctrl *ctrl);
 void nvme_put_ns(struct nvme_ns *ns);
 
 int nvme_submit_sync_cmd(struct request_queue *q, struct 

nvme_command *cmd,

drivers/nvme/host/pci.c +3 -11
index 2c13d7a..7a25d6f 100644
@@ -127,8 +127,6 @@ struct nvme_dev {
 	struct work_struct probe_work;
 	struct work_struct scan_work;
 	bool subsystem;
-	u32 max_hw_sectors;
-	u32 stripe_size;
 	u32 page_size;
 	void __iomem *cmb;
 	dma_addr_t cmb_dma_addr;
@@ -1650,13 +1648,13 @@ static void nvme_alloc_ns(struct nvme_dev 
 *dev, unsigned nsid)
 	list_add_tail(&ns->list, &dev->namespaces);
 
 	blk_queue_logical_block_size(ns->queue, 1 << ns->lba_shift);
-	if (dev->max_hw_sectors) {
-		blk_queue_max_hw_sectors(ns->queue, dev
->max_hw_sectors);
+	if (dev->ctrl.max_hw_sectors) {
+		blk_queue_max_hw_sectors(ns->queue, dev
->ctrl.max_hw_sectors);
 		blk_queue_max_segments(ns->queue,
-			((dev->max_hw_sectors << 9) / dev
->page_size) + 1);
+			((dev->ctrl.max_hw_sectors << 9) / dev
->page_size) + 1);
 	}
-	if (dev->stripe_size)
-		blk_queue_chunk_sectors(ns->queue, dev->stripe_size 
Quoted 1 line
Quoted 1 line

9);

drivers/nvme/host/pci.c +14 -33
+	if (dev->ctrl.stripe_size)
+		blk_queue_chunk_sectors(ns->queue, dev
->ctrl.stripe_size >> 9);
 	if (dev->ctrl.vwc & NVME_CTRL_VWC_PRESENT)
 		blk_queue_flush(ns->queue, REQ_FLUSH | REQ_FUA);
 	blk_queue_virt_boundary(ns->queue, dev->page_size - 1);
@@ -1992,36 +1990,10 @@ static void nvme_dev_scan(struct work_struct 
 *work)
 static int nvme_dev_add(struct nvme_dev *dev)
 {
 	int res;
-	struct nvme_id_ctrl *ctrl;
-	int shift = NVME_CAP_MPSMIN(readq(dev->bar + NVME_REG_CAP)) 
+ 12;
-
-	res = nvme_identify_ctrl(&dev->ctrl, &ctrl);
-	if (res) {
-		dev_err(dev->dev, "Identify Controller failed 
 (%d)\n", res);
-		return -EIO;
-	}
-
-	dev->ctrl.oncs = le16_to_cpup(&ctrl->oncs);
-	dev->ctrl.abort_limit = ctrl->acl + 1;
-	dev->ctrl.vwc = ctrl->vwc;
-	memcpy(dev->ctrl.serial, ctrl->sn, sizeof(ctrl->sn));
-	memcpy(dev->ctrl.model, ctrl->mn, sizeof(ctrl->mn));
-	memcpy(dev->ctrl.firmware_rev, ctrl->fr, sizeof(ctrl->fr));
-	if (ctrl->mdts)
-		dev->max_hw_sectors = 1 << (ctrl->mdts + shift - 9);
-
-	if ((dev->ctrl.quirks & NVME_QUIRK_STRIPE_SIZE) && ctrl
->vs[3]) {
-		unsigned int max_hw_sectors;
-
-		dev->stripe_size = 1 << (ctrl->vs[3] + shift);
-		max_hw_sectors = dev->stripe_size >> (shift - 9);
-		if (dev->max_hw_sectors) {
-			dev->max_hw_sectors = min(max_hw_sectors,
-							dev
->max_hw_sectors);
-		} else
-			dev->max_hw_sectors = max_hw_sectors;
-	}
-	kfree(ctrl);
+
+	res = nvme_init_identify(&dev->ctrl);
+	if (res)
+		return res;
 
 	if (!dev->tagset.tags) {
 		dev->tagset.ops = &nvme_mq_ops;
@@ -2642,8 +2614,15 @@ static int nvme_pci_reg_read32(struct 
 nvme_ctrl *ctrl, u32 off, u32 *val)
 	return 0;
 }
 
+static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 
 *val)
+{
+	*val = readq(to_nvme_dev(ctrl)->bar + off);
+	return 0;
+}
+
 static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.reg_read32		= nvme_pci_reg_read32,
+	.reg_read64		= nvme_pci_reg_read64,
 	.free_ctrl		= nvme_pci_free_ctrl,

};

author maintainer
#43 lore raw

PATCH 15/18 nvme: add a common helper to read Identify Controller data

Quoted 9 lines · J Freyensee
Quoted 1 line · Christoph Hellwig

And add the 64-bit register read operation for it.

I apologize, but I am getting tripped up by the subject line of this patch and then the following description.

To me, this patch sounds and looks like it is doing two distinct, separate things, thereby two separate patches:

Quoted 6 lines

1. Add a helper function to read and cache Identify Controller data
2. Add a 64-bit read register function pointer to nvme_ctrl_ops

I think it was PATCH 6 of this series that introduced "int (*reg_read32)()". If it is easier, it would also make sense to also add "int (*reg_read64)()" there, instead of its own separate patch.

Through the whole series I've introduced the abstractions on an as-needed basis, and this patch follows that style.

author maintainer
#44 lore raw

PATCH 16/18 nvme: move the call to nvme_init_identify earlier

In reply to Christoph Hellwig

We want to record the identify and CAP values even if no I/O queue is available.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/nvme/host/pci.c +4 -6
index 7a25d6f..adff1cb 100644
@@ -1989,12 +1989,6 @@ static void nvme_dev_scan(struct work_struct *work)
  */
 static int nvme_dev_add(struct nvme_dev *dev)
 {
-	int res;
-
-	res = nvme_init_identify(&dev->ctrl);
-	if (res)
-		return res;
-
 	if (!dev->tagset.tags) {
 		dev->tagset.ops = &nvme_mq_ops;
 		dev->tagset.nr_hw_queues = dev->online_queues - 1;
@@ -2484,6 +2478,10 @@ static void nvme_probe_work(struct work_struct *work)
 	if (result)
 		goto disable;
 
+	result = nvme_init_identify(&dev->ctrl);
+	if (result)
+		goto free_tags;
+
 	result = nvme_setup_io_queues(dev);
 	if (result)
 		goto free_tags;
Signature · 1 line
1.9.1
author maintainer
#45 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

In reply to Christoph Hellwig

The namespace scanning code has been mostly generic already, we just need to store a pointer to the tagset in the nvme_ctrl structure, and add a method to check if a controller is I/O incapable. The latter will hopefully be replaced by a proper controller state machine soon.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 190 +++++++++++++++++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h |  25 +++++-
 drivers/nvme/host/pci.c  | 221 +++++++----------------------------------------
 3 files changed, 239 insertions(+), 197 deletions(-)
drivers/nvme/host/core.c +187 -3
index 3e88901..a01ab5a 100644
@@ -17,6 +17,8 @@
 #include <linux/errno.h>
 #include <linux/hdreg.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/list_sort.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/ptrace.h>
@@ -26,6 +28,9 @@
 
 #include "nvme.h"
 
+static int nvme_major;
+module_param(nvme_major, int, 0);
+
 DEFINE_SPINLOCK(dev_list_lock);
 
 static void nvme_free_ns(struct kref *kref)
@@ -41,7 +46,7 @@ static void nvme_free_ns(struct kref *kref)
 	kfree(ns);
 }
 
-void nvme_put_ns(struct nvme_ns *ns)
+static void nvme_put_ns(struct nvme_ns *ns)
 {
 	kref_put(&ns->kref, nvme_free_ns);
 }
@@ -503,7 +508,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
 	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
 }
 
-int nvme_revalidate_disk(struct gendisk *disk)
+static int nvme_revalidate_disk(struct gendisk *disk)
 {
 	struct nvme_ns *ns = disk->private_data;
 	struct nvme_id_ns *id;
@@ -564,7 +569,7 @@ int nvme_revalidate_disk(struct gendisk *disk)
 	return 0;
 }
 
-const struct block_device_operations nvme_fops = {
+static const struct block_device_operations nvme_fops = {
 	.owner		= THIS_MODULE,
 	.ioctl		= nvme_ioctl,
 	.compat_ioctl	= nvme_compat_ioctl,
@@ -591,6 +596,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		return ret;
 	}
 	page_shift = NVME_CAP_MPSMIN(cap) + 12;
+	ctrl->page_size = 1 << page_shift;
 
 	ret = nvme_identify_ctrl(ctrl, &id);
 	if (ret) {
@@ -636,3 +642,181 @@ void nvme_put_ctrl(struct nvme_ctrl *ctrl)
 	kref_put(&ctrl->kref, nvme_free_ctrl);
 }
 
+static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
+{
+	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
+	struct nvme_ns *nsb = container_of(b, struct nvme_ns, list);
+
+	return nsa->ns_id - nsb->ns_id;
+}
+
+static struct nvme_ns *nvme_find_ns(struct nvme_ctrl *ctrl, unsigned nsid)
+{
+	struct nvme_ns *ns;
+
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		if (ns->ns_id == nsid)
+			return ns;
+		if (ns->ns_id > nsid)
+			break;
+	}
+	return NULL;
+}
+
+static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
+{
+	struct nvme_ns *ns;
+	struct gendisk *disk;
+	int node = dev_to_node(ctrl->dev);
+
+	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
+	if (!ns)
+		return;
+
+	ns->queue = blk_mq_init_queue(ctrl->tagset);
+	if (IS_ERR(ns->queue))
+		goto out_free_ns;
+	queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, ns->queue);
+	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, ns->queue);
+	ns->queue->queuedata = ns;
+	ns->ctrl = ctrl;
+
+	disk = alloc_disk_node(0, node);
+	if (!disk)
+		goto out_free_queue;
+
+	kref_init(&ns->kref);
+	ns->ns_id = nsid;
+	ns->disk = disk;
+	ns->lba_shift = 9; /* set to a default value for 512 until disk is validated */
+	list_add_tail(&ns->list, &ctrl->namespaces);
+
+	blk_queue_logical_block_size(ns->queue, 1 << ns->lba_shift);
+	if (ctrl->max_hw_sectors) {
+		blk_queue_max_hw_sectors(ns->queue, ctrl->max_hw_sectors);
+		blk_queue_max_segments(ns->queue,
+			((ctrl->max_hw_sectors << 9) / ctrl->page_size) + 1);
+	}
+	if (ctrl->stripe_size)
+		blk_queue_chunk_sectors(ns->queue, ctrl->stripe_size >> 9);
+	if (ctrl->vwc & NVME_CTRL_VWC_PRESENT)
+		blk_queue_flush(ns->queue, REQ_FLUSH | REQ_FUA);
+	blk_queue_virt_boundary(ns->queue, ctrl->page_size - 1);
+
+	disk->major = nvme_major;
+	disk->first_minor = 0;
+	disk->fops = &nvme_fops;
+	disk->private_data = ns;
+	disk->queue = ns->queue;
+	disk->driverfs_dev = ctrl->device;
+	disk->flags = GENHD_FL_EXT_DEVT;
+	sprintf(disk->disk_name, "nvme%dn%d", ctrl->instance, nsid);
+
+	/*
+	 * Initialize capacity to 0 until we establish the namespace format and
+	 * setup integrity extentions if necessary. The revalidate_disk after
+	 * add_disk allows the driver to register with integrity if the format
+	 * requires it.
+	 */
+	set_capacity(disk, 0);
+	if (nvme_revalidate_disk(ns->disk))
+		goto out_free_disk;
+
+	kref_get(&ctrl->kref);
+	add_disk(ns->disk);
+	if (ns->ms) {
+		struct block_device *bd = bdget_disk(ns->disk, 0);
+		if (!bd)
+			return;
+		if (blkdev_get(bd, FMODE_READ, NULL)) {
+			bdput(bd);
+			return;
+		}
+		blkdev_reread_part(bd);
+		blkdev_put(bd, FMODE_READ);
+	}
+	return;
+ out_free_disk:
+	kfree(disk);
+	list_del(&ns->list);
+ out_free_queue:
+	blk_cleanup_queue(ns->queue);
+ out_free_ns:
+	kfree(ns);
+}
+
+static void nvme_ns_remove(struct nvme_ns *ns)
+{
+	bool kill = nvme_io_incapable(ns->ctrl) &&
+			!blk_queue_dying(ns->queue);
+
+	if (kill)
+		blk_set_queue_dying(ns->queue);
+	if (ns->disk->flags & GENHD_FL_UP) {
+		if (blk_get_integrity(ns->disk))
+			blk_integrity_unregister(ns->disk);
+		del_gendisk(ns->disk);
+	}
+	if (kill || !blk_queue_dying(ns->queue)) {
+		blk_mq_abort_requeue_list(ns->queue);
+		blk_cleanup_queue(ns->queue);
+	}
+	list_del_init(&ns->list);
+	nvme_put_ns(ns);
+}
+
+static void __nvme_scan_namespaces(struct nvme_ctrl *ctrl, unsigned nn)
+{
+	struct nvme_ns *ns, *next;
+	unsigned i;
+
+	for (i = 1; i <= nn; i++) {
+		ns = nvme_find_ns(ctrl, i);
+		if (ns) {
+			if (revalidate_disk(ns->disk))
+				nvme_ns_remove(ns);
+		} else
+			nvme_alloc_ns(ctrl, i);
+	}
+	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list) {
+		if (ns->ns_id > nn)
+			nvme_ns_remove(ns);
+	}
+	list_sort(NULL, &ctrl->namespaces, ns_cmp);
+}
+
+void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
+{
+	struct nvme_id_ctrl *id;
+
+	if (nvme_identify_ctrl(ctrl, &id))
+		return;
+	__nvme_scan_namespaces(ctrl, le32_to_cpup(&id->nn));
+	kfree(id);
+}
+
+void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns, *next;
+
+	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
+		nvme_ns_remove(ns);
+}
+
+int __init nvme_core_init(void)
+{
+	int result;
+
+	result = register_blkdev(nvme_major, "nvme");
+	if (result < 0)
+		return result;
+	else if (result > 0)
+		nvme_major = result;
+
+	return 0;
+}
+
+void nvme_core_exit(void)
+{
+	unregister_blkdev(nvme_major, "nvme");
+}
drivers/nvme/host/nvme.h +22 -3
index 48b221a..53e82feb 100644
@@ -45,6 +45,9 @@ struct nvme_ctrl {
 	struct device *dev;
 	struct kref kref;
 	int instance;
+	struct blk_mq_tag_set *tagset;
+	struct list_head namespaces;
+	struct device *device;	/* char device */
 
 	char name[12];
 	char serial[20];
@@ -52,6 +55,7 @@ struct nvme_ctrl {
 	char firmware_rev[8];
 	u32 max_hw_sectors;
 	u32 stripe_size;
+	u32 page_size;
 	u16 oncs;
 	u16 abort_limit;
 	u8 event_limit;
@@ -83,6 +87,7 @@ struct nvme_ns {
 struct nvme_ctrl_ops {
 	int (*reg_read32)(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);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 };
 
@@ -95,6 +100,17 @@ 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 false;
+	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &val))
+		return false;
+	return val & NVME_CSTS_CFS;
+}
+
 static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector)
 {
 	return (sector >> (ns->lba_shift - 9));
@@ -165,7 +181,9 @@ static inline int nvme_error_status(u16 status)
 
 void nvme_put_ctrl(struct nvme_ctrl *ctrl);
 int nvme_init_identify(struct nvme_ctrl *ctrl);
-void nvme_put_ns(struct nvme_ns *ns);
+
+void nvme_scan_namespaces(struct nvme_ctrl *ctrl);
+void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
 
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void *buf, unsigned bufflen);
@@ -187,10 +205,8 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
 int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 			dma_addr_t dma_addr, u32 *result);
 
-extern const struct block_device_operations nvme_fops;
 extern spinlock_t dev_list_lock;
 
-int nvme_revalidate_disk(struct gendisk *disk);
 int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			struct nvme_passthru_cmd __user *ucmd);
 
@@ -200,4 +216,7 @@ int nvme_sg_io(struct nvme_ns *ns, struct sg_io_hdr __user *u_hdr);
 int nvme_sg_io32(struct nvme_ns *ns, unsigned long arg);
 int nvme_sg_get_version_num(int __user *ip);
 
+int __init nvme_core_init(void);
+void nvme_core_exit(void);
+
 #endif /* _NVME_H */
drivers/nvme/host/pci.c +30 -191

367 lines not on this page · raw message

Signature · 1 line
1.9.1
#46 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

Quoted 23 lines · Christoph Hellwig
drivers/nvme/host/pci.c +2 -4
@@ -2810,11 +2651,9 @@ static int __init nvme_init(void)
 	if (!nvme_workq)
 		return -ENOMEM;
 
-	result = register_blkdev(nvme_major, "nvme");
+	result = nvme_core_init();
 	if (result < 0)
 		goto kill_workq;
-	else if (result > 0)
-		nvme_major = result;
 
 	result = __register_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme",
 							&nvme_dev_fops);
@@ -2839,7 +2678,7 @@ static int __init nvme_init(void)
  unregister_chrdev:
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");
  unregister_blkdev:
-	unregister_blkdev(nvme_major, "nvme");
+	nvme_core_exit();
  kill_workq:
 	destroy_workqueue(nvme_workq);
 	return result;

drivers/nvme/host/pci.c static int __init nvme_init(void) · line 2693

Quoted 8 lines
drivers/nvme/host/pci.c +1 -1
@@ -2848,7 +2687,7 @@ static int __init nvme_init(void)
 static void __exit nvme_exit(void)
 {
 	pci_unregister_driver(&nvme_driver);
-	unregister_blkdev(nvme_major, "nvme");
+	nvme_core_exit();
 	destroy_workqueue(nvme_workq);
 	class_destroy(nvme_class);
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");

Should nvme_core_{init,exit} be called in core.c?

I did this:
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/commit/?h=nvme-split/virtio&id=76fa970

author maintainer
#47 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

Quoted 1 line · Ming Lin

Should nvme_core_{init,exit} be called in core.c?

We can't do that until core.c is built into a separate module. Your patch will only work when NVMe is built into the kernel.

I want to defer the split until it can actually be used in tree.

#48 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

In reply to Christoph Hellwig
Quoted 88 lines · Christoph Hellwig

The namespace scanning code has been mostly generic already, we just need to store a pointer to the tagset in the nvme_ctrl structure, and add a method to check if a controller is I/O incapable. The latter will hopefully be replaced by a proper controller state machine soon.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 190 
+++++++++++++++++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h |  25 +++++-
 drivers/nvme/host/pci.c  | 221 +++++++------------------------------
----------
 3 files changed, 239 insertions(+), 197 deletions(-)
drivers/nvme/host/core.c +7 -2
index 3e88901..a01ab5a 100644
@@ -17,6 +17,8 @@
 #include <linux/errno.h>
 #include <linux/hdreg.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/list_sort.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/ptrace.h>
@@ -26,6 +28,9 @@
 
 #include "nvme.h"
 
+static int nvme_major;
+module_param(nvme_major, int, 0);
+
 DEFINE_SPINLOCK(dev_list_lock);
 
 static void nvme_free_ns(struct kref *kref)
@@ -41,7 +46,7 @@ static void nvme_free_ns(struct kref *kref)
 	kfree(ns);
 }
 
-void nvme_put_ns(struct nvme_ns *ns)
+static void nvme_put_ns(struct nvme_ns *ns)
 {
 	kref_put(&ns->kref, nvme_free_ns);
 }
@@ -503,7 +508,7 @@ static void nvme_config_discard(struct nvme_ns 
 *ns)
 	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
 }
 
-int nvme_revalidate_disk(struct gendisk *disk)
+static int nvme_revalidate_disk(struct gendisk *disk)
 {
 	struct nvme_ns *ns = disk->private_data;
drivers/nvme/host/core.c +3 -1
 	struct nvme_id_ns *id;
@@ -564,7 +569,7 @@ int nvme_revalidate_disk(struct gendisk *disk)
 	return 0;
 }
 
-const struct block_device_operations nvme_fops = {
+static const struct block_device_operations nvme_fops = {
 	.owner		= THIS_MODULE,
 	.ioctl		= nvme_ioctl,
 	.compat_ioctl	= nvme_compat_ioctl,
@@ -591,6 +596,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		return ret;
 	}
 	page_shift = NVME_CAP_MPSMIN(cap) + 12;
+	ctrl->page_size = 1 << page_shift;
 
 	ret = nvme_identify_ctrl(ctrl, &id);
 	if (ret) {
@@ -636,3 +642,181 @@ void nvme_put_ctrl(struct nvme_ctrl *ctrl)
 	kref_put(&ctrl->kref, nvme_free_ctrl);
 }
 
+static int ns_cmp(void *priv, struct list_head *a, struct list_head 

*b)

drivers/nvme/host/core.c +8 -0
+{
+	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
+	struct nvme_ns *nsb = container_of(b, struct nvme_ns, list);
+
+	return nsa->ns_id - nsb->ns_id;
+}
+
+static struct nvme_ns *nvme_find_ns(struct nvme_ctrl *ctrl, unsigned 

nsid)

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

Quoted 4 lines
drivers/nvme/host/core.c +4 -0
+{
+	struct nvme_ns *ns;
+
+	list_for_each_entry(ns, &ctrl->namespaces, list) {

I know this is basically just a move into core.c, but with the concept of namespace management in the 1.2 spec, would something like list_for_each_entry_safe() and/or a spinlock be more appropriate?

Quoted 7 lines
drivers/nvme/host/core.c +7 -0
+		if (ns->ns_id == nsid)
+			return ns;
+		if (ns->ns_id > nsid)
+			break;
+	}
+	return NULL;
+}

<snip>

Quoted 9 lines
drivers/nvme/host/core.c +2 -0
+
 static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.reg_read32		= nvme_pci_reg_read32,
 	.reg_read64		= nvme_pci_reg_read64,
+	.io_incapable		= nvme_pci_io_incapable,
 	.free_ctrl		= nvme_pci_free_ctrl,
 };

drivers/nvme/host/core.c static int nvme_probe(struct pci_dev *pdev, · line 2490

Quoted 6 lines
drivers/nvme/host/core.c +1 -1
@@ -2645,7 +2486,7 @@ static int nvme_probe(struct pci_dev *pdev, 
 const struct pci_device_id *id)
 	if (!dev->queues)
 		goto free;
 
-	INIT_LIST_HEAD(&dev->namespaces);
+	INIT_LIST_HEAD(&dev->ctrl.namespaces);

OK, so I'm looking at your git repo:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme
-split.5

and I see:

struct nvme_ctrl {
.
.

	struct list_head namespaces;

.
.
}

So that makes more sense, and I must have missed some code or future patch.

Quoted 90 lines
drivers/nvme/host/core.c +0 -0
 	INIT_WORK(&dev->reset_work, nvme_reset_work);
 	dev->dev = get_device(&pdev->dev);
drivers/nvme/host/core.c +6 -7
 	pci_set_drvdata(pdev, dev);
@@ -2664,17 +2505,17 @@ static int nvme_probe(struct pci_dev *pdev, 
 const struct pci_device_id *id)
 		goto release;
 
 	kref_init(&dev->ctrl.kref);
-	dev->device = device_create(nvme_class, &pdev->dev,
+	dev->ctrl.device = device_create(nvme_class, &pdev->dev,
 				MKDEV(nvme_char_major, dev
->ctrl.instance),
 				dev, "nvme%d", dev->ctrl.instance);
-	if (IS_ERR(dev->device)) {
-		result = PTR_ERR(dev->device);
+	if (IS_ERR(dev->ctrl.device)) {
+		result = PTR_ERR(dev->ctrl.device);
 		goto release_pools;
 	}
-	get_device(dev->device);
-	dev_set_drvdata(dev->device, dev);
+	get_device(dev->ctrl.device);
+	dev_set_drvdata(dev->ctrl.device, dev);
 
-	result = device_create_file(dev->device, 
 &dev_attr_reset_controller);
+	result = device_create_file(dev->ctrl.device, 

&dev_attr_reset_controller);

drivers/nvme/host/core.c +3 -4
 	if (result)
 		goto put_dev;
@@ -2686,7 +2527,7 @@ static int nvme_probe(struct pci_dev *pdev, 
 const struct pci_device_id *id)
 
  put_dev:
 	device_destroy(nvme_class, MKDEV(nvme_char_major, dev
->ctrl.instance));
-	put_device(dev->device);
+	put_device(dev->ctrl.device);
  release_pools:
 	nvme_release_prp_pools(dev);
  release:
@@ -2728,8 +2569,8 @@ static void nvme_remove(struct pci_dev *pdev)
 	flush_work(&dev->probe_work);
 	flush_work(&dev->reset_work);
 	flush_work(&dev->scan_work);
-	device_remove_file(dev->device, &dev_attr_reset_controller);
-	nvme_dev_remove(dev);
+	device_remove_file(dev->ctrl.device, 
 &dev_attr_reset_controller);
+	nvme_remove_namespaces(&dev->ctrl);
 	nvme_dev_shutdown(dev);
 	nvme_dev_remove_admin(dev);
drivers/nvme/host/core.c +1 -4
 	device_destroy(nvme_class, MKDEV(nvme_char_major, dev
->ctrl.instance));
@@ -2810,11 +2651,9 @@ static int __init nvme_init(void)
 	if (!nvme_workq)
 		return -ENOMEM;
 
-	result = register_blkdev(nvme_major, "nvme");
+	result = nvme_core_init();
 	if (result < 0)
 		goto kill_workq;
-	else if (result > 0)
-		nvme_major = result;
 
 	result = __register_chrdev(nvme_char_major, 0, NVME_MINORS, 
 "nvme",

&nvme_dev_fo
ps);

drivers/nvme/host/core.c +1 -1
@@ -2839,7 +2678,7 @@ static int __init nvme_init(void)
  unregister_chrdev:
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, 
 "nvme");
  unregister_blkdev:
-	unregister_blkdev(nvme_major, "nvme");
+	nvme_core_exit();
  kill_workq:
 	destroy_workqueue(nvme_workq);
drivers/nvme/host/core.c +1 -1
 	return result;
@@ -2848,7 +2687,7 @@ static int __init nvme_init(void)
 static void __exit nvme_exit(void)
 {
 	pci_unregister_driver(&nvme_driver);
-	unregister_blkdev(nvme_major, "nvme");
+	nvme_core_exit();
 	destroy_workqueue(nvme_workq);
 	class_destroy(nvme_class);
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, 

"nvme");

author maintainer
#49 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

Quoted 3 lines · J Freyensee

I know this is basically just a move into core.c, but with the concept of namespace management in the 1.2 spec, would something like list_for_each_entry_safe() and/or a spinlock be more appropriate?

Yes, someone should sit down and think about the concurrency model for namespace scanning. Also we really should look into using Identify for the active name space list as that will be a lot more efficient for controllers with lots of namespaces.

#50 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

Quoted 9 lines

-----Original Message-----
From: Christoph Hellwig [mailto:hch at lst.de]
Sent: Thursday, October 22, 2015 1:40 AM
To: J Freyensee
Cc: Christoph Hellwig; axboe at fb.com; Busch, Keith; Sternberg, Jay E; Cayton, Phil; mlin at kernel.org; linux-nvme at lists.infradead.org
Subject: Re: [PATCH 17/18] nvme: move namespace scanning to common code

On Wed, Oct 21, 2015@04:27:39PM -0700, J Freyensee wrote:

Quoted 8 lines
Quoted 3 lines

I know this is basically just a move into core.c, but with the concept of namespace management in the 1.2 spec, would something like list_for_each_entry_safe() and/or a spinlock be more appropriate?

Yes, someone should sit down and think about the concurrency model for namespace scanning. Also we really should look into using Identify for the active name space list as that will be a lot more efficient for controllers with lots of namespaces.

Here's a namespace list proposal:

http://lists.infradead.org/pipermail/linux-nvme/2015-September/002325.html

I'll rebase it on the latest and resend.

author maintainer
#51 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

Quoted 10 lines · Keith
Quoted 4 lines

Yes, someone should sit down and think about the concurrency model for namespace scanning. Also we really should look into using Identify for the active name space list as that will be a lot more efficient for controllers with lots of namespaces.

Here's a namespace list proposal:

http://lists.infradead.org/pipermail/linux-nvme/2015-September/002325.html

I'll rebase it on the latest and resend.

Looks good. I suspect we might want a fallback if the controller doesn't support Identify 0x2 - NVMe 1.1 doesn't mention anything about Identify subcommand being optional or not and 1.2 says "Controllers that support specification revision 1.1 or later shall support this capability." which isn't as a strong as a must. And some lazy people like me implemented NVMe targets systes that don't support it but claim to be version 1.2, although I'll try to get that fixed ASAP.

#52 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Keith

Here's a namespace list proposal:

http://lists.infradead.org/pipermail/linux-nvme/2015-September/002325.html

I'll rebase it on the latest and resend.

Quoted 7 lines

Looks good. I suspect we might want a fallback if the controller doesn't support Identify 0x2 - NVMe 1.1 doesn't mention anything about Identify subcommand being optional or not and 1.2 says "Controllers that support specification revision 1.1 or later shall support this capability." which isn't as a strong as a must. And some lazy people like me implemented NVMe targets systes that don't support it but claim to be version 1.2, although I'll try to get that fixed ASAP.

It does fall through to the older way if identify list fails. My concern is when it doesn't fail when it should have. Some controllers claim 1.1 or higher, but do not interpret the Identify Namespace List correctly. They just check that CNS != 0, and if true, returns success with an Identify Namespace structure, so the driver misinterprets the data.

I filed bugs with the vendors I know about, but there may be others I haven't tested.

author maintainer
#53 lore raw

PATCH 17/18 nvme: move namespace scanning to common code

Quoted 8 lines · Keith

It does fall through to the older way if identify list fails. My concern is when it doesn't fail when it should have. Some controllers claim 1.1 or higher, but do not interpret the Identify Namespace List correctly. They just check that CNS != 0, and if true, returns success with an Identify Namespace structure, so the driver misinterprets the data.

I filed bugs with the vendors I know about, but there may be others I haven't tested.

Can you blacklist the known bad controller using the quirks mechanism I added during the driver split?

author maintainer
#54 lore raw

PATCH 18/18 nvme: move chardev and sysfs interface to common code

In reply to Christoph Hellwig

For this we need to add a proper controller init routine and a list of all controllers that is in addition to the list of PCIe controllers, which stays in pci.c. Note that we remove the sysfs device when the last reference to a controller is dropped now - the old code would have kept it around longer, which doesn't make much sense.

This requires a new ->reset_ctrl operation to implement controleller resets, and a new ->write_reg32 operation that is required to implement subsystem resets. We also now store caches copied of the NVMe compliance version and the flag if a controller is attached to a subsystem or not in the generic controller structure now.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 217 +++++++++++++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  20 +++--
 drivers/nvme/host/pci.c  | 202 +++++--------------------------------------
 drivers/nvme/host/scsi.c |   9 +-
 4 files changed, 250 insertions(+), 198 deletions(-)
drivers/nvme/host/core.c +211 -6
index a01ab5a..b8c72d2 100644
@@ -28,11 +28,19 @@
 
 #include "nvme.h"
 
+#define NVME_MINORS		(1U << MINORBITS)
+
 static int nvme_major;
 module_param(nvme_major, int, 0);
 
+static int nvme_char_major;
+module_param(nvme_char_major, int, 0);
+
+static LIST_HEAD(nvme_ctrl_list);
 DEFINE_SPINLOCK(dev_list_lock);
 
+static struct class *nvme_class;
+
 static void nvme_free_ns(struct kref *kref)
 {
 	struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);
@@ -358,7 +366,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 			metadata, meta_len, io.slba, NULL, 0);
 }
 
-int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			struct nvme_passthru_cmd __user *ucmd)
 {
 	struct nvme_passthru_cmd cmd;
@@ -590,6 +598,12 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	u64 cap;
 	int ret, page_shift;
 
+	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
+	if (ret) {
+		dev_err(ctrl->dev, "Reading VS failed (%d)\n", ret);
+		return ret;
+	}
+
 	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &cap);
 	if (ret) {
 		dev_err(ctrl->dev, "Reading CAP failed (%d)\n", ret);
@@ -598,6 +612,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	page_shift = NVME_CAP_MPSMIN(cap) + 12;
 	ctrl->page_size = 1 << page_shift;
 
+	if (ctrl->vs >= NVME_VS(1, 1))
+		ctrl->subsystem = NVME_CAP_NSSRC(cap);
+
 	ret = nvme_identify_ctrl(ctrl, &id);
 	if (ret) {
 		dev_err(ctrl->dev, "Identify Controller failed (%d)\n", ret);
@@ -630,18 +647,85 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	return 0;
 }
 
-static void nvme_free_ctrl(struct kref *kref)
+static int nvme_dev_open(struct inode *inode, struct file *file)
 {
-	struct nvme_ctrl *ctrl = container_of(kref, struct nvme_ctrl, kref);
+	struct nvme_ctrl *ctrl;
+	int instance = iminor(inode);
+	int ret = -ENODEV;
 
-	ctrl->ops->free_ctrl(ctrl);
+	spin_lock(&dev_list_lock);
+	list_for_each_entry(ctrl, &nvme_ctrl_list, node) {
+		if (ctrl->instance != instance)
+			continue;
+
+		if (!ctrl->admin_q) {
+			ret = -EWOULDBLOCK;
+			break;
+		}
+		if (!kref_get_unless_zero(&ctrl->kref))
+			break;
+		file->private_data = ctrl;
+		ret = 0;
+		break;
+	}
+	spin_unlock(&dev_list_lock);
+
+	return ret;
 }
 
-void nvme_put_ctrl(struct nvme_ctrl *ctrl)
+static int nvme_dev_release(struct inode *inode, struct file *file)
 {
-	kref_put(&ctrl->kref, nvme_free_ctrl);
+	nvme_put_ctrl(file->private_data);
+	return 0;
+}
+
+static long nvme_dev_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg)
+{
+	struct nvme_ctrl *ctrl = file->private_data;
+	void __user *argp = (void __user *)arg;
+	struct nvme_ns *ns;
+
+	switch (cmd) {
+	case NVME_IOCTL_ADMIN_CMD:
+		return nvme_user_cmd(ctrl, NULL, argp);
+	case NVME_IOCTL_IO_CMD:
+		if (list_empty(&ctrl->namespaces))
+			return -ENOTTY;
+		ns = list_first_entry(&ctrl->namespaces, struct nvme_ns, list);
+		return nvme_user_cmd(ctrl, ns, argp);
+	case NVME_IOCTL_RESET:
+		dev_warn(ctrl->dev, "resetting controller\n");
+		return ctrl->ops->reset_ctrl(ctrl);
+	case NVME_IOCTL_SUBSYS_RESET:
+		return nvme_reset_subsystem(ctrl);
+	default:
+		return -ENOTTY;
+	}
 }
 
+static const struct file_operations nvme_dev_fops = {
+	.owner		= THIS_MODULE,
+	.open		= nvme_dev_open,
+	.release	= nvme_dev_release,
+	.unlocked_ioctl	= nvme_dev_ioctl,
+	.compat_ioctl	= nvme_dev_ioctl,
+};
+
+static ssize_t nvme_sysfs_reset(struct device *dev,
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	int ret;
+
+	ret = ctrl->ops->reset_ctrl(ctrl);
+	if (ret < 0)
+		return ret;
+	return count;
+}
+static DEVICE_ATTR(reset_controller, S_IWUSR, NULL, nvme_sysfs_reset);
+
 static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
 {
 	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
@@ -803,6 +887,106 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 		nvme_ns_remove(ns);
 }
 
+static DEFINE_IDA(nvme_instance_ida);
+
+static int nvme_set_instance(struct nvme_ctrl *ctrl)
+{
+	int instance, error;
+
+	do {
+		if (!ida_pre_get(&nvme_instance_ida, GFP_KERNEL))
+			return -ENODEV;
+
+		spin_lock(&dev_list_lock);
+		error = ida_get_new(&nvme_instance_ida, &instance);
+		spin_unlock(&dev_list_lock);
+	} while (error == -EAGAIN);
+
+	if (error)
+		return -ENODEV;
+
+	ctrl->instance = instance;
+	return 0;
+}
+
+static void nvme_release_instance(struct nvme_ctrl *ctrl)
+{
+	spin_lock(&dev_list_lock);
+	ida_remove(&nvme_instance_ida, ctrl->instance);
+	spin_unlock(&dev_list_lock);
+}
+
+static void nvme_free_ctrl(struct kref *kref)
+{
+	struct nvme_ctrl *ctrl = container_of(kref, struct nvme_ctrl, kref);
+
+	spin_lock(&dev_list_lock);
+	list_del(&ctrl->node);
+	spin_unlock(&dev_list_lock);
+
+	put_device(ctrl->device);
+	nvme_release_instance(ctrl);
+	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
+
+	ctrl->ops->free_ctrl(ctrl);
+}
+
+void nvme_put_ctrl(struct nvme_ctrl *ctrl)
+{
+	kref_put(&ctrl->kref, nvme_free_ctrl);
+}
+
+/*
+ * Initialize a NVMe controller structures.  This needs to be called during
+ * earliest initialization so that we have the initialized structured around
+ * during probing.
+ */
+int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
+		const struct nvme_ctrl_ops *ops, u16 vendor,
+		unsigned long quirks)
+{
+	int ret;
+
+	INIT_LIST_HEAD(&ctrl->namespaces);
+	kref_init(&ctrl->kref);
+	ctrl->dev = dev;
+	ctrl->ops = ops;
+	ctrl->vendor = vendor;
+	ctrl->quirks = quirks;
+
+	ret = nvme_set_instance(ctrl);
+	if (ret)
+		goto out;
+
+	ctrl->device = device_create(nvme_class, ctrl->dev,
+				MKDEV(nvme_char_major, ctrl->instance),
+				dev, "nvme%d", ctrl->instance);
+	if (IS_ERR(ctrl->device)) {
+		ret = PTR_ERR(ctrl->device);
+		goto out_release_instance;
+	}
+	get_device(ctrl->device);
+	dev_set_drvdata(ctrl->device, ctrl);
+
+	ret = device_create_file(ctrl->device, &dev_attr_reset_controller);
+	if (ret)
+		goto out_put_device;
+
+	spin_lock(&dev_list_lock);
+	list_add_tail(&ctrl->node, &nvme_ctrl_list);
+	spin_unlock(&dev_list_lock);
+
+	return 0;
+
+out_put_device:
+	put_device(ctrl->device);
+	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
+out_release_instance:
+	nvme_release_instance(ctrl);
+out:
+	return ret;
+}
+
 int __init nvme_core_init(void)
 {
 	int result;
@@ -813,10 +997,31 @@ int __init nvme_core_init(void)
 	else if (result > 0)
 		nvme_major = result;
 
+	result = __register_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme",
+							&nvme_dev_fops);
+	if (result < 0)
+		goto unregister_blkdev;
+	else if (result > 0)
+		nvme_char_major = result;
+
+	nvme_class = class_create(THIS_MODULE, "nvme");
+	if (IS_ERR(nvme_class)) {
+		result = PTR_ERR(nvme_class);
+		goto unregister_chrdev;
+	}
+
 	return 0;
+
+ unregister_chrdev:
+	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");
+ unregister_blkdev:
+	unregister_blkdev(nvme_major, "nvme");
+	return result;
 }
 
 void nvme_core_exit(void)
 {
 	unregister_blkdev(nvme_major, "nvme");
+	class_destroy(nvme_class);
+	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");
 }
drivers/nvme/host/nvme.h +15 -5
index 53e82feb..da63835 100644
@@ -19,8 +19,6 @@
 #include <linux/kref.h>
 #include <linux/blk-mq.h>
 
-struct nvme_passthru_cmd;
-
 extern unsigned char nvme_io_timeout;
 #define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
@@ -48,6 +46,7 @@ struct nvme_ctrl {
 	struct blk_mq_tag_set *tagset;
 	struct list_head namespaces;
 	struct device *device;	/* char device */
+	struct list_head node;
 
 	char name[12];
 	char serial[20];
@@ -60,6 +59,8 @@ struct nvme_ctrl {
 	u16 abort_limit;
 	u8 event_limit;
 	u8 vwc;
+	u32 vs;
+	bool subsystem;
 	u16 vendor;
 	unsigned long quirks;
 };
@@ -87,7 +88,9 @@ struct nvme_ns {
 struct nvme_ctrl_ops {
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
+	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
 	bool (*io_incapable)(struct nvme_ctrl *ctrl);
+	int (*reset_ctrl)(struct nvme_ctrl *ctrl);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 };
 
@@ -111,6 +114,13 @@ static inline bool nvme_io_incapable(struct nvme_ctrl *ctrl)
 	return val & NVME_CSTS_CFS;
 }
 
+static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
+{
+	if (!ctrl->subsystem)
+		return -ENOTTY;
+	return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 0x4E564D65);
+}
+
 static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector)
 {
 	return (sector >> (ns->lba_shift - 9));
@@ -179,6 +189,9 @@ static inline int nvme_error_status(u16 status)
 	}
 }
 
+int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
+		const struct nvme_ctrl_ops *ops, u16 vendor,
+		unsigned long quirks);
 void nvme_put_ctrl(struct nvme_ctrl *ctrl);
 int nvme_init_identify(struct nvme_ctrl *ctrl);
 
@@ -207,9 +220,6 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 
 extern spinlock_t dev_list_lock;
 
-int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-			struct nvme_passthru_cmd __user *ucmd);
-
 struct sg_io_hdr;
 
 int nvme_sg_io(struct nvme_ns *ns, struct sg_io_hdr __user *u_hdr);
drivers/nvme/host/pci.c +22 -180

315 lines not on this page · raw message

drivers/nvme/host/scsi.c +2 -7
index b673fe4..34c65b9 100644
@@ -606,16 +606,11 @@ static int nvme_trans_device_id_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	int res;
 	int nvme_sc;
 	int xfer_len;
-	u32 vs;
 	__be32 tmp_id = cpu_to_be32(ns->ns_id);
 
-	res = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &vs);
-	if (res)
-		return res;
-
 	memset(inq_response, 0, alloc_len);
 	inq_response[1] = INQ_DEVICE_IDENTIFICATION_PAGE;    /* Page Code */
-	if (vs >= NVME_VS(1, 1)) {
+	if (ctrl->vs >= NVME_VS(1, 1)) {
 		struct nvme_id_ns *id_ns;
 		void *eui;
 		int len;
@@ -627,7 +622,7 @@ static int nvme_trans_device_id_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 
 		eui = id_ns->eui64;
 		len = sizeof(id_ns->eui64);
-		if (vs >= NVME_VS(1, 2)) {
+		if (ctrl->vs >= NVME_VS(1, 2)) {
 			if (bitmap_empty(eui, len * 8)) {
 				eui = id_ns->nguid;
 				len = sizeof(id_ns->nguid);
Signature · 1 line
1.9.1
#55 lore raw

PATCH 18/18 nvme: move chardev and sysfs interface to common code

Quoted 9 lines · Christoph Hellwig

For this we need to add a proper controller init routine and a list of
all controllers that is in addition to the list of PCIe controllers, which stays in pci.c. Note that we remove the sysfs device when the last reference to a controller is dropped now - the old code would have
kept it around longer, which doesn't make much sense.

This requires a new ->reset_ctrl operation to implement controleller

Controller got misspelled above.

Quoted 84 lines

resets, and a new ->write_reg32 operation that is required to implement
subsystem resets. We also now store caches copied of the NVMe compliance
version and the flag if a controller is attached to a subsystem or not in
the generic controller structure now.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 217 
+++++++++++++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  20 +++--
 drivers/nvme/host/pci.c  | 202 +++++--------------------------------
------
 drivers/nvme/host/scsi.c |   9 +-
 4 files changed, 250 insertions(+), 198 deletions(-)
drivers/nvme/host/core.c +8 -0
index a01ab5a..b8c72d2 100644
@@ -28,11 +28,19 @@
 
 #include "nvme.h"
 
+#define NVME_MINORS		(1U << MINORBITS)
+
 static int nvme_major;
 module_param(nvme_major, int, 0);
 
+static int nvme_char_major;
+module_param(nvme_char_major, int, 0);
+
+static LIST_HEAD(nvme_ctrl_list);
 DEFINE_SPINLOCK(dev_list_lock);
 
+static struct class *nvme_class;
+
 static void nvme_free_ns(struct kref *kref)
 {
 	struct nvme_ns *ns = container_of(kref, struct nvme_ns, 

kref);

drivers/nvme/host/core.c +1 -1
@@ -358,7 +366,7 @@ static int nvme_submit_io(struct nvme_ns *ns, 
 struct nvme_user_io __user *uio)
 			metadata, meta_len, io.slba, NULL, 0);
 }
 
-int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			struct nvme_passthru_cmd __user *ucmd)
 {
drivers/nvme/host/core.c +6 -0
 	struct nvme_passthru_cmd cmd;
@@ -590,6 +598,12 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	u64 cap;
 	int ret, page_shift;
 
+	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
+	if (ret) {
+		dev_err(ctrl->dev, "Reading VS failed (%d)\n", ret);
+		return ret;
+	}
+
 	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &cap);
 	if (ret) {
 		dev_err(ctrl->dev, "Reading CAP failed (%d)\n", 

ret);

drivers/nvme/host/core.c +3 -0
@@ -598,6 +612,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	page_shift = NVME_CAP_MPSMIN(cap) + 12;
 	ctrl->page_size = 1 << page_shift;
 
+	if (ctrl->vs >= NVME_VS(1, 1))
+		ctrl->subsystem = NVME_CAP_NSSRC(cap);
+
 	ret = nvme_identify_ctrl(ctrl, &id);
 	if (ret) {
 		dev_err(ctrl->dev, "Identify Controller failed 

(%d)\n", ret);

drivers/nvme/host/core.c +1 -1
@@ -630,18 +647,85 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	return 0;
 }
 
-static void nvme_free_ctrl(struct kref *kref)
+static int nvme_dev_open(struct inode *inode, struct file *file)

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 658

Quoted 10 lines
drivers/nvme/host/core.c +5 -2
 {
-	struct nvme_ctrl *ctrl = container_of(kref, struct 
 nvme_ctrl, kref);
+	struct nvme_ctrl *ctrl;
+	int instance = iminor(inode);
+	int ret = -ENODEV;
 
-	ctrl->ops->free_ctrl(ctrl);
+	spin_lock(&dev_list_lock);
+	list_for_each_entry(ctrl, &nvme_ctrl_list, node) {

list_for_each_entry_safe() and/or some type of lock access?

Quoted 199 lines
drivers/nvme/host/core.c +65 -2
+		if (ctrl->instance != instance)
+			continue;
+
+		if (!ctrl->admin_q) {
+			ret = -EWOULDBLOCK;
+			break;
+		}
+		if (!kref_get_unless_zero(&ctrl->kref))
+			break;
+		file->private_data = ctrl;
+		ret = 0;
+		break;
+	}
+	spin_unlock(&dev_list_lock);
+
+	return ret;
 }
 
-void nvme_put_ctrl(struct nvme_ctrl *ctrl)
+static int nvme_dev_release(struct inode *inode, struct file *file)
 {
-	kref_put(&ctrl->kref, nvme_free_ctrl);
+	nvme_put_ctrl(file->private_data);
+	return 0;
+}
+
+static long nvme_dev_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg)
+{
+	struct nvme_ctrl *ctrl = file->private_data;
+	void __user *argp = (void __user *)arg;
+	struct nvme_ns *ns;
+
+	switch (cmd) {
+	case NVME_IOCTL_ADMIN_CMD:
+		return nvme_user_cmd(ctrl, NULL, argp);
+	case NVME_IOCTL_IO_CMD:
+		if (list_empty(&ctrl->namespaces))
+			return -ENOTTY;
+		ns = list_first_entry(&ctrl->namespaces, struct 
 nvme_ns, list);
+		return nvme_user_cmd(ctrl, ns, argp);
+	case NVME_IOCTL_RESET:
+		dev_warn(ctrl->dev, "resetting controller\n");
+		return ctrl->ops->reset_ctrl(ctrl);
+	case NVME_IOCTL_SUBSYS_RESET:
+		return nvme_reset_subsystem(ctrl);
+	default:
+		return -ENOTTY;
+	}
 }
 
+static const struct file_operations nvme_dev_fops = {
+	.owner		= THIS_MODULE,
+	.open		= nvme_dev_open,
+	.release	= nvme_dev_release,
+	.unlocked_ioctl	= nvme_dev_ioctl,
+	.compat_ioctl	= nvme_dev_ioctl,
+};
+
+static ssize_t nvme_sysfs_reset(struct device *dev,
+				struct device_attribute *attr, const 
 char *buf,
+				size_t count)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	int ret;
+
+	ret = ctrl->ops->reset_ctrl(ctrl);
+	if (ret < 0)
+		return ret;
+	return count;
+}
+static DEVICE_ATTR(reset_controller, S_IWUSR, NULL, 

nvme_sysfs_reset);
+
static int ns_cmp(void *priv, struct list_head *a, struct list_head
*b)
{

drivers/nvme/host/core.c +51 -1
 	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
@@ -803,6 +887,106 @@ void nvme_remove_namespaces(struct nvme_ctrl 
 *ctrl)
 		nvme_ns_remove(ns);
 }
 
+static DEFINE_IDA(nvme_instance_ida);
+
+static int nvme_set_instance(struct nvme_ctrl *ctrl)
+{
+	int instance, error;
+
+	do {
+		if (!ida_pre_get(&nvme_instance_ida, GFP_KERNEL))
+			return -ENODEV;
+
+		spin_lock(&dev_list_lock);
+		error = ida_get_new(&nvme_instance_ida, &instance);
+		spin_unlock(&dev_list_lock);
+	} while (error == -EAGAIN);
+
+	if (error)
+		return -ENODEV;
+
+	ctrl->instance = instance;
+	return 0;
+}
+
+static void nvme_release_instance(struct nvme_ctrl *ctrl)
+{
+	spin_lock(&dev_list_lock);
+	ida_remove(&nvme_instance_ida, ctrl->instance);
+	spin_unlock(&dev_list_lock);
+}
+
+static void nvme_free_ctrl(struct kref *kref)
+{
+	struct nvme_ctrl *ctrl = container_of(kref, struct 
 nvme_ctrl, kref);
+
+	spin_lock(&dev_list_lock);
+	list_del(&ctrl->node);
+	spin_unlock(&dev_list_lock);
+
+	put_device(ctrl->device);
+	nvme_release_instance(ctrl);
+	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl
->instance));
+
+	ctrl->ops->free_ctrl(ctrl);
+}
+
+void nvme_put_ctrl(struct nvme_ctrl *ctrl)
+{
+	kref_put(&ctrl->kref, nvme_free_ctrl);
+}
+
+/*
+ * Initialize a NVMe controller structures.  This needs to be called 

during
+ * earliest initialization so that we have the initialized structured around
+ * during probing.
+ */

drivers/nvme/host/core.c +28 -1
+int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
+		const struct nvme_ctrl_ops *ops, u16 vendor,
+		unsigned long quirks)
+{
+	int ret;
+
+	INIT_LIST_HEAD(&ctrl->namespaces);
+	kref_init(&ctrl->kref);
+	ctrl->dev = dev;
+	ctrl->ops = ops;
+	ctrl->vendor = vendor;
+	ctrl->quirks = quirks;
+
+	ret = nvme_set_instance(ctrl);
+	if (ret)
+		goto out;
+
+	ctrl->device = device_create(nvme_class, ctrl->dev,
+				MKDEV(nvme_char_major, ctrl
->instance),
+				dev, "nvme%d", ctrl->instance);
+	if (IS_ERR(ctrl->device)) {
+		ret = PTR_ERR(ctrl->device);
+		goto out_release_instance;
+	}
+	get_device(ctrl->device);
+	dev_set_drvdata(ctrl->device, ctrl);
+
+	ret = device_create_file(ctrl->device, 

&dev_attr_reset_controller);

drivers/nvme/host/core.c +18 -1
+	if (ret)
+		goto out_put_device;
+
+	spin_lock(&dev_list_lock);
+	list_add_tail(&ctrl->node, &nvme_ctrl_list);
+	spin_unlock(&dev_list_lock);
+
+	return 0;
+
+out_put_device:
+	put_device(ctrl->device);
+	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl
->instance));
+out_release_instance:
+	nvme_release_instance(ctrl);
+out:
+	return ret;
+}
+
 int __init nvme_core_init(void)
 {
 	int result;
@@ -813,10 +997,31 @@ int __init nvme_core_init(void)
 	else if (result > 0)
 		nvme_major = result;
 

drivers/nvme/host/core.c int __init nvme_core_init(void) · line 1009

Quoted 10 lines
drivers/nvme/host/core.c +8 -0
+	result = __register_chrdev(nvme_char_major, 0, NVME_MINORS, 
 "nvme",
+							&nvme_dev_fo
 ps);
+	if (result < 0)
+		goto unregister_blkdev;
+	else if (result > 0)
+		nvme_char_major = result;
+
+	nvme_class = class_create(THIS_MODULE, "nvme");

It would be better to have "nvme" as a #define somewhere, probably in the .h?

Quoted 44 lines
drivers/nvme/host/core.c +13 -0
+	if (IS_ERR(nvme_class)) {
+		result = PTR_ERR(nvme_class);
+		goto unregister_chrdev;
+	}
+
 	return 0;
+
+ unregister_chrdev:
+	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, 
 "nvme");
+ unregister_blkdev:
+	unregister_blkdev(nvme_major, "nvme");
+	return result;
 }
 
 void nvme_core_exit(void)
 {
 	unregister_blkdev(nvme_major, "nvme");
+	class_destroy(nvme_class);
+	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, 

"nvme");
}

drivers/nvme/host/nvme.h +1 -2
index 53e82feb..da63835 100644
@@ -19,8 +19,6 @@
 #include <linux/kref.h>
 #include <linux/blk-mq.h>
 
-struct nvme_passthru_cmd;
-
 extern unsigned char nvme_io_timeout;
 #define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
@@ -48,6 +46,7 @@ struct nvme_ctrl {
 	struct blk_mq_tag_set *tagset;
 	struct list_head namespaces;
 	struct device *device;	/* char device */
+	struct list_head node;
 
 	char name[12];
 	char serial[20];

drivers/nvme/host/nvme.h struct nvme_ctrl { · line 63

Quoted 5 lines
drivers/nvme/host/nvme.h +2 -0
@@ -60,6 +59,8 @@ struct nvme_ctrl {
 	u16 abort_limit;
 	u8 event_limit;
 	u8 vwc;
+	u32 vs;
+	bool subsystem;

OK, so 'bool subsystem' got added back in. Not sure still how a bool helps define a controller into a given subsystem. Isn't the definition of an NVM subsystem 1 or more controllers? So every new 'struct nvme_ctrl' instance is going to set this to 'true'?

Or looking into the future, say if this is on a future fabric connection, there could be lots of controllers under lots of distinct subsystems. Then I don't know how 'bool subsystem' makes sense and distinguishes a controller in a given NVM subsystem.

drivers/nvme/host/nvme.h struct nvme_ns { · line 94

Quoted 11 lines
drivers/nvme/host/nvme.h +1 -0
 	u16 vendor;
 	unsigned long quirks;
 };
@@ -87,7 +88,9 @@ struct nvme_ns {
 struct nvme_ctrl_ops {
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 
 *val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 
 *val);
+	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 
 val);

I don't think they are being used currently, but the ACQ and ASQ 8-byte registers do have RW fields. Maybe add "int (*reg_write64)()" as well?

drivers/nvme/host/nvme.h struct nvme_ns { · line 93

Quoted 2 lines
drivers/nvme/host/nvme.h +1 -0
 	bool (*io_incapable)(struct nvme_ctrl *ctrl);
+	int (*reset_ctrl)(struct nvme_ctrl *ctrl);

Probably would want a "(*shutdown_ctrl)()" as well?

Quoted 4 lines
drivers/nvme/host/nvme.h +0 -0
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 };

drivers/nvme/host/nvme.h static inline bool nvme_io_incapable(struct · line 123

Quoted 10 lines
drivers/nvme/host/nvme.h +5 -0
@@ -111,6 +114,13 @@ static inline bool nvme_io_incapable(struct 
 nvme_ctrl *ctrl)
 	return val & NVME_CSTS_CFS;
 }
 
+static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
+{
+	if (!ctrl->subsystem)
+		return -ENOTTY;
+	return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 
 0x4E564D65);

It would be really good to have the hex value in a #define, most likely located in the nvme.h file.

drivers/nvme/host/nvme.h static inline bool nvme_io_incapable(struct · line 126

Quoted 3 lines
drivers/nvme/host/nvme.h +2 -0
+}
+
 

Lots of good work Christoph, thanks,
Jay

author maintainer
#56 lore raw

PATCH 18/18 nvme: move chardev and sysfs interface to common code

Quoted 4 lines · J Freyensee

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 657

Quoted 2 lines
drivers/nvme/host/core.c +2 -0
+	spin_lock(&dev_list_lock);
+	list_for_each_entry(ctrl, &nvme_ctrl_list, node) {

list_for_each_entry_safe() and/or some type of lock access?

list_for_each_entry_safe does not synchronize, it just ensures you can continue to iterate after deleting the current item.

And the spin_lock() call above provides the required synchronization.

Quoted 9 lines

drivers/nvme/host/core.c int __init nvme_core_init(void) · line 1007

Quoted 6 lines
drivers/nvme/host/core.c +6 -0
+	if (result < 0)
+		goto unregister_blkdev;
+	else if (result > 0)
+		nvme_char_major = result;
+
+	nvme_class = class_create(THIS_MODULE, "nvme");

It would be better to have "nvme" as a #define somewhere, probably in the .h?

Why?

Quoted 5 lines
Quoted 5 lines
drivers/nvme/host/nvme.h +0 -0
 	char name[12];
 	char serial[20];
@@ -60,6 +59,8 @@ struct nvme_ctrl {
 	u16 abort_limit;
 	u8 event_limit;
Quoted 8 lines

drivers/nvme/host/nvme.h struct nvme_ctrl { · line 63

Quoted 3 lines
drivers/nvme/host/nvme.h +2 -0
 	u8 vwc;
+	u32 vs;
+	bool subsystem;

OK, so 'bool subsystem' got added back in. Not sure still how a bool helps define a controller into a given subsystem. Isn't the definition of an NVM subsystem 1 or more controllers? So every new 'struct nvme_ctrl' instance is going to set this to 'true'?

In NVMe 1.0 the concept of subsystems didn't exist. Now strictly speaking what we care about here is if a subsystem _reset_ is supported, but I've kept the name from the existing code for now.

Quoted 4 lines

Or looking into the future, say if this is on a future fabric connection, there could be lots of controllers under lots of distinct subsystems. Then I don't know how 'bool subsystem' makes sense and distinguishes a controller in a given NVM subsystem.

That's not what the flag is used for. It just indicates if we can do a subsystem reset.

Quoted 11 lines

Review struct nvme_ns { · line 94

Quoted 8 lines
(quoted diff) +1 -0
@@ -87,7 +88,9 @@ struct nvme_ns {
 struct nvme_ctrl_ops {
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 
 *val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 
 *val);
+	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 
 val);

I don't think they are being used currently, but the ACQ and ASQ 8-byte registers do have RW fields. Maybe add "int (*reg_write64)()" as well?

We can do that once we actually need it. But at least ACQ and ASQ are deeply tied to PCI specific initialization so right now there is no need for that.

Quoted 4 lines

drivers/nvme/host/nvme.h struct nvme_ns { · line 93

Quoted 2 lines
drivers/nvme/host/nvme.h +1 -0
 	bool (*io_incapable)(struct nvme_ctrl *ctrl);
+	int (*reset_ctrl)(struct nvme_ctrl *ctrl);

Probably would want a "(*shutdown_ctrl)()" as well?

We currently don't shut the controller down from generic code, so until we have a state machine that might do that there's no need for that.

Quoted 6 lines
Quoted 3 lines
(quoted diff) +1 -0
+	return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 

0x4E564D65);

It would be really good to have the hex value in a #define, most likely located in the nvme.h file.

Feel free to send incremental patches for these sorts of cleanups!

#57 lore raw

PATCH 18/18 nvme: move chardev and sysfs interface to common code

Quoted 22 lines · Christoph Hellwig
Quoted 4 lines · J Freyensee

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 657

Quoted 2 lines
drivers/nvme/host/core.c +2 -0
+	spin_lock(&dev_list_lock);
+	list_for_each_entry(ctrl, &nvme_ctrl_list, node) {

list_for_each_entry_safe() and/or some type of lock access?

list_for_each_entry_safe does not synchronize, it just ensures you can continue to iterate after deleting the current item.

And the spin_lock() call above provides the required synchronization.

Quoted 10 lines

drivers/nvme/host/core.c int __init nvme_core_init(void) · line 1007

Quoted 6 lines
drivers/nvme/host/core.c +6 -0
+	if (result < 0)
+		goto unregister_blkdev;
+	else if (result > 0)
+		nvme_char_major = result;
+
+	nvme_class = class_create(THIS_MODULE, "nvme");

It would be better to have "nvme" as a #define somewhere, probably in
the .h?

Quoted 1 line

Why?

Well, for starters, it's good practice to have strings like this in a #define.

I can send a patch to redefine this and 0x4E564D65 in the nvme.h.

Quoted 17 lines
Quoted 16 lines

drivers/nvme/host/nvme.h struct nvme_ctrl { · line 63

Quoted 9 lines
drivers/nvme/host/nvme.h +2 -0
 	char name[12];
 	char serial[20];
@@ -60,6 +59,8 @@ struct nvme_ctrl {
 	u16 abort_limit;
 	u8 event_limit;
 	u8 vwc;
+	u32 vs;
+	bool subsystem;

OK, so 'bool subsystem' got added back in. Not sure still how a bool
helps define a controller into a given subsystem. Isn't the definition
of an NVM subsystem 1 or more controllers? So every new 'struct nvme_ctrl' instance is going to set this to 'true'?

Quoted 5 lines

In NVMe 1.0 the concept of subsystems didn't exist. Now strictly speaking
what we care about here is if a subsystem _reset_ is supported, but I've
kept the name from the existing code for now.

OK understood.

Quoted 48 lines
Quoted 6 lines

Or looking into the future, say if this is on a future fabric connection, there could be lots of controllers under lots of distinct
subsystems. Then I don't know how 'bool subsystem' makes sense and distinguishes a controller in a given NVM subsystem.

That's not what the flag is used for. It just indicates if we can do a subsystem reset.

Quoted 5 lines
Quoted 5 lines
(quoted diff) +0 -0
@@ -87,7 +88,9 @@ struct nvme_ns {
 struct nvme_ctrl_ops {
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 
 *val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 
Quoted 8 lines

Review struct nvme_ns { · line 94

Quoted 3 lines
(quoted diff) +1 -0
 *val);
+	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 
 val);

I don't think they are being used currently, but the ACQ and ASQ 8 -byte
registers do have RW fields. Maybe add "int (*reg_write64)()" as well?

We can do that once we actually need it. But at least ACQ and ASQ are
deeply tied to PCI specific initialization so right now there is no need
for that.

Quoted 4 lines

drivers/nvme/host/nvme.h struct nvme_ns { · line 93

Quoted 2 lines
drivers/nvme/host/nvme.h +1 -0
 	bool (*io_incapable)(struct nvme_ctrl *ctrl);
+	int (*reset_ctrl)(struct nvme_ctrl *ctrl);

Probably would want a "(*shutdown_ctrl)()" as well?

We currently don't shut the controller down from generic code, so until
we have a state machine that might do that there's no need for that.

Quoted 7 lines
Quoted 3 lines
(quoted diff) +1 -0
+	return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 

0x4E564D65);

It would be really good to have the hex value in a #define, most likely
located in the nvme.h file.

Feel free to send incremental patches for these sorts of cleanups!

#58 lore raw

PATCH 18/18 nvme: move chardev and sysfs interface to common code

Quoted 6 lines
Quoted 1 line

Why?

Well, for starters, it's good practice to have strings like this in a #define.

I can send a patch to redefine this and 0x4E564D65 in the nvme.h.

IMO, make the hex "NVMe" a #define or at least keep its comment.
Keep "nvme" in the class_create call because there is no point in obfuscating that :)