Re: [PATCH i2c-tools v3 1/8] decode-dimms: Implement DDR5 checksum parsing

["Brigham Campbell" <me@brighamcampbell.com>]

Hello!

I'm cross-referencing the code you've written with JEDEC Standard No.
400-5D.01. If I have any questions, they will likely be because of
perceived discrepancies between your code and the standard. Please know
the following:

 * I'm not very familiar with perl
 * I'm not familiar with this JEDEC standard
 * I have compiled your code, but have been unable to test it

Please treat my feedback with healthy skepticism. Keep the above in mind
when reviewing any feedback I have on other patches in this series.

On Mon Jun 29, 2026 at 1:10 AM MDT, Stephen Horvath wrote:
> @@ -2402,7 +2403,12 @@ sub spd_sizes($)
>  	my $bytes = shift;
>  	my $type = $bytes->[2];
>  
> -	if ($type == 12 || $type == 14 || $type == 16 || $type == 17) {
> +	if ($type == 18 || $type == 19 || $type == 20 || $type == 21) {
> +		# DDR5
> +		my $spd_len = 256 * ((($bytes->[0] >> 4) & 7) + 1);

Shouldn't this actually be as follows (exponent and no +1):
		my $spd_len = 2 ** (7 + (($bytes->[0] >> 4) & 7));

Reading the datasheet, it looks like your algorithm only happens to work
for the particular value which we expect SPD5118 devices to have
(0b011). This makes me suspect that the algorithm for DDR4 may also be
wrong, but I don't have the datasheet pulled up...

See Section 8.1.1.

> +		my $used = $spd_len;
> +		return ($spd_len, $used);
> +	} elsif ($type == 12 || $type == 14 || $type == 16 || $type == 17) {
>  		# DDR4
>  		my $spd_len = 256 * (($bytes->[0] >> 4) & 7);
>  		my $used = 128 * ($bytes->[0] & 15);
> @@ -2684,6 +2698,12 @@ for my $i (0 .. $#dimm) {
>  		 $dimm[$i]->{chk_spd}, $dimm[$i]->{chk_calc}) =
>  			checksum(\@bytes);
>  	} else {
> +

No need for this empty line.

> +		# Check for DDR5 protocol type 18, 19, 20, 21 and size >= 512
> +		if ($bytes[2] >= 18 && $bytes[2] <= 21 && ($bytes[0] & 0x70) >= 0x30) {
> +			# DDR5's checksum is at 510-511
> +			push(@bytes, readspd(@bytes, 512, $dimm[$i]->{file}));
> +		}
>  		($dimm[$i]->{chk_label}, $dimm[$i]->{chk_valid},
>  		 $dimm[$i]->{chk_spd}, $dimm[$i]->{chk_calc}) =
>  			check_crc(\@bytes);

Thanks,
-- 
Brigham Campbell
https://brighamcampbell.com