linux-kernel - [PATCH RFD] thermal: core: Browse the trip points in reverse order and exit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <20201206123753.28440-1-daniel.lezcano@linaro.org>
Date:   Sun,  6 Dec 2020 13:37:53 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     daniel.lezcano@...aro.org
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Lukasz Luba <lukasz.luba@....com>,
        Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Thara Gopinath <thara.gopinath@...aro.org>,
        Amit Kucheria <amitk@...nel.org>, Len Brown <lenb@...nel.org>
Subject: [PATCH RFD] thermal: core: Browse the trip points in reverse order and exit

When reading the code it is unclear if the loop is there to handle one
trip point or all the trip points above a certain temperature.

With the current code, the throttle function is called for every trip
point crossed and it is ambiguous if that is made on purpose.

Even digging into the history up to April, 16th 2015, the initial git
import of the acpi implementation of the loop before being converted
into a generic code, it is unclear if all trip points must be handled
or not.

When the code was intially written, was it assuming there can be only
one passive or active trip point ?

The cooling effect of the system will be very different depending on
how this loop is considered.

Even the IPA governor is filtering out multiple trip points to stick
to one value. Was the code to accomodate with a bogus loop and based
on the multiple non-critical trip points ?

Another example: arch/arm64/boot/dts/exynos/exynos5433-tmu.dtsi
defines 7 passive trip points, each of them mapped to the *same*
cooling device but with different min-max states. That means the
handle trip points loop will go through all these seven trip points
and change the state of the cooling device seven times.

On the other hand, a thermal zone can have two different cooling
devices mapped to two trip points, shall we consider having both
cooling devices acting together when the second trip point is crossed
(eg. 1st trip point: cpufreq cooling device + 2nd trip point: fan), or
the second cooling device takes over the first one ? IOW, combine the
cooling effects or not ?

Depending on the expected behavior, the loop can be done in the
reverse order and exit after processing the highest trip point.

Cc: Lukasz Luba <lukasz.luba@....com>
Cc: Kukjin Kim <kgene@...nel.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Zhang Rui <rui.zhang@...el.com>
Cc: Thara Gopinath <thara.gopinath@...aro.org>
Cc: Amit Kucheria <amitk@...nel.org>
Cc: Len Brown <lenb@...nel.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
---
 drivers/thermal/thermal_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 9d6a7b7f8ab4..d745b62a63af 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -562,8 +562,9 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,

 	tz->notify_event = event;

-	for (count = 0; count < tz->trips; count++)
-		handle_thermal_trip(tz, count);
+	for (count = tz->trips - 1; count >= 0; count--)
+		if (handle_thermal_trip(tz, count))
+			break;

 	monitor_thermal_zone(tz);
 }
-- 
2.17.1