RFD: Illegal code in lm75.c (and other drivers?)

RFD: Illegal code in lm75.c (and other drivers?)

[Michael Lawnick]

Hi all,

Request For Discussion

I've already patched lm75.c for me in kernel 2.6.26.6, now I find the
same code again in 2.6.28.7:

> 	/* Now, we do the remaining detection. There is no identification-
> 	   dedicated register so we have to rely on several tricks:
> 	   unused bits, registers cycling over 8-address boundaries,
> 	   addresses 0x04-0x07 returning the last read value.
> 	   The cycling+unused addresses combination is not tested,
> 	   since it would significantly slow the detection down and would
> 	   hardly add any value. */
> 	if (kind < 0) {
> 		int cur, conf, hyst, os;
> 
> 		/* Unused addresses */
> 		cur = i2c_smbus_read_word_data(new_client, 0);
> 		conf = i2c_smbus_read_byte_data(new_client, 1);
> 		hyst = i2c_smbus_read_word_data(new_client, 2);
> 		if (i2c_smbus_read_word_data(new_client, 4) != hyst
> 		 || i2c_smbus_read_word_data(new_client, 5) != hyst
> 		 || i2c_smbus_read_word_data(new_client, 6) != hyst
> 		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> 			return -ENODEV;
>
> 		os = i2c_smbus_read_word_data(new_client, 3);
> 		if (i2c_smbus_read_word_data(new_client, 4) != os
> 		 || i2c_smbus_read_word_data(new_client, 5) != os
> 		 || i2c_smbus_read_word_data(new_client, 6) != os
> 		 || i2c_smbus_read_word_data(new_client, 7) != os)
> 			return -ENODEV;
> 
> 		/* Unused bits */
> 		if (conf & 0xe0)
> 			return -ENODEV;
> 
> 		/* Addresses cycling */
> 		for (i = 8; i < 0xff; i += 8)
> 			if (i2c_smbus_read_byte_data(new_client, i + 1) != conf
> 			 || i2c_smbus_read_word_data(new_client, i + 2) != hyst
> 			 || i2c_smbus_read_word_data(new_client, i + 3) != os)
> 				return -ENODEV;
> 	}
> 

LM75 Manual (National Semiconductors, April 2001) states on page 10:
> 1.11 POINTER REGISTER
> (Selects which registers will be read from or written to):
>           +--+--+--+--+--+--+----+----+
> |P7|P6|P5|P4|P3|P2| P1 | P0 |
> +--+--+--+--+--+--+----+----+
> | 0| 0| 0| 0| 0| 0|Register |
> |  |  |  |  |  |  | Select  |
> +--+--+--+--+--+--+----+----+
> 
> P0-P1: Register Select:
>           +--+--+------------------------------------------+
>           |P1|P0|Register                                  |
>           +--+--+------------------------------------------+
>           | 0| 0|Temperature (Read only) (Power-up default)|
>           +--+--+------------------------------------------+
>           | 0| 1|Configuration (Read/Write)                |
>           +--+--+------------------------------------------+
>           | 1| 0|THYST (Read/Write)                        |
>           +--+--+------------------------------------------+
>           | 1| 1|TOS (Read/Write)                          |
>           +--+--+------------------------------------------+
> 
> P2–P7: Must be kept zero.

Therefore the above code does illegal actions. It tries to access
offsets, that are not allowed.
The code may work on system with SMBUS controllers, but it does not (on
all) systems with IIC:
Already the offset-write phase fails (NACK from LM75) and so
i2c_smbus_read_byte_data() returns with an error code.

My proposal:
reduce LM75 code (and of all other devices that do same illegal things)
to test what is testable:

> 	if (kind < 0) {
> 		/* Check config register for the unused bits to be 0 */
> 		if (i2c_smbus_read_byte_data(new_client, 1) & 0xe0)
> 			return -ENODEV;
>
> 	}
>

Any comments before patch?

-- 
Kind regards,
Michael

Re: RFD: Illegal code in lm75.c (and other drivers?)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

On Wed, Mar 11, 2009 at 12:30:31PM +0100, Michael Lawnick wrote:
> Any comments before patch?

Yep, try using the hwmon-ML instead of I2C ;)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: RFD: Illegal code in lm75.c (and other drivers?)

[Michael Lawnick]

Wolfram Sang said the following:
> On Wed, Mar 11, 2009 at 12:30:31PM +0100, Michael Lawnick wrote:
>> Any comments before patch?
> 
> Yep, try using the hwmon-ML instead of I2C ;)
> 
:-P

Ok.
-- 
Regards,
Michael