[PATCH 1/2] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[PATCH 1/2] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[Thomas Renninger]

Some BIOSes return a negative value for the critical trip point.
We currently invalidate the whole thermal zone in this case.
But it may still be needed for cooling, also without critical
trip point:


Same if there is no critical trip point, but ACPI spec says
there must at least be one trip point, it need not to be a
critical (see next patch).

Reference: http://bugzilla.novell.com/show_bug.cgi?id=531547

Signed-off-by: Thomas Renninger <trenn@suse.de>
Tested-by: clarkt@cnsp.com
CC: Len Brown <lenb@kernel.org>
CC: linux-acpi@vger.kernel.org
---
 drivers/acpi/thermal.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 9073ada..8fa71b8 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -368,7 +368,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
 	int valid = 0;
 	int i;
 
-	/* Critical Shutdown (required) */
+	/* Critical Shutdown */
 	if (flag & ACPI_TRIPS_CRITICAL) {
 		status = acpi_evaluate_integer(tz->device->handle,
 				"_CRT", NULL, &tmp);
@@ -379,17 +379,19 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
 		 * Below zero (Celsius) values clearly aren't right for sure..
 		 * ... so lets discard those as invalid.
 		 */
-		if (ACPI_FAILURE(status) ||
-				tz->trips.critical.temperature <= 2732) {
+		if (ACPI_FAILURE(status)) {
+			tz->trips.critical.flags.valid = 0;
+			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+					  "No critical threshold\n"));
+		} else if (tmp <= 2732) {
+			printk(KERN_WARNING FW_BUG "Invalid critical threshold "
+			       "(_CRT:%)\n", tmp);
 			tz->trips.critical.flags.valid = 0;
-			ACPI_EXCEPTION((AE_INFO, status,
-					"No or invalid critical threshold"));
-			return -ENODEV;
 		} else {
 			tz->trips.critical.flags.valid = 1;
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-					"Found critical threshold [%lu]\n",
-					tz->trips.critical.temperature));
+					  "Found critical threshold [%lu]\n",
+					  tz->trips.critical.temperature));
 		}
 		if (tz->trips.critical.flags.valid == 1) {
 			if (crt == -1) {
-- 
1.6.3

[PATCH 2/2] ACPI thermal: Check for thermal zone requirement

[Thomas Renninger]

ACPI spec says (11.5 Thermal Zone Interface Requirements):
   A thermal zone must contain at least one trip point
   (critical, near critical, active, or passive)

Check this once at init time.

Signed-off-by: Thomas Renninger <trenn@suse.de>
Tested-by: clarkt@cnsp.com
CC: Len Brown <lenb@kernel.org>
CC: linux-acpi@vger.kernel.org
---
 drivers/acpi/thermal.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 8fa71b8..09b757a 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -577,7 +577,23 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
 
 static int acpi_thermal_get_trip_points(struct acpi_thermal *tz)
 {
-	return acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
+	int i, valid, ret = acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
+
+	if (ret)
+		return ret;
+
+	valid = tz->trips.critical.flags.valid |
+		tz->trips.hot.flags.valid |
+		tz->trips.passive.flags.valid;
+
+	for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++)
+		valid |= tz->trips.active[i].flags.valid;
+
+	if (!valid) {
+		printk(KERN_WARNING FW_BUG "No valid trip found\n");
+		return -ENODEV;
+	}
+	return 0;
 }
 
 static void acpi_thermal_check(void *data)
-- 
1.6.3

Re: [PATCH 1/2] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[Len Brown]

applied

This issue rings a bell -- I think we discussed that it was a theoretical 
problem a while back.  I'm glad that SuSE got a live tester and you were 
able to follow up, Thomas.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 2/2] ACPI thermal: Check for thermal zone requiremen

[Len Brown]

Thomas,

What good things happen after this patch that didn't happen before it?

thanks,
Len Brown, Intel Open Source Technology Center

On Tue, 16 Feb 2010, Thomas Renninger wrote:

