linux-kernel - Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 2 Jun 2024 09:55:48 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Armin Wolf <W_Armin@....de>
Cc: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org, 
	devicetree@...r.kernel.org, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org, 
	René Rebe <rene@...ctcode.de>, Wolfram Sang <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD
 data

On 2024-06-01 21:23:24+0000, Armin Wolf wrote:
> Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:
> 
> > On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
> > 
> > <snip>
> > 
> > > Makes sense. Another question:
> > > 
> > > This:
> > > 
> > > +        struct nvmem_config nvmem_config = {
> > > +               .type = NVMEM_TYPE_EEPROM,
> > > +               .name = dev_name(dev),
> > > +               .id = NVMEM_DEVID_AUTO,
> > > 
> > > results in:
> > > 
> > > $ ls /sys/bus/nvmem/devices
> > > 0-00501  0-00512  0-00523  0-00534  cmos_nvram0
> > > ^^^^^^^  ^^^^^^^  ^^^^^^^  ^^^^^^^
> > > 
> > > which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
> > > which results in
> > > 
> > > $ ls /sys/bus/nvmem/devices
> > > 0-0050	0-0051	0-0052	0-0053	cmos_nvram0
> > > 
> > > We could also used fixed strings, but "spd" results in "spd[1-4]" which
> > > I think would be a bit misleading since the DDR3/4 SPD data format is
> > > different, and "spd5118" would result in "spd5118[1-4]" which again would
> > > look odd. Any suggestions ?
> > In order of descending, personal preference:
> > 
> > * spd-ddr5-[0-3] (.id = client->address - 0x50)
> 
> Hi,
> 
> this will break as soon as more than 8 DDR5 DIMMs are installed.

i2c_register_spd() only handles 8 DIMMs, too.
JESD 300-5B.01 (section 2.6.5) also defines i2c addresses for 8 DIMMS only.

Outside of that range we could fall back to something else.

> > * spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
> > * Same with only "ddr5-"
> > * spd5118-[0-3]
> > * Your proposal from above
> > * nvmem[0-3] (default handling)
> > * 0-0050-[0-3]
> > 
> > Also can't a user of the eeprom already figure out which kind of module
> > it is by looking at the eeprom contents?
> > The first few bytes used for that seem to be compatible between at least
> > DDR4 and DDR5.
> > 
> > So using plain spd[1-4] could be enough.
> 
> This could cause problems when DDR6 arrives.
> Personally i would prefer the spd5118-X (NVMEM_DEVID_AUTO) format.

I have the impression that the eeprom layouts are designed to be
forward and backward compatible.

If a non-DDR5-aware parser reads the contents of a DDR5 eeprom it will
fail the CRC check, so there can be no accidental misinterpretation.
(Because the CRC'ed area is larger and the CRC is at another location)

On the other hand the first bytes of DDR4 and DDR5 are compatible, so
even an unaware parser can recognize that a SPD eeprom is being read and
which DIMM type and specification revision it is.

This seems intentional and therefore should also hold true for DDR5 to DDR6.


Thomas