[PATCH] Read SPD data from a hexdump (take 2)

[PATCH] Read SPD data from a hexdump (take 2)

[Trent Piepho]

This is updated version of the previous patch.  I realized that I used four
space indent in the last one.  I changed it to 8, and then mode the hexdump
reading code to another function as it was getting too indented.  Also, it
can now understand the format of "eeprog".

This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
file names to read SPD data from.  It can parse various hexdump formats,
such as the output from i2cdump and the util-linux and Busybox hexdump
progams run on a sysfs eeprom file.

Useful for decoding SPD data that you cut and pasted from a manufacturer's
website or from a DIMM installed on an embeded system that does not have
perl/etc, but does have a serial console with busybox.

-----
--- i2c-tools/eeprom/decode-dimms.pl.orig	2008-03-16 14:19:21.000000000 -0700
+++ i2c-tools/eeprom/decode-dimms.pl	2008-03-16 14:27:15.000000000 -0700
@@ -46,7 +46,7 @@
 use strict;
 use POSIX;
 use Fcntl qw(:DEFAULT :seek);
-use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs
+use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs $use_hexdump
 	    @vendors %decode_callback $revision);

 $revision = '$Revision: 5089 $ ($Date: 2008-01-04 08:34:36 -0800 (Fri, 04 Jan 2008) $)';
@@ -1081,10 +1081,51 @@
 	printl $l, $temp;
 }

+# Read various hex dump style formats:	hexdump, hexdump -C, i2cdump, eeprog
+# note that normal 'hexdump' format on a little-endian system byte-swaps
+# words, using hexdump -C is better.
+sub readhexdump ($) {
+	my $addr = 0;
+	my $repstart = 0;
+	my @bytes;
+
+	open F, '<', $_[0] or die "Unable to open: $_[0]";
+	while(<F>) {
+		chomp;
+		if(/^\*$/) {
+			$repstart = $addr;
+			next;
+		}
+		/^(?:0000 )?([a-fA-F\d]{2,7}):? ((:?[a-fA-F\d]{4}\s*){8}|(:?[a-fA-F\d]{2}\s*){16})/ ||
+		/^(?:0000 )?([a-fA-F\d]{2,7}):?\s*$/ or die "Can't parse data";
+		$addr = hex $1;
+		if ($repstart) {
+			@bytes[$repstart .. ($addr-1)] = ($bytes[$repstart-1]) x ($addr-$repstart);
+			$repstart = 0;
+		}
+		last unless defined $2;
+		foreach (split(/\s+/, $2)) {
+			if(/^(..)(..)$/) {
+				$bytes[$addr++] = hex($1);
+				$bytes[$addr++] = hex($2);
+			} elsif(/^(..)$/) {
+				$bytes[$addr++] = hex($1);
+			} else {
+				print "Can't parse hex word '$_'\n";
+			}
+		}
+	}
+	close F;
+	return @bytes;
+}
+
 sub readspd64 ($$) { # reads 64 bytes from SPD-EEPROM
 	my ($offset, $dimm_i) = @_;
 	my @bytes;
-	if ($use_sysfs) {
+	if ($use_hexdump) {
+		@bytes = readhexdump($dimm_i);
+		return @bytes[$offset..($offset+63)];
+	} elsif ($use_sysfs) {
 		# Kernel 2.6 with sysfs
 		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
 			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
@@ -1103,20 +1144,25 @@
 	return @bytes;
 }

+my @dimm_list;
+
 for (@ARGV) {
     if (/-h/) {
-		print "Usage: $0 [-c] [-f [-b]]\n",
+		print "Usage: $0 [-c] [-f [-b]] [-x file [files..]]\n",
 			"       $0 -h\n\n",
 			"  -f, --format            print nice html output\n",
 			"  -b, --bodyonly          don't print html header\n",
 			"                          (useful for postprocessing the output)\n",
 			"  -c, --checksum          decode completely even if checksum fails\n",
+			"  -x,                     Read data from hexdump files\n",
 			"  -h, --help              display this usage summary\n";
 		exit;
     }
     $opt_html = 1 if (/-f/);
     $opt_bodyonly = 1 if (/-b/);
     $opt_igncheck = 1 if (/-c/);
+    $use_hexdump = 1 if(/-x/);
+    push @dimm_list, $_ if($use_hexdump && !/^-[fbcx]$/);
 }
 $opt_body = $opt_html && ! $opt_bodyonly;

@@ -1136,21 +1182,24 @@


 my $dimm_count=0;
-my @dimm_list;
 my $dir;
-if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
-else { $dir = '/proc/sys/dev/sensors'; }
-if (-d $dir) {
-	@dimm_list = split(/\s+/, `ls $dir`);
-} elsif (! -d '/sys/module/eeprom') {
-	print "No EEPROM found, are you sure the eeprom module is loaded?\n";
-	exit;
+
+if (!$use_hexdump) {
+	if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
+	else { $dir = '/proc/sys/dev/sensors'; }
+	if (-d $dir) {
+		@dimm_list = split(/\s+/, `ls $dir`);
+	} elsif (! -d '/sys/module/eeprom') {
+		print "No EEPROM found, are you sure the eeprom module is loaded?\n";
+		exit;
+	}
 }

 for my $i ( 0 .. $#dimm_list ) {
 	$_=$dimm_list[$i];
 	if (($use_sysfs && /^\d+-\d+$/)
-	 || (!$use_sysfs && /^eeprom-/)) {
+	 || (!$use_sysfs&& /^eeprom-/)
+	 || $use_hexdump) {
 		my @bytes = readspd64(0, $dimm_list[$i]);
 		my $dimm_checksum = 0;
 		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
@@ -1160,15 +1209,18 @@
 		$dimm_count++;

 		print "<b><u>" if $opt_html;
-		printl2 "\n\nDecoding EEPROM", ($use_sysfs ?
+		printl2 "\n\nDecoding EEPROM",
+		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
 			"/sys/bus/i2c/drivers/eeprom/$dimm_list[$i]" :
 			"/proc/sys/dev/sensors/$dimm_list[$i]");
 		print "</u></b>" if $opt_html;
 		print "<table border=1>\n" if $opt_html;
-		if (($use_sysfs && /^[^-]+-([^-]+)$/)
-		 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
-			my $dimm_num=$1 - 49;
-			printl "Guessing DIMM is in", "bank $dimm_num";
+		if (!$use_hexdump) {
+			if (($use_sysfs && /^[^-]+-([^-]+)$/)
+			 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
+				my $dimm_num=$1 - 49;
+				printl "Guessing DIMM is in", "bank $dimm_num";
+			}
 		}

 # Decode first 3 bytes (0-2)

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Jean Delvare]

Hi Trent,

On Sun, 16 Mar 2008 14:33:23 -0700 (PDT), Trent Piepho wrote:
> This is updated version of the previous patch.  I realized that I used four
> space indent in the last one.  I changed it to 8, and then mode the hexdump
> reading code to another function as it was getting too indented.  Also, it
> can now understand the format of "eeprog".
> 
> This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
> file names to read SPD data from.  It can parse various hexdump formats,
> such as the output from i2cdump and the util-linux and Busybox hexdump
> progams run on a sysfs eeprom file.
> 
> Useful for decoding SPD data that you cut and pasted from a manufacturer's
> website or from a DIMM installed on an embeded system that does not have
> perl/etc, but does have a serial console with busybox.

FYI, we already have a way to achieve this, using the i2c-stub kernel
driver and the standard i2c tools. For simple cases, the
i2c-stub-from-dump helper script (in the i2c-tools SVN repository [1])
automates almost everything. That's what I used to test your new DDR2
timing decoding code, as my workstation doesn't have DDR2 memory.

The main drawback compared to your approach is that one needs root
access to the development system, but in practice I doubt that this is
a problem. The advantage is that it works without any change to the
application (e.g. decode-dimms) so all applications are supported right
away.

i2c-stub-from-dump is currently limited in the dump format it supports
(it only takes byte and word dumps from i2cdump) but extending it to
support more dump formats would be trivial. We could even extend it to
support binary input.

[1] http://www.lm-sensors.org/browser/i2c-tools/trunk/stub

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Trent Piepho]

On Thu, 20 Mar 2008, Jean Delvare wrote:
> Hi Trent,
>
> On Sun, 16 Mar 2008 14:33:23 -0700 (PDT), Trent Piepho wrote:
> > This is updated version of the previous patch.  I realized that I used four
> > space indent in the last one.  I changed it to 8, and then mode the hexdump
> > reading code to another function as it was getting too indented.  Also, it
> > can now understand the format of "eeprog".
> >
> > This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
> > file names to read SPD data from.  It can parse various hexdump formats,
> > such as the output from i2cdump and the util-linux and Busybox hexdump
> > progams run on a sysfs eeprom file.
> >
> > Useful for decoding SPD data that you cut and pasted from a manufacturer's
> > website or from a DIMM installed on an embeded system that does not have
> > perl/etc, but does have a serial console with busybox.
>
> FYI, we already have a way to achieve this, using the i2c-stub kernel
> driver and the standard i2c tools. For simple cases, the
> i2c-stub-from-dump helper script (in the i2c-tools SVN repository [1])
> automates almost everything. That's what I used to test your new DDR2
> timing decoding code, as my workstation doesn't have DDR2 memory.
>
> The main drawback compared to your approach is that one needs root
> access to the development system, but in practice I doubt that this is
> a problem. The advantage is that it works without any change to the
> application (e.g. decode-dimms) so all applications are supported right
> away.

It's also seriously inconvenient to have to compile and install a kernel
module just to decode some SPD data.  You can also only decode one file at
a time, since you can only load a single i2c-stub module.  It also requires
one to be running Linux, a new enough kernel that has i2c-stub, and have
the kernel source if your distribution didn't come with i2c-stub (we have
people using Linux vmware images that aren't loaded with all this extra
stuff).  And there is no way to _not_ decode all the other DIMMs you have
in your system at the same time as one of files you want to inspect.

And of course one needs root access.  Doesn't that seem a little
ridiculous, that one needs to be root to decode a spd data file?  I can
hear the windows developers mocking me already.  "I can't decode SDP data
with your lunix tools because I need root access?  That's stupid!  My
windows tools for decoding spd don't need to be run as administrator!  Do
you need to be root to uncompress zip files?"

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Jean Delvare]

On Thu, 20 Mar 2008 07:59:37 -0700 (PDT), Trent Piepho wrote:
> On Thu, 20 Mar 2008, Jean Delvare wrote:
> > FYI, we already have a way to achieve this, using the i2c-stub kernel
> > driver and the standard i2c tools. For simple cases, the
> > i2c-stub-from-dump helper script (in the i2c-tools SVN repository [1])
> > automates almost everything. That's what I used to test your new DDR2
> > timing decoding code, as my workstation doesn't have DDR2 memory.
> >
> > The main drawback compared to your approach is that one needs root
> > access to the development system, but in practice I doubt that this is
> > a problem. The advantage is that it works without any change to the
> > application (e.g. decode-dimms) so all applications are supported right
> > away.
> 
> It's also seriously inconvenient to have to compile and install a kernel
> module just to decode some SPD data.  You can also only decode one file at
> a time, since you can only load a single i2c-stub module.

In fact you can decode up to 8 files at once (since kernel 2.6.24), as
i2c-stub can create up to 10 virtual devices (could be increased if
needed) and the eeprom driver attaches to devices at addresses
0x50-0x57. We could also create more than one I2C adapter in i2c-stub
if needed.

> It also requires
> one to be running Linux, a new enough kernel that has i2c-stub, and have
> the kernel source if your distribution didn't come with i2c-stub (we have
> people using Linux vmware images that aren't loaded with all this extra
> stuff).  And there is no way to _not_ decode all the other DIMMs you have
> in your system at the same time as one of files you want to inspect.

You can unload the on-board SMBus adapter driver, that's what I did to
test your DDR2 decoding patch. This might not be an option if there are
other chips on the SMBus and you can't live without them, though.

> 
> And of course one needs root access.  Doesn't that seem a little
> ridiculous, that one needs to be root to decode a spd data file?  I can
> hear the windows developers mocking me already.  "I can't decode SDP data
> with your lunix tools because I need root access?  That's stupid!  My
> windows tools for decoding spd don't need to be run as administrator!  Do
> you need to be root to uncompress zip files?"

I fail to see what Windows developers [1] have to do in the picture.
Either the requirement to be root for this operation is acceptable to
us in practice, or it is not. If it isn't, we fix it, for example by
applying the patch you submitted. What people NOT using the code in
question think about its usage requirements, I couldn't care less.

I didn't mean to suggest that your patch was not welcome. I was only
mentioning an alternative which is already implemented, for your
information (thus the leading "FYI" in my reply.) Of course i2c-stub is
not perfect for the job, it was not designed for it. i2c-stub was
designed to allow I2C device emulation at kernel level. Using it for a
purely user-space job as I suggested is a side use.

-- 
Jean Delvare

[1] Especially when your Windows developer friend is most probably
always logged as Administrator anyway because there are so many things
that don't work properly as a regular user ;)


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Jean Delvare]

