[lm-sensors] Added sysfs support for decode-dimms.pl

[lm-sensors] Added sysfs support for decode-dimms.pl

[Burkart Lingner]

Hello everyone!

I needed to take a closer look at my RAM's SPD-EEPROMs today. Too bad the
sysfs of Kernel 2.6 wasn't supported by decode-dimms.pl, so I had to add
support for that myself. Since there probably are more people out there
who'd like to use this tool on a Kernel 2.6 system, it might be a good idea
to include the patched file in the next release of lm_sensors. The new
version can be found at http://www.bollchen.de/decode-dimms.pl. Of course
the "ADDED" comments should be removed then, I just left them in for now and
I can strip them later on demand. The new version does work with Kernel 2.6
and it should as well work with older Kernels and their proc-fs. Since I
only have a system running 2.6, could anyone with an older Kernel please do
a test run? That would be greatly appreciated, to make sure I didn't mess
anything up.

I tested the script on a Fedora Core 3 system with Kernel 2.6.11 and
lm_sensors 2.8.7.

Bye, Burkart 

[lm-sensors] Added sysfs support for decode-dimms.pl

[Grant Coady]

On Thu, 16 Jun 2005 00:29:17 +0200, "Burkart Lingner" <webmaster@bollchen.de> wrote:
>version can be found at http://www.bollchen.de/decode-dimms.pl. Of course

>lm_sensors 2.8.7.
What of 2.9.1?

--Grant.

[lm-sensors] Added sysfs support for decode-dimms.pl

[Per Jessen]

Burkart Lingner wrote:

> The new version can be found at http://www.bollchen.de/decode-dimms.pl. Of course
> the "ADDED" comments should be removed then, I just left them in for now and
> I can strip them later on demand. The new version does work with Kernel 2.6
> and it should as well work with older Kernels and their proc-fs. Since I
> only have a system running 2.6, could anyone with an older Kernel please do
> a test run? That would be greatly appreciated, to make sure I didn't mess
> anything up.

Hi Burkart,

I ran a comparison of the output on a 2.4.30 system - the diff is:

diff -aur listing1 listing2
--- listing1    2005-06-16 10:55:49.000000000 +0200
+++ listing2    2005-06-16 10:55:53.000000000 +0200
@@ -1,7 +1,7 @@


 PC DIMM Serial Presence Detect Tester/Decoder
-By Philip Edelbrock, Christian Zuckschwerdt, Burkart Lingner and others
+By Philip Edelbrock, Christian Zuckschwerdt and others
 Version 2.6.6

 Decoding EEPROM         /proc/sys/dev/sensors/eeprom-i2c-0-50
@@ -86,4 +86,4 @@

 Number of SDRAM DIMMs detected and decoded     1

-Try './decode-dimms.pl --format' for html output.
+Try '/usr/bin/decode-dimms.pl --format' for html output.


Looks absolutely fine to me.


-- 
/Per Jessen, Z?rich
http://www.spamchek.ch/freetrial - ?berzeugen Sie sich - 30 Tage kostenlos und unverbindlich!

[lm-sensors] Added sysfs support for decode-dimms.pl

[Jean Delvare]

Hi Grant,

On 2005-06-16, Grant Coady:
> >lm_sensors 2.8.7.
>
> What of 2.9.1?

The decode-dimms.pl script did not change at all since 2.8.4, so it
won't make a difference. Grant, you could have checked that before
complaining ;)

--
Jean Delvare

[lm-sensors] Added sysfs support for decode-dimms.pl

[Jean Delvare]

Hallo Burkart,

On 2005-06-15, Burkart Lingner:
> I needed to take a closer look at my RAM's SPD-EEPROMs today. Too bad
> the sysfs of Kernel 2.6 wasn't supported by decode-dimms.pl, so I had
> to add support for that myself. Since there probably are more people
> out there who'd like to use this tool on a Kernel 2.6 system, it might
> be a good idea to include the patched file in the next release of
> lm_sensors.

Yes, definitely.

