restore correct vaio handling in eeprom driver

Here is a patch to the eeprom driver in 2.6.1 that changes the way Vaio eeproms are handled. In 2.6.1, the eeprom is readable only by root, which isn't consistent with the 2.4 driver which simple hides (i.e. fills it with zeroes) the first row (16 bytes) of the eeprom to regular users.

The patch restores a similar behaviour in the 2.6 driver. Greg, feel free to apply it if you like it.

I'd also have a remark. In the 2.4 driver I had divided the driver into arbitrary slices (32 bytes each) in order to improve sensors' responsiveness. As the driver was ported to 2.6, it was dicussed wether such an optimization would be possible, since the whole eeprom is now exported as a single file. Our conclusion back to that time was that the optimization wasn't possible anymore. But now that I saw the actual code, it seems clear to me that it is. The eeprom_read fuction can return any part of the file that is requested, not necessarily the whole file. So the same mechanism used in the 2.4 driver could be added to the 2.6 driver. Two questions then:

1* Would the current 2.6 tweaks in libsensors let sensors take any benefit of it?

2* Such a trick makes the driver a little bigger (more internal vars, more code), and slightly slower in the case you really want to read the whole file. The step was taken for 2.4, will we do the same for 2.6?

Comments welcome.

@@ -27,6 +27,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/sched.h> /* for capable() */
 #include <linux/i2c.h>
 #include <linux/i2c-sensor.h>
 

@@ -62,6 +63,7 @@
 	char valid;			/* !=0 if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
 	u8 data[EEPROM_SIZE];		/* Register values */
+	enum eeprom_nature nature;
 };
 
 

@@ -127,7 +129,16 @@
 	if (off + count > EEPROM_SIZE)
 		count = EEPROM_SIZE - off;
 
-	memcpy(buf, &data->data[off], count);
+	/* Hide Vaio security settings to regular users (16 first bytes) */
+	if (data->nature = VAIO && off < 16 && !capable(CAP_SYS_ADMIN)) {
+		int in_row1 = 16 - off;
+		memset(buf, 0, in_row1);
+		if (count - in_row1 > 0)
+			memcpy(buf + in_row1, &data->data[16], count - in_row1);
+	} else {
+		memcpy(buf, &data->data[off], count);
+	}
+
 	return count;
 }
 

@@ -151,7 +162,6 @@
 	int i, cs;
 	struct i2c_client *new_client;
 	struct eeprom_data *data;
-	enum eeprom_nature nature = UNKNOWN;
 	int err = 0;
 
 	/* Make sure we aren't probing the ISA bus!! This is just a safety check

@@ -199,6 +209,7 @@
 			goto exit_kfree;
 	}
 
+	data->nature = UNKNOWN;
 	/* Detect the Vaio nature of EEPROMs.
 	   We use the "PCG-" prefix as the signature. */
 	if (address = 0x57) {

@@ -206,17 +217,7 @@
 		    i2c_smbus_read_byte_data(new_client, 0x81) = 'C' && 
 		    i2c_smbus_read_byte_data(new_client, 0x82) = 'G' &&
 		    i2c_smbus_read_byte_data(new_client, 0x83) = '-')
