Bug 2588

Summary:	Can set all but active[2] temperature via trip_points
Product:	ACPI	Reporter:	Hugo Haas (hugo)
Component:	Config-Other	Assignee:	acpi_config-other
Status:	CLOSED CODE_FIX
Severity:	normal	CC:	acpi-bugzilla, stefan.seyfried, trenn
Priority:	P2
Hardware:	i386
OS:	Linux
Kernel Version:	2.6.5 + swsusp2	Subsystem:
Regression:	---	Bisected commit-id:
Attachments:	dmesg dmidecode output acpidmp output Patch making acpi_thermal_write_trip_points() consider all trip points Patch making acpi_thermal_write_trip_points() consider all trip points v2 another (shorter) patch for the same problem corrected patch #2730, against vanilla 2.6.7

Description Hugo Haas 2004-04-24 23:21:02 UTC

Distribution: Debian
Hardware Environment: Compaq Evo N410c
Problem Description:

I have found a weird problem: using
/proc/acpi/thermal_zone/TZ1/trip_points, I can set all but the
active[2] temperature.

Steps to reproduce:

root@buena:/proc/acpi/thermal_zone/TZ1# cat *
<not supported>
<polling disabled>
state:                   active[2]
temperature:             44 C
critical (S5):           94 C
passive:                 90 C: tc1=1 tc2=2 tsp=100 devices=0xc1522c00
active[0]:               80 C: devices=0xc1524d20
active[1]:               65 C: devices=0xc1524c80
active[2]:               40 C: devices=0xc1524c00

root@buena:/proc/acpi/thermal_zone/TZ1# echo -n "95:0:91:81:66:41" > trip_points

root@buena:/proc/acpi/thermal_zone/TZ1# cat *
<not supported>
<polling disabled>
state:                   active[2]
temperature:             44 C
critical (S5):           95 C
passive:                 91 C: tc1=1 tc2=2 tsp=100 devices=0xc1522c00
active[0]:               81 C: devices=0xc1524d20
active[1]:               66 C: devices=0xc1524c80
active[2]:               40 C: devices=0xc1524c00

Regards,

Hugo

Comment 1 Hugo Haas 2004-04-24 23:22:43 UTC

Created attachment 2693 [details]
dmesg

Comment 2 Hugo Haas 2004-04-24 23:23:56 UTC

Created attachment 2694 [details]
dmidecode output

Comment 3 Hugo Haas 2004-04-24 23:25:23 UTC

Created attachment 2695 [details]
acpidmp output

Comment 4 Hugo Haas 2004-04-25 00:15:16 UTC

acpi_thermal_write_trip_points() in thermal.c only reads up to active1. It
doesn't take into account other values.

Comment 5 Hugo Haas 2004-04-25 03:36:37 UTC

Created attachment 2697 [details]
Patch making acpi_thermal_write_trip_points() consider all trip points

The current acpi_thermal_write_trip_points() only considers 5 values: critical,
hot, passive, active0 and active1.

This patch makes it consider all the relevant values.

One can now do, with 1 active point:

  echo -n "100:90:80:70" > trip_points

With 3, you can do:

  echo -n "100:90:80:70:60:50" > trip_points

Etc, trip_points being under /proc/acpi/thermal_zone/*.

With this code, the limitation on the number of trip points is set by
ACPI_THERMAL_MAX_ACTIVE, which is AFAICT a global limitation.

I have tested it and it works for me.

Regards,

Hugo

Comment 6 Hugo Haas 2004-04-25 07:36:42 UTC

Created attachment 2698 [details]
Patch making acpi_thermal_write_trip_points() consider all trip points v2

The patch I previously sent had some memory management issues. This version
should be OK. For a description, please refer to #2697.

Comment 7 Stefan Seyfried 2004-04-27 02:05:59 UTC

Created attachment 2730 [details]
another (shorter) patch for the same problem

Note that with my patch, you need to supply at least 5 values, to be compatible
to the old behaviour, but lowering the limit is trivial.

Comment 8 Hugo Haas 2004-04-27 02:24:21 UTC

Yes, my patch is slightly more complex but doesn't have any hard-coded limits. 

The drawback with the simpler patch is that, would somebody have 50 trip points
(would that ever happen?), we would need to change the code again.

Another one is, as you mention, that one needs to echo a lot of trailing :0's.

But it does fix my problem (well, at least for a while as mentioned in #2592),
so either is fine by me.

Comment 9 Stefan Seyfried 2004-04-27 09:29:10 UTC

the limit of 10 trip points is in the ACPI spec,
you don't need to echo additional ":0", since sscanf just returns the number of
matches found. Thus the for "(i = 0; i < num - 3; i++)" loop. I explicitly tested
this case (my machine has four trip points and i set only 2 or three). The lower
limit of 5 arguments (2 trip points) is just for backwards compatibility with
the old behaviour where you always needed to provide exactly 5 values and can
easily be changed to allow setting of a single trip point).

So let the gurus decide on what they like better, i believe both are working well. 
;-)

Comment 10 Wang, Zhenyu Z 2004-06-28 22:34:00 UTC

Stefan,
Could you update and clear your patch? So, Len will apply it.

thanks,

Comment 11 Stefan Seyfried 2004-06-29 14:48:47 UTC

Created attachment 3284 [details]
corrected patch #2730, against vanilla 2.6.7

the change to patch #2730 is

-	int    active[9];
+	int    active[ACPI_THERMAL_MAX_ACTIVE];

it is obviously correct since i use active[0]...active[9],
ACPI_THERMAL_MAX_ACTIVE is defined as 10.

Comment 12 Len Brown 2004-11-04 13:03:57 UTC

shipped in 2.6.9
closing

Comment 13 Len Brown 2004-11-15 20:59:08 UTC

*** Bug 2074 has been marked as a duplicate of this bug. ***