[PATCH v3 0/2] Support ACPI 6.1 update in NFIT Control Region Structure

[PATCH v3 0/2] Support ACPI 6.1 update in 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 (SPD values) are stored as arrays of bytes (i.e. big-endian
   format).  The spec clarifies that they need to be represented
   as arrays of bytes as well.

Patch 1 changes the NFIT driver to comply with ACPI 6.1.
Patch 2 adds a new sysfs file "id" to show NVDIMM ID defined in ACPI 6.1.

The patch-set applies on linux-pm.git acpica.

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

---
v3:
 - Need to coordinate with ACPICA update (Bob Moore, Dan Williams)
 - Integrate with ACPICA changes in struct acpi_nfit_control_region.
   (commit 138a95547ab0)

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/2 acpi/nfit: Update nfit driver to comply with ACPI 6.1
 2/3 acpi/nfit: Add sysfs "id" for NVDIMM ID

---
 drivers/acpi/nfit.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

[PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with 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 (SPD values) are stored as arrays of bytes (i.e. big-endian
   format).  The spec clarifies that they need to be represented
   as arrays of bytes as well.

This patch makes the following changes to support this update.
 - Change the NFIT driver to show SPD ID values in big-endian
   format.
 - Change sprintf format to use "0x" instead of "#" since "%#02x"
   does not prepend '0'.

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: Robert Elliott <elliott@hpe.com>
---
 drivers/acpi/nfit.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index d0f35e6..5dc243c 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -816,7 +816,7 @@ 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%04x\n", be16_to_cpu(dcr->vendor_id));
 }
 static DEVICE_ATTR_RO(vendor);
 
@@ -825,7 +825,7 @@ 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%04x\n", be16_to_cpu(dcr->revision_id));
 }
 static DEVICE_ATTR_RO(rev_id);
 
@@ -834,7 +834,7 @@ 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%04x\n", be16_to_cpu(dcr->device_id));
 }
 static DEVICE_ATTR_RO(device);
 
@@ -843,7 +843,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%04x\n", be16_to_cpu(dcr->code));
 }
 static DEVICE_ATTR_RO(format);
 
@@ -852,7 +852,7 @@ 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%08x\n", be32_to_cpu(dcr->serial_number));
 }
 static DEVICE_ATTR_RO(serial);
 

Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Dan Williams]

On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <toshi.kani-ZPxbGqLxI0U@public.gmane.org> wrote:
>
> 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 (SPD values) are stored as arrays of bytes (i.e. big-endian
>    format).  The spec clarifies that they need to be represented
>    as arrays of bytes as well.
>

Circling back on this a couple years too late... where are you reading
this "arrays of bytes" note. As far as I can see this is wrong. JEDEC
says that vendor id is stored LSB of the id is stored at the lowest
byte in SPD, which is little endian. So it seems Linux has showing the
incorrect value for a long time now.

Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Kani, Toshi]

On Mon, 2018-06-18 at 12:01 -0700, Dan Williams wrote:
> On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <toshi.kani-ZPxbGqLxI0U@public.gmane.org> wrote:
> > 
> > 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 (SPD values) are stored as arrays of bytes (i.e. big-endian
> >    format).  The spec clarifies that they need to be represented
> >    as arrays of bytes as well.
> > 
> 
> Circling back on this a couple years too late... where are you reading
> this "arrays of bytes" note. As far as I can see this is wrong. JEDEC
> says that vendor id is stored LSB of the id is stored at the lowest
> byte in SPD, which is little endian. So it seems Linux has showing the
> incorrect value for a long time now.

This follows ACPI 6.2a section 5.2.25.10 NVDIMM Representation Format,
which Robert cited in his comment below:
https://patchwork.kernel.org/patch/10237609/

Thanks,
-Toshi

Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Dan Williams]

