[lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Guenter Roeck]

A chip returning 0x00 in all registers is erroneously detected
as LM75. Check hysteresis and temperature limit registers and
abort if both are 0 to reduce the likelyhood for this to happen.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm75.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index f58439b..6753fd9 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -415,6 +415,12 @@ static int lm75_detect(struct i2c_client *new_client,
 		 || i2c_smbus_read_byte_data(new_client, 7) != os)
 			return -ENODEV;
 	}
+	/*
+	 * It is very unlikely that this is a LM75 if both
+	 * hysteresis and temperature limit registers are 0.
+	 */
+	if (hyst = 0 && os = 0)
+		return -ENODEV;
 
 	/* Addresses cycling */
 	for (i = 8; i <= 248; i += 40) {
-- 
1.9.1


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

Re: [lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Robert Coulson]

Hello Guenter,

does it also make sense to include a check for the conf != 0 here (it
should be set to 0 for POR from the datasheet)? Other than this
quesiton/comment, the code looks fine to me.

thanks,

*** Rob.



On Thu, Dec 4, 2014 at 10:04 AM, Guenter Roeck <linux@roeck-us.net> wrote:

> A chip returning 0x00 in all registers is erroneously detected
> as LM75. Check hysteresis and temperature limit registers and
> abort if both are 0 to reduce the likelyhood for this to happen.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/lm75.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index f58439b..6753fd9 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -415,6 +415,12 @@ static int lm75_detect(struct i2c_client *new_client,
>                  || i2c_smbus_read_byte_data(new_client, 7) != os)
>                         return -ENODEV;
>         }
> +       /*
> +        * It is very unlikely that this is a LM75 if both
> +        * hysteresis and temperature limit registers are 0.
> +        */
> +       if (hyst = 0 && os = 0)
> +               return -ENODEV;
>
>         /* Addresses cycling */
>         for (i = 8; i <= 248; i += 40) {
> --
> 1.9.1
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Guenter Roeck]

On Thu, Dec 04, 2014 at 12:16:29PM -0800, Robert Coulson wrote:
> Hello Guenter,
> 
> does it also make sense to include a check for the conf != 0 here (it
> should be set to 0 for POR from the datasheet)? Other than this
> quesiton/comment, the code looks fine to me.
> 
Hi Rob,

the configuration register could be set to a non-zero value,
for example by the BIOS or rommon. Sure, that is in theory possible
for the hysteresis and limit registers as well, but that would be
both unlikely and unreasonable.

Thanks,
Guenter

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

Re: [lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Robert Coulson]

On Thu, Dec 4, 2014 at 1:08 PM, Guenter Roeck <linux@roeck-us.net> wrote:

> On Thu, Dec 04, 2014 at 12:16:29PM -0800, Robert Coulson wrote:
> > Hello Guenter,
> >
> > does it also make sense to include a check for the conf != 0 here (it
> > should be set to 0 for POR from the datasheet)? Other than this
> > quesiton/comment, the code looks fine to me.
> >
> Hi Rob,
>
> the configuration register could be set to a non-zero value,
> for example by the BIOS or rommon. Sure, that is in theory possible
> for the hysteresis and limit registers as well, but that would be
> both unlikely and unreasonable.
>
> Thanks,
> Guenter
>

 Good points. Without explicit values for chip detection, it will never be
guaranteed.

I appreciate your feedback,

*** Rob.

Reviewed-by: <rob.coulson@gmail.com>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Jean Delvare]

Hi Guenter,

On Thu,  4 Dec 2014 10:04:10 -0800, Guenter Roeck wrote:
> A chip returning 0x00 in all registers is erroneously detected
> as LM75. Check hysteresis and temperature limit registers and
> abort if both are 0 to reduce the likelyhood for this to happen.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/lm75.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index f58439b..6753fd9 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -415,6 +415,12 @@ static int lm75_detect(struct i2c_client *new_client,
>  		 || i2c_smbus_read_byte_data(new_client, 7) != os)
>  			return -ENODEV;
>  	}
> +	/*
> +	 * It is very unlikely that this is a LM75 if both
> +	 * hysteresis and temperature limit registers are 0.
> +	 */
> +	if (hyst = 0 && os = 0)
> +		return -ENODEV;
>  
>  	/* Addresses cycling */
>  	for (i = 8; i <= 248; i += 40) {

Looks reasonable.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

Did sensors-detect misdetect that chip as an LM75 too, or was the
extended detection logic there good enough already?

-- 
Jean Delvare
SUSE L3 Support

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

Re: [lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Guenter Roeck]

On 12/05/2014 03:57 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Thu,  4 Dec 2014 10:04:10 -0800, Guenter Roeck wrote:
>> A chip returning 0x00 in all registers is erroneously detected
>> as LM75. Check hysteresis and temperature limit registers and
>> abort if both are 0 to reduce the likelyhood for this to happen.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/hwmon/lm75.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
>> index f58439b..6753fd9 100644
>> --- a/drivers/hwmon/lm75.c
>> +++ b/drivers/hwmon/lm75.c
>> @@ -415,6 +415,12 @@ static int lm75_detect(struct i2c_client *new_client,
>>   		 || i2c_smbus_read_byte_data(new_client, 7) != os)
>>   			return -ENODEV;
>>   	}
>> +	/*
>> +	 * It is very unlikely that this is a LM75 if both
>> +	 * hysteresis and temperature limit registers are 0.
>> +	 */
>> +	if (hyst = 0 && os = 0)
>> +		return -ENODEV;
>>
>>   	/* Addresses cycling */
>>   	for (i = 8; i <= 248; i += 40) {
>
> Looks reasonable.
>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>
> Did sensors-detect misdetect that chip as an LM75 too, or was the
> extended detection logic there good enough already?
>

Hi Jean,

sensors-detect is fine. Easy to test -load i2c-stub and see what
happens. I assume that is due to the "All registers hold same value"
test. Should I use that test instead ? I kind of prefer it.

Thanks,
Guenter


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

Re: [lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Jean Delvare]

On Fri, 05 Dec 2014 06:14:00 -0800, Guenter Roeck wrote:
> > Did sensors-detect misdetect that chip as an LM75 too, or was the
> > extended detection logic there good enough already?
> 
> sensors-detect is fine. Easy to test -load i2c-stub and see what
> happens.

That doesn't always work because the value returned for some
non-existent register addresses depend on the previous read. You can
only test that with a real chip. Plus I didn't know which chip was
being misdetected for you ;-)

