[lm-sensors] [PATCH RFC] hwmon: (max16065) Add chip access warning

[lm-sensors] [PATCH RFC] hwmon: (max16065) Add chip access warning

[Guenter Roeck]

The chips supported by the max16065 driver should not be accessed using direct
i2ctools commands. Add warning to driver documentation to alert users.

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
RFC: Do we want this kind of warning in driver documentation ?

 Documentation/hwmon/max16065 |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/hwmon/max16065 b/Documentation/hwmon/max16065
index 44b4f61..c11f64a 100644
--- a/Documentation/hwmon/max16065
+++ b/Documentation/hwmon/max16065
@@ -62,6 +62,13 @@ can be safely used to identify the chip. You will have to instantiate
 the devices explicitly. Please see Documentation/i2c/instantiating-devices for
 details.
 
+WARNING: Do not access chip registers using the i2cdump command, and do not use
+any of the i2ctools commands on a command register (0xa5 to 0xac). The chips
+supported by this driver interpret any access to a command register (including
+read commands) as request to execute the command in question. This may result in
+power loss, board resets, and/or Flash corruption. Worst case, your board may
+turn into a brick.
+
 
 Sysfs entries
 -------------
-- 
1.7.3.1


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH RFC] hwmon: (max16065) Add chip access

[Jean Delvare]

On Mon, 29 Aug 2011 22:57:56 -0700, Guenter Roeck wrote:
> The chips supported by the max16065 driver should not be accessed using direct
> i2ctools commands. Add warning to driver documentation to alert users.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
> RFC: Do we want this kind of warning in driver documentation ?

Sure, this can't hurt. Maybe we could add a section in i2c-tools'
documentation as well, listing the dangerous chips, starting with this
one? i2cdetect already has a note about the AT24RF08 for historical
reasons, I wouldn't mind documenting the "dangerous" chips more
prominently. Hopefully the list will stay short.

> 
>  Documentation/hwmon/max16065 |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/hwmon/max16065 b/Documentation/hwmon/max16065
> index 44b4f61..c11f64a 100644
> --- a/Documentation/hwmon/max16065
> +++ b/Documentation/hwmon/max16065
> @@ -62,6 +62,13 @@ can be safely used to identify the chip. You will have to instantiate
>  the devices explicitly. Please see Documentation/i2c/instantiating-devices for
>  details.
>  
> +WARNING: Do not access chip registers using the i2cdump command, and do not use
> +any of the i2ctools commands on a command register (0xa5 to 0xac). The chips
> +supported by this driver interpret any access to a command register (including
> +read commands) as request to execute the command in question. This may result in
> +power loss, board resets, and/or Flash corruption. Worst case, your board may
> +turn into a brick.
> +
>  
>  Sysfs entries
>  -------------

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH RFC] hwmon: (max16065) Add chip access

[Guenter Roeck]

On Tue, Aug 30, 2011 at 03:14:12AM -0400, Jean Delvare wrote:
> On Mon, 29 Aug 2011 22:57:56 -0700, Guenter Roeck wrote:
> > The chips supported by the max16065 driver should not be accessed using direct
> > i2ctools commands. Add warning to driver documentation to alert users.
> > 
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> > RFC: Do we want this kind of warning in driver documentation ?
> 
> Sure, this can't hurt. Maybe we could add a section in i2c-tools'
> documentation as well, listing the dangerous chips, starting with this
> one? i2cdetect already has a note about the AT24RF08 for historical
> reasons, I wouldn't mind documenting the "dangerous" chips more
> prominently. Hopefully the list will stay short.
> 
Hopefully yes. Many of the PMBus chips are potentially affected, though.
They typically have a means to protect settings from overwrite, but if that
is not enabled, one can be in hot water. Worst I have seen so far was
to execute i2cdump on an eval board and it lost its configuration :(.

[ ... ]

> 
> Acked-by: Jean Delvare <khali@linux-fr.org>
> 
Thanks for the review!

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH RFC] hwmon: (max16065) Add chip access