On Mon, Jun 18, 2018 at 12:39 PM, Kani, Toshi <toshi.kani@hpe.com> wrote:
> On Mon, 2018-06-18 at 12:01 -0700, Dan Williams wrote:
>> On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <toshi.kani@hpe.com> wrote:
>> >
>> > 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 (SPD values) are stored as arrays of bytes (i.e. big-endian
>> >    format).  The spec clarifies that they need to be represented
>> >    as arrays of bytes as well.
>> >
>>
>> Circling back on this a couple years too late... where are you reading
>> this "arrays of bytes" note. As far as I can see this is wrong. JEDEC
>> says that vendor id is stored LSB of the id is stored at the lowest
>> byte in SPD, which is little endian. So it seems Linux has showing the
>> incorrect value for a long time now.
>
> This follows ACPI 6.2a section 5.2.25.10 NVDIMM Representation Format,
> which Robert cited in his comment below:
> https://patchwork.kernel.org/patch/10237609/

Right, the representation format has the fields big-endian for some
reason, but the individual values for sysfs should be show
little-endian as far as I can see. What am I missing?

RE: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org]
> Sent: Monday, June 18, 2018 2:47 PM
> To: Kani, Toshi <toshi.kani-ZPxbGqLxI0U@public.gmane.org>
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org; Moore, Robert
> <robert.moore-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Li, Juston <juston.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>;
> rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org; linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Elliott, Robert (Persistent
> Memory) <elliott-ZPxbGqLxI0U@public.gmane.org>
> Subject: Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with
> ACPI 6.1
> 
> On Mon, Jun 18, 2018 at 12:39 PM, Kani, Toshi <toshi.kani-ZPxbGqLxI0U@public.gmane.org> wrote:
> > On Mon, 2018-06-18 at 12:01 -0700, Dan Williams wrote:
> >> On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <toshi.kani-ZPxbGqLxI0U@public.gmane.org> wrote:
> >> >
> >> > 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 (SPD values) are stored as arrays of bytes (i.e. big-endian
> >> >    format).  The spec clarifies that they need to be represented
> >> >    as arrays of bytes as well.
> >> >
> >>
> >> Circling back on this a couple years too late... where are you reading
> >> this "arrays of bytes" note. As far as I can see this is wrong. JEDEC
> >> says that vendor id is stored LSB of the id is stored at the lowest
> >> byte in SPD, which is little endian. So it seems Linux has showing the
> >> incorrect value for a long time now.
> >
> > This follows ACPI 6.2a section 5.2.25.10 NVDIMM Representation Format,
> > which Robert cited in his comment below:
> > https://patchwork.kernel.org/patch/10237609/
> 
> Right, the representation format has the fields big-endian for some
> reason, but the individual values for sysfs should be show
> little-endian as far as I can see. What am I missing?

In practice, the serial numbers from three major DDR4 DIMM manufacturers
are being assigned as big-endian, like in this set of NVDIMM-Ns:
/sys/bus/nd/devices/nmem0/nfit/serial:0x122f8255
/sys/bus/nd/devices/nmem1/nfit/serial:0x122f7f5e
/sys/bus/nd/devices/nmem2/nfit/serial:0x122f818f
/sys/bus/nd/devices/nmem3/nfit/serial:0x122f821c
/sys/bus/nd/devices/nmem4/nfit/serial:0x122f817e
/sys/bus/nd/devices/nmem5/nfit/serial:0x122f81cd
/sys/bus/nd/devices/nmem6/nfit/serial:0x122f821e
/sys/bus/nd/devices/nmem7/nfit/serial:0x122f819b
/sys/bus/nd/devices/nmem8/nfit/serial:0x122f81a2
/sys/bus/nd/devices/nmem9/nfit/serial:0x122f8198
/sys/bus/nd/devices/nmem10/nfit/serial:0x122f8193
/sys/bus/nd/devices/nmem11/nfit/serial:0x122f7f58
/sys/bus/nd/devices/nmem12/nfit/serial:0x122f81cb
/sys/bus/nd/devices/nmem13/nfit/serial:0x122f8181
/sys/bus/nd/devices/nmem14/nfit/serial:0x122f8210
/sys/bus/nd/devices/nmem15/nfit/serial:0x122f821f

and this set of regular DIMMs:
396851B4
3968134C
396852DA
396850AB
39685A13
39685317
396852DD
396852D9

Of the possible approaches for the sysfs nfit field decodes:
fixed big-endian:
  matches printed label content (text and barcode)
  matches ACPI display advice for management tools
  probably matches SMBIOS Serial Number string format (although
    that depends on the system firmware)
  requires user to know that this OS uses big-endian
  has been upstream for a while now
fixed little-endian:
  harder to see that cd812f12 matches 122f81cd seen elsewhere
  harder to notice that B4516839 is a peer of 4C136839
  might match other little-endian-only OSes
  requires user to know that this OS uses little-endian
native endian:
  most confusing
  config/status files referencing the DIMMs are not portable
  requires user to know that this OS uses native endianness
  requires user to know the CPU endianness
  was upstream several years ago

Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Dan Williams]

On Mon, Jun 18, 2018 at 2:37 PM, Elliott, Robert (Persistent Memory)
<elliott-ZPxbGqLxI0U@public.gmane.org> wrote:
>
>
>> -----Original Message-----
>> From: Dan Williams [mailto:dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org]
>> Sent: Monday, June 18, 2018 2:47 PM
>> To: Kani, Toshi <toshi.kani-ZPxbGqLxI0U@public.gmane.org>
>> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org; Moore, Robert
>> <robert.moore-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Li, Juston <juston.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>;
>> rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org; linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Elliott, Robert (Persistent
>> Memory) <elliott-ZPxbGqLxI0U@public.gmane.org>
>> Subject: Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with
>> ACPI 6.1
>>
>> On Mon, Jun 18, 2018 at 12:39 PM, Kani, Toshi <toshi.kani-ZPxbGqLxI0U@public.gmane.org> wrote:
>> > On Mon, 2018-06-18 at 12:01 -0700, Dan Williams wrote:
>> >> On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <toshi.kani-ZPxbGqLxI0U@public.gmane.org> wrote:
>> >> >
>> >> > 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 (SPD values) are stored as arrays of bytes (i.e. big-endian
>> >> >    format).  The spec clarifies that they need to be represented
>> >> >    as arrays of bytes as well.
>> >> >
>> >>
>> >> Circling back on this a couple years too late... where are you reading
>> >> this "arrays of bytes" note. As far as I can see this is wrong. JEDEC
>> >> says that vendor id is stored LSB of the id is stored at the lowest
>> >> byte in SPD, which is little endian. So it seems Linux has showing the
>> >> incorrect value for a long time now.
>> >
>> > This follows ACPI 6.2a section 5.2.25.10 NVDIMM Representation Format,
>> > which Robert cited in his comment below:
>> > https://patchwork.kernel.org/patch/10237609/
>>
>> Right, the representation format has the fields big-endian for some
>> reason, but the individual values for sysfs should be show
>> little-endian as far as I can see. What am I missing?
>
> In practice, the serial numbers from three major DDR4 DIMM manufacturers
> are being assigned as big-endian, like in this set of NVDIMM-Ns:
> /sys/bus/nd/devices/nmem0/nfit/serial:0x122f8255
> /sys/bus/nd/devices/nmem1/nfit/serial:0x122f7f5e
> /sys/bus/nd/devices/nmem2/nfit/serial:0x122f818f
> /sys/bus/nd/devices/nmem3/nfit/serial:0x122f821c
> /sys/bus/nd/devices/nmem4/nfit/serial:0x122f817e
> /sys/bus/nd/devices/nmem5/nfit/serial:0x122f81cd
> /sys/bus/nd/devices/nmem6/nfit/serial:0x122f821e
> /sys/bus/nd/devices/nmem7/nfit/serial:0x122f819b
> /sys/bus/nd/devices/nmem8/nfit/serial:0x122f81a2
> /sys/bus/nd/devices/nmem9/nfit/serial:0x122f8198
> /sys/bus/nd/devices/nmem10/nfit/serial:0x122f8193
> /sys/bus/nd/devices/nmem11/nfit/serial:0x122f7f58
> /sys/bus/nd/devices/nmem12/nfit/serial:0x122f81cb
> /sys/bus/nd/devices/nmem13/nfit/serial:0x122f8181
> /sys/bus/nd/devices/nmem14/nfit/serial:0x122f8210
> /sys/bus/nd/devices/nmem15/nfit/serial:0x122f821f
>
> and this set of regular DIMMs:
> 396851B4
> 3968134C
> 396852DA
> 396850AB
> 39685A13
> 39685317
> 396852DD
> 396852D9