> I assume that is due to the "All registers hold same value"
> test. Should I use that test instead ? I kind of prefer it.

The tests are different and thus may result in different outcomes for
various chips. It's very hard to predict which is better. All I can say
is that the sensors-detect code needs the value of the current
temperature register, which isn't read during detection currently, so
using that logic would make driver loading slightly slower. Not sure if
it really matters.

All in all, I think that having the same detection code on both side is
important, as it avoids unexpected results. I don't really care which
one you pick, it can always be adjusted later (as is has already been
over the years) if misdetections are reported.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

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

Re: [lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Guenter Roeck]

Hi Jean,

On Fri, Dec 05, 2014 at 04:15:41PM +0100, Jean Delvare wrote:
> On Fri, 05 Dec 2014 06:14:00 -0800, Guenter Roeck wrote:
> > > Did sensors-detect misdetect that chip as an LM75 too, or was the
> > > extended detection logic there good enough already?
> > 
> > sensors-detect is fine. Easy to test -load i2c-stub and see what
> > happens.
> 
> That doesn't always work because the value returned for some
> non-existent register addresses depend on the previous read. You can

Good point.

> only test that with a real chip. Plus I didn't know which chip was
> being misdetected for you ;-)
> 
I was trying to run a module test for tmp421 while having the lm75 driver
loaded. Loading the i2c-stub driver with address 0x4c caused lm75 detection
to run, which detected the simulated chip at 0x4c as lm75 (because all
registers in i2c-stub are initially set to 0) and instantiated it. This of
course was a bit of a bummer when I tried to manually instantiate the tmp421
afterwards, causing my module test to fail. So it wasn't a real chip, just
an annoying side effect of trying to use i2c-stub while the lm75 driver
was loaded.

> > I assume that is due to the "All registers hold same value"
> > test. Should I use that test instead ? I kind of prefer it.
> 
> The tests are different and thus may result in different outcomes for
> various chips. It's very hard to predict which is better. All I can say
> is that the sensors-detect code needs the value of the current
> temperature register, which isn't read during detection currently, so
> using that logic would make driver loading slightly slower. Not sure if

Good point.

> it really matters.
> 
> All in all, I think that having the same detection code on both side is
> important, as it avoids unexpected results. I don't really care which
> one you pick, it can always be adjusted later (as is has already been
> over the years) if misdetections are reported.
> 
Guess I'll apply the patch as-is. If anything I wonder if it would make sense
to add the same test to sensors-detect (in addition to the comparison).

Thanks,
Guenter

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

Re: [lm-sensors] [PATCH] hwmon: (lm75) Strengthen detect function

[Jean Delvare]

Hi Guenter,

On Fri, 5 Dec 2014 08:47:24 -0800, Guenter Roeck wrote:
> I was trying to run a module test for tmp421 while having the lm75 driver
> loaded. Loading the i2c-stub driver with address 0x4c caused lm75 detection
> to run, which detected the simulated chip at 0x4c as lm75 (because all
> registers in i2c-stub are initially set to 0) and instantiated it. This of
> course was a bit of a bummer when I tried to manually instantiate the tmp421
> afterwards, causing my module test to fail. So it wasn't a real chip, just
> an annoying side effect of trying to use i2c-stub while the lm75 driver
> was loaded.

Yes, I had the same problem with i2c-stub in the past while testing
multiple drivers. I've fixed detection functions each time it happened.
Feel tree to fix every i2c detection function which trips on all-zeroes.

> (...)
> Guess I'll apply the patch as-is. If anything I wonder if it would make sense
> to add the same test to sensors-detect (in addition to the comparison).

As you wish, I'm fine both ways.

-- 
Jean Delvare
SUSE L3 Support

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