checksum in (i2c) eeprom driver

checksum in (i2c) eeprom driver

[Jean Delvare]

Hi all,

Any objection to me removing the checksumming code from the (i2c) eeprom
driver? Deepak had suggested we should do so a long time ago [1], and I
fully agree with his position. The checksum is application-specific and
verifying it doesn't belong to the kernel-space. The checksumming code
we (optionally) use at the moment only covers memory module EEPROMs as
far as I know, while EEPROMs exposed on I2C/SMBus may be of a variety of
other natures.

[1] http://archives.andrew.net.au/lm-sensors/msg21194.html

Thanks,
--
Jean Delvare

checksum in (i2c) eeprom driver

[Jean Delvare]

Hi all,

Any objection to me removing the checksumming code from the (i2c) eeprom
driver? Deepak had suggested we should do so a long time ago [1], and I
fully agree with his position. The checksum is application-specific and
verifying it doesn't belong to the kernel-space. The checksumming code
we (optionally) use at the moment only covers memory module EEPROMs as
far as I know, while EEPROMs exposed on I2C/SMBus may be of a variety of
other natures.

[1] http://archives.andrew.net.au/lm-sensors/msg21194.html

Thanks,
--
Jean Delvare

checksum in (i2c) eeprom driver

[Mark Studebaker]

IMHO the eeprom driver is more of a demonstration driver than one of
great and obvious value, so achieving consensus on the value of sub-features
(checksum, Vaio) is difficult, and performace concerns are secondary.
So I don't see any value removing the code. If you want to make it
super-clean shouldn't the Vaio stuff come out too?
But I'm sure you'll disagree...
mds