[Jean Delvare]

On Tue, 30 Aug 2011 07:26:44 -0700, Guenter Roeck wrote:
> On Tue, Aug 30, 2011 at 03:14:12AM -0400, Jean Delvare wrote:
> > On Mon, 29 Aug 2011 22:57:56 -0700, Guenter Roeck wrote:
> > > The chips supported by the max16065 driver should not be accessed using direct
> > > i2ctools commands. Add warning to driver documentation to alert users.
> > > 
> > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > ---
> > > RFC: Do we want this kind of warning in driver documentation ?
> > 
> > Sure, this can't hurt. Maybe we could add a section in i2c-tools'
> > documentation as well, listing the dangerous chips, starting with this
> > one? i2cdetect already has a note about the AT24RF08 for historical
> > reasons, I wouldn't mind documenting the "dangerous" chips more
> > prominently. Hopefully the list will stay short.
> > 
> Hopefully yes. Many of the PMBus chips are potentially affected, though.
> They typically have a means to protect settings from overwrite, but if that
> is not enabled, one can be in hot water. Worst I have seen so far was
> to execute i2cdump on an eval board and it lost its configuration :(.

You are lucky. Back in 2002, worse a few lm-sensors users saw was their
shiny new Thinkpad laptop tuned into a brick by sensors-detect (not
even i2c-tools) due to what ended up being a state machine bug in the
AT24RF08.)

Speaking of this... At which I2C addresses to these PMBus devices live?
MAX16065/66 in particular but also other chips with similar problems.
If these are addresses sensors-detect is probing, then it's only a
matter of time before we get a report from a user hitting the problem.

Probably it's about time to let the kernel block user-space probing of
specific I2C buses.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH RFC] hwmon: (max16065) Add chip access

[Guenter Roeck]

On Tue, Aug 30, 2011 at 11:26:05AM -0400, Jean Delvare wrote:
> On Tue, 30 Aug 2011 07:26:44 -0700, Guenter Roeck wrote:
> > On Tue, Aug 30, 2011 at 03:14:12AM -0400, Jean Delvare wrote:
> > > On Mon, 29 Aug 2011 22:57:56 -0700, Guenter Roeck wrote:
> > > > The chips supported by the max16065 driver should not be accessed using direct
> > > > i2ctools commands. Add warning to driver documentation to alert users.
> > > > 
> > > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > > ---
> > > > RFC: Do we want this kind of warning in driver documentation ?
> > > 
> > > Sure, this can't hurt. Maybe we could add a section in i2c-tools'
> > > documentation as well, listing the dangerous chips, starting with this
> > > one? i2cdetect already has a note about the AT24RF08 for historical
> > > reasons, I wouldn't mind documenting the "dangerous" chips more
> > > prominently. Hopefully the list will stay short.
> > > 
> > Hopefully yes. Many of the PMBus chips are potentially affected, though.
> > They typically have a means to protect settings from overwrite, but if that
> > is not enabled, one can be in hot water. Worst I have seen so far was
> > to execute i2cdump on an eval board and it lost its configuration :(.
> 
> You are lucky. Back in 2002, worse a few lm-sensors users saw was their
> shiny new Thinkpad laptop tuned into a brick by sensors-detect (not
> even i2c-tools) due to what ended up being a state machine bug in the
> AT24RF08.)
> 
> Speaking of this... At which I2C addresses to these PMBus devices live?
> MAX16065/66 in particular but also other chips with similar problems.

MAX16065/66 is not a PMBus device ... by default, it resides at 0x50..0x53
(possibly the worst address space available for such a device).
That address can be overwritten by software to any valid address.
It does not have an ID register, so it can not be auto-detected.

PMBus devices can unfortunately be at any valid address. Many chips use a set
of resistors to set the address. Even those not using resistors can often
be programmed by SW to any address.

