[lm-sensors] [PATCH 2/3] hwmon: Fix applesmc temperature handling

[lm-sensors] [PATCH 2/3] hwmon: Fix applesmc temperature handling

[Matthew Garrett]

The temperature registers in the applesmc hardware are signed. It's likely
that a negative value is invalid, so trigger on that and return an error
rather than an erroneous value.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/hwmon/applesmc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 62e2493..4540bb1 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -781,6 +781,10 @@ static ssize_t applesmc_show_temperature(struct device *dev,
 		return ret;
 
 	if (entry->len = 2) {
+		if (buffer[0] >= 0x80) {
+			/* The two byte format is signed - ignore negative */
+			return -EINVAL;
+		}
 		temp = buffer[0] * 1000;
 		temp += (buffer[1] >> 6) * 250;
 	} else {
-- 
1.7.6


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

Re: [lm-sensors] [PATCH 2/3] hwmon: Fix applesmc temperature

[Guenter Roeck]

On Tue, 2011-08-09 at 12:14 -0400, Matthew Garrett wrote:
> The temperature registers in the applesmc hardware are signed. It's likely
> that a negative value is invalid, so trigger on that and return an error
> rather than an erroneous value.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/hwmon/applesmc.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 62e2493..4540bb1 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -781,6 +781,10 @@ static ssize_t applesmc_show_temperature(struct device *dev,
>  		return ret;
>  
>  	if (entry->len = 2) {
> +		if (buffer[0] >= 0x80) {
> +			/* The two byte format is signed - ignore negative */
> +			return -EINVAL;
> +		}

Kind of unusual. Do you have evidence that such an error is in fact
seen ? If there is no such evidence, I'd rather keep the original code.
We don't usually return errors for negative temperature readings just
because we assume that the readings might be erroneous.

Thanks,
Guenter



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

Re: [lm-sensors] [PATCH 2/3] hwmon: Fix applesmc temperature

[Matthew Garrett]

On Wed, Aug 10, 2011 at 10:05:05AM -0700, Guenter Roeck wrote:
> >  	if (entry->len = 2) {
> > +		if (buffer[0] >= 0x80) {
> > +			/* The two byte format is signed - ignore negative */
> > +			return -EINVAL;
> > +		}
> 
> Kind of unusual. Do you have evidence that such an error is in fact
> seen ? If there is no such evidence, I'd rather keep the original code.
> We don't usually return errors for negative temperature readings just
> because we assume that the readings might be erroneous.

Yeah, my MBA returns these from TCZ3 and TH0F. Typical read value is 
0xf978. There may be some other underlying issue that's triggering this, 
but returning data that's known to be bogus seems unhelpful.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

Re: [lm-sensors] [PATCH 2/3] hwmon: Fix applesmc temperature

[Guenter Roeck]

On Wed, 2011-08-10 at 13:17 -0400, Matthew Garrett wrote:
> On Wed, Aug 10, 2011 at 10:05:05AM -0700, Guenter Roeck wrote:
> > >  	if (entry->len = 2) {
> > > +		if (buffer[0] >= 0x80) {
> > > +			/* The two byte format is signed - ignore negative */
> > > +			return -EINVAL;
> > > +		}
> > 
> > Kind of unusual. Do you have evidence that such an error is in fact
> > seen ? If there is no such evidence, I'd rather keep the original code.
> > We don't usually return errors for negative temperature readings just
> > because we assume that the readings might be erroneous.
> 
> Yeah, my MBA returns these from TCZ3 and TH0F. Typical read value is 
> 0xf978. There may be some other underlying issue that's triggering this, 
> but returning data that's known to be bogus seems unhelpful.
> 
Sounds like that is a persistent condition, though, not a temporary one.
Negative values returned from temperature sensors often indicate either
a non-existing thermistor or a sensor fault.

Personally, I think it would be better to just ignore the returned
value. Ultimately, you'll have to add an entry into sensors.conf anyway,
and returning an error just because the returned value _might_ be wrong
does not seem to be a good idea to me.

Either case, -EINVAL seems wrong. Maybe -EIO.

Thanks,
Guenter



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

Re: [lm-sensors] [PATCH 2/3] hwmon: Fix applesmc temperature

[Matthew Garrett]

On Wed, Aug 10, 2011 at 10:41:06AM -0700, Guenter Roeck wrote:
> On Wed, 2011-08-10 at 13:17 -0400, Matthew Garrett wrote:
> > Yeah, my MBA returns these from TCZ3 and TH0F. Typical read value is 
> > 0xf978. There may be some other underlying issue that's triggering this, 
> > but returning data that's known to be bogus seems unhelpful.
> > 
> Sounds like that is a persistent condition, though, not a temporary one.
> Negative values returned from temperature sensors often indicate either
> a non-existing thermistor or a sensor fault.
> 
> Personally, I think it would be better to just ignore the returned
> value. Ultimately, you'll have to add an entry into sensors.conf anyway,
> and returning an error just because the returned value _might_ be wrong
> does not seem to be a good idea to me.

We can pretty much guarantee that this hardware won't be running below 
freezing in this case. Smarter userspace can see an error and then 
ignore the sensor in the UI.

> Either case, -EINVAL seems wrong. Maybe -EIO.

Seems reasonable.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

Re: [lm-sensors] [PATCH 2/3] hwmon: Fix applesmc temperature

[Henrik Rydberg]

On Wed, Aug 10, 2011 at 06:57:09PM +0100, Matthew Garrett wrote:
> On Wed, Aug 10, 2011 at 10:41:06AM -0700, Guenter Roeck wrote:
> > On Wed, 2011-08-10 at 13:17 -0400, Matthew Garrett wrote:
> > > Yeah, my MBA returns these from TCZ3 and TH0F. Typical read value is 
> > > 0xf978. There may be some other underlying issue that's triggering this, 
> > > but returning data that's known to be bogus seems unhelpful.
> > > 
> > Sounds like that is a persistent condition, though, not a temporary one.
> > Negative values returned from temperature sensors often indicate either
> > a non-existing thermistor or a sensor fault.
> > 
> > Personally, I think it would be better to just ignore the returned
> > value. Ultimately, you'll have to add an entry into sensors.conf anyway,
> > and returning an error just because the returned value _might_ be wrong
> > does not seem to be a good idea to me.
> 
> We can pretty much guarantee that this hardware won't be running below 
> freezing in this case. Smarter userspace can see an error and then 
> ignore the sensor in the UI.

To that end, userspace can equally well read the negative value.

Bogus sensor values is an integral part of some of the machines. For
instance, on some models, remove the battery and some temperature
sensors will start to oscillate between zero and max. Some time ago, I
tried to come up with a patch which would detect this behavior, so far
without positive reports. If anything, it would make sense to filter
the list of available sensors, either dynamically (removing it from
the sysfs tree) or by clamping the value to zero or
something. Reporting an error does not seem very useful.

Thanks,
Henrik

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

Re: [lm-sensors] [PATCH 2/3] hwmon: Fix applesmc temperature

[Matthew Garrett]

On Thu, Aug 11, 2011 at 09:36:22PM +0200, Henrik Rydberg wrote:
> instance, on some models, remove the battery and some temperature
> sensors will start to oscillate between zero and max. Some time ago, I
> tried to come up with a patch which would detect this behavior, so far
> without positive reports. If anything, it would make sense to filter
> the list of available sensors, either dynamically (removing it from
> the sysfs tree) or by clamping the value to zero or
> something. Reporting an error does not seem very useful.

We *know* that the value is incorrect. How is giving it to userspace 
without that information in any way helpful? Once userspace has opened 
the file and performed read(), we need to give it something.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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