Let's take something simple like Vendor ID. What is the Vendor ID for
these DIMMs and what does Linux print in sysfs?

> Of the possible approaches for the sysfs nfit field decodes:
> fixed big-endian:
>   matches printed label content (text and barcode)
>   matches ACPI display advice for management tools
>   probably matches SMBIOS Serial Number string format (although
>     that depends on the system firmware)
>   requires user to know that this OS uses big-endian
>   has been upstream for a while now
> fixed little-endian:
>   harder to see that cd812f12 matches 122f81cd seen elsewhere
>   harder to notice that B4516839 is a peer of 4C136839
>   might match other little-endian-only OSes
>   requires user to know that this OS uses little-endian
> native endian:
>   most confusing
>   config/status files referencing the DIMMs are not portable
>   requires user to know that this OS uses native endianness
>   requires user to know the CPU endianness
>   was upstream several years ago

The time upstream is painful, but if Linux is needlessly swizzling the
bits this needs to be fixed.

RE: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org]
> Sent: Monday, June 18, 2018 4:47 PM
> To: Elliott, Robert (Persistent Memory) <elliott-ZPxbGqLxI0U@public.gmane.org>
> Cc: Kani, Toshi <toshi.kani-ZPxbGqLxI0U@public.gmane.org>; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org; Moore, Robert <robert.moore-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Li, Juston
> <juston.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org; linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with
> ACPI 6.1


> Let's take something simple like Vendor ID. What is the Vendor ID for
> these DIMMs and what does Linux print in sysfs?

Here are some examples (kernel 4.17):

$ cd /sys/bus/nd/devices/nmem0/nfit
$ grep -s . *
device:0x314e
dsm_mask:0x3c76
family:1
flags:smart_notify
format:0x0101
formats:1
handle:0x1
id:802c-0f-1612-122f8255	[SPD bytes 320-328, in that order left-to-right]
phys_id:0x16
rev_id:0x3100
serial:0x122f8255
subsystem_device:0x3141
subsystem_rev_id:0x0100
subsystem_vendor:0x8034	[Cypress Semiconductor]
vendor:0x802c		[Micron]

$ cd /sys/bus/nd/devices/nmem1/nfit
$ grep -s . *
device:0x314e
dsm_mask:0x3c76
family:1
flags:smart_notify
format:0x0101
formats:1
handle:0x2
id:802c-0f-1612-122f7f5e
phys_id:0x15
rev_id:0x3100
serial:0x122f7f5e
subsystem_device:0x3141
subsystem_rev_id:0x0100
subsystem_vendor:0x8034
vendor:0x802c

Some corresponding information for those NVDIMM-Ns as reported by dmidecode:

Handle 0x00A8, DMI type 237, 9 bytes
OEM-specific Type
        Header and Data:
                ED 09 A8 00 16 00 01 02 03
        Strings:
                Micron		[Module manufacturer]
                18ASF1G72XF12G1Y11AA	[Module part number]
                122F8255	[Module serial number]

