2016-04-25 21:43:47

by Kani, Toshimitsu

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

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


2016-04-25 21:43:49

by Kani, Toshimitsu

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

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 <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Robert Moore <[email protected]>
Cc: Robert Elliott <[email protected]>
---
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,
};


2016-04-25 21:44:52

by Kani, Toshimitsu

[permalink] [raw]
Subject: [PATCH v3 1/2] acpi/nfit: Update nfit driver to comply with 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 (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 <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Robert Moore <[email protected]>
Cc: Robert Elliott <[email protected]>
---
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);


2016-04-26 23:54:41

by Dan Williams

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

On Mon, Apr 25, 2016 at 2:34 PM, Toshi Kani <[email protected]> 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.

2018-06-18 19:02:47

by Dan Williams

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

On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <[email protected]> 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.

2018-06-18 19:42:05

by Kani, Toshimitsu

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

On Mon, 2018-06-18 at 12:01 -0700, Dan Williams wrote:
> On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <[email protected]> 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

2018-06-18 19:48:54

by Dan Williams

[permalink] [raw]
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 <[email protected]> wrote:
> On Mon, 2018-06-18 at 12:01 -0700, Dan Williams wrote:
>> On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <[email protected]> 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?

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



> -----Original Message-----
> From: Dan Williams [mailto:[email protected]]
> Sent: Monday, June 18, 2018 2:47 PM
> To: Kani, Toshi <[email protected]>
> Cc: [email protected]; [email protected]; Moore, Robert
> <[email protected]>; Li, Juston <[email protected]>;
> [email protected]; [email protected]; Elliott, Robert (Persistent
> Memory) <[email protected]>
> 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 <[email protected]> wrote:
> > On Mon, 2018-06-18 at 12:01 -0700, Dan Williams wrote:
> >> On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <[email protected]> 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


2018-06-18 21:48:00

by Dan Williams

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

On Mon, Jun 18, 2018 at 2:37 PM, Elliott, Robert (Persistent Memory)
<[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: Dan Williams [mailto:[email protected]]
>> Sent: Monday, June 18, 2018 2:47 PM
>> To: Kani, Toshi <[email protected]>
>> Cc: [email protected]; [email protected]; Moore, Robert
>> <[email protected]>; Li, Juston <[email protected]>;
>> [email protected]; [email protected]; Elliott, Robert (Persistent
>> Memory) <[email protected]>
>> 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 <[email protected]> wrote:
>> > On Mon, 2018-06-18 at 12:01 -0700, Dan Williams wrote:
>> >> On Mon, Apr 25, 2016 at 2:43 PM Toshi Kani <[email protected]> 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.

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



> -----Original Message-----
> From: Dan Williams [mailto:[email protected]]
> Sent: Monday, June 18, 2018 4:47 PM
> To: Elliott, Robert (Persistent Memory) <[email protected]>
> Cc: Kani, Toshi <[email protected]>; [email protected]; linux-
> [email protected]; Moore, Robert <[email protected]>; Li, Juston
> <[email protected]>; [email protected]; [email protected]
> 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

2018-06-19 15:29:54

by Dan Williams

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

On Tue, Jun 19, 2018 at 7:31 AM, Elliott, Robert (Persistent Memory)
<[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: Dan Williams [mailto:[email protected]]
>> Sent: Monday, June 18, 2018 4:47 PM
>> To: Elliott, Robert (Persistent Memory) <[email protected]>
>> Cc: Kani, Toshi <[email protected]>; [email protected]; linux-
>> [email protected]; Moore, Robert <[email protected]>; Li, Juston
>> <[email protected]>; [email protected]; [email protected]
>> 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?

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



> -----Original Message-----
> From: [email protected] [mailto:linux-kernel-
> [email protected]] 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.

2018-06-19 23:48:48

by Dan Williams

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

On Tue, Jun 19, 2018 at 12:53 PM, Elliott, Robert (Persistent Memory)
<[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: [email protected] [mailto:linux-kernel-
>> [email protected]] 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.