| Message ID | 1461620099-11933-2-git-send-email-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5ad9a7fde07a |
| Headers | show |
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.
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/ Thanks, -Toshi
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?
> -----Original Message----- > From: Dan Williams [mailto:dan.j.williams@intel.com] > Sent: Monday, June 18, 2018 2:47 PM > To: Kani, Toshi <toshi.kani@hpe.com> > Cc: linux-kernel@vger.kernel.org; linux-nvdimm@lists.01.org; Moore, Robert > <robert.moore@intel.com>; Li, Juston <juston.li@intel.com>; > rjw@rjwysocki.net; linux-acpi@vger.kernel.org; Elliott, Robert (Persistent > Memory) <elliott@hpe.com> > 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@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? 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
On Mon, Jun 18, 2018 at 2:37 PM, Elliott, Robert (Persistent Memory) <elliott@hpe.com> wrote: > > >> -----Original Message----- >> From: Dan Williams [mailto:dan.j.williams@intel.com] >> Sent: Monday, June 18, 2018 2:47 PM >> To: Kani, Toshi <toshi.kani@hpe.com> >> Cc: linux-kernel@vger.kernel.org; linux-nvdimm@lists.01.org; Moore, Robert >> <robert.moore@intel.com>; Li, Juston <juston.li@intel.com>; >> rjw@rjwysocki.net; linux-acpi@vger.kernel.org; Elliott, Robert (Persistent >> Memory) <elliott@hpe.com> >> 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@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? > > 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.
> -----Original Message----- > From: Dan Williams [mailto:dan.j.williams@intel.com] > Sent: Monday, June 18, 2018 4:47 PM > To: Elliott, Robert (Persistent Memory) <elliott@hpe.com> > Cc: Kani, Toshi <toshi.kani@hpe.com>; linux-kernel@vger.kernel.org; linux- > nvdimm@lists.01.org; Moore, Robert <robert.moore@intel.com>; Li, Juston > <juston.li@intel.com>; rjw@rjwysocki.net; linux-acpi@vger.kernel.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
On Tue, Jun 19, 2018 at 7:31 AM, Elliott, Robert (Persistent Memory) <elliott@hpe.com> wrote: > > >> -----Original Message----- >> From: Dan Williams [mailto:dan.j.williams@intel.com] >> Sent: Monday, June 18, 2018 4:47 PM >> To: Elliott, Robert (Persistent Memory) <elliott@hpe.com> >> Cc: Kani, Toshi <toshi.kani@hpe.com>; linux-kernel@vger.kernel.org; linux- >> nvdimm@lists.01.org; Moore, Robert <robert.moore@intel.com>; Li, Juston >> <juston.li@intel.com>; rjw@rjwysocki.net; linux-acpi@vger.kernel.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?
> -----Original Message----- > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > owner@vger.kernel.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.
On Tue, Jun 19, 2018 at 12:53 PM, Elliott, Robert (Persistent Memory) <elliott@hpe.com> wrote: > > >> -----Original Message----- >> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- >> owner@vger.kernel.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.
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);
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(-)