Handle 0x00A7, DMI type 237, 9 bytes
OEM-specific Type
        Header and Data:
                ED 09 A7 00 15 00 01 02 03
        Strings:
                Micron
                18ASF1G72XF12G1Y11AA
                122F7F5E

Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Dan Williams]

On Tue, Jun 19, 2018 at 7:31 AM, Elliott, Robert (Persistent Memory)
<elliott-ZPxbGqLxI0U@public.gmane.org> wrote:
>
>
>> -----Original Message-----
>> From: Dan Williams [mailto:dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org]
>> Sent: Monday, June 18, 2018 4:47 PM
>> To: Elliott, Robert (Persistent Memory) <elliott-ZPxbGqLxI0U@public.gmane.org>
>> Cc: Kani, Toshi <toshi.kani-ZPxbGqLxI0U@public.gmane.org>; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>> nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org; Moore, Robert <robert.moore-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Li, Juston
>> <juston.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org; linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with
>> ACPI 6.1
>
>
>> Let's take something simple like Vendor ID. What is the Vendor ID for
>> these DIMMs and what does Linux print in sysfs?
>
> Here are some examples (kernel 4.17):
>
> $ cd /sys/bus/nd/devices/nmem0/nfit
> $ grep -s . *
> device:0x314e
> dsm_mask:0x3c76
> family:1
> flags:smart_notify
> format:0x0101
> formats:1
> handle:0x1
> id:802c-0f-1612-122f8255[SPD bytes 320-328, in that order left-to-right]
> phys_id:0x16
> rev_id:0x3100
> serial:0x122f8255
> subsystem_device:0x3141
> subsystem_rev_id:0x0100
> subsystem_vendor:0x8034[Cypress Semiconductor]
> vendor:0x802c[Micron]

Ok, so the lowest significant byte of the Micron id is supposed to be
0x2c and this text representation matches that. So the bytes are being
endian swapped when written to the SPD?

RE: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: linux-kernel-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-kernel-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Dan Williams
> Sent: Tuesday, June 19, 2018 10:29 AM
> Subject: Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with
> ACPI 6.1
...
> >
> > Here are some examples (kernel 4.17):

Note that these values were as reported on a little-endian system.

> Ok, so the lowest significant byte of the Micron id is supposed to be
> 0x2c and this text representation matches that. So the bytes are being
> endian swapped when written to the SPD?

SPD byte 320 is 0x80. That's the bank number byte (with odd parity).
SPD byte 321 is 0x2c. That's the manufacturer code byte (with odd parity).

If treated as a single 2-byte value, that is:
* 0x802c (32812 in decimal) if interpreted as big-endian 
* 0x2c80 (11392 in decimal) if interpreted as little-endian

Reviewing the _show() functions in drivers/acpi/nfit/core.c:
  BE device:0x314e
  NE dsm_mask:0x3c76
  NE family:1
  N/A flags:smart_notify
  LE format:0x0101
  N/A formats:1
  BE handle:0x1
  BE id:802c-0f-1612-122f8255 [SPD bytes 320-328]
  NE phys_id:0x16
  BE rev_id:0x3100
  BE serial:0x122f8255
  BE subsystem_device:0x3141
  BE subsystem_rev_id:0x0100
  BE subsystem_vendor:0x8034 [Cypress Semiconductor]
  BE vendor:0x802c [Micron]

(BE=fixed big-endian, LE=fixed little-endian, NE=native-endian,
 N/A=not applicable)

BE and LE will print the same regardless of the endianness
of the system, while NE will vary.

Reviewing the NE sysfs files:

phys_id:
This NFIT field reports an SMBIOS handle (instance ID), a LE number
from 0..n (0x103 on one of my systems).  dmidecode shows handles
like this:
  Handle 0x0016, DMI type 17, 40 bytes
  Memory Device
        Array Handle: 0x000A