Hi Trent,

On Sun, 16 Mar 2008 14:33:23 -0700 (PDT), Trent Piepho wrote:
> This is updated version of the previous patch.  I realized that I used four
> space indent in the last one.  I changed it to 8, and then mode the hexdump
> reading code to another function as it was getting too indented.  Also, it
> can now understand the format of "eeprog".
> 
> This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
> file names to read SPD data from.  It can parse various hexdump formats,
> such as the output from i2cdump and the util-linux and Busybox hexdump
> progams run on a sysfs eeprom file.
> 
> Useful for decoding SPD data that you cut and pasted from a manufacturer's
> website or from a DIMM installed on an embeded system that does not have
> perl/etc, but does have a serial console with busybox.

Sounds like a good idea. Review:

> 
> -----
> --- i2c-tools/eeprom/decode-dimms.pl.orig	2008-03-16 14:19:21.000000000 -0700
> +++ i2c-tools/eeprom/decode-dimms.pl	2008-03-16 14:27:15.000000000 -0700
> @@ -46,7 +46,7 @@
>  use strict;
>  use POSIX;
>  use Fcntl qw(:DEFAULT :seek);
> -use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs
> +use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs $use_hexdump
>  	    @vendors %decode_callback $revision);
> 
>  $revision = '$Revision: 5089 $ ($Date: 2008-01-04 08:34:36 -0800 (Fri, 04 Jan 2008) $)';
> @@ -1081,10 +1081,51 @@
>  	printl $l, $temp;
>  }
> 
> +# Read various hex dump style formats:	hexdump, hexdump -C, i2cdump, eeprog

Space instead of tab after the colon.

> +# note that normal 'hexdump' format on a little-endian system byte-swaps
> +# words, using hexdump -C is better.

The function below chokes on the header line of i2cdump output:

<file>
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 80 08 08 0e 0b 60 48 00 05 50 60 02 82 04 04 00    ?????`H.?P`????.
10: ...
</file>

Can't parse data at eeprom/decode-dimms.pl line 1119, <F> line 1.