> The new version can be found at http://www.bollchen.de/decode-dimms.pl.
> Of course the "ADDED" comments should be removed then, I just left them
> in for now and I can strip them later on demand. The new version does
> work with Kernel 2.6 and it should as well work with older Kernels and
> their proc-fs. Since I only have a system running 2.6, could anyone
> with an older Kernel please do a test run? That would be greatly
> appreciated, to make sure I didn't mess anything up.

I just took a look at your script. It's not bad but I would like the
sysfs support to be added in a cleaner manner if possible. Rather than
having a if(sysfs) test on each read, we could have a single function
returning the wanted bytes, and all the checks would be in there. The
advantage is that it would avoid code duplication, would make the code
easier to read, and would allow for yet others access methods to be
added more easily in the future (e.g. reading from a dump file). Could
you try to modify your script to do that? You're probably not that far.
You may check how I did this in decode-vaio.pl, although you should be
able to do something more simple.

Other comments on the code itself:

1* Your sysfs version doesn't print the guessed bank number anymore?

2* Your sysfs version reads the whole EEPROM contents while only the
lower half is used. This will make the script twice as slow, so it
should be avoided if possible. Maybe the read method I implemented in
decode-vaio.pl may help (or use head -c, but I'm not sure it's
portable).

So it would be great if you could work on this. If you come up with a new
version, I'll test it on my systems (both 2.4 and 2.6 kernels).

Last point, when submitting changes, the best method is to provide the
output of diff -u between the original and modified file. This is way
easier than looking at "ADDED" and "MODIFIED" tags, and can be
applied to CVS directly.

Thanks,
--
Jean Delvare

[lm-sensors] Added sysfs support for decode-dimms.pl

[Burkart Lingner]

Hello, Jean!

> On 2005-06-15, Burkart Lingner:
> > I needed to take a closer look at my RAM's SPD-EEPROMs today. Too bad
> > the sysfs of Kernel 2.6 wasn't supported by decode-dimms.pl, so I had
> > to add support for that myself. [...]
>
> I just took a look at your script. It's not bad but I would like the
> sysfs support to be added in a cleaner manner if possible. Rather than
> having a if(sysfs) test on each read, we could have a single function
> returning the wanted bytes, and all the checks would be in there. The
> advantage is that it would avoid code duplication, would make the code
> easier to read, and would allow for yet others access methods to be
> added more easily in the future (e.g. reading from a dump file). Could
> you try to modify your script to do that? You're probably not that far.
> You may check how I did this in decode-vaio.pl, although you should be
> able to do something more simple.

Very good idea, thanks for the hint. For my excuse I have to say that my 
previous modification was just a quick'n'dirty one to get me access to the 
SPD data I needed. Yet I understand that for a release, the code should be 
nicer and so I'll implement it somewhat in the way you suggested.

> Other comments on the code itself:
>
> 1* Your sysfs version doesn't print the guessed bank number anymore?

Honestly I was just too lazy to decode the regular expression in the first 
place. Also I don't know the naming conventions for the EEPROMs on a Kernel 
2.4 system. Could you fill me in on the details about that? That would help 
me a lot on adapting this code fragment to the sysfs.

> 2* Your sysfs version reads the whole EEPROM contents while only the
> lower half is used. This will make the script twice as slow, so it
> should be avoided if possible. Maybe the read method I implemented in
> decode-vaio.pl may help (or use head -c, but I'm not sure it's
> portable).

You're right and this could of course be done better using binary file 
access via read(). On the other hand it sure wouldn't hurt too much to read 
128 bytes in vain. However I do suppose that in implementing a sub routine 
for reading the EEPROM I'd switch to read() and thereby to reading only the 
necessary 128 bytes.

> So it would be great if you could work on this. If you come up with a new
> version, I'll test it on my systems (both 2.4 and 2.6 kernels).

Sure. Actually I'm quite honored to work on an open source project. Never 
done that before.

Please give me a couple of days, though. There's a couple of other things I 
ought to work out right now, so it might not be before the weekend until I 
apply the abovementioned changes.

> Last point, when submitting changes, the best method is to provide the
> output of diff -u between the original and modified file. This is way
> easier than looking at "ADDED" and "MODIFIED" tags, and can be
> applied to CVS directly.

