[PATCH v2 0/3] ACPI 6.1 update for NFIT Control Region Structure

[PATCH v2 0/3] ACPI 6.1 update for NFIT Control Region Structure

[Toshi Kani]

ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure
as follows.
 - Valid Fields, Manufacturing Location, and Manufacturing Date
   are added from reserved range.  No change in the structure size.
 - IDs defined as SPD values are arrays of bytes.  The spec 
   clarified that they need to be represented as arrays of bytes
   as well.

Patch 1 changes 'struct acpi_nfit_control_region' and the NFIT driver to
comply ACPI 6.1.
Patch 2 adds a new sysfs file "id" to show NVDIMM ID defined in ACPI 6.1.
Patch 3 changes the nfit test driver.

link: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf

---
v2:
 - Remove 'mfg_location' and 'mfg_date'. (Dan Williams)
 - Rename 'unique_id' to 'id' and make this change as a separate patch.
   (Dan Williams)

---
Toshi Kani (3):
 1/3 ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1
 2/3 ACPI/NFIT: Add NVDIMM ID "id" under sysfs
 3/3 nfit_test: Update SPD ID init handlings

---
 drivers/acpi/nfit.c              | 41 ++++++++++++++++++++-----
 include/acpi/actbl1.h            | 24 +++++++++------
 tools/testing/nvdimm/test/nfit.c | 64 ++++++++++++++++++++++++----------------
 3 files changed, 88 insertions(+), 41 deletions(-)

[PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Toshi Kani]

ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure
as follows.
 - Valid Fields, Manufacturing Location, and Manufacturing Date
   are added from reserved range.  No change in the structure size.
 - IDs defined as SPD values are arrays of bytes.  The spec
   clarified that they need to be represented as arrays of bytes
   as well.

This patch makes the following changes to support this update.
 - Change 'struct acpi_nfit_control_region' to reflect the update.
   SPD IDs are defined as arrays of bytes, so that they can be
   treated in the same way regardless of CPU endianness and are
   not miss-treated as little-endian numeric values.
 - Change the NFIT driver to show SPD ID values as array of bytes
   in sysfs.
 - Change sprintf format to use "0x" instead of "#" since "%#02x"
   does not prepend '0' in some reason.

