[PATCH] nvme: add a constant for the identify payload size

[PATCH] nvme: add a constant for the identify payload size

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4304be0..64d8ede 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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;
 
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index d4f81f0..da236f0 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -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);
 }
 
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e9f18e1..1d1c6d1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
 }
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index a55986f..b0909ad 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -153,6 +153,8 @@ struct nvme_lbaf {
 	__u8			rp;
 };
 
+#define NVME_IDENTIFY_DATA_SIZE	0x1000
+
 struct nvme_id_ns {
 	__le64			nsze;
 	__le64			ncap;
-- 
2.1.4

[PATCH] nvme: add a constant for the identify payload size

[Sagi Grimberg]

Looks good,

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

[PATCH] nvme: add a constant for the identify payload size

[Matthew Wilcox]

On Wed, Mar 09, 2016@05:54:37PM +0100, Christoph Hellwig wrote:
> 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:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e2d11d6..a4a7210 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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;
 
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index aff71ef..ec2198c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
 }
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index a55986f..e9122cf 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -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,

[PATCH] nvme: add a constant for the identify payload size

[Christoph Hellwig]

On Wed, Mar 09, 2016@02:07:31PM -0500, Matthew Wilcox wrote:
> On Wed, Mar 09, 2016@05:54:37PM +0100, Christoph Hellwig wrote:
> > 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."

[PATCH] nvme: add a constant for the identify payload size

[Matthew Wilcox]

On Thu, Mar 10, 2016@09:06:28AM +0100, Christoph Hellwig wrote:
> On Wed, Mar 09, 2016@02:07:31PM -0500, Matthew Wilcox wrote:
> > On Wed, Mar 09, 2016@05:54:37PM +0100, Christoph Hellwig wrote:
> > > 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.

[PATCH] nvme: add a constant for the identify payload size

[Johannes Thumshirn]

On Wed, Mar 09, 2016@02:07:31PM -0500, Matthew Wilcox wrote:
> On Wed, Mar 09, 2016@05:54:37PM +0100, Christoph Hellwig wrote:
> > 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:
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e2d11d6..a4a7210 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -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;
>  
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index aff71ef..ec2198c 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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);
>  }
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index a55986f..e9122cf 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -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,
> 
> 
> _______________________________________________
> 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

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

[PATCH] nvme: add a constant for the identify payload size

[Johannes Thumshirn]

On Wed, Mar 09, 2016@05:54:37PM +0100, Christoph Hellwig wrote:
> 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>

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