Alright, thanks.

Bye, Burkart 

[lm-sensors] Added sysfs support for decode-dimms.pl

[Burkart Lingner]

Salut, Jean!

I modified the code for decode-dimms.pl as you suggested. Now only the first 
128 bytes of the SPD EEPROM are read, moreover this is done in a central sub 
routine rather than at 8 different places in the code. The bank number 
guessing is now also working with the sysfs.

The new version can be found at http://www.bollchen.de/decode-dimms.pl and 
the output of diff -u is located at http://www.bollchen.de/diff.out

Let me know what you think.
Bye, Burkart 

[lm-sensors] Added sysfs support for decode-dimms.pl

[Jean Delvare]

Hallo Burkart,

> I modified the code for decode-dimms.pl as you suggested. Now only the
> first  128 bytes of the SPD EEPROM are read, moreover this is done in
> a central sub  routine rather than at 8 different places in the code.
> The bank number  guessing is now also working with the sysfs.
> 
> The new version can be found at http://www.bollchen.de/decode-dimms.pl
> and  the output of diff -u is located at
> http://www.bollchen.de/diff.out
> 
> Let me know what you think.

It's much better, but I think we can do even better:

1*

		# Kernel 2.4 with procfs
		if ($offset = 0) {		# bytes 0..15
			$_=`cat /proc/sys/dev/sensors/$dimm_i/00`;
		} elsif ($offset = 16) {	# bytes 16..31
			$_=`cat /proc/sys/dev/sensors/$dimm_i/10`;
		} elsif ($offset = 32) {	# bytes 32..47
			$_=`cat /proc/sys/dev/sensors/$dimm_i/20`;
		} elsif ($offset = 48) {	# bytes 48..63
			$_=`cat /proc/sys/dev/sensors/$dimm_i/30`;
		} elsif ($offset = 64) {	# bytes 64..79
			$_=`cat /proc/sys/dev/sensors/$dimm_i/40`;
		} elsif ($offset = 80) {	# bytes 80..95
			$_=`cat /proc/sys/dev/sensors/$dimm_i/50`;
		} elsif ($offset = 96) {	# bytes 96..111
			$_=`cat /proc/sys/dev/sensors/$dimm_i/60`;
		} elsif ($offset = 112) {	# bytes 112..127
			$_=`cat /proc/sys/dev/sensors/$dimm_i/70`;
		} else {			# illegal offset
			$_='255 'x15 . '255';
		}

This is slow and inefficient. What about:

$offset=sprintf('%02x', $offset);
$_=`cat /proc/sys/dev/sensors/$dimm_i/$offset`

2* In various places you are doing:
if (-e "/sys/bus/i2c/drivers/eeprom")
to test if sysfs is used or not.

I think you could check for /sys/bus at the very beginning of the
script, and have a global variable $use_sysfs. So you would only have to
check for that variable in the rest of the script. I think it would make
the code much clearer.

Other than that it looks OK, except that your patch changes whitespace
in various places, making the patch look much bigger than it actually
is. I suspect that your editor strips whitespace at end of lines or
something like that... Not a big deal though.

Thanks,
-- 
Jean Delvare

[lm-sensors] Added sysfs support for decode-dimms.pl

[Jean Delvare]

Hallo Burkart,

> I modified the code for decode-dimms.pl as you suggested. Now only the
> first 128 bytes of the SPD EEPROM are read, moreover this is done in
> a central sub routine rather than at 8 different places in the code.
> The bank number guessing is now also working with the sysfs.
> 
> The new version can be found at http://www.bollchen.de/decode-dimms.pl
> and the output of diff -u is located at
> http://www.bollchen.de/diff.out
> 
> Let me know what you think.

I finally took some time to commit your changes. Sorry for the delay. I
additionally refactored the code as I had previously suggested, and
fixed a few other things. The new version of the script is in lm_sensors
CVS now, so the upcoming 2.9.2 release will have it. Feel tree to test
that it still works OK for you:

  http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/prog/eeprom/decode-dimms.pl

Vielen Dank f?r deine gute Arbeit :)

-- 
Jean Delvare