nvme: add a constant for the identify payload size

PATCH New Christoph Hellwig 4 participants 7 messages Mar 9, 2016 – Mar 10, 2016 NVMe

Share & tools

Christoph Hellwig

author maintainer

2016-03-09 16:54 UTC #0 lore raw

PATCH 1/1 nvme: add a constant for the identify payload size

The NVMe spec specifies a hardcoded payload length of 4k for all types of Identify commands. Use a constant instead of hardcoded values that can be confused for the page size.

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

---
 drivers/nvme/host/core.c     | 5 +++--
 drivers/nvme/host/lightnvm.c | 2 +-
 drivers/nvme/host/pci.c      | 4 ++--
 include/linux/nvme.h         | 2 ++
 4 files changed, 8 insertions(+), 5 deletions(-)

drivers/nvme/host/core.c +3 -2

index 4304be0..64d8ede 100644
@@ -302,7 +302,8 @@ static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *n
 	c.identify.opcode = nvme_admin_identify;
 	c.identify.cns = cpu_to_le32(2);
 	c.identify.nsid = cpu_to_le32(nsid);
-	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list, 0x1000);
+	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list,
+			NVME_IDENTIFY_DATA_SIZE);
 }
 
 int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,

@@ -1294,7 +1295,7 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl, unsigned nn)
 	unsigned i, j, nsid, prev = 0, num_lists = DIV_ROUND_UP(nn, 1024);
 	int ret = 0;
 
-	ns_list = kzalloc(0x1000, GFP_KERNEL);
+	ns_list = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
 	if (!ns_list)
 		return -ENOMEM;

drivers/nvme/host/lightnvm.c +1 -1

index d4f81f0..da236f0 100644
@@ -239,7 +239,7 @@ static inline void _nvme_nvm_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_nvm_erase_blk) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_nvm_id_group) != 960);
 	BUILD_BUG_ON(sizeof(struct nvme_nvm_addr_format) != 128);
-	BUILD_BUG_ON(sizeof(struct nvme_nvm_id) != 4096);
+	BUILD_BUG_ON(sizeof(struct nvme_nvm_id) != NVME_IDENTIFY_DATA_SIZE);
 	BUILD_BUG_ON(sizeof(struct nvme_nvm_bb_tbl) != 512);
 }

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

index e9f18e1..1d1c6d1 100644
@@ -172,8 +172,8 @@ static inline void _nvme_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_format_cmd) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_abort_cmd) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
-	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl) != 4096);
-	BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
+	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl) != NVME_IDENTIFY_DATA_SIZE);
+	BUILD_BUG_ON(sizeof(struct nvme_id_ns) != NVME_IDENTIFY_DATA_SIZE);
 	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
 }

include/linux/nvme.h +2 -0

index a55986f..b0909ad 100644
@@ -153,6 +153,8 @@ struct nvme_lbaf {
 	__u8			rp;
 };
 
+#define NVME_IDENTIFY_DATA_SIZE	0x1000
+
 struct nvme_id_ns {
 	__le64			nsze;
 	__le64			ncap;

Signature · 1 line

2.1.4

Sagi Grimberg

2016-03-09 16:57 UTC #1 lore raw

PATCH 1/1 nvme: add a constant for the identify payload size

Looks good,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>

Matthew Wilcox

2016-03-09 19:07 UTC #2 lore raw

PATCH 1/1 nvme: add a constant for the identify payload size

In reply to Christoph Hellwig

Quoted 3 lines · Christoph Hellwig

The NVMe spec specifies a hardcoded payload length of 4k for all types of Identify commands. Use a constant instead of hardcoded values that can be confused for the page size.

I think the problem here is that we don't have a struct for nvme_ns_list.

Something like this:

drivers/nvme/host/core.c +7 -5

index e2d11d6..a4a7210 100644
@@ -296,14 +296,16 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 	return error;
 }
 
-static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
+static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid,
+				struct nvme_id_ns_list *ns_list)
 {
 	struct nvme_command c = { };
 
 	c.identify.opcode = nvme_admin_identify;
 	c.identify.cns = cpu_to_le32(2);
 	c.identify.nsid = cpu_to_le32(nsid);
-	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list, 0x1000);
+	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list,
+					sizeof(*ns_list));
 }
 
 int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,

@@ -1296,11 +1298,11 @@ static void nvme_validate_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 static int nvme_scan_ns_list(struct nvme_ctrl *ctrl, unsigned nn)
 {
 	struct nvme_ns *ns;
-	__le32 *ns_list;
+	struct nvme_id_ns_list *ns_list;
 	unsigned i, j, nsid, prev = 0, num_lists = DIV_ROUND_UP(nn, 1024);
 	int ret = 0;
 
-	ns_list = kzalloc(0x1000, GFP_KERNEL);
+	ns_list = kzalloc(sizeof(*ns_list), GFP_KERNEL);
 	if (!ns_list)
 		return -ENOMEM;

@@ -1310,7 +1312,7 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl, unsigned nn)
 			goto out;
 
 		for (j = 0; j < min(nn, 1024U); j++) {
-			nsid = le32_to_cpu(ns_list[j]);
+			nsid = le32_to_cpu(ns_list->nsid[j]);
 			if (!nsid)
 				goto out;

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

index aff71ef..ec2198c 100644
@@ -175,6 +175,7 @@ static inline void _nvme_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl) != 4096);
 	BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
+	BUILD_BUG_ON(sizeof(struct nvme_id_ns_list) != 4096);
 	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
 }

