[PATCH 0/2] nfit, libnvdimm: ars fixups for acpi6.1 compatibility

[PATCH 0/2] nfit, libnvdimm: ars fixups for acpi6.1 compatibility

[Dan Williams]

The address range scrub format is formalized in acpi6.1 and the existing
Linux implementation needs to be updated accordingly.  In the course of
testing this update a bug in how Linux handles the buffer size for
ars_status commands was discovered/fixed.

---

Dan Williams (2):
      libnvdimm, tools/testing/nvdimm: fix 'ars_status' output buffer sizing
      nfit: update address range scrub commands to the acpi 6.1 format


 drivers/acpi/nfit.c              |    6 +++---
 drivers/nvdimm/bus.c             |   20 ++++++++++----------
 include/linux/libnvdimm.h        |    3 +--
 include/uapi/linux/ndctl.h       |   11 +++++++++--
 tools/testing/nvdimm/test/nfit.c |    8 ++++++--
 5 files changed, 29 insertions(+), 19 deletions(-)

[PATCH 1/2] libnvdimm, tools/testing/nvdimm: fix 'ars_status' output buffer sizing

[Dan Williams]

Use the output length specified in the command to size the receive
buffer rather than the arbitrary 4K limit.

This bug was hiding the fact that the ndctl implementation of
ndctl_bus_cmd_new_ars_status() was not specifying an output buffer size.

Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/bus.c             |    8 ++++----
 include/linux/libnvdimm.h        |    1 -
 tools/testing/nvdimm/test/nfit.c |    8 ++++++--
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f701bc..99953b34fa1d 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -392,8 +392,8 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
 		.out_sizes = { 4, },
 	},
 	[ND_CMD_ARS_STATUS] = {
-		.out_num = 2,
-		.out_sizes = { 4, UINT_MAX, },
+		.out_num = 3,
+		.out_sizes = { 4, 4, UINT_MAX, },
 	},
 };
 
@@ -442,8 +442,8 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd,
 		return in_field[1];
 	else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2)
 		return out_field[1];