Jean Delvare wrote:
> On 2004-12-09, Mark Studebaker wrote:
> 
> 
>>I think the checksum code is useful because checksum=1 prevents the module
>>from claiming ddc monitor eeproms and other devices in its address space
>>50-57.
> 
> 
> DDC monitor EEPROMs *are* EEPROMs so there is no reason to exclude them
> from this driver. We used to have a specific (ddcmon) driver for these
> but this too is an error IMHO. Developping different eeprom drivers for
> different natures of eeproms is silly (how many more?). What the ddcmon
> driver was doing really belongs to user-space, not kernel-space.
> 
> There are not that many non-EEPROMs chips in the 0x50-0x57 range, only
> the Maxim MAX6900 RTC according to sensors-detect (quite a rare chip at
> that, we don't even have a driver for it yet).
> 
> 
>>Since detection for eeproms is otherwise poor, it's the only way we have
>>for robust detection.
> 
> 
> Except that it only works with memory module EEPROMs.
> 
> If the checksumming was that important, I guess it would have been the
> default, which it was not. If it is there for the sole purpose of
> allowing the user to prevent the eeprom driver from taking over
> non-eeprom chips, then the "ignore" module parameter can be used to
> achieve the same effect, faster, plus it is configurable on a
> per-address basis, while the checksum parameter isn't.
> 
> Thanks,
> --
> Jean Delvare
> 
> 

Re: checksum in (i2c) eeprom driver

[Jean Delvare]

On 2004-12-10, Mark Studebaker wrote:

> IMHO the eeprom driver is more of a demonstration driver than one of
> great and obvious value, so achieving consensus on the value of
> sub-features (checksum, Vaio) is difficult, and performace concerns are
> secondary.

This has certainly been true when the driver was first written and then
maintained as a driver of the lm_sensors project, and was only used for
memory module EEPROMs. However, we now start seeing more different
natures of EEPROMs (proprietary on laptops, ethernet devices to name
only two of them) for which the eeprom driver can be useful. Remember
that a number of people even asked for write support in the driver (and
this might as well happen in the future).

> So I don't see any value removing the code. If you want to make it
> super-clean shouldn't the Vaio stuff come out too?
> But I'm sure you'll disagree...

I would be happy to remove all Vaio stuff if there were no security
concern in doing so. Unfortunately it happens that Vaio EEPROMs contain
passwords in a very lightly encoded form and I thought that we didn't
want every user of the system to be able to read it. If there is a
better way to achieve the same goal, I'd gladly hear about it. I can
also get rid of the test altogether if a majority of people think it's
not necessary to hide the machine password from users - after all, only
a limited number of machines are Vaio laptops, of which a limited number
actually have a system password set, of which a limited number have more
than one user, of which a limited number have untrusted users.

The value of removing the code is, unsurprisingly, to reduce the amount
of code to maintain and the amount of memory used by the driver when
loaded. I am also trying to comply with the kernel rules about what
belongs to the kernel-space and what belongs to the user-space.

Thanks,
--
Jean Delvare

checksum in (i2c) eeprom driver

[Jean Delvare]

On 2004-12-10, Mark Studebaker wrote:

> IMHO the eeprom driver is more of a demonstration driver than one of
> great and obvious value, so achieving consensus on the value of
> sub-features (checksum, Vaio) is difficult, and performace concerns are
> secondary.

This has certainly been true when the driver was first written and then
maintained as a driver of the lm_sensors project, and was only used for
memory module EEPROMs. However, we now start seeing more different
natures of EEPROMs (proprietary on laptops, ethernet devices to name
only two of them) for which the eeprom driver can be useful. Remember
that a number of people even asked for write support in the driver (and
this might as well happen in the future).

> So I don't see any value removing the code. If you want to make it
> super-clean shouldn't the Vaio stuff come out too?
> But I'm sure you'll disagree...

I would be happy to remove all Vaio stuff if there were no security
concern in doing so. Unfortunately it happens that Vaio EEPROMs contain
passwords in a very lightly encoded form and I thought that we didn't
want every user of the system to be able to read it. If there is a
better way to achieve the same goal, I'd gladly hear about it. I can
also get rid of the test altogether if a majority of people think it's
not necessary to hide the machine password from users - after all, only
a limited number of machines are Vaio laptops, of which a limited number
actually have a system password set, of which a limited number have more
than one user, of which a limited number have untrusted users.

The value of removing the code is, unsurprisingly, to reduce the amount
of code to maintain and the amount of memory used by the driver when
loaded. I am also trying to comply with the kernel rules about what
belongs to the kernel-space and what belongs to the user-space.

Thanks,
--
Jean Delvare

Re: checksum in (i2c) eeprom driver

[Mark M. Hoffman]

Hello:

(summary of some IRC discussions...)

> On 2004-12-10, Mark Studebaker wrote:
> > IMHO the eeprom driver is more of a demonstration driver than one of
> > great and obvious value, so achieving consensus on the value of
> > sub-features (checksum, Vaio) is difficult, and performace concerns are
> > secondary.

In as much as eeprom is a demonstration driver, with very little actual
usefulness except as a test device for us sensors people... I think it
could probably be removed from the kernel altogether if we create a user-
space (w/ i2c-dev interface) replacement for it.

* Jean Delvare <khali@linux-fr.org> [2004-12-10 09:42:21 +0100]:
> This has certainly been true when the driver was first written and then
> maintained as a driver of the lm_sensors project, and was only used for
> memory module EEPROMs. However, we now start seeing more different
> natures of EEPROMs (proprietary on laptops, ethernet devices to name
> only two of them) for which the eeprom driver can be useful. Remember
> that a number of people even asked for write support in the driver (and
> this might as well happen in the future).

If read/write support is needed, IMHO it should be implemented as a proper
char device.  The sysfs interface of the current driver makes little sense.

OTOH, note that it would be possible to break RAM modules *permanently* by
misusing such a device.  The eeprom itself would still work, but the SIMM
or DIMM that it sits on would be effectively broken.  I don't personally
consider that a good argument against an eeprom char device, but some do.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

checksum in (i2c) eeprom driver

[Mark M. Hoffman]

Hello:

(summary of some IRC discussions...)

> On 2004-12-10, Mark Studebaker wrote:
> > IMHO the eeprom driver is more of a demonstration driver than one of
> > great and obvious value, so achieving consensus on the value of
> > sub-features (checksum, Vaio) is difficult, and performace concerns are
> > secondary.

In as much as eeprom is a demonstration driver, with very little actual
usefulness except as a test device for us sensors people... I think it
could probably be removed from the kernel altogether if we create a user-
space (w/ i2c-dev interface) replacement for it.

* Jean Delvare <khali@linux-fr.org> [2004-12-10 09:42:21 +0100]:
> This has certainly been true when the driver was first written and then
> maintained as a driver of the lm_sensors project, and was only used for
> memory module EEPROMs. However, we now start seeing more different
> natures of EEPROMs (proprietary on laptops, ethernet devices to name
> only two of them) for which the eeprom driver can be useful. Remember
> that a number of people even asked for write support in the driver (and
> this might as well happen in the future).

If read/write support is needed, IMHO it should be implemented as a proper
char device.  The sysfs interface of the current driver makes little sense.

OTOH, note that it would be possible to break RAM modules *permanently* by
misusing such a device.  The eeprom itself would still work, but the SIMM
or DIMM that it sits on would be effectively broken.  I don't personally
consider that a good argument against an eeprom char device, but some do.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: checksum in (i2c) eeprom driver

[Jean Delvare]

On 2004-15-12, Mark M. Hoffman wrote:

> In as much as eeprom is a demonstration driver, with very little actual
> usefulness except as a test device for us sensors people... I think it
> could probably be removed from the kernel altogether if we create a user-
> space (w/ i2c-dev interface) replacement for it.

As I already objected on IRC, I wonder how you would make concurent
accesses safe in this case. Using (continuous) byte reads is not safe in
this context, while it is the more performant read method among those
widely available (byte data reads are 50% slower, and i2c block read is
usually not supported by SMBus Masters.)

Of course we have potentially similar issues with other tools using
i2c-dev such as sensors-detect and i2cdump, but these tools are not
meant for daily use. Especially the latter is for
investigation/debugging purposes only. The former will not attempt to
identify devices already in use by a kernel driver. If we start using
i2c-dev for non-debug stuff, then sensors-detect becomes unsafe.

Think of the following scenario:
You read and/or write EEPROMs using your proposed i2c-dev-based tool,
using continuous byte read/write. At the same time, someone on the same
machine runs sensors-detect. sensors-detect will scan these EEPROMs at
some point, changing their internal address pointer, and as a
consequence possibly trashing the data read or written by your EEPROM
manipulation tool.

You may argue that it is unlikely that someone will run sensors-detect at
the same time he/she reprograms EEPROMs, but 1* you don't really know
what people do and 2* since people can technically to it, it better be
safe and 3* sensors-detect is soon to be part of a hotplug solution for
I2C/SMBus devices so it might be run anytime.

Summary: your proposal doesn't sound safe at all as long as there is no
possibility for i2c-dev to "request" an address for exclusive use like
kernel drivers can do. Looking at i2c-dev.h I see no such function at
the moment. There is also a permission issue because you need to have
write access to /dev/i2c-* even to only read from a chip (eeprom in this
case). Giving write access to tehse devices to everyone is of course not
safe, so your solution would only work for root, right?

Also, having an in-kernel i2c driver allows us to cache the data for some
times, which not only improves performance (the same could be done in
user-space) but prevents non-root users from saturating the SMBus.

> If read/write support is needed, IMHO it should be implemented as a proper
> char device.  The sysfs interface of the current driver makes little sense.

I don't really understand why exactly the sysfs interface isn't
correct. It does the job as far as I can see. Also think of devices that
have both an EEPROM and other functions (I remember one RTC chip like
that). They fit fine in our sysfs design (eeprom file for eeprom access,
other files for the other functions). Having a separate char device
would make things more difficult to understand. Just a thought though, I
am not opposed to the idea of a char device. If there are benefits, that
is.

> OTOH, note that it would be possible to break RAM modules *permanently* by
> misusing such a device.  The eeprom itself would still work, but the SIMM
> or DIMM that it sits on would be effectively broken.  I don't personally
> consider that a good argument against an eeprom char device, but some do.

You can also trash all your data if you play with, say, /dev/hda, and
losing personal data may be worth than losing a memory module. The
can-make-the-module-useless issue is to be considered but certainly not
a valid reason to never, ever have write support for EEPROMs. Note that
with the current implementation we could easily have a module parameter
for explicitely enabling write support (that might not be the best
solution though).

About "trashed" memory modules, I now wonder... OK, the system will not
recognize the module anymore, but if there is anotgher working module in
the machine, I'd expect it to still boot, then the EEPROM should still
be visible on the I2C bus and could be reprogrammed, no? Or if the
system checks and stops booting on bad modules, maybe hotplugging it at
a later time would do it? I'm not willing to sacrifice a memory module
of mine just to do the test, but I wonder if either of these techniques
would work.

Back to the original topic... Your ideas about a new way to access the
EEPROMs is interesting but are hardly related to my original question of
whether I can remove the checksumming code from the existing driver or
not. I will continue to maintain this driver as long as no other
alternative is proposed, accepted, implemented and tested.

Thanks,
--
Jean Delvare

checksum in (i2c) eeprom driver

[Jean Delvare]

On 2004-15-12, Mark M. Hoffman wrote:

> In as much as eeprom is a demonstration driver, with very little actual
> usefulness except as a test device for us sensors people... I think it
> could probably be removed from the kernel altogether if we create a user-
> space (w/ i2c-dev interface) replacement for it.

As I already objected on IRC, I wonder how you would make concurent
accesses safe in this case. Using (continuous) byte reads is not safe in
this context, while it is the more performant read method among those
widely available (byte data reads are 50% slower, and i2c block read is
usually not supported by SMBus Masters.)

Of course we have potentially similar issues with other tools using
i2c-dev such as sensors-detect and i2cdump, but these tools are not
meant for daily use. Especially the latter is for
investigation/debugging purposes only. The former will not attempt to
identify devices already in use by a kernel driver. If we start using
i2c-dev for non-debug stuff, then sensors-detect becomes unsafe.

Think of the following scenario:
You read and/or write EEPROMs using your proposed i2c-dev-based tool,
using continuous byte read/write. At the same time, someone on the same
machine runs sensors-detect. sensors-detect will scan these EEPROMs at
some point, changing their internal address pointer, and as a
consequence possibly trashing the data read or written by your EEPROM
manipulation tool.

You may argue that it is unlikely that someone will run sensors-detect at
the same time he/she reprograms EEPROMs, but 1* you don't really know
what people do and 2* since people can technically to it, it better be
safe and 3* sensors-detect is soon to be part of a hotplug solution for
I2C/SMBus devices so it might be run anytime.

Summary: your proposal doesn't sound safe at all as long as there is no
possibility for i2c-dev to "request" an address for exclusive use like
kernel drivers can do. Looking at i2c-dev.h I see no such function at
the moment. There is also a permission issue because you need to have
write access to /dev/i2c-* even to only read from a chip (eeprom in this
case). Giving write access to tehse devices to everyone is of course not
safe, so your solution would only work for root, right?

Also, having an in-kernel i2c driver allows us to cache the data for some
times, which not only improves performance (the same could be done in
user-space) but prevents non-root users from saturating the SMBus.

> If read/write support is needed, IMHO it should be implemented as a proper
> char device.  The sysfs interface of the current driver makes little sense.

I don't really understand why exactly the sysfs interface isn't
correct. It does the job as far as I can see. Also think of devices that
have both an EEPROM and other functions (I remember one RTC chip like
that). They fit fine in our sysfs design (eeprom file for eeprom access,
other files for the other functions). Having a separate char device
would make things more difficult to understand. Just a thought though, I
am not opposed to the idea of a char device. If there are benefits, that
is.

> OTOH, note that it would be possible to break RAM modules *permanently* by
> misusing such a device.  The eeprom itself would still work, but the SIMM
> or DIMM that it sits on would be effectively broken.  I don't personally
> consider that a good argument against an eeprom char device, but some do.

You can also trash all your data if you play with, say, /dev/hda, and
losing personal data may be worth than losing a memory module. The
can-make-the-module-useless issue is to be considered but certainly not
a valid reason to never, ever have write support for EEPROMs. Note that
with the current implementation we could easily have a module parameter
for explicitely enabling write support (that might not be the best
solution though).

About "trashed" memory modules, I now wonder... OK, the system will not
recognize the module anymore, but if there is anotgher working module in
the machine, I'd expect it to still boot, then the EEPROM should still
be visible on the I2C bus and could be reprogrammed, no? Or if the
system checks and stops booting on bad modules, maybe hotplugging it at
a later time would do it? I'm not willing to sacrifice a memory module
of mine just to do the test, but I wonder if either of these techniques
would work.

Back to the original topic... Your ideas about a new way to access the
EEPROMs is interesting but are hardly related to my original question of
whether I can remove the checksumming code from the existing driver or
not. I will continue to maintain this driver as long as no other
alternative is proposed, accepted, implemented and tested.

Thanks,
--
Jean Delvare

checksum in (i2c) eeprom driver

[Mark Studebaker]

I think the checksum code is useful because checksum=1 prevents the module from claiming ddc monitor eeproms
and other devices in its address space 50-57.
Since detection for eeproms is otherwise poor, it's the only way we have for robust detection.

Jean Delvare wrote:
> Hi all,
> 
> Any objection to me removing the checksumming code from the (i2c) eeprom
> driver? Deepak had suggested we should do so a long time ago [1], and I
> fully agree with his position. The checksum is application-specific and
> verifying it doesn't belong to the kernel-space. The checksumming code
> we (optionally) use at the moment only covers memory module EEPROMs as
> far as I know, while EEPROMs exposed on I2C/SMBus may be of a variety of
> other natures.
> 
> [1] http://archives.andrew.net.au/lm-sensors/msg21194.html
> 
> Thanks,
> --
> Jean Delvare
> 
> 

Re: checksum in (i2c) eeprom driver

[Jean Delvare]

On 2004-12-09, Mark Studebaker wrote:

> I think the checksum code is useful because checksum=1 prevents the module
> from claiming ddc monitor eeproms and other devices in its address space
> 50-57.

DDC monitor EEPROMs *are* EEPROMs so there is no reason to exclude them
from this driver. We used to have a specific (ddcmon) driver for these
but this too is an error IMHO. Developping different eeprom drivers for
different natures of eeproms is silly (how many more?). What the ddcmon
driver was doing really belongs to user-space, not kernel-space.

There are not that many non-EEPROMs chips in the 0x50-0x57 range, only
the Maxim MAX6900 RTC according to sensors-detect (quite a rare chip at
that, we don't even have a driver for it yet).

> Since detection for eeproms is otherwise poor, it's the only way we have
> for robust detection.

Except that it only works with memory module EEPROMs.

If the checksumming was that important, I guess it would have been the
default, which it was not. If it is there for the sole purpose of
allowing the user to prevent the eeprom driver from taking over
non-eeprom chips, then the "ignore" module parameter can be used to
achieve the same effect, faster, plus it is configurable on a
per-address basis, while the checksum parameter isn't.

Thanks,
--
Jean Delvare

checksum in (i2c) eeprom driver

[Jean Delvare]

On 2004-12-09, Mark Studebaker wrote:

> I think the checksum code is useful because checksum=1 prevents the module
> from claiming ddc monitor eeproms and other devices in its address space
> 50-57.

DDC monitor EEPROMs *are* EEPROMs so there is no reason to exclude them
from this driver. We used to have a specific (ddcmon) driver for these
but this too is an error IMHO. Developping different eeprom drivers for
different natures of eeproms is silly (how many more?). What the ddcmon
driver was doing really belongs to user-space, not kernel-space.

There are not that many non-EEPROMs chips in the 0x50-0x57 range, only
the Maxim MAX6900 RTC according to sensors-detect (quite a rare chip at
that, we don't even have a driver for it yet).

> Since detection for eeproms is otherwise poor, it's the only way we have
> for robust detection.

Except that it only works with memory module EEPROMs.

If the checksumming was that important, I guess it would have been the
default, which it was not. If it is there for the sole purpose of
allowing the user to prevent the eeprom driver from taking over
non-eeprom chips, then the "ignore" module parameter can be used to
achieve the same effect, faster, plus it is configurable on a
per-address basis, while the checksum parameter isn't.

Thanks,
--
Jean Delvare

Re: checksum in (i2c) eeprom driver

[Greg KH]

On Thu, Dec 09, 2004 at 03:45:48PM +0100, Jean Delvare wrote:
> 
> If the checksumming was that important, I guess it would have been the
> default, which it was not. If it is there for the sole purpose of
> allowing the user to prevent the eeprom driver from taking over
> non-eeprom chips, then the "ignore" module parameter can be used to
> achieve the same effect, faster, plus it is configurable on a
> per-address basis, while the checksum parameter isn't.

I agree, what's wrong with using the ignore stuff instead?

thanks,

greg k-h

checksum in (i2c) eeprom driver

[Greg KH]

On Thu, Dec 09, 2004 at 03:45:48PM +0100, Jean Delvare wrote:
> 
> If the checksumming was that important, I guess it would have been the
> default, which it was not. If it is there for the sole purpose of
> allowing the user to prevent the eeprom driver from taking over
> non-eeprom chips, then the "ignore" module parameter can be used to
> achieve the same effect, faster, plus it is configurable on a
> per-address basis, while the checksum parameter isn't.

I agree, what's wrong with using the ignore stuff instead?

thanks,

greg k-h