Of course I can delete this first line from the file and then it works,
but it would be better if the function could simply ignore this header
line.

> +sub readhexdump ($) {

Coding style: opening curly brace goes on the next line. I'd also
suggest renaming the function to read_hexdump.

> +	my $addr = 0;
> +	my $repstart = 0;
> +	my @bytes;
> +
> +	open F, '<', $_[0] or die "Unable to open: $_[0]";
> +	while(<F>) {

Coding style: space after while.

> +		chomp;
> +		if(/^\*$/) {

Coding style: space after if, and again several times below.

> +			$repstart = $addr;
> +			next;
> +		}
> +		/^(?:0000 )?([a-fA-F\d]{2,7}):? ((:?[a-fA-F\d]{4}\s*){8}|(:?[a-fA-F\d]{2}\s*){16})/ ||
> +		/^(?:0000 )?([a-fA-F\d]{2,7}):?\s*$/ or die "Can't parse data";

You could use the /i regexp modifier so that you don't have to specify
both a-f and A-F each time.

> +		$addr = hex $1;
> +		if ($repstart) {
> +			@bytes[$repstart .. ($addr-1)] = ($bytes[$repstart-1]) x ($addr-$repstart);

You repeat the same byte for all the omitted block... this isn't how I
read man hexdump(1). As I understand it, you are supposed to repeat the
last line, not the last byte.

Alternatively you could drop support for this special format, which
doesn't make much sense for a 256-byte EEPROM if you ask me. And in
practice the only cases where it could happen is for rows of 0x00s or
0xffs - it doesn't make a difference if you omit them completely.

> +			$repstart = 0;
> +		}
> +		last unless defined $2;
> +		foreach (split(/\s+/, $2)) {
> +			if(/^(..)(..)$/) {
> +				$bytes[$addr++] = hex($1);
> +				$bytes[$addr++] = hex($2);

I'm unsure if we want to support this ambiguous format. You assume
big-endian byte order here. Even though you document it in the comment
above, users might not read the source code and so might feed the
script with little-endian byte order data and get noise out of the
script.

> +			} elsif(/^(..)$/) {
> +				$bytes[$addr++] = hex($1);
> +			} else {
> +				print "Can't parse hex word '$_'\n";

I fail to see how this could ever happen.

> +			}
> +		}
> +	}
> +	close F;
> +	return @bytes;
> +}
> +
>  sub readspd64 ($$) { # reads 64 bytes from SPD-EEPROM
>  	my ($offset, $dimm_i) = @_;
>  	my @bytes;
> -	if ($use_sysfs) {
> +	if ($use_hexdump) {
> +		@bytes = readhexdump($dimm_i);
> +		return @bytes[$offset..($offset+63)];
> +	} elsif ($use_sysfs) {
>  		# Kernel 2.6 with sysfs
>  		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
>  			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
> @@ -1103,20 +1144,25 @@
>  	return @bytes;
>  }
> 
> +my @dimm_list;

This one probably deserves being moved to the use vars declaration
above.

> +
>  for (@ARGV) {
>      if (/-h/) {
> -		print "Usage: $0 [-c] [-f [-b]]\n",
> +		print "Usage: $0 [-c] [-f [-b]] [-x file [files..]]\n",
>  			"       $0 -h\n\n",
>  			"  -f, --format            print nice html output\n",
>  			"  -b, --bodyonly          don't print html header\n",
>  			"                          (useful for postprocessing the output)\n",
>  			"  -c, --checksum          decode completely even if checksum fails\n",
> +			"  -x,                     Read data from hexdump files\n",
>  			"  -h, --help              display this usage summary\n";
>  		exit;
>      }
>      $opt_html = 1 if (/-f/);
>      $opt_bodyonly = 1 if (/-b/);
>      $opt_igncheck = 1 if (/-c/);
> +    $use_hexdump = 1 if(/-x/);
> +    push @dimm_list, $_ if($use_hexdump && !/^-[fbcx]$/);

In anticipation of more options being added in the future, I'd
use !/^-/ instead. Your variant above also fails if long options are
used.

>  }
>  $opt_body = $opt_html && ! $opt_bodyonly;
> 
> @@ -1136,21 +1182,24 @@
> 
> 
>  my $dimm_count=0;
> -my @dimm_list;
>  my $dir;
> -if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
> -else { $dir = '/proc/sys/dev/sensors'; }
> -if (-d $dir) {
> -	@dimm_list = split(/\s+/, `ls $dir`);
> -} elsif (! -d '/sys/module/eeprom') {
> -	print "No EEPROM found, are you sure the eeprom module is loaded?\n";
> -	exit;
> +
> +if (!$use_hexdump) {
> +	if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
> +	else { $dir = '/proc/sys/dev/sensors'; }
> +	if (-d $dir) {
> +		@dimm_list = split(/\s+/, `ls $dir`);
> +	} elsif (! -d '/sys/module/eeprom') {
> +		print "No EEPROM found, are you sure the eeprom module is loaded?\n";
> +		exit;
> +	}
>  }
> 
>  for my $i ( 0 .. $#dimm_list ) {
>  	$_=$dimm_list[$i];
>  	if (($use_sysfs && /^\d+-\d+$/)
> -	 || (!$use_sysfs && /^eeprom-/)) {
> +	 || (!$use_sysfs&& /^eeprom-/)

Coding style: space before &&.

> +	 || $use_hexdump) {
>  		my @bytes = readspd64(0, $dimm_list[$i]);
>  		my $dimm_checksum = 0;
>  		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
> @@ -1160,15 +1209,18 @@
>  		$dimm_count++;
> 
>  		print "<b><u>" if $opt_html;
> -		printl2 "\n\nDecoding EEPROM", ($use_sysfs ?
> +		printl2 "\n\nDecoding EEPROM",
> +		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
>  			"/sys/bus/i2c/drivers/eeprom/$dimm_list[$i]" :
>  			"/proc/sys/dev/sensors/$dimm_list[$i]");
>  		print "</u></b>" if $opt_html;
>  		print "<table border=1>\n" if $opt_html;
> -		if (($use_sysfs && /^[^-]+-([^-]+)$/)
> -		 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
> -			my $dimm_num=$1 - 49;
> -			printl "Guessing DIMM is in", "bank $dimm_num";
> +		if (!$use_hexdump) {
> +			if (($use_sysfs && /^[^-]+-([^-]+)$/)
> +			 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
> +				my $dimm_num=$1 - 49;
> +				printl "Guessing DIMM is in", "bank $dimm_num";
> +			}
>  		}
> 
>  # Decode first 3 bytes (0-2)

Please provide an updated patch and I'll commit it.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Trent Piepho]

On Sat, 22 Mar 2008, Jean Delvare wrote:
> The function below chokes on the header line of i2cdump output:
>
> <file>
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 80 08 08 0e 0b 60 48 00 05 50 60 02 82 04 04 00    ?????`H.?P`????.
> 10: ...
> </file>
>
> Can't parse data at eeprom/decode-dimms.pl line 1119, <F> line 1.
>
> Of course I can delete this first line from the file and then it works,
> but it would be better if the function could simply ignore this header
> line.

[...]
> You repeat the same byte for all the omitted block... this isn't how I
> read man hexdump(1). As I understand it, you are supposed to repeat the
> last line, not the last byte.
>
> Alternatively you could drop support for this special format, which
> doesn't make much sense for a 256-byte EEPROM if you ask me. And in
> practice the only cases where it could happen is for rows of 0x00s or
> 0xffs - it doesn't make a difference if you omit them completely.

I've added code to ignore header lines.  The code to parse the repeat line
syntax is there for the same reason:  hexdump produces that syntax when you
run it on an eeprom file.  Not parsing it requires editing the hexdumps by
hand, which is a lot more trouble than just deleting a one line header.

As far as I can tell, hexdump and od only produce the line repeat when
every byte in the line is the same, so repeating the last byte vs the last
16 bytes is effectively the same.  My goal wasn't to code some complex
"perfect" hex dump parser, but just to work correctly on the formats one
typically has at hand.

> You could use the /i regexp modifier so that you don't have to specify
> both a-f and A-F each time.

Good idea.

> > +		foreach (split(/\s+/, $2)) {
> > +			if(/^(..)(..)$/) {
> > +				$bytes[$addr++] = hex($1);
> > +				$bytes[$addr++] = hex($2);
>
> I'm unsure if we want to support this ambiguous format. You assume
> big-endian byte order here. Even though you document it in the comment
> above, users might not read the source code and so might feed the
> script with little-endian byte order data and get noise out of the
> script.

I've added some better documentation to the help output.  It's the default
format of "hexdump" and I'm not sure if old versions of busybox can produce
the -C format when compiled with certain options.  I think if you turn on
hexdump but not od, you get a limited version that can't do everything.

> > +			} elsif(/^(..)$/) {
> > +				$bytes[$addr++] = hex($1);
> > +			} else {
> > +				print "Can't parse hex word '$_'\n";
>
> I fail to see how this could ever happen.

It was just defensive programming.  In case my regexs to parse the line
were less than perfect, maybe it would get caught here.

> >      $opt_html = 1 if (/-f/);
> >      $opt_bodyonly = 1 if (/-b/);
> >      $opt_igncheck = 1 if (/-c/);
> > +    $use_hexdump = 1 if(/-x/);
> > +    push @dimm_list, $_ if($use_hexdump && !/^-[fbcx]$/);
>
> In anticipation of more options being added in the future, I'd
> use !/^-/ instead. Your variant above also fails if long options are
> used.

Fixed.  Note that all the option regexes should be changed to something
like '/^-?-f/'.  Otherwise a filename like hys72d32500gr-7-b will trigger
the $opt_bodyonly option.  (that's a real example, I named my eeprom dumps
after the part number, but in caps)

---------------------------------------------------------------------------
This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
file names to read SPD data from.  It can parse various hexdump formats,
such as the output from i2cdump and the util-linux and Busybox hexdump
progams run on a sysfs eeprom file.

Useful for decoding SPD data that you cut and pasted from a manufacturer's
website or from a DIMM installed on an embeded system that does not have
perl/etc, but does have a serial console with busybox.

---
Index: eeprom/decode-dimms.pl
===================================================================
--- eeprom/decode-dimms.pl	(revision 5156)
+++ eeprom/decode-dimms.pl	(working copy)
@@ -46,8 +46,8 @@
 use strict;
 use POSIX;
 use Fcntl qw(:DEFAULT :seek);
-use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs
-	    @vendors %decode_callback $revision);
+use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs $use_hexdump
+	    @vendors %decode_callback $revision @dimm_list);

 $revision = '$Revision$ ($Date$)';
 $revision =~ s/\$\w+: (.*?) \$/$1/g;
@@ -1100,10 +1100,59 @@
 	printl $l, $temp;
 }

+# Read various hex dump style formats: hexdump, hexdump -C, i2cdump, eeprog
+# note that normal 'hexdump' format on a little-endian system byte-swaps
+# words, using hexdump -C is better.
+sub read_hexdump ($)
+{
+	my $addr = 0;
+	my $repstart = 0;
+	my @bytes;
+	my $header = 1;
+
+	open F, '<', $_[0] or die "Unable to open: $_[0]";
+	while (<F>) {
+		my $ok = 1;
+
+		chomp;
+		if (/^\*$/) {
+			$repstart = $addr;
+			next;
+		}
+		/^(?:0000 )?([a-f\d]{2,7}):? ((:?[a-f\d]{4}\s*){8}|(:?[a-f\d]{2}\s*){16})/i ||
+		/^(?:0000 )?([a-f\d]{2,7}):?\s*$/i or $ok = 0;
+		next if (!$ok && $header);		# skip leading unparsed lines
+
+		$ok or die "Unable to parse input";
+		$header = 0;
+
+		$addr = hex $1;
+		if ($repstart) {
+			@bytes[$repstart .. ($addr-1)] = ($bytes[$repstart-1]) x ($addr-$repstart);
+			$repstart = 0;
+		}
+		last unless defined $2;
+		foreach (split(/\s+/, $2)) {
+			if (/^(..)(..)$/) {
+				$bytes[$addr++] = hex($1);
+				$bytes[$addr++] = hex($2);
+			} else {
+				/^(..)$/;
+				$bytes[$addr++] = hex($1);
+			}
+		}
+	}
+	close F;
+	return @bytes;
+}
+
 sub readspd64 ($$) { # reads 64 bytes from SPD-EEPROM
 	my ($offset, $dimm_i) = @_;
 	my @bytes;
-	if ($use_sysfs) {
+	if ($use_hexdump) {
+		@bytes = read_hexdump($dimm_i);
+		return @bytes[$offset..($offset+63)];
+	} elsif ($use_sysfs) {
 		# Kernel 2.6 with sysfs
 		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
 			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
@@ -1123,19 +1172,29 @@
 }

 for (@ARGV) {
-    if (/-h/) {
-		print "Usage: $0 [-c] [-f [-b]]\n",
+    if (/^-?-h/) {
+		print "Usage: $0 [-c] [-f [-b]] [-x file [files..]]\n",
 			"       $0 -h\n\n",
 			"  -f, --format            print nice html output\n",
 			"  -b, --bodyonly          don't print html header\n",
 			"                          (useful for postprocessing the output)\n",
 			"  -c, --checksum          decode completely even if checksum fails\n",
+			"  -x,                     Read data from hexdump files\n",
 			"  -h, --help              display this usage summary\n";
+		print <<"EOF";
+
+Hexdumps can be the output from hexdump, hexdump -C, i2cdump, eeprog and
+likely many other progams producing hex dumps of one kind or another.  Note
+that the default output of "hexdump" will be byte-swapped on little-endian
+systems.  It is better to use "hexdump -C", which is not ambiguous.
+EOF
 		exit;
     }
-    $opt_html = 1 if (/-f/);
-    $opt_bodyonly = 1 if (/-b/);
-    $opt_igncheck = 1 if (/-c/);
+    $opt_html = 1 if (/^-?-f/);
+    $opt_bodyonly = 1 if (/^-?-b/);
+    $opt_igncheck = 1 if (/^-?-c/);
+    $use_hexdump = 1 if (/^-x/);
+    push @dimm_list, $_ if ($use_hexdump && !/^-/);
 }
 $opt_body = $opt_html && ! $opt_bodyonly;

@@ -1155,21 +1214,23 @@


 my $dimm_count = 0;
-my @dimm_list;
 my $dir;
-if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
-else { $dir = '/proc/sys/dev/sensors'; }
-if (-d $dir) {
-	@dimm_list = split(/\s+/, `ls $dir`);
-} elsif (! -d '/sys/module/eeprom') {
-	print "No EEPROM found, are you sure the eeprom module is loaded?\n";
-	exit;
+if (!$use_hexdump) {
+	if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
+	else { $dir = '/proc/sys/dev/sensors'; }
+	if (-d $dir) {
+		@dimm_list = split(/\s+/, `ls $dir`);
+	} elsif (! -d '/sys/module/eeprom') {
+		print "No EEPROM found, are you sure the eeprom module is loaded?\n";
+		exit;
+	}
 }

 for my $i ( 0 .. $#dimm_list ) {
 	$_ = $dimm_list[$i];
 	if (($use_sysfs && /^\d+-\d+$/)
-	 || (!$use_sysfs && /^eeprom-/)) {
+	 || (!$use_sysfs && /^eeprom-/)
+	 || $use_hexdump) {
 		my @bytes = readspd64(0, $dimm_list[$i]);
 		my $dimm_checksum = 0;
 		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
@@ -1179,15 +1240,18 @@
 		$dimm_count++;

 		print "<b><u>" if $opt_html;
-		printl2 "\n\nDecoding EEPROM", ($use_sysfs ?
+		printl2 "\n\nDecoding EEPROM",
+		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
 			"/sys/bus/i2c/drivers/eeprom/$dimm_list[$i]" :
 			"/proc/sys/dev/sensors/$dimm_list[$i]");
 		print "</u></b>" if $opt_html;
 		print "<table border=1>\n" if $opt_html;
-		if (($use_sysfs && /^[^-]+-([^-]+)$/)
-		 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
-			my $dimm_num = $1 - 49;
-			printl "Guessing DIMM is in", "bank $dimm_num";
+		if (!$use_hexdump) {
+			if (($use_sysfs && /^[^-]+-([^-]+)$/)
+			 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
+				my $dimm_num = $1 - 49;
+				printl "Guessing DIMM is in", "bank $dimm_num";
+			}
 		}

 # Decode first 3 bytes (0-2)

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Trent Piepho]

The regex for parsing hexdump -C was wrong.  I remember fixing that before,
so I think some old code got in these patches.  It's a real pain to deal
with a stack of outstanding patches with svn and merge against conflicts
from upstream.


This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
file names to read SPD data from.  It can parse various hexdump formats,
such as the output from i2cdump and the util-linux and Busybox hexdump
progams run on a sysfs eeprom file.

Useful for decoding SPD data that you cut and pasted from a manufacturer's
website or from a DIMM installed on an embeded system that does not have
perl/etc, but does have a serial console with busybox.

---
Index: eeprom/decode-dimms.pl
===================================================================
--- eeprom/decode-dimms.pl	(revision 5156)
+++ eeprom/decode-dimms.pl	(working copy)
@@ -46,8 +46,8 @@
 use strict;
 use POSIX;
 use Fcntl qw(:DEFAULT :seek);
-use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs
-	    @vendors %decode_callback $revision);
+use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs $use_hexdump
+	    @vendors %decode_callback $revision @dimm_list);

 $revision = '$Revision$ ($Date$)';
 $revision =~ s/\$\w+: (.*?) \$/$1/g;
@@ -1100,10 +1100,60 @@
 	printl $l, $temp;
 }

+# Read various hex dump style formats: hexdump, hexdump -C, i2cdump, eeprog
+# note that normal 'hexdump' format on a little-endian system byte-swaps
+# words, using hexdump -C is better.
+sub read_hexdump ($)
+{
+	my $addr = 0;
+	my $repstart = 0;
+	my @bytes;
+	my $header = 1;
+
+	open F, '<', $_[0] or die "Unable to open: $_[0]";
+	while (<F>) {
+		my $ok = 1;
+
+		chomp;
+		if (/^\*$/) {
+			$repstart = $addr;
+			next;
+		}
+		/^(?:0000 )?([a-f\d]{2,8}):?\s+((:?[a-f\d]{4}\s*){8}|(:?[a-f\d]{2}\s*){16})/i ||
+		/^(?:0000 )?([a-f\d]{2,8}):?\s*$/i or $ok = 0;
+		next if (!$ok && $header);		# skip leading unparsed lines
+
+		$ok or die "Unable to parse input";
+		$header = 0;
+
+		$addr = hex $1;
+		if ($repstart) {
+			@bytes[$repstart .. ($addr-1)] = ($bytes[$repstart-1]) x ($addr-$repstart);
+			$repstart = 0;
+		}
+		last unless defined $2;
+		foreach (split(/\s+/, $2)) {
+			if (/^(..)(..)$/) {
+				$bytes[$addr++] = hex($1);
+				$bytes[$addr++] = hex($2);
+			} else {
+				/^(..)$/;
+				$bytes[$addr++] = hex($1);
+			}
+		}
+	}
+	close F;
+	$header and die "Unable to parse any data from hexdump '$_[0]'";
+	return @bytes;
+}
+
 sub readspd64 ($$) { # reads 64 bytes from SPD-EEPROM
 	my ($offset, $dimm_i) = @_;
 	my @bytes;
-	if ($use_sysfs) {
+	if ($use_hexdump) {
+		@bytes = read_hexdump($dimm_i);
+		return @bytes[$offset..($offset+63)];
+	} elsif ($use_sysfs) {
 		# Kernel 2.6 with sysfs
 		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
 			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
@@ -1123,19 +1173,29 @@
 }

 for (@ARGV) {
-    if (/-h/) {
-		print "Usage: $0 [-c] [-f [-b]]\n",
+    if (/^-?-h/) {
+		print "Usage: $0 [-c] [-f [-b]] [-x file [files..]]\n",
 			"       $0 -h\n\n",
 			"  -f, --format            print nice html output\n",
 			"  -b, --bodyonly          don't print html header\n",
 			"                          (useful for postprocessing the output)\n",
 			"  -c, --checksum          decode completely even if checksum fails\n",
+			"  -x,                     Read data from hexdump files\n",
 			"  -h, --help              display this usage summary\n";
+		print <<"EOF";
+
+Hexdumps can be the output from hexdump, hexdump -C, i2cdump, eeprog and
+likely many other progams producing hex dumps of one kind or another.  Note
+that the default output of "hexdump" will be byte-swapped on little-endian
+systems.  It is better to use "hexdump -C", which is not ambiguous.
+EOF
 		exit;
     }
-    $opt_html = 1 if (/-f/);
-    $opt_bodyonly = 1 if (/-b/);
-    $opt_igncheck = 1 if (/-c/);
+    $opt_html = 1 if (/^-?-f/);
+    $opt_bodyonly = 1 if (/^-?-b/);
+    $opt_igncheck = 1 if (/^-?-c/);
+    $use_hexdump = 1 if (/^-x/);
+    push @dimm_list, $_ if ($use_hexdump && !/^-/);
 }
 $opt_body = $opt_html && ! $opt_bodyonly;

@@ -1155,21 +1215,23 @@


 my $dimm_count = 0;
-my @dimm_list;
 my $dir;
-if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
-else { $dir = '/proc/sys/dev/sensors'; }
-if (-d $dir) {
-	@dimm_list = split(/\s+/, `ls $dir`);
-} elsif (! -d '/sys/module/eeprom') {
-	print "No EEPROM found, are you sure the eeprom module is loaded?\n";
-	exit;
+if (!$use_hexdump) {
+	if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
+	else { $dir = '/proc/sys/dev/sensors'; }
+	if (-d $dir) {
+		@dimm_list = split(/\s+/, `ls $dir`);
+	} elsif (! -d '/sys/module/eeprom') {
+		print "No EEPROM found, are you sure the eeprom module is loaded?\n";
+		exit;
+	}
 }

 for my $i ( 0 .. $#dimm_list ) {
 	$_ = $dimm_list[$i];
 	if (($use_sysfs && /^\d+-\d+$/)
-	 || (!$use_sysfs && /^eeprom-/)) {
+	 || (!$use_sysfs && /^eeprom-/)
+	 || $use_hexdump) {
 		my @bytes = readspd64(0, $dimm_list[$i]);
 		my $dimm_checksum = 0;
 		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
@@ -1179,15 +1241,18 @@
 		$dimm_count++;

 		print "<b><u>" if $opt_html;
-		printl2 "\n\nDecoding EEPROM", ($use_sysfs ?
+		printl2 "\n\nDecoding EEPROM",
+		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
 			"/sys/bus/i2c/drivers/eeprom/$dimm_list[$i]" :
 			"/proc/sys/dev/sensors/$dimm_list[$i]");
 		print "</u></b>" if $opt_html;
 		print "<table border=1>\n" if $opt_html;
-		if (($use_sysfs && /^[^-]+-([^-]+)$/)
-		 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
-			my $dimm_num = $1 - 49;
-			printl "Guessing DIMM is in", "bank $dimm_num";
+		if (!$use_hexdump) {
+			if (($use_sysfs && /^[^-]+-([^-]+)$/)
+			 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
+				my $dimm_num = $1 - 49;
+				printl "Guessing DIMM is in", "bank $dimm_num";
+			}
 		}

 # Decode first 3 bytes (0-2)

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Jean Delvare]

Hi Trent,

On Sat, 22 Mar 2008 16:59:05 -0700 (PDT), Trent Piepho wrote:
> The regex for parsing hexdump -C was wrong.  I remember fixing that before,
> so I think some old code got in these patches.  It's a real pain to deal
> with a stack of outstanding patches with svn and merge against conflicts
> from upstream.

Sorry about that, I should have delayed my cleanups until this patch
was merged, my bad.

> 
> 
> This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
> file names to read SPD data from.  It can parse various hexdump formats,
> such as the output from i2cdump and the util-linux and Busybox hexdump
> progams run on a sysfs eeprom file.
> 
> Useful for decoding SPD data that you cut and pasted from a manufacturer's
> website or from a DIMM installed on an embeded system that does not have
> perl/etc, but does have a serial console with busybox.
> 
> ---
> Index: eeprom/decode-dimms.pl
> ===================================================================
> --- eeprom/decode-dimms.pl	(revision 5156)
> +++ eeprom/decode-dimms.pl	(working copy)
> @@ -46,8 +46,8 @@
>  use strict;
>  use POSIX;
>  use Fcntl qw(:DEFAULT :seek);
> -use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs
> -	    @vendors %decode_callback $revision);
> +use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs $use_hexdump
> +	    @vendors %decode_callback $revision @dimm_list);
> 
>  $revision = '$Revision$ ($Date$)';
>  $revision =~ s/\$\w+: (.*?) \$/$1/g;
> @@ -1100,10 +1100,60 @@
>  	printl $l, $temp;
>  }
> 
> +# Read various hex dump style formats: hexdump, hexdump -C, i2cdump, eeprog
> +# note that normal 'hexdump' format on a little-endian system byte-swaps
> +# words, using hexdump -C is better.
> +sub read_hexdump ($)
> +{
> +	my $addr = 0;
> +	my $repstart = 0;
> +	my @bytes;
> +	my $header = 1;
> +
> +	open F, '<', $_[0] or die "Unable to open: $_[0]";
> +	while (<F>) {
> +		my $ok = 1;
> +
> +		chomp;
> +		if (/^\*$/) {
> +			$repstart = $addr;
> +			next;
> +		}
> +		/^(?:0000 )?([a-f\d]{2,8}):?\s+((:?[a-f\d]{4}\s*){8}|(:?[a-f\d]{2}\s*){16})/i ||
> +		/^(?:0000 )?([a-f\d]{2,8}):?\s*$/i or $ok = 0;
> +		next if (!$ok && $header);		# skip leading unparsed lines

I think you can do without $ok, just check for defined $1? Or use a
more standard if (m//) {} construct.

> +
> +		$ok or die "Unable to parse input";
> +		$header = 0;
> +
> +		$addr = hex $1;
> +		if ($repstart) {
> +			@bytes[$repstart .. ($addr-1)] = ($bytes[$repstart-1]) x ($addr-$repstart);

As said in my other reply, this doesn't match what hexdump and od
output.

> +			$repstart = 0;
> +		}
> +		last unless defined $2;
> +		foreach (split(/\s+/, $2)) {
> +			if (/^(..)(..)$/) {
> +				$bytes[$addr++] = hex($1);
> +				$bytes[$addr++] = hex($2);
> +			} else {
> +				/^(..)$/;
> +				$bytes[$addr++] = hex($1);

Why not just
				$bytes[$addr++] = hex;
?

> +			}
> +		}
> +	}
> +	close F;
> +	$header and die "Unable to parse any data from hexdump '$_[0]'";
> +	return @bytes;
> +}
> +
>  sub readspd64 ($$) { # reads 64 bytes from SPD-EEPROM
>  	my ($offset, $dimm_i) = @_;
>  	my @bytes;
> -	if ($use_sysfs) {
> +	if ($use_hexdump) {
> +		@bytes = read_hexdump($dimm_i);
> +		return @bytes[$offset..($offset+63)];
> +	} elsif ($use_sysfs) {
>  		# Kernel 2.6 with sysfs
>  		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
>  			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
> @@ -1123,19 +1173,29 @@
>  }
> 
>  for (@ARGV) {
> -    if (/-h/) {
> -		print "Usage: $0 [-c] [-f [-b]]\n",
> +    if (/^-?-h/) {
> +		print "Usage: $0 [-c] [-f [-b]] [-x file [files..]]\n",
>  			"       $0 -h\n\n",
>  			"  -f, --format            print nice html output\n",
>  			"  -b, --bodyonly          don't print html header\n",
>  			"                          (useful for postprocessing the output)\n",
>  			"  -c, --checksum          decode completely even if checksum fails\n",
> +			"  -x,                     Read data from hexdump files\n",
>  			"  -h, --help              display this usage summary\n";
> +		print <<"EOF";
> +
> +Hexdumps can be the output from hexdump, hexdump -C, i2cdump, eeprog and
> +likely many other progams producing hex dumps of one kind or another.  Note
> +that the default output of "hexdump" will be byte-swapped on little-endian
> +systems.  It is better to use "hexdump -C", which is not ambiguous.

Maybe state more explicitly that such little-endian dumps are not
supported?

> +EOF
>  		exit;
>      }
> -    $opt_html = 1 if (/-f/);
> -    $opt_bodyonly = 1 if (/-b/);
> -    $opt_igncheck = 1 if (/-c/);
> +    $opt_html = 1 if (/^-?-f/);
> +    $opt_bodyonly = 1 if (/^-?-b/);
> +    $opt_igncheck = 1 if (/^-?-c/);
> +    $use_hexdump = 1 if (/^-x/);
> +    push @dimm_list, $_ if ($use_hexdump && !/^-/);
>  }
>  $opt_body = $opt_html && ! $opt_bodyonly;
> 
> @@ -1155,21 +1215,23 @@
> 
> 
>  my $dimm_count = 0;
> -my @dimm_list;
>  my $dir;
> -if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
> -else { $dir = '/proc/sys/dev/sensors'; }
> -if (-d $dir) {
> -	@dimm_list = split(/\s+/, `ls $dir`);
> -} elsif (! -d '/sys/module/eeprom') {
> -	print "No EEPROM found, are you sure the eeprom module is loaded?\n";
> -	exit;
> +if (!$use_hexdump) {
> +	if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
> +	else { $dir = '/proc/sys/dev/sensors'; }
> +	if (-d $dir) {
> +		@dimm_list = split(/\s+/, `ls $dir`);
> +	} elsif (! -d '/sys/module/eeprom') {
> +		print "No EEPROM found, are you sure the eeprom module is loaded?\n";
> +		exit;
> +	}
>  }
> 
>  for my $i ( 0 .. $#dimm_list ) {
>  	$_ = $dimm_list[$i];
>  	if (($use_sysfs && /^\d+-\d+$/)
> -	 || (!$use_sysfs && /^eeprom-/)) {
> +	 || (!$use_sysfs && /^eeprom-/)
> +	 || $use_hexdump) {
>  		my @bytes = readspd64(0, $dimm_list[$i]);
>  		my $dimm_checksum = 0;
>  		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
> @@ -1179,15 +1241,18 @@
>  		$dimm_count++;
> 
>  		print "<b><u>" if $opt_html;
> -		printl2 "\n\nDecoding EEPROM", ($use_sysfs ?
> +		printl2 "\n\nDecoding EEPROM",
> +		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
>  			"/sys/bus/i2c/drivers/eeprom/$dimm_list[$i]" :
>  			"/proc/sys/dev/sensors/$dimm_list[$i]");
>  		print "</u></b>" if $opt_html;
>  		print "<table border=1>\n" if $opt_html;
> -		if (($use_sysfs && /^[^-]+-([^-]+)$/)
> -		 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
> -			my $dimm_num = $1 - 49;
> -			printl "Guessing DIMM is in", "bank $dimm_num";
> +		if (!$use_hexdump) {
> +			if (($use_sysfs && /^[^-]+-([^-]+)$/)
> +			 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
> +				my $dimm_num = $1 - 49;
> +				printl "Guessing DIMM is in", "bank $dimm_num";
> +			}
>  		}
> 
>  # Decode first 3 bytes (0-2)


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Jean Delvare]

Hi Trent,

On Sat, 22 Mar 2008 14:46:43 -0700 (PDT), Trent Piepho wrote:
> On Sat, 22 Mar 2008, Jean Delvare wrote:
> > You repeat the same byte for all the omitted block... this isn't how I
> > read man hexdump(1). As I understand it, you are supposed to repeat the
> > last line, not the last byte.
> >
> > Alternatively you could drop support for this special format, which
> > doesn't make much sense for a 256-byte EEPROM if you ask me. And in
> > practice the only cases where it could happen is for rows of 0x00s or
> > 0xffs - it doesn't make a difference if you omit them completely.
> 
> I've added code to ignore header lines.  The code to parse the repeat line
> syntax is there for the same reason:  hexdump produces that syntax when you
> run it on an eeprom file.  Not parsing it requires editing the hexdumps by
> hand, which is a lot more trouble than just deleting a one line header.

Maybe I did not express myself clearly enough. I didn't mean to stop
supporting the special "compact" format by choking on it, but by
simply ignoring the "*" line as you do for i2cdump's header line (and
filling the @bytes array with zeroes, which would be presumably simpler
than what you did.)

> As far as I can tell, hexdump and od only produce the line repeat when
> every byte in the line is the same, so repeating the last byte vs the last
> 16 bytes is effectively the same.

Your assumption is incorrect. You can test it easily:

khali@hyperion:~> dd if=/dev/random of=random16.bin bs=16 count=1
1+0 records in
1+0 records out
16 bytes (16 B) copied, 6.5427e-05 s, 245 kB/s
khali@hyperion:~> hexdump random16.bin
0000000 9d89 66d7 d2a5 9118 86b9 85c6 9454 cc8e
0000010
khali@hyperion:~> cat random16.bin random16.bin random16.bin random16.bin > random64.bin
khali@hyperion:~> hexdump random64.bin
0000000 9d89 66d7 d2a5 9118 86b9 85c6 9454 cc8e
*
0000040
khali@hyperion:~> od -x random64.bin
0000000 9d89 66d7 d2a5 9118 86b9 85c6 9454 cc8e
*
0000100
khali@hyperion:~>

And I agree that it doesn't make much sense (at least when od's -w
option isn't used) but that's the way it is.

> My goal wasn't to code some complex
> "perfect" hex dump parser, but just to work correctly on the formats one
> typically has at hand.

Fine with me, but either implement the "repeat" mode properly (it
shouldn't be that difficult), or don't implement it at all.
Implementing it in a way which doesn't match what hexdump and od output
doesn't make any sense to me.

> > (...)
> > I'm unsure if we want to support this ambiguous format. You assume
> > big-endian byte order here. Even though you document it in the comment
> > above, users might not read the source code and so might feed the
> > script with little-endian byte order data and get noise out of the
> > script.
> 
> I've added some better documentation to the help output.  It's the default

Good start, but I'd go further and warn the user at runtime when
parsing hexdumps using this format. For example:

Warning: Assuming hexdump uses big-endian byte order

> > (...)
> > In anticipation of more options being added in the future, I'd
> > use !/^-/ instead. Your variant above also fails if long options are
> > used.
> 
> Fixed.  Note that all the option regexes should be changed to something
> like '/^-?-f/'.  Otherwise a filename like hys72d32500gr-7-b will trigger
> the $opt_bodyonly option.  (that's a real example, I named my eeprom dumps
> after the part number, but in caps)

You are correct. Even better would be to explicitly match the short and
long options. I'll fix that later.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Trent Piepho]

On Sun, 23 Mar 2008, Jean Delvare wrote:
> Hi Trent,
>
> On Sat, 22 Mar 2008 14:46:43 -0700 (PDT), Trent Piepho wrote:
> > On Sat, 22 Mar 2008, Jean Delvare wrote:
> > > You repeat the same byte for all the omitted block... this isn't how I
> > > read man hexdump(1). As I understand it, you are supposed to repeat the
> > > last line, not the last byte.
> > >
> > > Alternatively you could drop support for this special format, which
> > > doesn't make much sense for a 256-byte EEPROM if you ask me. And in
> > > practice the only cases where it could happen is for rows of 0x00s or
> > > 0xffs - it doesn't make a difference if you omit them completely.
> >
> > I've added code to ignore header lines.  The code to parse the repeat line
> > syntax is there for the same reason:  hexdump produces that syntax when you
> > run it on an eeprom file.  Not parsing it requires editing the hexdumps by
> > hand, which is a lot more trouble than just deleting a one line header.
>
> Maybe I did not express myself clearly enough. I didn't mean to stop
> supporting the special "compact" format by choking on it, but by
> simply ignoring the "*" line as you do for i2cdump's header line (and
> filling the @bytes array with zeroes, which would be presumably simpler
> than what you did.)

It seems like doing that would be even worse than the almost correct way
I'm handing the repeat lines now.  Any hex dump that my code would parse
incorrectly would also be parsed incorrectly by ignoring repeat lines.
Plus some far more likely hexdumps would also be parsed incorrectly when
ignoring the repeat codes that are handled correctly with my code.

Correctly handling the repeat lines also means one can use the same code to
turn a hexdump back into a binary file.

I've improved the hexdump reading code to handle line repeats correctly.

> > I've added some better documentation to the help output.  It's the default
>
> Good start, but I'd go further and warn the user at runtime when
> parsing hexdumps using this format. For example:
>
> Warning: Assuming hexdump uses big-endian byte order

Ok, added that.

This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
file names to read SPD data from.  It can parse various hexdump formats, such
as the output from i2cdump, eeprog, and the util-linux and Busybox hexdump
progams run on a sysfs eeprom file.

Useful for decoding SPD data that you cut and pasted from a manufacturer's
website or from a DIMM installed on an embeded system that does not have
perl/etc, but does have a serial console with busybox.

---
Index: eeprom/decode-dimms.pl
===================================================================
--- eeprom/decode-dimms.pl	(revision 5156)
+++ eeprom/decode-dimms.pl	(working copy)
@@ -46,8 +46,8 @@
 use strict;
 use POSIX;
 use Fcntl qw(:DEFAULT :seek);
-use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs
-	    @vendors %decode_callback $revision);
+use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs $use_hexdump
+	    @vendors %decode_callback $revision @dimm_list);

 $revision = '$Revision$ ($Date$)';
 $revision =~ s/\$\w+: (.*?) \$/$1/g;
@@ -1100,10 +1100,63 @@
 	printl $l, $temp;
 }

+# Read various hex dump style formats: hexdump, hexdump -C, i2cdump, eeprog
+# note that normal 'hexdump' format on a little-endian system byte-swaps
+# words, using hexdump -C is better.
+sub read_hexdump ($)
+{
+	my $addr = 0;
+	my $repstart = 0;
+	my @bytes;
+	my $header = 1;
+	my $word = 0;
+
+	open F, '<', $_[0] or die "Unable to open: $_[0]";
+	while (<F>) {
+		my $ok = 1;
+
+		chomp;
+		if (/^\*$/) {
+			$repstart = $addr;
+			next;
+		}
+		/^(?:0000 )?([a-f\d]{2,8}):?\s+((:?[a-f\d]{4}\s*){8}|(:?[a-f\d]{2}\s*){16})/i ||
+		/^(?:0000 )?([a-f\d]{2,8}):?\s*$/i;
+		next if (!defined $1 && $header);		# skip leading unparsed lines
+
+		defined $1 or die "Unable to parse input";
+		$header = 0;
+
+		$addr = hex $1;
+		if ($repstart) {
+			@bytes[$repstart .. ($addr-1)] =
+				(@bytes[($repstart-16)..($repstart-1)]) x (($addr-$repstart)/16);
+			$repstart = 0;
+		}
+		last unless defined $2;
+		foreach (split(/\s+/, $2)) {
+			if (/^(..)(..)$/) {
+			        $word |= 1;
+				$bytes[$addr++] = hex($1);
+				$bytes[$addr++] = hex($2);
+			} else {
+				$bytes[$addr++] = hex($_);
+			}
+		}
+	}
+	close F;
+	$header and die "Unable to parse any data from hexdump '$_[0]'";
+	$word and printc "Warning: Assuming big-endian order 16-bit hex dump";
+	return @bytes;
+}
+
 sub readspd64 ($$) { # reads 64 bytes from SPD-EEPROM
 	my ($offset, $dimm_i) = @_;
 	my @bytes;
-	if ($use_sysfs) {
+	if ($use_hexdump) {
+		@bytes = read_hexdump($dimm_i);
+		return @bytes[$offset..($offset+63)];
+	} elsif ($use_sysfs) {
 		# Kernel 2.6 with sysfs
 		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
 			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
@@ -1123,19 +1176,30 @@
 }

 for (@ARGV) {
-    if (/-h/) {
-		print "Usage: $0 [-c] [-f [-b]]\n",
+    if (/^-?-h/) {
+		print "Usage: $0 [-c] [-f [-b]] [-x file [files..]]\n",
 			"       $0 -h\n\n",
 			"  -f, --format            print nice html output\n",
 			"  -b, --bodyonly          don't print html header\n",
 			"                          (useful for postprocessing the output)\n",
 			"  -c, --checksum          decode completely even if checksum fails\n",
+			"  -x,                     Read data from hexdump files\n",
 			"  -h, --help              display this usage summary\n";
+		print <<"EOF";
+
+Hexdumps can be the output from hexdump, hexdump -C, i2cdump, eeprog and
+likely many other progams producing hex dumps of one kind or another.  Note
+that the default output of "hexdump" will be byte-swapped on little-endian
+systems and will therefor not be parsed correctly.  It is better to use
+"hexdump -C", which is not ambiguous.
+EOF
 		exit;
     }
-    $opt_html = 1 if (/-f/);
-    $opt_bodyonly = 1 if (/-b/);
-    $opt_igncheck = 1 if (/-c/);
+    $opt_html = 1 if (/^-?-f/);
+    $opt_bodyonly = 1 if (/^-?-b/);
+    $opt_igncheck = 1 if (/^-?-c/);
+    $use_hexdump = 1 if (/^-x/);
+    push @dimm_list, $_ if ($use_hexdump && !/^-/);
 }
 $opt_body = $opt_html && ! $opt_bodyonly;

@@ -1155,21 +1219,23 @@


 my $dimm_count = 0;
-my @dimm_list;
 my $dir;
-if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
-else { $dir = '/proc/sys/dev/sensors'; }
-if (-d $dir) {
-	@dimm_list = split(/\s+/, `ls $dir`);
-} elsif (! -d '/sys/module/eeprom') {
-	print "No EEPROM found, are you sure the eeprom module is loaded?\n";
-	exit;
+if (!$use_hexdump) {
+	if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
+	else { $dir = '/proc/sys/dev/sensors'; }
+	if (-d $dir) {
+		@dimm_list = split(/\s+/, `ls $dir`);
+	} elsif (! -d '/sys/module/eeprom') {
+		print "No EEPROM found, are you sure the eeprom module is loaded?\n";
+		exit;
+	}
 }

 for my $i ( 0 .. $#dimm_list ) {
 	$_ = $dimm_list[$i];
 	if (($use_sysfs && /^\d+-\d+$/)
-	 || (!$use_sysfs && /^eeprom-/)) {
+	 || (!$use_sysfs && /^eeprom-/)
+	 || $use_hexdump) {
 		my @bytes = readspd64(0, $dimm_list[$i]);
 		my $dimm_checksum = 0;
 		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
@@ -1179,15 +1245,18 @@
 		$dimm_count++;

 		print "<b><u>" if $opt_html;
-		printl2 "\n\nDecoding EEPROM", ($use_sysfs ?
+		printl2 "\n\nDecoding EEPROM",
+		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
 			"/sys/bus/i2c/drivers/eeprom/$dimm_list[$i]" :
 			"/proc/sys/dev/sensors/$dimm_list[$i]");
 		print "</u></b>" if $opt_html;
 		print "<table border=1>\n" if $opt_html;
-		if (($use_sysfs && /^[^-]+-([^-]+)$/)
-		 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
-			my $dimm_num = $1 - 49;
-			printl "Guessing DIMM is in", "bank $dimm_num";
+		if (!$use_hexdump) {
+			if (($use_sysfs && /^[^-]+-([^-]+)$/)
+			 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
+				my $dimm_num = $1 - 49;
+				printl "Guessing DIMM is in", "bank $dimm_num";
+			}
 		}

 # Decode first 3 bytes (0-2)

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Read SPD data from a hexdump (take 2)

[Jean Delvare]

Hi Trent,

On Sun, 23 Mar 2008 13:46:38 -0700 (PDT), Trent Piepho wrote:
> This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
> file names to read SPD data from.  It can parse various hexdump formats, such
> as the output from i2cdump, eeprog, and the util-linux and Busybox hexdump
> progams run on a sysfs eeprom file.
> 
> Useful for decoding SPD data that you cut and pasted from a manufacturer's
> website or from a DIMM installed on an embeded system that does not have
> perl/etc, but does have a serial console with busybox.
> 
> ---
> Index: eeprom/decode-dimms.pl
> ===================================================================
> --- eeprom/decode-dimms.pl	(revision 5156)
> +++ eeprom/decode-dimms.pl	(working copy)
> (...)

Patch applied, thanks for your contribution.

If you intend to work more on decode-dimms.pl or anything else in the
i2c-tools package, and would like write access to the SVN repository,
please let me know.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c