-			nature = VAIO;
-	}
-
-	/* If this is a VIAO, then we only allow root to read from this file,
-	   as BIOS passwords can be present here in plaintext */
-	switch (nature) {
- 	case VAIO:
-		eeprom_attr.attr.mode = S_IRUSR;
-		break;
-	default:
-		eeprom_attr.attr.mode = S_IRUGO;
+			data->nature = VAIO;
 	}
 
 	/* Fill in the remaining client fields */

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Hm, why not just prevent the user from reading those bytes at all? Something like what the pci core does for the pci config files (see drivers/pci/pci-sysfs.c::pci_read_config().

I'd take that kind of a patch :)

Is it really worth it? This isn't a speed issue at all, right? Code simplicity should win out in such a case.

thanks,

greg k-h

I took a look at that piece of code this morning. I don't see how I could apply the same here.

In pci-sysfs.c, the result of capable() defines how much bytes you can read (with offset=0). 256 bytes for sysadmin, else 128 or 64 depending on some device type. This makes sense, because if for example a non-sysadmin user asks for 256, he/she will get the first 128 or 64 only, and will be warned about that.

Now for the eeprom driver, the sensible part that should be hidden to non-sysadmin users is the *first* row (16 bytes) of data. This means that if a non-sysadmin user asks for the whole eeprom (256 bytes) we cannot give him/her only 240. There's no way the caller would know that these are *not* the first 240 bytes but 240 bytes starting at offset 16. This means that we would have to fail and return 0 for such requests. Doesn't sound like the think to do, does it? If we do, I'd expect simple commands such as "cat eeprom" to fail. And even if it wouldn't, this prevent any further automated processing of the data read, because of the offset that would be introduced.

If I missed something, please let me know. For now my method seems to fit our specific needs better.

This is about speed only. I made some stats back then, showing how faster "sensors" was with the enhancement. Since the 2.4 driver has is, it looks like a regression that the 2.6 driver doesn't and thus is slower.

I agree that code simplicity matters at least as much as speed, but the speedup is as important as *8 for regular eeproms and *4 for Vaio eeproms. This is much IMHO, and at least enough to consider it as a serious option. Maybe you could take a look at how it is done in our 2.4 driver and see if you really don't want something of that kind in 2.6? The eeprom driver is so simple that a little more complexity should be still easily understandable.

Thanks.

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Ah, oops, you are correct, sorry about that. I'll go apply that patch then...

Depends on what the patch looks like. Care to make it up to see if it's not too ugly?

thanks,

greg k-h

Applied, thanks.

greg k-h

Here you go. Comments at the bottom.

@@ -6,6 +6,11 @@
     Copyright (C) 2003 Greg Kroah-Hartman <[email protected]>
     Copyright (C) 2003 IBM Corp.
 
+    2004-01-16  Jean Delvare <[email protected]>
+    Divide the eeprom in 32-byte (arbitrary) slices. This significantly
+    speeds sensors up, as well as various scripts using the eeprom
+    module.
+
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
     the Free Software Foundation; either version 2 of the License, or

@@ -60,8 +65,8 @@
 /* Each client has this additional data */
 struct eeprom_data {
 	struct semaphore update_lock;
-	char valid;			/* !=0 if following fields are valid */
-	unsigned long last_updated;	/* In jiffies */
+	u8 valid;			/* bitfield, bit!=0 if slice is valid */
+	unsigned long last_updated[8];	/* In jiffies, 8 slices */
 	u8 data[EEPROM_SIZE];		/* Register values */
 	enum eeprom_nature nature;
 };

@@ -83,35 +88,36 @@
 
 static int eeprom_id = 0;
 
-static void eeprom_update_client(struct i2c_client *client)
+static void eeprom_update_client(struct i2c_client *client, u8 slice)
 {
 	struct eeprom_data *data = i2c_get_clientdata(client);
 	int i, j;
 
 	down(&data->update_lock);
 
-	if ((jiffies - data->last_updated > 300 * HZ) |
-	    (jiffies < data->last_updated) || !data->valid) {
-		dev_dbg(&client->dev, "Starting eeprom update\n");
+	if (!(data->valid & (1 << slice)) ||
+	    (jiffies - data->last_updated[slice] > 300 * HZ) ||
+	    (jiffies < data->last_updated[slice])) {
+		dev_dbg(&client->dev, "Starting eeprom update, slice %u\n", slice);
 
 		if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
-			for (i=0; i < EEPROM_SIZE; i += I2C_SMBUS_I2C_BLOCK_MAX)
+			for (i = slice << 5; i < (slice + 1) << 5; i += I2C_SMBUS_I2C_BLOCK_MAX)
 				if (i2c_smbus_read_i2c_block_data(client, i, data->data + i) != I2C_SMBUS_I2C_BLOCK_MAX)
 					goto exit;
 		} else {
-			if (i2c_smbus_write_byte(client, 0)) {
+			if (i2c_smbus_write_byte(client, slice << 5)) {
 				dev_dbg(&client->dev, "eeprom read start has failed!\n");
 				goto exit;
 			}
-			for (i = 0; i < EEPROM_SIZE; i++) {
+			for (i = slice << 5; i < (slice + 1) << 5; i++) {
 				j = i2c_smbus_read_byte(client);
 				if (j < 0)
 					goto exit;
 				data->data[i] = (u8) j;
 			}
 		}
-		data->last_updated = jiffies;
-		data->valid = 1;
+		data->last_updated[slice] = jiffies;
+		data->valid |= (1 << slice);
 	}
 exit:
 	up(&data->update_lock);

@@ -121,14 +127,17 @@
 {
 	struct i2c_client *client = to_i2c_client(container_of(kobj, struct device, kobj));
 	struct eeprom_data *data = i2c_get_clientdata(client);
-
-	eeprom_update_client(client);
+	u8 slice;
 
 	if (off > EEPROM_SIZE)
 		return 0;
 	if (off + count > EEPROM_SIZE)
 		count = EEPROM_SIZE - off;
 
+	/* Only refresh slices which contain requested bytes */
+	for (slice = off >> 5; slice <= (off + count - 1) >> 5; slice++)
+		eeprom_update_client(client, slice);
+
 	/* Hide Vaio security settings to regular users (16 first bytes) */
 	if (data->nature = VAIO && off < 16 && !capable(CAP_SYS_ADMIN)) {
 		int in_row1 = 16 - off;

Basically, I divide the eeprom in 8 32-byte slices. Each of it has a "valid" boolean (combined together into a bitfield) and a jiffies counter. Eeprom_update_client() is added a new parameter to specify which slice to update. Code updated accordingly. Finally, the read function updates only slices that need to be. The code is heavily inspired from what was done in the CVS driver, of course.

Three additional notes:

1* This fixes a bug in eeprom_update_client()'s refresh condition. We used to check jiffies before valid, although jiffies are not defined if valid isn't true. That was already fixed in our CVS driver but for some reason the fix did not go into 2.6 yet.

2* This also skips the update if the read if out of bounds. I think this is the thing to do.

3* It can be discussed wether eeprom_update_client() should take two slice parameters instead of one (start slice, end slice). This would make things slightly faster when consecutive slices are requested. Maybe the code would even be clearer. It wasn't done so far because the CVS driver wouldn't benefit from it (because the EEPROM's contents are split over several "output" files in that version of the driver) but I'll probably give it a try in 2.6.

So, what do you think? Too ugly?

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Ok, very persuasive argument, I've applied this :)

One comment:

Hi Greg,

A few more facts so that you can take the Right Decision (TM):

1* "sensors" time with 1 SPD EEPROM + 1 VAIO EEPROM:
2.4.24 (w/ optimization): 0m1.560s
2.6.1 (w/o optimization): 0m1.039s
2.6.1 (w/ optimization): 0m0.229s

One interesting thing here is that the 2.6 drivers set is already faster that the 2.4 one, even without the optimization. This is a great mystery to me, if someone can explain me why... Is the 2.6 i2c subsystem supposed to be that faster than in 2.4? I expected that the bottleneck would be the relatively low smbus speed, so that nothing could make things go faster. I was obviously wrong. This is with i2c-i801 as the bus driver, if that matters.

Second thing is: yes, the optimization makes sensors damn faster (ratio 4.5).

So on the one hand it seems that optimizing isn't very important (because it's already fast), and on the other hand the speed improvement is real, so I'd say it's worth it. I don't find the optimization ugly, but that's my code so I cannot be objective I guess ;)

2* As of now, libsensors cannot take benefit of the optimization because it uses fopen+getc to read the eeprom file (buffered so the whole file is read, whichever byte you try to read). I had to convert it to open+lseek so that the optimization takes effect. I will of course commit the change if and only if you accept my driver optimization.

The only other programs I know of that will access the eeprom file are our (four) Perl eeprom decoding scripts. They are not converted to the sysfs interface yet, but as they will be, they will take benefit of the optimization.

Hope that helps,

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Well, what the code is meant to do looks clear to me (do not refresh more often than every 300 seconds, do not get hosed by jiffies cycling). So I'd expect it to just work, and BTW it wouldn't be much of a problem if it did not (OK, it'll refresh more often, shouldn't harm too much). OTOH, if a macro exists that does this, and maybe handle more corner cases (not that I can think of any, but who knows), sure we should use it. Can you point us to it?

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

For information, I converted one of them (decode-vaio.pl) to sysfs today. Works great, and the driver optimization serves its purpose:

decode-vaio
2.4.24 : 0m1.990s
2.6.1 : 0m0.550s
2.6.1+patch : 0m0.440s

The benefit is less impressive because the script gathers more information than "sensors" does, so it reads 6 out of the 8 slices ("sensors" needs only 2). The three other scripts should take more benefit of the optimization (I'd expect a 4:1 speedup for decode-dimms.pl for example).

Another thing I forgot to mention in my previous posts: the speed improvement applies only to the first time you run sensors or the scripts, or after 5 minutes have passed. Consecutive reads do not refresh the data so the speedup doesn't apply here. Still I think it's great because the first run is what most users will remember of, especially if they find it slow ;) and the buffer wears of after 5 minutes anyway.

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

time_after()
time_before()
time_after_eq()
time_before_eq()

We should use these instead of our checks to make sure everything is always correct.

thanks,

greg k-h