include/linux/nvme.h +4 -0

index a55986f..e9122cf 100644
@@ -183,6 +183,10 @@ struct nvme_id_ns {
 	__u8			vs[3712];
 };
 
+struct nvme_id_ns_list {
+	__le32			nsid[1024];
+};
+
 enum {
 	NVME_NS_FEAT_THIN	= 1 << 0,
 	NVME_NS_FLBAS_LBA_MASK	= 0xf,

Christoph Hellwig

author maintainer

2016-03-10 08:06 UTC #3 lore raw

PATCH 1/1 nvme: add a constant for the identify payload size

Quoted 6 lines · Matthew Wilcox

Quoted 3 lines · Christoph Hellwig
The NVMe spec specifies a hardcoded payload length of 4k for all types of Identify commands. Use a constant instead of hardcoded values that can be confused for the page size.
I think the problem here is that we don't have a struct for nvme_ns_list.

I think this structure is useful, but I'd still like to see a constant for the identify payload - the spec clearly specifies it as a hardcoded limit for all of identify. E.g. from NVMe 1.2, section 5.11:

"5.11 Identify command The Identify command returns a data buffer that
describes information about the NVM subsystem, the controller or the namespace(s). The data structure is 4096 bytes in size."

Matthew Wilcox

2016-03-10 14:30 UTC #4 lore raw

PATCH 1/1 nvme: add a constant for the identify payload size

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Matthew Wilcox
Quoted 3 lines · Christoph Hellwig
The NVMe spec specifies a hardcoded payload length of 4k for all types of Identify commands. Use a constant instead of hardcoded values that can be confused for the page size.
I think the problem here is that we don't have a struct for nvme_ns_list.
I think this structure is useful, but I'd still like to see a constant for the identify payload - the spec clearly specifies it as a hardcoded limit for all of identify. E.g. from NVMe 1.2, section 5.11:

The spec clearly specifies a lot of things as hardcoded values, and our reaction to that is to create structs that are that size, and assert that they are that size (in case somebody mistakenly tweaks the struct). This patch makes the ns_list be treated the same way as every other structure, rather than treating the size of the data block returned from identify as being special.

Johannes Thumshirn

2016-03-10 08:27 UTC #5 lore raw

PATCH 1/1 nvme: add a constant for the identify payload size

In reply to Matthew Wilcox

Quoted 84 lines · Matthew Wilcox

Quoted 3 lines · Christoph Hellwig

The NVMe spec specifies a hardcoded payload length of 4k for all types of Identify commands. Use a constant instead of hardcoded values that can be confused for the page size.

I think the problem here is that we don't have a struct for nvme_ns_list.

Something like this:

drivers/nvme/host/core.c +7 -5

index e2d11d6..a4a7210 100644
@@ -296,14 +296,16 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 	return error;
 }
 
-static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
+static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid,
+				struct nvme_id_ns_list *ns_list)
 {
 	struct nvme_command c = { };
 
 	c.identify.opcode = nvme_admin_identify;
 	c.identify.cns = cpu_to_le32(2);
 	c.identify.nsid = cpu_to_le32(nsid);
-	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list, 0x1000);
+	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list,
+					sizeof(*ns_list));
 }
 
 int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,

@@ -1296,11 +1298,11 @@ static void nvme_validate_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 static int nvme_scan_ns_list(struct nvme_ctrl *ctrl, unsigned nn)
 {
 	struct nvme_ns *ns;
-	__le32 *ns_list;
+	struct nvme_id_ns_list *ns_list;
 	unsigned i, j, nsid, prev = 0, num_lists = DIV_ROUND_UP(nn, 1024);
 	int ret = 0;
 
-	ns_list = kzalloc(0x1000, GFP_KERNEL);
+	ns_list = kzalloc(sizeof(*ns_list), GFP_KERNEL);
 	if (!ns_list)
 		return -ENOMEM;

@@ -1310,7 +1312,7 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl, unsigned nn)
 			goto out;
 
 		for (j = 0; j < min(nn, 1024U); j++) {
-			nsid = le32_to_cpu(ns_list[j]);
+			nsid = le32_to_cpu(ns_list->nsid[j]);
 			if (!nsid)
 				goto out;

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

index aff71ef..ec2198c 100644
@@ -175,6 +175,7 @@ static inline void _nvme_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl) != 4096);
 	BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
+	BUILD_BUG_ON(sizeof(struct nvme_id_ns_list) != 4096);
 	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
 }

include/linux/nvme.h +4 -0

index a55986f..e9122cf 100644
@@ -183,6 +183,10 @@ struct nvme_id_ns {
 	__u8			vs[3712];
 };
 
+struct nvme_id_ns_list {
+	__le32			nsid[1024];
+};
+
 enum {
 	NVME_NS_FEAT_THIN	= 1 << 0,
 	NVME_NS_FLBAS_LBA_MASK	= 0xf,

Quoted 4 lines

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

IMHO Christoph's version is more readable.

Johannes

Signature · 6 lines

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

Johannes Thumshirn

2016-03-10 08:26 UTC #6 lore raw

PATCH 1/1 nvme: add a constant for the identify payload size

In reply to Christoph Hellwig

Quoted 5 lines · Christoph Hellwig

The NVMe spec specifies a hardcoded payload length of 4k for all types of Identify commands. Use a constant instead of hardcoded values that can be confused for the page size.
Signed-off-by Christoph Hellwig <hch at lst.de>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines

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