Re: [PATCH 0/6] decode-dimms: Implement DDR5 decoding

[Kamil Aronowski <kamil.aronowski@3mdeb.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4116 bytes --]

On 11/14/24 07:37, Stephen Horvath via B4 Relay wrote:
> Hi, this series of patches adds DDR5 support to decode-dimms.
>
> I'm not too experienced with perl or the JEDEC specs, so there's probably
> going to be some questionable choices here, but I'd love to hear
> feedback.

The patchset doesn't seem to work out-of-the-box. When running the patched
`decode-dimms`, an error is thrown:

```
$ sudo ./eeprom/decode-dimms
Cannot read /sys/bus/i2c/drivers/spd5118/16-0050/eeprom at ./eeprom/decode-dimms line 2940.
```

I've checked with more than one machine to confirm, that it's not an individual
case.

Modifying the patched script, so that it reads one byte in a loop does seem to
fix some things, though, at least for the first 128 iterations:

```diff
diff --git a/eeprom/decode-dimms b/eeprom/decode-dimms
index a6a1669..1ff6741 100755
--- a/eeprom/decode-dimms
+++ b/eeprom/decode-dimms
@@ -2937,13 +2937,18 @@ sub readspd($$$)
  		binmode HANDLE;
  		sysseek(HANDLE, $offset, SEEK_SET)
  			or die "Cannot seek $dimm_i/eeprom";
-		$read = sysread(HANDLE, my $eeprom, $size)
-				or die "Cannot read $dimm_i/eeprom";
+		#$read = sysread(HANDLE, my $eeprom, $size)
+		#		or die "Cannot read $dimm_i/eeprom";
+		my $read = '';
+		my $buffer;
+		while (sysread(HANDLE, $buffer, 1) == 1) {
+			$read .= $buffer;
+		}
  		close HANDLE;
  		if ($read < $size) {
  			print STDERR "WARNING: $dimm_i/eeprom is smaller than expected\n";
  		}
-		@bytes = unpack("C*", $eeprom);
+		@bytes = unpack("C*", $buffer);
  	} else {
  		# Kernel 2.4 with procfs
  		for my $i (0 .. ($size-1)/16) {
```

Nevertheless, it still doesn't work as expected. A (trimmed) listing shows only
the following, except all the trimmed warnings:

```
Use of uninitialized value in numeric eq (==) at ./eeprom/decode-dimms line 2945.
Argument "0^P^R^C^D\0 b\0\0\0\0M-^P^B\0\0\0\0\0\0e^A�^Cr�\0\0\0\0,..." isn't numeric in numeric lt (<) at ./eeprom/decode-dimms line 2948.
WARNING: /sys/bus/i2c/drivers/spd5118/20-0050/eeprom is smaller than expected
Use of uninitialized value in addition (+) at ./eeprom/decode-dimms line 2969.
Use of uninitialized value in addition (+) at ./eeprom/decode-dimms line 2969.
[...]
Use of uninitialized value $b in numeric eq (==) at ./eeprom/decode-dimms line 505.
Use of uninitialized value $b in numeric eq (==) at ./eeprom/decode-dimms line 506.
Use of uninitialized value $_[0] in sprintf at ./eeprom/decode-dimms line 2555.
Use of uninitialized value $_[1] in sprintf at ./eeprom/decode-dimms line 2555.
Use of uninitialized value $_[2] in sprintf at ./eeprom/decode-dimms line 2555.
Use of uninitialized value $_[3] in sprintf at ./eeprom/decode-dimms line 2555.
# decode-dimms version 4.4

Memory Serial Presence Detect Decoder
By Philip Edelbrock, Christian Zuckschwerdt, Burkart Lingner,
Jean Delvare, Trent Piepho and others


Decoding EEPROM: /sys/bus/i2c/drivers/spd5118/20-0050
Guessing DIMM is in                              bank 1
Kernel driver used                               spd5118

---=== SPD EEPROM Information ===---
EEPROM Checksum of bytes 0-62                    OK (0x00)
SPD Revision                                     Invalid
Fundamental Memory type                          Unknown (0x00)

---=== Manufacturing Information ===---
Manufacturer                                     Undefined
Part Number                                      Undefined


Number of SDRAM DIMMs detected and decoded: 1
```

> The first 4 patches (1, 2, 3, 4) add the essential information to
> decode-dimms.
>
> The next 2 patches (5, 6) haven't really been tested on hardware
> implementations so I'm happy for them to be dropped if they're not
> useful.

Was the patchset tested on any hardware or some other intended environment
outside of my use case? The latter is about the most recent revision of Fedora
Rawhide with the spd5118 module on various DDR5-equipped laptops.

-- 
Kamil Aronowski
Junior Embedded Firmware Engineer
GPG: 3510148A5CD67908
https://3mdeb.com | @3mdeb_com


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 5529 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]