link: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Robert Moore <robert.moore@intel.com>
Cc: Lv Zheng <lv.zheng@intel.com>
Cc: Robert Elliott <elliott@hpe.com>
Cc: <devel@acpica.org>
---
 drivers/acpi/nfit.c   |   20 +++++++++++++-------
 include/acpi/actbl1.h |   24 +++++++++++++++---------
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index ad6d8c6..4982a18 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -722,7 +722,8 @@ static ssize_t vendor_show(struct device *dev,
 {
 	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
 
-	return sprintf(buf, "%#x\n", dcr->vendor_id);
+	return sprintf(buf, "0x%02x%02x\n",
+				dcr->vendor_id[0], dcr->vendor_id[1]);
 }
 static DEVICE_ATTR_RO(vendor);
 
@@ -731,7 +732,8 @@ static ssize_t rev_id_show(struct device *dev,
 {
 	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
 
-	return sprintf(buf, "%#x\n", dcr->revision_id);
+	return sprintf(buf, "0x%02x%02x\n",
+				dcr->revision_id[0], dcr->revision_id[1]);
 }
 static DEVICE_ATTR_RO(rev_id);
 
@@ -740,7 +742,8 @@ static ssize_t device_show(struct device *dev,
 {
 	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
 
-	return sprintf(buf, "%#x\n", dcr->device_id);
+	return sprintf(buf, "0x%02x%02x\n",
+				dcr->device_id[0], dcr->device_id[1]);
 }
 static DEVICE_ATTR_RO(device);
 
@@ -749,7 +752,7 @@ static ssize_t format_show(struct device *dev,
 {
 	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
 
-	return sprintf(buf, "%#x\n", dcr->code);
+	return sprintf(buf, "0x%02x%02x\n", dcr->code[0], dcr->code[1]);
 }
 static DEVICE_ATTR_RO(format);
 
@@ -758,7 +761,9 @@ static ssize_t serial_show(struct device *dev,
 {
 	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
 
-	return sprintf(buf, "%#x\n", dcr->serial_number);
+	return sprintf(buf, "0x%02x%02x%02x%02x\n",
+			dcr->serial_number[0], dcr->serial_number[1],
+			dcr->serial_number[2], dcr->serial_number[3]);
 }
 static DEVICE_ATTR_RO(serial);
 
@@ -956,7 +961,7 @@ static const struct attribute_group *acpi_nfit_region_attribute_groups[] = {
 struct nfit_set_info {
 	struct nfit_set_info_map {
 		u64 region_offset;
-		u32 serial_number;
+		u8 serial_number[4];
 		u32 pad;
 	} mapping[0];
 };
@@ -1025,7 +1030,8 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc,
 		}
 
 		map->region_offset = memdev->region_offset;
-		map->serial_number = nfit_mem->dcr->serial_number;
+		memcpy(map->serial_number, nfit_mem->dcr->serial_number,
+				sizeof(map->serial_number));
 	}
 
 	sort(&info->mapping[0], nr, sizeof(struct nfit_set_info_map),
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 16e0136..d8df62c 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -1040,15 +1040,18 @@ struct acpi_nfit_smbios {
 struct acpi_nfit_control_region {
 	struct acpi_nfit_header header;
 	u16 region_index;
-	u16 vendor_id;
-	u16 device_id;
-	u16 revision_id;
-	u16 subsystem_vendor_id;
-	u16 subsystem_device_id;
-	u16 subsystem_revision_id;
-	u8 reserved[6];		/* Reserved, must be zero */
-	u32 serial_number;
-	u16 code;
+	u8 vendor_id[2];
+	u8 device_id[2];
+	u8 revision_id[2];
+	u8 subsystem_vendor_id[2];
+	u8 subsystem_device_id[2];
+	u8 subsystem_revision_id[2];
+	u8 valid_fields;
+	u8 manufacturing_location;
+	u8 manufacturing_date[2];
+	u8 reserved[2];		/* Reserved, must be zero */
+	u8 serial_number[4];
+	u8 code[2];
 	u16 windows;
 	u64 window_size;
 	u64 command_offset;
@@ -1059,6 +1062,9 @@ struct acpi_nfit_control_region {
 	u8 reserved1[6];	/* Reserved, must be zero */
 };
 
+/* Valid Fields */
+#define ACPI_NFIT_CONTROL_MFG_INFO_VALID (1)	/* Manufacturing fields are valid */
+
 /* Flags */
 
 #define ACPI_NFIT_CONTROL_BUFFERED      (1)	/* Block Data Windows implementation is buffered */

RE: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Moore, Robert]

> -----Original Message-----
> From: Toshi Kani [mailto:toshi.kani@hpe.com]
> Sent: Monday, February 22, 2016 1:55 PM
> To: rjw@rjwysocki.net; Williams, Dan J
> Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org;
> linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
> devel@acpica.org; Toshi Kani
> Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to
> comply ACPI 6.1
> 
> ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as follows.
>  - Valid Fields, Manufacturing Location, and Manufacturing Date
>    are added from reserved range.  No change in the structure size.
>  - IDs defined as SPD values are arrays of bytes.  The spec
>    clarified that they need to be represented as arrays of bytes
>    as well.
> 
> This patch makes the following changes to support this update.
>  - Change 'struct acpi_nfit_control_region' to reflect the update.
>    SPD IDs are defined as arrays of bytes, so that they can be
>    treated in the same way regardless of CPU endianness and are
>    not miss-treated as little-endian numeric values.


I don't think we are going to start changing the ACPI tables defined in the ACPICA headers because of this. We do in fact have macros for this purpose.



>  - Change the NFIT driver to show SPD ID values as array of bytes
>    in sysfs.
>  - Change sprintf format to use "0x" instead of "#" since "%#02x"
>    does not prepend '0' in some reason.
> 
> link: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Robert Moore <robert.moore@intel.com>
> Cc: Lv Zheng <lv.zheng@intel.com>
> Cc: Robert Elliott <elliott@hpe.com>
> Cc: <devel@acpica.org>
> ---
>  drivers/acpi/nfit.c   |   20 +++++++++++++-------
>  include/acpi/actbl1.h |   24 +++++++++++++++---------
>  2 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index
> ad6d8c6..4982a18 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -722,7 +722,8 @@ static ssize_t vendor_show(struct device *dev,  {
>  	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
> 
> -	return sprintf(buf, "%#x\n", dcr->vendor_id);
> +	return sprintf(buf, "0x%02x%02x\n",
> +				dcr->vendor_id[0], dcr->vendor_id[1]);
>  }
>  static DEVICE_ATTR_RO(vendor);
> 
> @@ -731,7 +732,8 @@ static ssize_t rev_id_show(struct device *dev,  {
>  	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
> 
> -	return sprintf(buf, "%#x\n", dcr->revision_id);
> +	return sprintf(buf, "0x%02x%02x\n",
> +				dcr->revision_id[0], dcr->revision_id[1]);
>  }
>  static DEVICE_ATTR_RO(rev_id);
> 
> @@ -740,7 +742,8 @@ static ssize_t device_show(struct device *dev,  {
>  	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
> 
> -	return sprintf(buf, "%#x\n", dcr->device_id);
> +	return sprintf(buf, "0x%02x%02x\n",
> +				dcr->device_id[0], dcr->device_id[1]);
>  }
>  static DEVICE_ATTR_RO(device);
> 
> @@ -749,7 +752,7 @@ static ssize_t format_show(struct device *dev,  {
>  	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
> 
> -	return sprintf(buf, "%#x\n", dcr->code);
> +	return sprintf(buf, "0x%02x%02x\n", dcr->code[0], dcr->code[1]);
>  }
>  static DEVICE_ATTR_RO(format);
> 
> @@ -758,7 +761,9 @@ static ssize_t serial_show(struct device *dev,  {
>  	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
> 
> -	return sprintf(buf, "%#x\n", dcr->serial_number);
> +	return sprintf(buf, "0x%02x%02x%02x%02x\n",
> +			dcr->serial_number[0], dcr->serial_number[1],
> +			dcr->serial_number[2], dcr->serial_number[3]);
>  }
>  static DEVICE_ATTR_RO(serial);
> 
> @@ -956,7 +961,7 @@ static const struct attribute_group
> *acpi_nfit_region_attribute_groups[] = {  struct nfit_set_info {
>  	struct nfit_set_info_map {
>  		u64 region_offset;
> -		u32 serial_number;
> +		u8 serial_number[4];
>  		u32 pad;
>  	} mapping[0];
>  };
> @@ -1025,7 +1030,8 @@ static int acpi_nfit_init_interleave_set(struct
> acpi_nfit_desc *acpi_desc,
>  		}
> 
>  		map->region_offset = memdev->region_offset;
> -		map->serial_number = nfit_mem->dcr->serial_number;
> +		memcpy(map->serial_number, nfit_mem->dcr->serial_number,
> +				sizeof(map->serial_number));
>  	}
> 
>  	sort(&info->mapping[0], nr, sizeof(struct nfit_set_info_map), diff -
> -git a/include/acpi/actbl1.h b/include/acpi/actbl1.h index
> 16e0136..d8df62c 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -1040,15 +1040,18 @@ struct acpi_nfit_smbios {  struct
> acpi_nfit_control_region {
>  	struct acpi_nfit_header header;
>  	u16 region_index;
> -	u16 vendor_id;
> -	u16 device_id;
> -	u16 revision_id;
> -	u16 subsystem_vendor_id;
> -	u16 subsystem_device_id;
> -	u16 subsystem_revision_id;
> -	u8 reserved[6];		/* Reserved, must be zero */
> -	u32 serial_number;
> -	u16 code;
> +	u8 vendor_id[2];
> +	u8 device_id[2];
> +	u8 revision_id[2];
> +	u8 subsystem_vendor_id[2];
> +	u8 subsystem_device_id[2];
> +	u8 subsystem_revision_id[2];
> +	u8 valid_fields;
> +	u8 manufacturing_location;
> +	u8 manufacturing_date[2];
> +	u8 reserved[2];		/* Reserved, must be zero */
> +	u8 serial_number[4];
> +	u8 code[2];
>  	u16 windows;
>  	u64 window_size;
>  	u64 command_offset;
> @@ -1059,6 +1062,9 @@ struct acpi_nfit_control_region {
>  	u8 reserved1[6];	/* Reserved, must be zero */
>  };
> 
> +/* Valid Fields */
> +#define ACPI_NFIT_CONTROL_MFG_INFO_VALID (1)	/* Manufacturing fields
> are valid */
> +
>  /* Flags */
> 
>  #define ACPI_NFIT_CONTROL_BUFFERED      (1)	/* Block Data Windows
> implementation is buffered */

Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Toshi Kani]

On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote:
> 
> > -----Original Message-----
> > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > Sent: Monday, February 22, 2016 1:55 PM
> > To: rjw@rjwysocki.net; Williams, Dan J
> > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.or
> > g;
> > linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
> > devel@acpica.org; Toshi Kani
> > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to
> > comply ACPI 6.1
> > 
> > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as
> > follows.
> >  - Valid Fields, Manufacturing Location, and Manufacturing Date
> >    are added from reserved range.  No change in the structure size.
> >  - IDs defined as SPD values are arrays of bytes.  The spec
> >    clarified that they need to be represented as arrays of bytes
> >    as well.
> > 
> > This patch makes the following changes to support this update.
> >  - Change 'struct acpi_nfit_control_region' to reflect the update.
> >    SPD IDs are defined as arrays of bytes, so that they can be
> >    treated in the same way regardless of CPU endianness and are
> >    not miss-treated as little-endian numeric values.
> 
> 
> I don't think we are going to start changing the ACPI tables defined in
> the ACPICA headers because of this. We do in fact have macros for this
> purpose.

Can you elaborate what macros you suggest to use for this purpose?

Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Moore, Robert]

We have a bunch of macros in include/acmacros.h -- like this:

ACPI_MOVE_16_TO_16(d, s)


> -----Original Message-----
> From: Toshi Kani [mailto:toshi.kani@hpe.com]
> Sent: Tuesday, March 01, 2016 8:38 AM
> To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J
> Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org
> Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to
> comply ACPI 6.1
> 
> On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote:
> >
> > > -----Original Message-----
> > > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > > Sent: Monday, February 22, 2016 1:55 PM
> > > To: rjw@rjwysocki.net; Williams, Dan J
> > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com;
> > > linux-nvdimm@lists.01.or g; linux-acpi@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; devel@acpica.org; Toshi Kani
> > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure
> > > to comply ACPI 6.1
> > >
> > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as
> > > follows.
> > >  - Valid Fields, Manufacturing Location, and Manufacturing Date
> > >    are added from reserved range.  No change in the structure size.
> > >  - IDs defined as SPD values are arrays of bytes.  The spec
> > >    clarified that they need to be represented as arrays of bytes
> > >    as well.
> > >
> > > This patch makes the following changes to support this update.
> > >  - Change 'struct acpi_nfit_control_region' to reflect the update.
> > >    SPD IDs are defined as arrays of bytes, so that they can be
> > >    treated in the same way regardless of CPU endianness and are
> > >    not miss-treated as little-endian numeric values.
> >
> >
> > I don't think we are going to start changing the ACPI tables defined
> > in the ACPICA headers because of this. We do in fact have macros for
> > this purpose.
> 
> Can you elaborate what macros you suggest to use for this purpose?
> 
> Thanks,
> -Toshi

Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Toshi Kani]

On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
> We have a bunch of macros in include/acmacros.h -- like this:
> 
> ACPI_MOVE_16_TO_16(d, s)

There is a problem in using the ACPICA byte-swap macros.  ACPI is little-
endian arch, so the macros are set to perform byte-swappings when the CPU
arch is big-endian.  This case, however, is the other way around.  The
fields in question are defined & stored as arrays of bytes.  If you treat
them as multi-bytes numeric values, then you need to byte-swap them when
the CPU arch is little-endian because arrays of bytes have the same
addressing as big-endian.

Another issue is that it is not clear who needs to perform the byte-
swapping among ACPICA and drivers.  If ACPICA, drivers must agree that
these fields are always treated as multi-bytes numeric values despite of
the spec.  If drivers, we need to make sure that only a single driver
performs this byte-swapping one time as ACPI tables are global structures.

I think it is much clearer to define the structure according to the ACPI
spec.

Thanks,
-Toshi

> > -----Original Message-----
> > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > Sent: Tuesday, March 01, 2016 8:38 AM
> > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J
> > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux-
> > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org
> > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure
> > to
> > comply ACPI 6.1
> > 
> > On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > > > Sent: Monday, February 22, 2016 1:55 PM
> > > > To: rjw@rjwysocki.net; Williams, Dan J
> > > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com;
> > > > linux-nvdimm@lists.01.or g; linux-acpi@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; devel@acpica.org; Toshi Kani
> > > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure
> > > > to comply ACPI 6.1
> > > > 
> > > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as
> > > > follows.
> > > >  - Valid Fields, Manufacturing Location, and Manufacturing Date
> > > >    are added from reserved range.  No change in the structure size.
> > > >  - IDs defined as SPD values are arrays of bytes.  The spec
> > > >    clarified that they need to be represented as arrays of bytes
> > > >    as well.
> > > > 
> > > > This patch makes the following changes to support this update.
> > > >  - Change 'struct acpi_nfit_control_region' to reflect the update.
> > > >    SPD IDs are defined as arrays of bytes, so that they can be
> > > >    treated in the same way regardless of CPU endianness and are
> > > >    not miss-treated as little-endian numeric values.
> > > 
> > > 
> > > I don't think we are going to start changing the ACPI tables defined
> > > in the ACPICA headers because of this. We do in fact have macros for
> > > this purpose.
> > 
> > Can you elaborate what macros you suggest to use for this purpose?
> > 
> > Thanks,
> > -Toshi
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{�i�b�{ay�\x1dʇڙ�,j\a��f���h���z�\x1e�w���
> \f���j:+v���w�j�m����\a����zZ+�����ݢj"��!�i
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Moore, Robert]

> -----Original Message-----
> From: Toshi Kani [mailto:toshi.kani@hpe.com]
> Sent: Tuesday, March 01, 2016 9:37 AM
> To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J
> Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org
> Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to
> comply ACPI 6.1
> 
> On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
> > We have a bunch of macros in include/acmacros.h -- like this:
> >
> > ACPI_MOVE_16_TO_16(d, s)
> 
> There is a problem in using the ACPICA byte-swap macros.  ACPI is little-
> endian arch, so the macros are set to perform byte-swappings when the CPU
> arch is big-endian.  This case, however, is the other way around.  The
> fields in question are defined & stored as arrays of bytes.

That's not what I see in the ACPI spec. The fields are defined like any other ACPI table.

Vendor ID 2 6 
Identifier indicating the vendor of the NVDIMM.
This field shall be set to the value of the NVDIMM SPD Module
Manufacturer ID Code field a with byte 0 set to DDR4 SPD byte
320 and byte 1 set to DDR4 SPD byte 321.

Device ID 2 8
Identifier for the NVDIMM, assigned by the module vendor.
This field shall be set to the value of the NVDIMM SPD Module
Product Identifier field b with byte 0 set to SPD byte 192 and
byte 1 set to SPD byte 193.

Revision ID 2 10 
Revision of the NVDIMM, assigned by the module vendor.
Byte 1 of this field is reserved.
Byte 0 of this field shall be set to the value of the NVDIMM SPD
Module Revision Code field a (i.e., SPD byte 349).


Etc.


If you treat
> them as multi-bytes numeric values, then you need to byte-swap them when
> the CPU arch is little-endian because arrays of bytes have the same
> addressing as big-endian.
> 
> Another issue is that it is not clear who needs to perform the byte-
> swapping among ACPICA and drivers.  If ACPICA, drivers must agree that
> these fields are always treated as multi-bytes numeric values despite of
> the spec.  If drivers, we need to make sure that only a single driver
> performs this byte-swapping one time as ACPI tables are global structures.
> 
> I think it is much clearer to define the structure according to the ACPI
> spec.
> 
> Thanks,
> -Toshi
> 
> > > -----Original Message-----
> > > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > > Sent: Tuesday, March 01, 2016 8:38 AM
> > > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J
> > > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux-
> > > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org
> > > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region
> > > Structure to comply ACPI 6.1
> > >
> > > On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > > > > Sent: Monday, February 22, 2016 1:55 PM
> > > > > To: rjw@rjwysocki.net; Williams, Dan J
> > > > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com;
> > > > > linux-nvdimm@lists.01.or g; linux-acpi@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org; devel@acpica.org; Toshi Kani
> > > > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region
> > > > > Structure to comply ACPI 6.1
> > > > >
> > > > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure
> > > > > as follows.
> > > > >  - Valid Fields, Manufacturing Location, and Manufacturing Date
> > > > >    are added from reserved range.  No change in the structure
> size.
> > > > >  - IDs defined as SPD values are arrays of bytes.  The spec
> > > > >    clarified that they need to be represented as arrays of bytes
> > > > >    as well.
> > > > >
> > > > > This patch makes the following changes to support this update.
> > > > >  - Change 'struct acpi_nfit_control_region' to reflect the update.
> > > > >    SPD IDs are defined as arrays of bytes, so that they can be
> > > > >    treated in the same way regardless of CPU endianness and are
> > > > >    not miss-treated as little-endian numeric values.
> > > >
> > > >
> > > > I don't think we are going to start changing the ACPI tables
> > > > defined in the ACPICA headers because of this. We do in fact have
> > > > macros for this purpose.
> > >
> > > Can you elaborate what macros you suggest to use for this purpose?
> > >
> > > Thanks,
> > > -Toshi
> > N     r  y   b X  ǧv ^ )޺{.n +    { i b {ay \x1dʇڙ ,j   f   h   z \x1e w
> >
> 
>    j:+v   w j m         zZ+     ݢj"  ! i

RE: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Moore, Robert]

> > Another issue is that it is not clear who needs to perform the byte-
> > swapping among ACPICA and drivers.  If ACPICA, drivers must agree that


ACPICA does not ever do anything with the "data tables" like NFIT, other than handing off the table when requested by a driver.


> -----Original Message-----
> From: Devel [mailto:devel-bounces@acpica.org] On Behalf Of Moore, Robert
> Sent: Tuesday, March 01, 2016 9:37 AM
> To: Toshi Kani; rjw@rjwysocki.net; Williams, Dan J
> Cc: linux-nvdimm@lists.01.org; linux-kernel@vger.kernel.org; linux-
> acpi@vger.kernel.org; elliott@hpe.com; devel@acpica.org
> Subject: Re: [Devel] [PATCH v2 1/3] ACPI/NFIT: Update Control Region
> Structure to comply ACPI 6.1
> 
> 
> 
> > -----Original Message-----
> > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > Sent: Tuesday, March 01, 2016 9:37 AM
> > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J
> > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux-
> > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org
> > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure
> > to comply ACPI 6.1
> >
> > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
> > > We have a bunch of macros in include/acmacros.h -- like this:
> > >
> > > ACPI_MOVE_16_TO_16(d, s)
> >
> > There is a problem in using the ACPICA byte-swap macros.  ACPI is
> > little- endian arch, so the macros are set to perform byte-swappings
> > when the CPU arch is big-endian.  This case, however, is the other way
> > around.  The fields in question are defined & stored as arrays of bytes.
> 
> That's not what I see in the ACPI spec. The fields are defined like any
> other ACPI table.
> 
> Vendor ID 2 6
> Identifier indicating the vendor of the NVDIMM.
> This field shall be set to the value of the NVDIMM SPD Module Manufacturer
> ID Code field a with byte 0 set to DDR4 SPD byte
> 320 and byte 1 set to DDR4 SPD byte 321.
> 
> Device ID 2 8
> Identifier for the NVDIMM, assigned by the module vendor.
> This field shall be set to the value of the NVDIMM SPD Module Product
> Identifier field b with byte 0 set to SPD byte 192 and byte 1 set to SPD
> byte 193.
> 
> Revision ID 2 10
> Revision of the NVDIMM, assigned by the module vendor.
> Byte 1 of this field is reserved.
> Byte 0 of this field shall be set to the value of the NVDIMM SPD Module
> Revision Code field a (i.e., SPD byte 349).
> 
> 
> Etc.
> 
> 
> If you treat
> > them as multi-bytes numeric values, then you need to byte-swap them when
> > the CPU arch is little-endian because arrays of bytes have the same
> > addressing as big-endian.
> >
> > Another issue is that it is not clear who needs to perform the byte-
> > swapping among ACPICA and drivers.  If ACPICA, drivers must agree that
> > these fields are always treated as multi-bytes numeric values despite of
> > the spec.  If drivers, we need to make sure that only a single driver
> > performs this byte-swapping one time as ACPI tables are global
> structures.
> >
> > I think it is much clearer to define the structure according to the ACPI
> > spec.
> >
> > Thanks,
> > -Toshi
> >
> > > > -----Original Message-----
> > > > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > > > Sent: Tuesday, March 01, 2016 8:38 AM
> > > > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J
> > > > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux-
> > > > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org
> > > > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region
> > > > Structure to comply ACPI 6.1
> > > >
> > > > On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > > > > > Sent: Monday, February 22, 2016 1:55 PM
> > > > > > To: rjw@rjwysocki.net; Williams, Dan J
> > > > > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com;
> > > > > > linux-nvdimm@lists.01.or g; linux-acpi@vger.kernel.org;
> > > > > > linux-kernel@vger.kernel.org; devel@acpica.org; Toshi Kani
> > > > > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region
> > > > > > Structure to comply ACPI 6.1
> > > > > >
> > > > > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure
> > > > > > as follows.
> > > > > >  - Valid Fields, Manufacturing Location, and Manufacturing Date
> > > > > >    are added from reserved range.  No change in the structure
> > size.
> > > > > >  - IDs defined as SPD values are arrays of bytes.  The spec
> > > > > >    clarified that they need to be represented as arrays of bytes
> > > > > >    as well.
> > > > > >
> > > > > > This patch makes the following changes to support this update.
> > > > > >  - Change 'struct acpi_nfit_control_region' to reflect the
> update.
> > > > > >    SPD IDs are defined as arrays of bytes, so that they can be
> > > > > >    treated in the same way regardless of CPU endianness and are
> > > > > >    not miss-treated as little-endian numeric values.
> > > > >
> > > > >
> > > > > I don't think we are going to start changing the ACPI tables
> > > > > defined in the ACPICA headers because of this. We do in fact have
> > > > > macros for this purpose.
> > > >
> > > > Can you elaborate what macros you suggest to use for this purpose?
> > > >
> > > > Thanks,
> > > > -Toshi
> > > N     r  y   b X  ǧv ^ )޺{.n +    { i b {ay \x1dʇڙ ,j   f   h   z \x1e w
> > >
> >
> >    j:+v   w j m         zZ+     ݢj"  ! i
> _______________________________________________
> Devel mailing list
> Devel@acpica.org
> https://lists.acpica.org/mailman/listinfo/devel

Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Toshi Kani]

On Tue, 2016-03-01 at 17:37 +0000, Moore, Robert wrote:
> 
> > -----Original Message-----
> > From: Toshi Kani [mailto:toshi.kani@hpe.com]
> > Sent: Tuesday, March 01, 2016 9:37 AM
> > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J
> > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux-
> > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org
> > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure
> > to
> > comply ACPI 6.1
> > 
> > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
> > > We have a bunch of macros in include/acmacros.h -- like this:
> > > 
> > > ACPI_MOVE_16_TO_16(d, s)
> > 
> > There is a problem in using the ACPICA byte-swap macros.  ACPI is
> > little-endian arch, so the macros are set to perform byte-swappings
> > when the CPU arch is big-endian.  This case, however, is the other way
> > around.  The fields in question are defined & stored as arrays of
> > bytes.
> 
> That's not what I see in the ACPI spec. The fields are defined like any
> other ACPI table.
> 
> Vendor ID 2 6 
> Identifier indicating the vendor of the NVDIMM.
> This field shall be set to the value of the NVDIMM SPD Module
> Manufacturer ID Code field a with byte 0 set to DDR4 SPD byte
> 320 and byte 1 set to DDR4 SPD byte 321.

They are different from other fields because the spec defines "byte
locations" of the fields.  The above case, Vendor ID, is defined that:
 - byte 0 set to DDR4 SPD byte 320
 - byte 1 set to DDR4 SPD byte 321

For instance, if SPD byte 320 is 0x12 and 321 is 0x34, then this field is
stored as (0x12, 0x34).  If you read this field as a 16-bit int value, you
will get 0x3412 on little-endian CPUs, such as x86.

Section 5.2.25.9 of ACPI 6.1 further clarifies that Vendor ID needs to be
represented as ("%02x%02x", byte0, byte1), which is "1234" in this case.

So, we will need to byte-swap when it is handled as a 16-bit int value on
little-endian CPUs.  This is different from other fields with multi-byte
numeric values, which are stored in little-endian format in ACPI.

Hence, my patch avoids this byte-swapping as I described in the change log
below.

|  - Change 'struct acpi_nfit_control_region' to reflect the update.
|    SPD IDs are defined as arrays of bytes, so that they can be
|    treated in the same way regardless of CPU endianness and are
|    not miss-treated as little-endian numeric values.

I hope this makes it clear now.

> > Another issue is that it is not clear who needs to perform the byte-
> > swapping among ACPICA and drivers.  If ACPICA, drivers must agree that
>
> ACPICA does not ever do anything with the "data tables" like NFIT, other
> than handing off the table when requested by a driver.

So, this means that the alternative is to change the NFIT driver to do the
byte-swapping for the fields when the CPU arch is little-endian.  If we
cannot change the structure, we will have to take this course...

Thanks,
-Toshi

Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Dan Williams]

On Tue, Mar 1, 2016 at 9:36 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
>> We have a bunch of macros in include/acmacros.h -- like this:
>>
>> ACPI_MOVE_16_TO_16(d, s)
>
> There is a problem in using the ACPICA byte-swap macros.  ACPI is little-
> endian arch, so the macros are set to perform byte-swappings when the CPU
> arch is big-endian.  This case, however, is the other way around.  The
> fields in question are defined & stored as arrays of bytes.  If you treat
> them as multi-bytes numeric values, then you need to byte-swap them when
> the CPU arch is little-endian because arrays of bytes have the same
> addressing as big-endian.
>
> Another issue is that it is not clear who needs to perform the byte-
> swapping among ACPICA and drivers.  If ACPICA, drivers must agree that
> these fields are always treated as multi-bytes numeric values despite of
> the spec.  If drivers, we need to make sure that only a single driver
> performs this byte-swapping one time as ACPI tables are global structures.
>
> I think it is much clearer to define the structure according to the ACPI
> spec.

I think the "ACPI tables are little-endian" assumption is pervasive
throughout the implementation.

Toshi, it seems all we need is conversions like:

- sprintf(buf, "%#x\n", dcr->vendor_id);
+ sprintf(buf, "%#x\n", le16_to_cpu(dcr->vendor_id));

...for the values exported to userspace through sysfs, but otherwise
leave the base table definitions as is.  Will this suffice?

Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Toshi Kani]

On Tue, 2016-03-01 at 10:14 -0800, Dan Williams wrote:
> On Tue, Mar 1, 2016 at 9:36 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
> > > We have a bunch of macros in include/acmacros.h -- like this:
> > > 
> > > ACPI_MOVE_16_TO_16(d, s)
> > 
> > There is a problem in using the ACPICA byte-swap macros.  ACPI is
> > little-
> > endian arch, so the macros are set to perform byte-swappings when the
> > CPU
> > arch is big-endian.  This case, however, is the other way around.  The
> > fields in question are defined & stored as arrays of bytes.  If you
> > treat
> > them as multi-bytes numeric values, then you need to byte-swap them
> > when
> > the CPU arch is little-endian because arrays of bytes have the same
> > addressing as big-endian.
> > 
> > Another issue is that it is not clear who needs to perform the byte-
> > swapping among ACPICA and drivers.  If ACPICA, drivers must agree that
> > these fields are always treated as multi-bytes numeric values despite
> > of
> > the spec.  If drivers, we need to make sure that only a single driver
> > performs this byte-swapping one time as ACPI tables are global
> > structures.
> > 
> > I think it is much clearer to define the structure according to the
> > ACPI
> > spec.
> 
> I think the "ACPI tables are little-endian" assumption is pervasive
> throughout the implementation.
> 
> Toshi, it seems all we need is conversions like:
> 
> - sprintf(buf, "%#x\n", dcr->vendor_id);
> + sprintf(buf, "%#x\n", le16_to_cpu(dcr->vendor_id));
> 
> ...for the values exported to userspace through sysfs, but otherwise
> leave the base table definitions as is.  Will this suffice?

Yes, I am OK to go with this option to get it done.

We still need to add new fields into the structure.  Is it OK to make this
change?  If not, we will need to have a local structure to define the new
fields...

Thanks,
-Toshi

Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Toshi Kani]

On Tue, 2016-03-01 at 10:14 -0800, Dan Williams wrote:
> On Tue, Mar 1, 2016 at 9:36 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
 :
> I think the "ACPI tables are little-endian" assumption is pervasive
> throughout the implementation.
> 
> Toshi, it seems all we need is conversions like:
> 
> - sprintf(buf, "%#x\n", dcr->vendor_id);
> + sprintf(buf, "%#x\n", le16_to_cpu(dcr->vendor_id));

nit - I think it needs to be be16_to_cpu() if I understand this macro
correctly.
-Toshi

> 
> ...for the values exported to userspace through sysfs, but otherwise
> leave the base table definitions as is.  Will this suffice?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] ACPI/NFIT: Add NVDIMM ID "id" under sysfs

[Toshi Kani]

ACPI 6.1, section 5.2.25.9, defines an identifier for an NVDIMM.

Change the NFIT driver to create read-only "id" file under NVDIMM's
nfit directory of sysfs, which shows the NVDIMM ID.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Robert Elliott <elliott@hpe.com>
---
 drivers/acpi/nfit.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 4982a18..677cf89 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -781,6 +781,26 @@ static ssize_t flags_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(flags);
 
+static ssize_t id_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
+
+	if (dcr->valid_fields & ACPI_NFIT_CONTROL_MFG_INFO_VALID)
+		return sprintf(buf, "%02x%02x-%02x-%02x%02x-%02x%02x%02x%02x\n",
+			dcr->vendor_id[0], dcr->vendor_id[1],
+			dcr->manufacturing_location,
+			dcr->manufacturing_date[0], dcr->manufacturing_date[1],
+			dcr->serial_number[0], dcr->serial_number[1],
+			dcr->serial_number[2], dcr->serial_number[3]);
+	else
+		return sprintf(buf, "%02x%02x-%02x%02x%02x%02x\n",
+			dcr->vendor_id[0], dcr->vendor_id[1],
+			dcr->serial_number[0], dcr->serial_number[1],
+			dcr->serial_number[2], dcr->serial_number[3]);
+}
+static DEVICE_ATTR_RO(id);
+
 static struct attribute *acpi_nfit_dimm_attributes[] = {
 	&dev_attr_handle.attr,
 	&dev_attr_phys_id.attr,
@@ -790,6 +810,7 @@ static struct attribute *acpi_nfit_dimm_attributes[] = {
 	&dev_attr_serial.attr,
 	&dev_attr_rev_id.attr,
 	&dev_attr_flags.attr,
+	&dev_attr_id.attr,
 	NULL,
 };
 

[PATCH v2 3/3] nfit_test: Update SPD ID init handlings

[Toshi Kani]

Update the test code that initializes SPD IDs of NFIT control
region structures.  nfit_set_spd_id() is added to handle
initialization of the SPD IDs, which are arrays of bytes.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
---
 tools/testing/nvdimm/test/nfit.c |   64 +++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 25 deletions(-)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 90bd2ea..283e187 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -160,6 +160,14 @@ static struct nfit_test *to_nfit_test(struct device *dev)
 	return container_of(pdev, struct nfit_test, pdev);
 }
 
+static void nfit_set_spd_id(u8 *dist, u64 value, size_t size)
+{
+	int shift;
+
+	for (shift = size - 1; shift >= 0; shift--, dist++)
+		*dist = (u8)((value >> (8 * shift)) & 0xff);
+}
+
 static int nfit_test_cmd_get_config_size(struct nd_cmd_get_config_size *nd_cmd,
 		unsigned int buf_len)
 {
@@ -815,10 +823,11 @@ static void nfit_test0_setup(struct nfit_test *t)
 	dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
 	dcr->header.length = sizeof(struct acpi_nfit_control_region);
 	dcr->region_index = 0+1;
-	dcr->vendor_id = 0xabcd;
-	dcr->device_id = 0;
-	dcr->revision_id = 1;
-	dcr->serial_number = ~handle[0];
+	nfit_set_spd_id(dcr->vendor_id, 0xabcd, sizeof(dcr->vendor_id));
+	nfit_set_spd_id(dcr->device_id, 0, sizeof(dcr->device_id));
+	nfit_set_spd_id(dcr->revision_id, 1, sizeof(dcr->revision_id));
+	nfit_set_spd_id(dcr->serial_number, ~handle[0],
+				sizeof(dcr->serial_number));
 	dcr->windows = 1;
 	dcr->window_size = DCR_SIZE;
 	dcr->command_offset = 0;
@@ -831,10 +840,11 @@ static void nfit_test0_setup(struct nfit_test *t)
 	dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
 	dcr->header.length = sizeof(struct acpi_nfit_control_region);
 	dcr->region_index = 1+1;
-	dcr->vendor_id = 0xabcd;
-	dcr->device_id = 0;
-	dcr->revision_id = 1;
-	dcr->serial_number = ~handle[1];
+	nfit_set_spd_id(dcr->vendor_id, 0xabcd, sizeof(dcr->vendor_id));
+	nfit_set_spd_id(dcr->device_id, 0, sizeof(dcr->device_id));
+	nfit_set_spd_id(dcr->revision_id, 1, sizeof(dcr->revision_id));
+	nfit_set_spd_id(dcr->serial_number, ~handle[1],
+				sizeof(dcr->serial_number));
 	dcr->windows = 1;
 	dcr->window_size = DCR_SIZE;
 	dcr->command_offset = 0;
@@ -847,10 +857,11 @@ static void nfit_test0_setup(struct nfit_test *t)
 	dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
 	dcr->header.length = sizeof(struct acpi_nfit_control_region);
 	dcr->region_index = 2+1;
-	dcr->vendor_id = 0xabcd;
-	dcr->device_id = 0;
-	dcr->revision_id = 1;
-	dcr->serial_number = ~handle[2];
+	nfit_set_spd_id(dcr->vendor_id, 0xabcd, sizeof(dcr->vendor_id));
+	nfit_set_spd_id(dcr->device_id, 0, sizeof(dcr->device_id));
+	nfit_set_spd_id(dcr->revision_id, 1, sizeof(dcr->revision_id));
+	nfit_set_spd_id(dcr->serial_number, ~handle[2],
+				sizeof(dcr->serial_number));
 	dcr->windows = 1;
 	dcr->window_size = DCR_SIZE;
 	dcr->command_offset = 0;
@@ -863,10 +874,11 @@ static void nfit_test0_setup(struct nfit_test *t)
 	dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
 	dcr->header.length = sizeof(struct acpi_nfit_control_region);
 	dcr->region_index = 3+1;
-	dcr->vendor_id = 0xabcd;
-	dcr->device_id = 0;
-	dcr->revision_id = 1;
-	dcr->serial_number = ~handle[3];
+	nfit_set_spd_id(dcr->vendor_id, 0xabcd, sizeof(dcr->vendor_id));
+	nfit_set_spd_id(dcr->device_id, 0, sizeof(dcr->device_id));
+	nfit_set_spd_id(dcr->revision_id, 1, sizeof(dcr->revision_id));
+	nfit_set_spd_id(dcr->serial_number, ~handle[3],
+				sizeof(dcr->serial_number));
 	dcr->windows = 1;
 	dcr->window_size = DCR_SIZE;
 	dcr->command_offset = 0;
@@ -959,10 +971,11 @@ static void nfit_test0_setup(struct nfit_test *t)
 		dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
 		dcr->header.length = sizeof(struct acpi_nfit_control_region);
 		dcr->region_index = 4+1;
-		dcr->vendor_id = 0xabcd;
-		dcr->device_id = 0;
-		dcr->revision_id = 1;
-		dcr->serial_number = ~handle[4];
+		nfit_set_spd_id(dcr->vendor_id, 0xabcd, sizeof(dcr->vendor_id));
+		nfit_set_spd_id(dcr->device_id, 0, sizeof(dcr->device_id));
+		nfit_set_spd_id(dcr->revision_id, 1, sizeof(dcr->revision_id));
+		nfit_set_spd_id(dcr->serial_number, ~handle[4],
+					sizeof(dcr->serial_number));
 		dcr->windows = 1;
 		dcr->window_size = DCR_SIZE;
 		dcr->command_offset = 0;
@@ -1128,11 +1141,12 @@ static void nfit_test1_setup(struct nfit_test *t)
 	dcr->header.type = ACPI_NFIT_TYPE_CONTROL_REGION;
 	dcr->header.length = sizeof(struct acpi_nfit_control_region);
 	dcr->region_index = 0+1;
-	dcr->vendor_id = 0xabcd;
-	dcr->device_id = 0;
-	dcr->revision_id = 1;
-	dcr->serial_number = ~0;
-	dcr->code = 0x201;
+	nfit_set_spd_id(dcr->vendor_id, 0xabcd, sizeof(dcr->vendor_id));
+	nfit_set_spd_id(dcr->device_id, 0, sizeof(dcr->device_id));
+	nfit_set_spd_id(dcr->revision_id, 1, sizeof(dcr->revision_id));
+	nfit_set_spd_id(dcr->serial_number, ~0,
+				sizeof(dcr->serial_number));
+	nfit_set_spd_id(dcr->code, 0x201, sizeof(dcr->code));
 	dcr->windows = 0;
 	dcr->window_size = 0;
 	dcr->command_offset = 0;