> ACPI spec says (11.5 Thermal Zone Interface Requirements):
>    A thermal zone must contain at least one trip point
>    (critical, near critical, active, or passive)
> 
> Check this once at init time.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> Tested-by: clarkt@cnsp.com
> CC: Len Brown <lenb@kernel.org>
> CC: linux-acpi@vger.kernel.org
> ---
>  drivers/acpi/thermal.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 8fa71b8..09b757a 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -577,7 +577,23 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
>  
>  static int acpi_thermal_get_trip_points(struct acpi_thermal *tz)
>  {
> -	return acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
> +	int i, valid, ret = acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT);
> +
> +	if (ret)
> +		return ret;
> +
> +	valid = tz->trips.critical.flags.valid |
> +		tz->trips.hot.flags.valid |
> +		tz->trips.passive.flags.valid;
> +
> +	for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++)
> +		valid |= tz->trips.active[i].flags.valid;
> +
> +	if (!valid) {
> +		printk(KERN_WARNING FW_BUG "No valid trip found\n");
> +		return -ENODEV;
> +	}
> +	return 0;
>  }
>  
>  static void acpi_thermal_check(void *data)
> -- 
> 1.6.3
> 

Re: [PATCH 1/2] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[Len Brown]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 152 bytes --]

drivers/acpi/thermal.c:388: warning: unknown conversion type character ‘)’ in format
drivers/acpi/thermal.c:388: warning: too many arguments for format

Re: [PATCH 2/2] ACPI thermal: Check for thermal zone requiremen

[Thomas Renninger]

On Friday 19 February 2010 07:39:43 Len Brown wrote:
> Thomas,
> 
> What good things happen after this patch that didn't happen before it?

The guy sees a valid temperature, in /proc-/sys and also in the
corresponding X-apps (which his original complaints were).

The thermal zone has a valid passive trip point (95 C), which now is 
active again.

He reported this as a regression, let me double check:

Oh dear..., _CRT doesn't return anything for Windows 2006
(or newer, not defined Windowses) -> broken trip point.

There is a Linux workaround (which does not work anymore,
this probably was the reason for the regression -> OSI!=Linux),
which make _CRT return a valid trip point like on old Windowses.

So this makes the behavior more "latest Windows OS" like.
Hmm, depends whether Windows still takes the thermal zone into
account if _CRT is not available or invalid.
The fact that a valid _HOT trip point is only exported on latest
Windowses, hardens the guess that Windows also serves this
thermal zone with an invalid critical trip point.
It looks like the (TPOS == 0x40) thermal workarounds wants to avoid
thermal shutdowns (critical) in case 100 C are reached and change it
into a (debug?) message (_HOT) on latest Windowses.

   Thomas

Related ASL output:


If (_OSI ("Linux"))
                {
                    Store (One, LINX)
                    Store (0x80, OSTB)
                    Store (0x80, TPOS)
                }
If (_OSI ("Windows 2006"))
                {
                    Store (0x40, OSTB)
                    Store (0x40, TPOS)
                }

...
Name (TPC, 0x64)
Method (_HOT, 0, Serialized)
{
    If (LEqual (TPOS, 0x40))
    {
        Return (Add (0x0AAC, Multiply (TPC, 0x0A)))
    }
}

Method (_CRT, 0, Serialized)
{
    If (LNotEqual (TPOS, 0x40))
    {
        Return (Add (0x0AAC, Multiply (TPC, 0x0A)))
    }
}

Re: [PATCH 2/2] ACPI thermal: Check for thermal zone requiremen

[Len Brown]

> > What good things happen after this patch that didn't happen before it?
> 
> The guy sees a valid temperature, in /proc-/sys and also in the
> corresponding X-apps (which his original complaints were).
>
> The thermal zone has a valid passive trip point (95 C), which now is 
> active again.

I understand the 1st patch to not disqualify a TZ if it has
a bogus _CRT.  That would make a TZ show up rather than
get discarded.  If that patch compiled cleanly, I'd apply it...

I don't understand this patch to scan a TZ for trip points
and print out a kernel warning about a firmware bug
for TZ's which have none, and disqualify those TZ's.
That isn't going to make temperature available when it
was not already available.  That is going to add an additional
kernel message about a firmware bug we can't fix, and could
actually delete a thermal zone with a working temperature
that used to be present, no?