On a big-endian system, I think we'll see
  phys_id:0x1600
which means 5632 decimal, which is misleading. Fixed LE would
be better.

dsm_mask:
The code that parses _DSM function 0 output, acpi_check_dsm(),
correctly interprets the bitmask as little-endian for internal
calculations.  I think a big-endian system will print 
dsm_mask:0x763c, which is confusing. Fixed LE would be better.  
The override_dsm_mask module parameter needs to match.

family:
This is an internal value, so NE is fine.

Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with ACPI 6.1

[Dan Williams]

On Tue, Jun 19, 2018 at 12:53 PM, Elliott, Robert (Persistent Memory)
<elliott-ZPxbGqLxI0U@public.gmane.org> wrote:
>
>
>> -----Original Message-----
>> From: linux-kernel-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-kernel-
>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Dan Williams
>> Sent: Tuesday, June 19, 2018 10:29 AM
>> Subject: Re: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with
>> ACPI 6.1
> ...
>> >
>> > Here are some examples (kernel 4.17):
>
> Note that these values were as reported on a little-endian system.
>
>> Ok, so the lowest significant byte of the Micron id is supposed to be
>> 0x2c and this text representation matches that. So the bytes are being
>> endian swapped when written to the SPD?
>
> SPD byte 320 is 0x80. That's the bank number byte (with odd parity).
> SPD byte 321 is 0x2c. That's the manufacturer code byte (with odd parity).
>
> If treated as a single 2-byte value, that is:
> * 0x802c (32812 in decimal) if interpreted as big-endian
> * 0x2c80 (11392 in decimal) if interpreted as little-endian

Ok, JEDEC defines byte 320 as the LSB, so the fact that Linux is
showing 0x2c as the LSB is wrong. Linux needs to be fixed.

[PATCH v3 2/2] acpi/nfit: Add sysfs "id" for NVDIMM ID

[Toshi Kani]

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

Change the NFIT driver to add a new sysfs file "id" under nfit
directory.

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: Robert Elliott <elliott@hpe.com>
---
 drivers/acpi/nfit.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 5dc243c..5a7199d 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -870,6 +870,24 @@ 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, "%04x-%02x-%04x-%08x\n",
+				be16_to_cpu(dcr->vendor_id),
+				dcr->manufacturing_location,
+				be16_to_cpu(dcr->manufacturing_date),
+				be32_to_cpu(dcr->serial_number));
+	else
+		return sprintf(buf, "%04x-%08x\n",
+				be16_to_cpu(dcr->vendor_id),
+				be32_to_cpu(dcr->serial_number));
+}
+static DEVICE_ATTR_RO(id);
+
 static struct attribute *acpi_nfit_dimm_attributes[] = {
 	&dev_attr_handle.attr,
 	&dev_attr_phys_id.attr,
@@ -879,6 +897,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,
 };
 

Re: [PATCH v3 0/2] Support ACPI 6.1 update in NFIT Control Region Structure

[Dan Williams]

On Mon, Apr 25, 2016 at 2:34 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> 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 (SPD values) are stored as arrays of bytes (i.e. big-endian
>    format).  The spec clarifies that they need to be represented
>    as arrays of bytes as well.
>
> Patch 1 changes the NFIT driver to comply with ACPI 6.1.
> Patch 2 adds a new sysfs file "id" to show NVDIMM ID defined in ACPI 6.1.
>
> The patch-set applies on linux-pm.git acpica.
>
> link: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
>
> ---
> v3:
>  - Need to coordinate with ACPICA update (Bob Moore, Dan Williams)
>  - Integrate with ACPICA changes in struct acpi_nfit_control_region.
>    (commit 138a95547ab0)
>
> 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/2 acpi/nfit: Update nfit driver to comply with ACPI 6.1
>  2/3 acpi/nfit: Add sysfs "id" for NVDIMM ID

Thanks Toshi, I've applied these.