> If these are addresses sensors-detect is probing, then it's only a
> matter of time before we get a report from a user hitting the problem.
> 
Might well be. Most don't react too badly on reads, though. I had the problem
specifically with the Intersil chips. On those, the byte reads done by i2cdump
on write-only command registers are accepted as write commands. This causes
the chip configuration to be lost unless all writes are disabled/secured.
Which was not the case on my eval board. Oops ...

Turning PMBus chips into bricks is actually quite simple - I managed to do it
with several chips from multiple vendors. Since one has to do that on purpose
or by being careless (as in my case ;), so I would not count that against
the chips.

> Probably it's about time to let the kernel block user-space probing of
> specific I2C buses.

Sounds like a good idea. This is one reason why we don't set I2C_CLASS_HWMON
in our internal I2C adapter drivers.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH RFC] hwmon: (max16065) Add chip access

[Jean Delvare]

Hi Guenter,

On Tue, 30 Aug 2011 09:13:24 -0700, Guenter Roeck wrote:
> On Tue, Aug 30, 2011 at 11:26:05AM -0400, Jean Delvare wrote:
> > You are lucky. Back in 2002, worse a few lm-sensors users saw was their
> > shiny new Thinkpad laptop tuned into a brick by sensors-detect (not
> > even i2c-tools) due to what ended up being a state machine bug in the
> > AT24RF08.)
> > 
> > Speaking of this... At which I2C addresses to these PMBus devices live?
> > MAX16065/66 in particular but also other chips with similar problems.
> 
> MAX16065/66 is not a PMBus device ... by default, it resides at 0x50..0x53
> (possibly the worst address space available for such a device).

Indeed. We might have to stop probing these addresses completely in
sensors-detect, too bad for the ADM1033/34. The detection currently
only accesses registers 0x3d, 0x3e and 0x3f, hopefully it's OK for the
MAX16065/66.

> That address can be overwritten by software to any valid address.
> It does not have an ID register, so it can not be auto-detected.

I've seen many chips with this software address change feature, but
I've rarely seen it used in practice. Such address changes are only
needed in case of address collisions, and in this case you can't access
the device in the first place without an alternative register access
method or a switch or multiplexer on the I2C bus. In which case
changing the address is no longer mandatory.

> PMBus devices can unfortunately be at any valid address. Many chips use a set
> of resistors to set the address. Even those not using resistors can often
> be programmed by SW to any address.

We will have to address problems as they show up on a case by case
basis then.

> > If these are addresses sensors-detect is probing, then it's only a
> > matter of time before we get a report from a user hitting the problem.
>
> Might well be. Most don't react too badly on reads, though.

The problem is that not all chips agree on what a register read is, due
to the flexibility (or lack of standardization) of the I2C protocol.
Thankfully PMBus devices should be implemented as SMBus slaves so they
should follow the standard SMBus transactions. However in practice you
found they don't always do.

> I had the problem
> specifically with the Intersil chips. On those, the byte reads done by i2cdump
> on write-only command registers are accepted as write commands. This causes
> the chip configuration to be lost unless all writes are disabled/secured.
> Which was not the case on my eval board. Oops ...
> 
> Turning PMBus chips into bricks is actually quite simple - I managed to do it
> with several chips from multiple vendors. Since one has to do that on purpose
> or by being careless (as in my case ;), so I would not count that against
> the chips.

You can always blame anyone when this happens... the loose I2C
specification, the chip manufacturer, Linux tools, yourself... It's
really a combination of all these that leads to disasters, and only the
last two we have a control over.

> > Probably it's about time to let the kernel block user-space probing of
> > specific I2C buses.
> 
> Sounds like a good idea. This is one reason why we don't set I2C_CLASS_HWMON
> in our internal I2C adapter drivers.

This doesn't currently protect you against sensors-detect, that's the
problem. The lack of I2C_CLASS_HWMON only protects you against kernel
hwmon drivers.

Some months ago I made sensors-detect more clever in which I2C adapters
it accepts to probe by default. In particular, we skip I2C adapters on
PCI multimedia controllers. However this is only an unreliable band
aid, there's definitely room for improvement.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors