[PATCH] eeprom: add non-cached sysfs read access to EEPROM data

[PATCH] eeprom: add non-cached sysfs read access to EEPROM data

[Petri Gynther]

Add "eeprom-nc" sysfs attribute to provide non-cached read access
to EEPROM data. This is needed because some EEPROM-like devices
contain constantly changing real-time diagnostic data that cannot
be cached in kernel memory.

Signed-off-by: Petri Gynther <pgynther-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 drivers/misc/eeprom/eeprom.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom.c b/drivers/misc/eeprom/eeprom.c
index 2c27193..101260b 100644
--- a/drivers/misc/eeprom/eeprom.c
+++ b/drivers/misc/eeprom/eeprom.c
@@ -39,6 +39,10 @@ I2C_CLIENT_INSMOD_1(eeprom);
 /* Size of EEPROM in bytes */
 #define EEPROM_SIZE		256
 
+/* EEPROM read modes */
+#define EEPROM_READ_CACHED      0
+#define EEPROM_READ_NONCACHED   1
+
 /* possible types of eeprom devices */
 enum eeprom_nature {
 	UNKNOWN,
@@ -55,14 +59,16 @@ struct eeprom_data {
 };
 
 
-static void eeprom_update_client(struct i2c_client *client, u8 slice)
+static void eeprom_update_client(struct i2c_client *client, u8 slice,
+				 int eeprom_read_mode)
 {
 	struct eeprom_data *data = i2c_get_clientdata(client);
 	int i;
 
 	mutex_lock(&data->update_lock);
 
-	if (!(data->valid & (1 << slice)) ||
+	if (eeprom_read_mode == EEPROM_READ_NONCACHED ||
+	    !(data->valid & (1 << slice)) ||
 	    time_after(jiffies, data->last_updated[slice] + 300 * HZ)) {
 		dev_dbg(&client->dev, "Starting eeprom update, slice %u\n", slice);
 
@@ -102,7 +108,7 @@ static ssize_t eeprom_read(struct kobject *kobj, struct bin_attribute *bin_attr,
 
 	/* Only refresh slices which contain requested bytes */
 	for (slice = off >> 5; slice <= (off + count - 1) >> 5; slice++)
-		eeprom_update_client(client, slice);
+		eeprom_update_client(client, slice, (int)bin_attr->private);
 
 	/* Hide Vaio private settings to regular users:
 	   - BIOS passwords: bytes 0x00 to 0x0f
@@ -132,6 +138,17 @@ static struct bin_attribute eeprom_attr = {
 	},
 	.size = EEPROM_SIZE,
 	.read = eeprom_read,
+	.private = (void *)EEPROM_READ_CACHED,
+};
+
+static struct bin_attribute eeprom_nc_attr = {
+	.attr = {
+		.name = "eeprom-nc",
+		.mode = S_IRUGO,
+	},
+	.size = EEPROM_SIZE,
+	.read = eeprom_read,
+	.private = (void *)EEPROM_READ_NONCACHED,
 };
 
 /* Return 0 if detection is successful, -ENODEV otherwise */
@@ -202,8 +219,14 @@ static int eeprom_probe(struct i2c_client *client,
 	if (err)
 		goto exit_kfree;
 
+	err = sysfs_create_bin_file(&client->dev.kobj, &eeprom_nc_attr);
+	if (err)
+		goto exit_sysfs_clean;
+
 	return 0;
 
+exit_sysfs_clean:
+	sysfs_remove_bin_file(&client->dev.kobj, &eeprom_attr);
 exit_kfree:
 	kfree(data);
 exit:
@@ -212,6 +235,7 @@ exit:
 
 static int eeprom_remove(struct i2c_client *client)
 {
+	sysfs_remove_bin_file(&client->dev.kobj, &eeprom_nc_attr);
 	sysfs_remove_bin_file(&client->dev.kobj, &eeprom_attr);
 	kfree(i2c_get_clientdata(client));
 
-- 
1.5.4.3

Re: [PATCH] eeprom: add non-cached sysfs read access to EEPROM data

[Greg KH]

On Tue, Sep 01, 2009 at 04:15:00PM -0700, Petri Gynther wrote:
> Add "eeprom-nc" sysfs attribute to provide non-cached read access
> to EEPROM data. This is needed because some EEPROM-like devices
> contain constantly changing real-time diagnostic data that cannot
> be cached in kernel memory.

How are we going to know which attribute to use?

For all new sysfs attributes, you have to add a Documentation/ABI/ file
as well, care to do that here?

thanks,

greg k-h

Re: [PATCH] eeprom: add non-cached sysfs read access to EEPROM data

[Jean Delvare]

Hi Petri,

On Tue,  1 Sep 2009 16:15:00 -0700 (PDT), Petri Gynther wrote:
> Add "eeprom-nc" sysfs attribute to provide non-cached read access
> to EEPROM data. This is needed because some EEPROM-like devices
> contain constantly changing real-time diagnostic data that cannot
> be cached in kernel memory.
> 
> Signed-off-by: Petri Gynther <pgynther-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/misc/eeprom/eeprom.c |   30 +++++++++++++++++++++++++++---
>  1 files changed, 27 insertions(+), 3 deletions(-)

You are using the wrong driver for the task. The "eeprom" driver is a
legacy driver solely meant for SPD and EDID EEPROMs, and these do never
change, thus the caching mechanism. Please look at the new "at24"
driver for accessing any other EEPROM type.

-- 
Jean Delvare

Re: [PATCH] eeprom: add non-cached sysfs read access to EEPROM data

[Petri Gynther]

Greg,

It is up to the userspace app to decide whether to use cached
("eeprom") or non-cached ("eeprom-nc") sysfs interface.

Example: SFP module
It exposes two 256-byte EEPROM(-like) devices at 0x50 and 0x51.
0x50 is static vendor/part info and can be cached.
0x51 is real-time diagnostic data and cannot be cached.

Unfortunately, 0x51 does not have any identifying bits telling us that
it is real-time data.
(If it did, it would be easy to check and remember this when 0x51 is probed.)

-- Petri

On Tue, Sep 1, 2009 at 4:39 PM, Greg KH<greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Sep 01, 2009 at 04:15:00PM -0700, Petri Gynther wrote:
>> Add "eeprom-nc" sysfs attribute to provide non-cached read access
>> to EEPROM data. This is needed because some EEPROM-like devices
>> contain constantly changing real-time diagnostic data that cannot
>> be cached in kernel memory.
>
> How are we going to know which attribute to use?
>
> For all new sysfs attributes, you have to add a Documentation/ABI/ file
> as well, care to do that here?
>
> thanks,
>
> greg k-h
>

Re: [PATCH] eeprom: add non-cached sysfs read access to EEPROM data

[Petri Gynther]

Jean,

In 2.6.25, I used this patch successfully for read-only access to some
SFP EEPROMs:
- 0x50 - vendor/part info (caching OK)
- 0x51 - real-time diagnostics data (caching not OK)

I only care about read-only access to these EEPROMs. And, actually, I
don't want to provide write access at all.

In 2.6.31, I'd like to continue using this same legacy driver for SFP
EEPROM access, with the option of bypassing the cache.

-- Petri

On Wed, Sep 2, 2009 at 1:30 AM, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Petri,
>
> On Tue,  1 Sep 2009 16:15:00 -0700 (PDT), Petri Gynther wrote:
>> Add "eeprom-nc" sysfs attribute to provide non-cached read access
>> to EEPROM data. This is needed because some EEPROM-like devices
>> contain constantly changing real-time diagnostic data that cannot
>> be cached in kernel memory.
>>
>> Signed-off-by: Petri Gynther <pgynther-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/misc/eeprom/eeprom.c |   30 +++++++++++++++++++++++++++---
>>  1 files changed, 27 insertions(+), 3 deletions(-)
>
> You are using the wrong driver for the task. The "eeprom" driver is a
> legacy driver solely meant for SPD and EDID EEPROMs, and these do never
> change, thus the caching mechanism. Please look at the new "at24"
> driver for accessing any other EEPROM type.
>
> --
> Jean Delvare
>

Re: [PATCH] eeprom: add non-cached sysfs read access to EEPROM data

[Jean Delvare]

On Wed, 2 Sep 2009 12:45:21 -0700, Petri Gynther wrote:
> In 2.6.25, I used this patch successfully for read-only access to some
> SFP EEPROMs:
> - 0x50 - vendor/part info (caching OK)
> - 0x51 - real-time diagnostics data (caching not OK)
> 
> I only care about read-only access to these EEPROMs. And, actually, I
> don't want to provide write access at all.
> 
> In 2.6.31, I'd like to continue using this same legacy driver for SFP
> EEPROM access, with the option of bypassing the cache.

This is not going to happen, sorry. "eeprom" is a legacy driver and we
certainly don't want to enhance it in any way. The "at24" driver is
much easier to extend as it was designed that way from the ground up.
You can add a SFP EEPROM type to it (whatever it is) and have the
driver automatically set the access to read-only and the caching
strategy for each part of the EEPROM.

I'm curious, why do you insist on using the eeprom driver?

-- 
Jean Delvare

Re: [PATCH] eeprom: add non-cached sysfs read access to EEPROM data

[Petri Gynther]

OK, fine. I'll look into extending "at24" instead.

The only reason I was going with "eeprom" driver here is that it
worked just fine (with this diff) for my needs in 2.6.25. And, I
thought maybe someone else in the community can benefit from cache
bypass as well.

-- Petri

On Wed, Sep 2, 2009 at 1:16 PM, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Wed, 2 Sep 2009 12:45:21 -0700, Petri Gynther wrote:
>> In 2.6.25, I used this patch successfully for read-only access to some
>> SFP EEPROMs:
>> - 0x50 - vendor/part info (caching OK)
>> - 0x51 - real-time diagnostics data (caching not OK)
>>
>> I only care about read-only access to these EEPROMs. And, actually, I
>> don't want to provide write access at all.
>>
>> In 2.6.31, I'd like to continue using this same legacy driver for SFP
>> EEPROM access, with the option of bypassing the cache.
>
> This is not going to happen, sorry. "eeprom" is a legacy driver and we
> certainly don't want to enhance it in any way. The "at24" driver is
> much easier to extend as it was designed that way from the ground up.
> You can add a SFP EEPROM type to it (whatever it is) and have the
> driver automatically set the access to read-only and the caching
> strategy for each part of the EEPROM.
>
> I'm curious, why do you insist on using the eeprom driver?
>
> --
> Jean Delvare
>