-	else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 1)
-		return ND_CMD_ARS_STATUS_MAX;
+	else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 2)
+		return out_field[1] - 8;
 
 	return UINT_MAX;
 }
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index bed40dff0e86..c736382fd260 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -28,7 +28,6 @@ enum {
 	ND_IOCTL_MAX_BUFLEN = SZ_4M,
 	ND_CMD_MAX_ELEM = 4,
 	ND_CMD_MAX_ENVELOPE = 16,
-	ND_CMD_ARS_STATUS_MAX = SZ_4K,
 	ND_MAX_MAPPINGS = 32,
 
 	/* region flag indicating to direct-map persistent memory by default */
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 90bd2ea41032..b3281dcd4a5d 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -217,13 +217,16 @@ static int nfit_test_cmd_set_config_data(struct nd_cmd_set_config_hdr *nd_cmd,
 	return rc;
 }
 
+#define NFIT_TEST_ARS_RECORDS 4
+
 static int nfit_test_cmd_ars_cap(struct nd_cmd_ars_cap *nd_cmd,
 		unsigned int buf_len)
 {
 	if (buf_len < sizeof(*nd_cmd))
 		return -EINVAL;
 
-	nd_cmd->max_ars_out = 256;
+	nd_cmd->max_ars_out = sizeof(struct nd_cmd_ars_status)
+		+ NFIT_TEST_ARS_RECORDS * sizeof(struct nd_ars_record);
 	nd_cmd->status = (ND_ARS_PERSISTENT | ND_ARS_VOLATILE) << 16;
 
 	return 0;
@@ -246,7 +249,8 @@ static int nfit_test_cmd_ars_status(struct nd_cmd_ars_status *nd_cmd,
 	if (buf_len < sizeof(*nd_cmd))
 		return -EINVAL;
 
-	nd_cmd->out_length = 256;
+	nd_cmd->out_length = sizeof(struct nd_cmd_ars_status);
+	/* TODO: emit error records */
 	nd_cmd->num_records = 0;
 	nd_cmd->address = 0;
 	nd_cmd->length = -1ULL;

[PATCH v2] libnvdimm, tools/testing/nvdimm: fix 'ars_status' output buffer sizing

[Dan Williams]

Use the output length specified in the command to size the receive
buffer rather than the arbitrary 4K limit.

This bug was hiding the fact that the ndctl implementation of
ndctl_bus_cmd_new_ars_status() was not specifying an output buffer size.

Cc: <stable@vger.kernel.org>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

Changes in v2:
Fixed ars_get_status() to pass the correct size of the output buffer.
It was only allowing enough space for the zero-records case.

 drivers/acpi/nfit.c              |   13 +++++++------
 drivers/nvdimm/bus.c             |    8 ++++----
 include/linux/libnvdimm.h        |    1 -
 tools/testing/nvdimm/test/nfit.c |    8 ++++++--
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 424b362e8fdc..1d4b9c6bdf45 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1516,13 +1516,13 @@ static int ars_do_start(struct nvdimm_bus_descriptor *nd_desc,
 }
 
 static int ars_get_status(struct nvdimm_bus_descriptor *nd_desc,
-		struct nd_cmd_ars_status *cmd)
+		struct nd_cmd_ars_status *cmd, u32 size)
 {
 	int rc;
 
 	while (1) {
 		rc = nd_desc->ndctl(nd_desc, NULL, ND_CMD_ARS_STATUS, cmd,
-			sizeof(*cmd));
+			size);
 		if (rc || cmd->status & 0xffff)
 			return -ENXIO;
 
@@ -1580,6 +1580,7 @@ static int acpi_nfit_find_poison(struct acpi_nfit_desc *acpi_desc,
 	struct nd_cmd_ars_start *ars_start = NULL;
 	struct nd_cmd_ars_cap *ars_cap = NULL;
 	u64 start, len, cur, remaining;
+	u32 ars_status_size;
 	int rc;
 
 	ars_cap = kzalloc(sizeof(*ars_cap), GFP_KERNEL);
@@ -1609,14 +1610,14 @@ static int acpi_nfit_find_poison(struct acpi_nfit_desc *acpi_desc,
 	 * Check if a full-range ARS has been run. If so, use those results
 	 * without having to start a new ARS.
 	 */
-	ars_status = kzalloc(ars_cap->max_ars_out + sizeof(*ars_status),
-			GFP_KERNEL);
+	ars_status_size = ars_cap->max_ars_out;
+	ars_status = kzalloc(ars_status_size, GFP_KERNEL);
 	if (!ars_status) {
 		rc = -ENOMEM;
 		goto out;
 	}
 
-	rc = ars_get_status(nd_desc, ars_status);
+	rc = ars_get_status(nd_desc, ars_status, ars_status_size);
 	if (rc)
 		goto out;
 
@@ -1646,7 +1647,7 @@ static int acpi_nfit_find_poison(struct acpi_nfit_desc *acpi_desc,
 		if (rc)
 			goto out;
 
-		rc = ars_get_status(nd_desc, ars_status);
+		rc = ars_get_status(nd_desc, ars_status, ars_status_size);
 		if (rc)
 			goto out;
 
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f701bc..99953b34fa1d 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -392,8 +392,8 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
 		.out_sizes = { 4, },
 	},
 	[ND_CMD_ARS_STATUS] = {
-		.out_num = 2,
-		.out_sizes = { 4, UINT_MAX, },
+		.out_num = 3,
+		.out_sizes = { 4, 4, UINT_MAX, },
 	},
 };
 
@@ -442,8 +442,8 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd,
 		return in_field[1];
 	else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2)
 		return out_field[1];
-	else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 1)
-		return ND_CMD_ARS_STATUS_MAX;
+	else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 2)
+		return out_field[1] - 8;
 
 	return UINT_MAX;
 }
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index bed40dff0e86..c736382fd260 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -28,7 +28,6 @@ enum {
 	ND_IOCTL_MAX_BUFLEN = SZ_4M,
 	ND_CMD_MAX_ELEM = 4,
 	ND_CMD_MAX_ENVELOPE = 16,
-	ND_CMD_ARS_STATUS_MAX = SZ_4K,
 	ND_MAX_MAPPINGS = 32,
 
 	/* region flag indicating to direct-map persistent memory by default */
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 90bd2ea41032..b3281dcd4a5d 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -217,13 +217,16 @@ static int nfit_test_cmd_set_config_data(struct nd_cmd_set_config_hdr *nd_cmd,
 	return rc;
 }
 
+#define NFIT_TEST_ARS_RECORDS 4
+
 static int nfit_test_cmd_ars_cap(struct nd_cmd_ars_cap *nd_cmd,
 		unsigned int buf_len)
 {
 	if (buf_len < sizeof(*nd_cmd))
 		return -EINVAL;
 
-	nd_cmd->max_ars_out = 256;
+	nd_cmd->max_ars_out = sizeof(struct nd_cmd_ars_status)
+		+ NFIT_TEST_ARS_RECORDS * sizeof(struct nd_ars_record);
 	nd_cmd->status = (ND_ARS_PERSISTENT | ND_ARS_VOLATILE) << 16;
 
 	return 0;
@@ -246,7 +249,8 @@ static int nfit_test_cmd_ars_status(struct nd_cmd_ars_status *nd_cmd,
 	if (buf_len < sizeof(*nd_cmd))
 		return -EINVAL;
 
-	nd_cmd->out_length = 256;
+	nd_cmd->out_length = sizeof(struct nd_cmd_ars_status);
+	/* TODO: emit error records */
 	nd_cmd->num_records = 0;
 	nd_cmd->address = 0;
 	nd_cmd->length = -1ULL;

[PATCH 2/2] nfit: update address range scrub commands to the acpi 6.1 format

[Dan Williams]

The original format of these commands from the "NVDIMM DSM Interface
Example" [1] are superseded by the ACPI 6.1 definition of the "NVDIMM Root
Device _DSMs" [2].

[1]: http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
[2]: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
     "9.20.7 NVDIMM Root Device _DSMs"

Changes include:
1/ New 'restart' fields in ars_status, unfortunately these are
   implemented in the middle of the existing definition so this change
   is not backwards compatible.  The expectation is that shipping
   platforms will only ever support the ACPI 6.1 definition.

2/ New status values for ars_start ('busy') and ars_status ('overflow').

Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Linda Knippers <linda.knippers@hpe.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit.c        |    6 +++---
 drivers/nvdimm/bus.c       |   12 ++++++------
 include/linux/libnvdimm.h  |    2 +-
 include/uapi/linux/ndctl.h |   11 +++++++++--
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 424b362e8fdc..3edb1324b9d5 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1503,9 +1503,7 @@ static int ars_do_start(struct nvdimm_bus_descriptor *nd_desc,
 		case 1:
 			/* ARS unsupported, but we should never get here */
 			return 0;
-		case 2:
-			return -EINVAL;
-		case 3:
+		case 6:
 			/* ARS is in progress */
 			msleep(1000);
 			break;
@@ -1537,6 +1535,8 @@ static int ars_get_status(struct nvdimm_bus_descriptor *nd_desc,
 		case 2:
 			/* No ARS performed for the current boot */
 			return 0;
+		case 3:
+			/* TODO: error list overflow support */
 		default:
 			return -ENXIO;
 		}
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 99953b34fa1d..5d28e9405f32 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -382,14 +382,14 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
 	[ND_CMD_ARS_CAP] = {
 		.in_num = 2,
 		.in_sizes = { 8, 8, },
-		.out_num = 2,
-		.out_sizes = { 4, 4, },
+		.out_num = 4,
+		.out_sizes = { 4, 4, 4, 4, },
 	},
 	[ND_CMD_ARS_START] = {
-		.in_num = 4,
-		.in_sizes = { 8, 8, 2, 6, },
-		.out_num = 1,
-		.out_sizes = { 4, },
+		.in_num = 5,
+		.in_sizes = { 8, 8, 2, 1, 5, },
+		.out_num = 2,
+		.out_sizes = { 4, 4, },
 	},
 	[ND_CMD_ARS_STATUS] = {
 		.out_num = 3,
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index c736382fd260..141ffdd59960 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -26,7 +26,7 @@ enum {
 
 	/* need to set a limit somewhere, but yes, this is likely overkill */
 	ND_IOCTL_MAX_BUFLEN = SZ_4M,
-	ND_CMD_MAX_ELEM = 4,
+	ND_CMD_MAX_ELEM = 5,
 	ND_CMD_MAX_ENVELOPE = 16,
 	ND_MAX_MAPPINGS = 32,
 
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 5b4a4be06e2b..cc68b92124d4 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -66,14 +66,18 @@ struct nd_cmd_ars_cap {
 	__u64 length;
 	__u32 status;
 	__u32 max_ars_out;
+	__u32 clear_err_unit;
+	__u32 reserved;
 } __packed;
 
 struct nd_cmd_ars_start {
 	__u64 address;
 	__u64 length;
 	__u16 type;
-	__u8 reserved[6];
+	__u8 flags;
+	__u8 reserved[5];
 	__u32 status;
+	__u32 scrub_time;
 } __packed;
 
 struct nd_cmd_ars_status {
@@ -81,11 +85,14 @@ struct nd_cmd_ars_status {
 	__u32 out_length;
 	__u64 address;
 	__u64 length;
+	__u64 restart_address;
+	__u64 restart_length;
 	__u16 type;
+	__u16 flags;
 	__u32 num_records;
 	struct nd_ars_record {
 		__u32 handle;
-		__u32 flags;
+		__u32 reserved;
 		__u64 err_address;
 		__u64 length;
 	} __packed records[0];

Re: [PATCH 2/2] nfit: update address range scrub commands to the acpi 6.1 format

[Linda Knippers]

On 2/20/2016 5:46 PM, Dan Williams wrote:
> The original format of these commands from the "NVDIMM DSM Interface
> Example" [1] are superseded by the ACPI 6.1 definition of the "NVDIMM Root
> Device _DSMs" [2].
> 
> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
> [2]: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
>      "9.20.7 NVDIMM Root Device _DSMs"
> 
> Changes include:
> 1/ New 'restart' fields in ars_status, unfortunately these are
>    implemented in the middle of the existing definition so this change
>    is not backwards compatible.  The expectation is that shipping
>    platforms will only ever support the ACPI 6.1 definition.

I agree with that.

> 
> 2/ New status values for ars_start ('busy') and ars_status ('overflow').
> 
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Linda Knippers <linda.knippers@hpe.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/acpi/nfit.c        |    6 +++---
>  drivers/nvdimm/bus.c       |   12 ++++++------
>  include/linux/libnvdimm.h  |    2 +-
>  include/uapi/linux/ndctl.h |   11 +++++++++--
>  4 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index 424b362e8fdc..3edb1324b9d5 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -1503,9 +1503,7 @@ static int ars_do_start(struct nvdimm_bus_descriptor *nd_desc,
>  		case 1:
>  			/* ARS unsupported, but we should never get here */
>  			return 0;
> -		case 2:
> -			return -EINVAL;
> -		case 3:
> +		case 6:
>  			/* ARS is in progress */
>  			msleep(1000);
>  			break;
> @@ -1537,6 +1535,8 @@ static int ars_get_status(struct nvdimm_bus_descriptor *nd_desc,
>  		case 2:
>  			/* No ARS performed for the current boot */
>  			return 0;
> +		case 3:
> +			/* TODO: error list overflow support */
>  		default:
>  			return -ENXIO;
>  		}
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 99953b34fa1d..5d28e9405f32 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -382,14 +382,14 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
>  	[ND_CMD_ARS_CAP] = {
>  		.in_num = 2,
>  		.in_sizes = { 8, 8, },
> -		.out_num = 2,
> -		.out_sizes = { 4, 4, },
> +		.out_num = 4,
> +		.out_sizes = { 4, 4, 4, 4, },

The status was 4 bytes but now it's 2 bytes of status and 2 bytes of extended
status.  Where things are didn't actually change but should the two status
fields be defined separately to match the spec?  It would save some shifting and
anding.  Maybe a nit...

>  	},
>  	[ND_CMD_ARS_START] = {
> -		.in_num = 4,
> -		.in_sizes = { 8, 8, 2, 6, },
> -		.out_num = 1,
> -		.out_sizes = { 4, },
> +		.in_num = 5,
> +		.in_sizes = { 8, 8, 2, 1, 5, },
> +		.out_num = 2,
> +		.out_sizes = { 4, 4, },
>  	},
>  	[ND_CMD_ARS_STATUS] = {
>  		.out_num = 3,
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index c736382fd260..141ffdd59960 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -26,7 +26,7 @@ enum {
>  
>  	/* need to set a limit somewhere, but yes, this is likely overkill */
>  	ND_IOCTL_MAX_BUFLEN = SZ_4M,
> -	ND_CMD_MAX_ELEM = 4,
> +	ND_CMD_MAX_ELEM = 5,
>  	ND_CMD_MAX_ENVELOPE = 16,
>  	ND_MAX_MAPPINGS = 32,
>  
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index 5b4a4be06e2b..cc68b92124d4 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -66,14 +66,18 @@ struct nd_cmd_ars_cap {
>  	__u64 length;
>  	__u32 status;
>  	__u32 max_ars_out;
> +	__u32 clear_err_unit;
> +	__u32 reserved;
>  } __packed;
>  
>  struct nd_cmd_ars_start {
>  	__u64 address;
>  	__u64 length;
>  	__u16 type;
> -	__u8 reserved[6];
> +	__u8 flags;
> +	__u8 reserved[5];
>  	__u32 status;
> +	__u32 scrub_time;
>  } __packed;
>  
>  struct nd_cmd_ars_status {
> @@ -81,11 +85,14 @@ struct nd_cmd_ars_status {
>  	__u32 out_length;
>  	__u64 address;
>  	__u64 length;
> +	__u64 restart_address;
> +	__u64 restart_length;
>  	__u16 type;
> +	__u16 flags;
>  	__u32 num_records;
>  	struct nd_ars_record {
>  		__u32 handle;
> -		__u32 flags;
> +		__u32 reserved;
>  		__u64 err_address;
>  		__u64 length;
>  	} __packed records[0];
> 

Re: [PATCH 2/2] nfit: update address range scrub commands to the acpi 6.1 format

[Dan Williams]

On Mon, Feb 22, 2016 at 2:22 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>
>
> On 2/20/2016 5:46 PM, Dan Williams wrote:
>> The original format of these commands from the "NVDIMM DSM Interface
>> Example" [1] are superseded by the ACPI 6.1 definition of the "NVDIMM Root
>> Device _DSMs" [2].
>>
>> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
>> [2]: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
>>      "9.20.7 NVDIMM Root Device _DSMs"
>>
>> Changes include:
>> 1/ New 'restart' fields in ars_status, unfortunately these are
>>    implemented in the middle of the existing definition so this change
>>    is not backwards compatible.  The expectation is that shipping
>>    platforms will only ever support the ACPI 6.1 definition.
>
> I agree with that.
>
>>
>> 2/ New status values for ars_start ('busy') and ars_status ('overflow').
>>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> Cc: Linda Knippers <linda.knippers@hpe.com>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
[..]
>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>> index 99953b34fa1d..5d28e9405f32 100644
>> --- a/drivers/nvdimm/bus.c
>> +++ b/drivers/nvdimm/bus.c
>> @@ -382,14 +382,14 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
>>       [ND_CMD_ARS_CAP] = {
>>               .in_num = 2,
>>               .in_sizes = { 8, 8, },
>> -             .out_num = 2,
>> -             .out_sizes = { 4, 4, },
>> +             .out_num = 4,
>> +             .out_sizes = { 4, 4, 4, 4, },
>
> The status was 4 bytes but now it's 2 bytes of status and 2 bytes of extended
> status.  Where things are didn't actually change but should the two status
> fields be defined separately to match the spec?  It would save some shifting and
> anding.  Maybe a nit...

For this patch, since I'm tagging it for -stable and ndctl.h is
exported to userspace, I don't want "status" to have a different
meaning depending on which version of the kernel header an application
happened to be compiled against.  I think we're stuck with the unified
u32.

Re: [PATCH 2/2] nfit: update address range scrub commands to the acpi 6.1 format

[Linda Knippers]

On 2/22/2016 5:41 PM, Dan Williams wrote:
> On Mon, Feb 22, 2016 at 2:22 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>
>>
>> On 2/20/2016 5:46 PM, Dan Williams wrote:
>>> The original format of these commands from the "NVDIMM DSM Interface
>>> Example" [1] are superseded by the ACPI 6.1 definition of the "NVDIMM Root
>>> Device _DSMs" [2].
>>>
>>> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
>>> [2]: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
>>>      "9.20.7 NVDIMM Root Device _DSMs"
>>>
>>> Changes include:
>>> 1/ New 'restart' fields in ars_status, unfortunately these are
>>>    implemented in the middle of the existing definition so this change
>>>    is not backwards compatible.  The expectation is that shipping
>>>    platforms will only ever support the ACPI 6.1 definition.
>>
>> I agree with that.
>>
>>>
>>> 2/ New status values for ars_start ('busy') and ars_status ('overflow').
>>>
>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>> Cc: Linda Knippers <linda.knippers@hpe.com>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>> ---
> [..]
>>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>>> index 99953b34fa1d..5d28e9405f32 100644
>>> --- a/drivers/nvdimm/bus.c
>>> +++ b/drivers/nvdimm/bus.c
>>> @@ -382,14 +382,14 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
>>>       [ND_CMD_ARS_CAP] = {
>>>               .in_num = 2,
>>>               .in_sizes = { 8, 8, },
>>> -             .out_num = 2,
>>> -             .out_sizes = { 4, 4, },
>>> +             .out_num = 4,
>>> +             .out_sizes = { 4, 4, 4, 4, },
>>
>> The status was 4 bytes but now it's 2 bytes of status and 2 bytes of extended
>> status.  Where things are didn't actually change but should the two status
>> fields be defined separately to match the spec?  It would save some shifting and
>> anding.  Maybe a nit...
> 
> For this patch, since I'm tagging it for -stable and ndctl.h is
> exported to userspace, I don't want "status" to have a different
> meaning depending on which version of the kernel header an application
> happened to be compiled against.  I think we're stuck with the unified
> u32.

I was originally thinking that this would be a good time to change since
some of the data formats were changing too but I've reconsidered based
on some other draft/example DSMs I've seen.

The one thing that's consistent is that there are 4 bytes of status.
Sometimes the bytes are split into sub-status fields but not necessarily
into 2 2-byte chunks.  It's more flexible the way it is.

-- ljk