> He reported this as a regression, let me double check:
> 
> Oh dear..., _CRT doesn't return anything for Windows 2006
> (or newer, not defined Windowses) -> broken trip point.
> 
> There is a Linux workaround (which does not work anymore,
> this probably was the reason for the regression -> OSI!=Linux),
> which make _CRT return a valid trip point like on old Windowses.
> 
> So this makes the behavior more "latest Windows OS" like.
> Hmm, depends whether Windows still takes the thermal zone into
> account if _CRT is not available or invalid.
> The fact that a valid _HOT trip point is only exported on latest
> Windowses, hardens the guess that Windows also serves this
> thermal zone with an invalid critical trip point.
> It looks like the (TPOS == 0x40) thermal workarounds wants to avoid
> thermal shutdowns (critical) in case 100 C are reached and change it
> into a (debug?) message (_HOT) on latest Windowses.


> Related ASL output:
> 
> 
> If (_OSI ("Linux"))
>                 {
>                     Store (One, LINX)
>                     Store (0x80, OSTB)
>                     Store (0x80, TPOS)
>                 }
> If (_OSI ("Windows 2006"))
>                 {
>                     Store (0x40, OSTB)
>                     Store (0x40, TPOS)
>                 }
> 
> ...
> Name (TPC, 0x64)
> Method (_HOT, 0, Serialized)
> {
>     If (LEqual (TPOS, 0x40))
>     {
>         Return (Add (0x0AAC, Multiply (TPC, 0x0A)))
>     }
> }
> 
> Method (_CRT, 0, Serialized)
> {
>     If (LNotEqual (TPOS, 0x40))
>     {
>         Return (Add (0x0AAC, Multiply (TPC, 0x0A)))
>     }
> }

It appears that if acpi_osi=Linux were set (or TPOS were
anything other than Windows 2006), then you
would not get a _HOT or _CRT in this TZ.
I don't understand how that is related to this issue,
as acpi_osi=Linux is not set, right?

acpi_osi="Windows 2006", on the other hand, should be set.
In that case the AML will return a valid trip point for
both _HOT and _CRT, yes?  So what is the problem?

-Len

Re: [PATCH 2/2] ACPI thermal: Check for thermal zone requiremen

[Thomas Renninger]

On Friday 19 February 2010 17:20:51 Len Brown wrote:
> > > What good things happen after this patch that didn't happen before it?
> > 
> > The guy sees a valid temperature, in /proc-/sys and also in the
> > corresponding X-apps (which his original complaints were).
> >
> > The thermal zone has a valid passive trip point (95 C), which now is 
> > active again.
> 
> I understand the 1st patch to not disqualify a TZ if it has
> a bogus _CRT.  That would make a TZ show up rather than
> get discarded.  If that patch compiled cleanly, I'd apply it...
> 
> I don't understand this patch to scan a TZ for trip points
> and print out a kernel warning about a firmware bug
> for TZ's which have none, and disqualify those TZ's.
> That isn't going to make temperature available when it
> was not already available.  That is going to add an additional
> kernel message about a firmware bug we can't fix,
Hmm, Firmware bugs which we can't fix and can't even work around always
should at least end up in a firmware bug/warning message.

> and could actually delete a thermal zone with a working temperature
> that used to be present, no?
Not really. At least not worse than before.
Before my previous patch every thermal zone without a valid critical
trip point was discarded with an error message.
Now you need at least one.

My patch sticks to the spec and only discards the thermal zone if there
is no trip point at all. In this case you want to throw a (firmware bug)
message as something is obviously wrong.

     Thomas

Re: [PATCH 2/2] ACPI thermal: Check for thermal zone requiremen

[Len Brown]

On Fri, 19 Feb 2010, Thomas Renninger wrote:

> On Friday 19 February 2010 17:20:51 Len Brown wrote:
> > > > What good things happen after this patch that didn't happen before it?
> > > 
> > > The guy sees a valid temperature, in /proc-/sys and also in the
> > > corresponding X-apps (which his original complaints were).
> > >
> > > The thermal zone has a valid passive trip point (95 C), which now is 
> > > active again.
> > 
> > I understand the 1st patch to not disqualify a TZ if it has
> > a bogus _CRT.  That would make a TZ show up rather than
> > get discarded.  If that patch compiled cleanly, I'd apply it...
> > 
> > I don't understand this patch to scan a TZ for trip points
> > and print out a kernel warning about a firmware bug
> > for TZ's which have none, and disqualify those TZ's.
> > That isn't going to make temperature available when it
> > was not already available.  That is going to add an additional
> > kernel message about a firmware bug we can't fix,
> Hmm, Firmware bugs which we can't fix and can't even work around always
> should at least end up in a firmware bug/warning message.
> 
> > and could actually delete a thermal zone with a working temperature
> > that used to be present, no?

> Not really. At least not worse than before.
> Before my previous patch every thermal zone without a valid critical
> trip point was discarded with an error message.

i agree with the 1st patch,
as I said, if it built cleanly (hint hint) I'd apply it.

> Now you need at least one.
> 
> My patch sticks to the spec and only discards the thermal zone if there
> is no trip point at all. In this case you want to throw a (firmware bug)
> message as something is obviously wrong.

This is the non-obvious part.
Yes, a thermal zone that has no trip point doesn't follow the spec.
I don't know if any exist or not, but I don't see any harm if they do.
I think it would be dandy for linuxfirmware test kit to look
for this BIOS issue, but I don't see how the user is helped
if the kernel looks for it.  All the get is an additional
kernel message and perhaps the loss of the ability to
tell the temperature when perhaps they could before.
That doesn't sound like a step forward.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 2/2] ACPI thermal: Check for thermal zone requiremen

[Thomas Renninger]

On Saturday 20 February 2010 05:57:30 am Len Brown wrote:
> On Fri, 19 Feb 2010, Thomas Renninger wrote:

> i agree with the 1st patch,
> as I said, if it built cleanly (hint hint) I'd apply it.
Coming soon.

> > Now you need at least one.
> >
> > My patch sticks to the spec and only discards the thermal zone if there
> > is no trip point at all. In this case you want to throw a (firmware bug)
> > message as something is obviously wrong.
>
> This is the non-obvious part.
> Yes, a thermal zone that has no trip point doesn't follow the spec.
> I don't know if any exist or not, but I don't see any harm if they do.
> I think it would be dandy for linuxfirmware test kit to look
> for this BIOS issue, but I don't see how the user is helped
> if the kernel looks for it.  All the get is an additional
> kernel message and perhaps the loss of the ability to
> tell the temperature when perhaps they could before.
> That doesn't sound like a step forward.

Thinking about this again:
I fully agree.
Just ignore the 2nd patch.

Thanks,

       Thomas

Re: [PATCH 2/2] ACPI thermal: Check for thermal zone requiremen

[Thomas Renninger]

Hi,

I haven't seen your post at then end of the mail...

On Friday 19 February 2010 05:20:51 pm Len Brown wrote:
> >
> > Related ASL output:
> >
> >
> > If (_OSI ("Linux"))
> >                 {
> >                     Store (One, LINX)
> >                     Store (0x80, OSTB)
> >                     Store (0x80, TPOS)
> >                 }
> > If (_OSI ("Windows 2006"))
> >                 {
> >                     Store (0x40, OSTB)
> >                     Store (0x40, TPOS)
> >                 }
> >
> > ...
> > Name (TPC, 0x64)
> > Method (_HOT, 0, Serialized)
> > {
> >     If (LEqual (TPOS, 0x40))
> >     {
> >         Return (Add (0x0AAC, Multiply (TPC, 0x0A)))
> >     }
> > }
> >
> > Method (_CRT, 0, Serialized)
> > {
> >     If (LNotEqual (TPOS, 0x40))
> >     {
> >         Return (Add (0x0AAC, Multiply (TPC, 0x0A)))
> >     }
> > }
>
> It appears that if acpi_osi=Linux were set (or TPOS were
> anything other than Windows 2006), then you
> would not get a _HOT or _CRT in this TZ.
> I don't understand how that is related to this issue,
> as acpi_osi=Linux is not set, right?
>
> acpi_osi="Windows 2006", on the other hand, should be set.
> In that case the AML will return a valid trip point for
> both _HOT and _CRT, yes?  So what is the problem?
The problem is that in:
      - _HOT it's a LEqual (TPOS, 0x40)
      - _CRT it's a LNotEqual (TPOS, 0x40)

You will get a valid hot tp on Windows 2006 (or newer) and an invalid critical 
one.
On all others you get an invalid hot, but a valid critical tp (which is also 
the case with osi=Linux).

      Thomas

[PATCH] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[Thomas Renninger]

Some BIOSes return a negative value for the critical trip point.
We currently invalidate the whole thermal zone in this case.
But it may still be needed for cooling, also without critical
trip point:


Same if there is no critical trip point, but ACPI spec says
there must at least be one trip point, it need not to be a
critical (see next patch).

Reference: http://bugzilla.novell.com/show_bug.cgi?id=531547

Signed-off-by: Thomas Renninger <trenn@suse.de>
Tested-by: clarkt@cnsp.com
CC: Len Brown <lenb@kernel.org>
CC: linux-acpi@vger.kernel.org
---
 drivers/acpi/thermal.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 9073ada..8fa71b8 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -368,7 +368,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
 	int valid = 0;
 	int i;
 
-	/* Critical Shutdown (required) */
+	/* Critical Shutdown */
 	if (flag & ACPI_TRIPS_CRITICAL) {
 		status = acpi_evaluate_integer(tz->device->handle,
 				"_CRT", NULL, &tmp);
@@ -379,17 +379,19 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
 		 * Below zero (Celsius) values clearly aren't right for sure..
 		 * ... so lets discard those as invalid.
 		 */
-		if (ACPI_FAILURE(status) ||
-				tz->trips.critical.temperature <= 2732) {
+		if (ACPI_FAILURE(status)) {
+			tz->trips.critical.flags.valid = 0;
+			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+					  "No critical threshold\n"));
+		} else if (tmp <= 2732) {
+			printk(KERN_WARNING FW_BUG "Invalid critical threshold "
+			       "(_CRT:%)\n", tmp);
 			tz->trips.critical.flags.valid = 0;
-			ACPI_EXCEPTION((AE_INFO, status,
-					"No or invalid critical threshold"));
-			return -ENODEV;
 		} else {
 			tz->trips.critical.flags.valid = 1;
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-					"Found critical threshold [%lu]\n",
-					tz->trips.critical.temperature));
+					  "Found critical threshold [%lu]\n",
+					  tz->trips.critical.temperature));
 		}
 		if (tz->trips.critical.flags.valid == 1) {
 			if (crt == -1) {
-- 
1.6.3

Re: [PATCH] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[Thomas Renninger]

On Saturday 20 February 2010 11:20:41 am Thomas Renninger wrote:

> +			printk(KERN_WARNING FW_BUG "Invalid critical threshold "
> +			       "(_CRT:%)\n", tmp);
Argh, I forgot to refresh, resending in a minute.

Sorry about that,

      Thomas

[PATCH] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[Thomas Renninger]

V2: Corrected integer/long conversion.

Some BIOSes return a negative value for the critical trip point.
Especially since Windows 2006...
We currently invalidate the whole thermal zone in this case.
But it may still be needed for cooling, also without critical
trip point.

This patch invalidates the critical trip point if no _CRT
function is found or if it returns negative values, but does
not invalidate the whole thermal zone in this case.

Reference: http://bugzilla.novell.com/show_bug.cgi?id=531547

Signed-off-by: Thomas Renninger <trenn@suse.de>
Tested-by: clarkt@cnsp.com
CC: clarkt@cnsp.com
CC: Len Brown <lenb@kernel.org>
CC: linux-acpi@vger.kernel.org
---
 drivers/acpi/thermal.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 9073ada..77b8e1e 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -368,7 +368,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
 	int valid = 0;
 	int i;
 
-	/* Critical Shutdown (required) */
+	/* Critical Shutdown */
 	if (flag & ACPI_TRIPS_CRITICAL) {
 		status = acpi_evaluate_integer(tz->device->handle,
 				"_CRT", NULL, &tmp);
@@ -379,17 +379,19 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
 		 * Below zero (Celsius) values clearly aren't right for sure..
 		 * ... so lets discard those as invalid.
 		 */
-		if (ACPI_FAILURE(status) ||
-				tz->trips.critical.temperature <= 2732) {
+		if (ACPI_FAILURE(status)) {
+			tz->trips.critical.flags.valid = 0;
+			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+					  "No critical threshold\n"));
+		} else if (tmp <= 2732) {
+			printk(KERN_WARNING FW_BUG "Invalid critical threshold "
+			       "(%llu)\n", tmp);
 			tz->trips.critical.flags.valid = 0;
-			ACPI_EXCEPTION((AE_INFO, status,
-					"No or invalid critical threshold"));
-			return -ENODEV;
 		} else {
 			tz->trips.critical.flags.valid = 1;
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-					"Found critical threshold [%lu]\n",
-					tz->trips.critical.temperature));
+					  "Found critical threshold [%lu]\n",
+					  tz->trips.critical.temperature));
 		}
 		if (tz->trips.critical.flags.valid == 1) {
 			if (crt == -1) {
-- 
1.6.3

Re: [PATCH] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[Zhang Rui]

On Sat, 2010-02-20 at 18:44 +0800, Thomas Renninger wrote:
> V2: Corrected integer/long conversion.
> 
> Some BIOSes return a negative value for the critical trip point.
> Especially since Windows 2006...
> We currently invalidate the whole thermal zone in this case.
> But it may still be needed for cooling, also without critical
> trip point.
> 
> This patch invalidates the critical trip point if no _CRT
> function is found or if it returns negative values, but does
> not invalidate the whole thermal zone in this case.
> 
> Reference: http://bugzilla.novell.com/show_bug.cgi?id=531547
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> Tested-by: clarkt@cnsp.com
> CC: clarkt@cnsp.com
> CC: Len Brown <lenb@kernel.org>
> CC: linux-acpi@vger.kernel.org
> ---
>  drivers/acpi/thermal.c |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 9073ada..77b8e1e 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -368,7 +368,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
>  	int valid = 0;
>  	int i;
>  
> -	/* Critical Shutdown (required) */
> +	/* Critical Shutdown */
>  	if (flag & ACPI_TRIPS_CRITICAL) {
>  		status = acpi_evaluate_integer(tz->device->handle,
>  				"_CRT", NULL, &tmp);
> @@ -379,17 +379,19 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
>  		 * Below zero (Celsius) values clearly aren't right for sure..
>  		 * ... so lets discard those as invalid.
>  		 */
> -		if (ACPI_FAILURE(status) ||
> -				tz->trips.critical.temperature <= 2732) {
> +		if (ACPI_FAILURE(status)) {
> +			tz->trips.critical.flags.valid = 0;
> +			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> +					  "No critical threshold\n"));

No critical threshold is also a violation of ACPI spec.
what about using FW_BUG here as well?

thanks,
rui

> +		} else if (tmp <= 2732) {
> +			printk(KERN_WARNING FW_BUG "Invalid critical threshold "
> +			       "(%llu)\n", tmp);
>  			tz->trips.critical.flags.valid = 0;
> -			ACPI_EXCEPTION((AE_INFO, status,
> -					"No or invalid critical threshold"));
> -			return -ENODEV;
>  		} else {
>  			tz->trips.critical.flags.valid = 1;
>  			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> -					"Found critical threshold [%lu]\n",
> -					tz->trips.critical.temperature));
> +					  "Found critical threshold [%lu]\n",
> +					  tz->trips.critical.temperature));
>  		}
>  		if (tz->trips.critical.flags.valid == 1) {
>  			if (crt == -1) {

Re: [PATCH] ACPI thermal: Don't invalidate thermal zone if critical trip point is bad

[Zhang Rui]

On Mon, 2010-02-22 at 07:02 +0800, Thomas Renninger wrote:
> On Sunday 21 February 2010 03:51:22 am Zhang Rui wrote:
> ...
> > > @@ -379,17 +379,19 @@ static int acpi_thermal_trips_update(struct
> > > acpi_thermal *tz, int flag) * Below zero (Celsius) values clearly aren't
> > > right for sure.. * ... so lets discard those as invalid.
> > >  		 */
> > > -		if (ACPI_FAILURE(status) ||
> > > -				tz->trips.critical.temperature <= 2732) {
> > > +		if (ACPI_FAILURE(status)) {
> > > +			tz->trips.critical.flags.valid = 0;
> > > +			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > > +					  "No critical threshold\n"));
> >
> > No critical threshold is also a violation of ACPI spec.
> > what about using FW_BUG here as well?
> 
> Could you point me to where this is stated, please.
> I only found Chapter 11.5 (ver. 3.0b):
>  Thermal Zone Interface Requirements:
>    A thermal zone must contain at least one trip point
>    (critical, near critical, active, or passive)
> 
> If at another place they state that a critical trip point is
> required, this would contradict with each other.
> 
you're right.
I thought I saw such statements somewhere in the ACPI spec but
apparently I'm wrong. Sorry for the noise.

Acked-by: Zhang Rui <rui.zhang@intel.com>


> Thanks,
> 
>     Thomas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html