Re: [PATCH 3/3] thermal/core: Fix thermal trip cross point

From: Daniel Lezcano
Date: Fri Jul 08 2022 - 06:15:29 EST



Hi Zhang,

thanks for reviewing

On 08/07/2022 05:56, Zhang Rui wrote:
On Thu, 2022-07-07 at 23:45 +0200, Daniel Lezcano wrote:
The routine doing trip point crossing the way up or down is actually
wrong.

A trip point is composed with a trip temperature and a hysteresis.

The trip temperature is used to detect when the trip point is crossed
the way up.

The trip temperature minus the hysteresis is used to detect when the
trip point is crossed the way down.

-----------low--------high------------|

|<--------->|
| hyst |
| |
| -|--> crossed the way up
|
<---|-- crossed the way down

For that, there is a two point comparison: the current temperature
and
the previous temperature.

The actual code assumes if the current temperature is greater than
the
trip temperature and the previous temperature was lesser, then the
trip point is crossed the way up. That is true only if we crossed the
way down the low temperature boundary from the previous temperature
or
if the hysteresis is zero. The temperature can decrease between the
low and high, so the trip point is not crossed the way down and then
increase again and cross the high temperature raising a new trip
point
crossed detection which is incorrect. The same scenario happens when
crossing the way down.

The trip point crossing the way up and down must act as parenthesis,
a
trip point down must close a trip point up. Today we have multiple
trip point up without the corresponding trip point down.

In order to fix that, we store the previous trip point which gives
the
information about the previous trip.

As a sidenote, the thermal_zone_device structure has already the
prev_trip_low and prev_trip_high information which are used by the
thermal_zone_set_trips() function. This one can be changed to be
triggered by the trip temperature crossing function, which makes more
sense, and the two fields will disappear.

Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
---
drivers/thermal/thermal_core.c | 32 ++++++++++++++++++++++----------
include/linux/thermal.h | 2 ++
2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/thermal_core.c
b/drivers/thermal/thermal_core.c
index f66036b3daae..92bc9ddb6904 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -357,19 +357,30 @@ static void handle_critical_trips(struct
thermal_zone_device *tz,
static void handle_thermal_trip_crossed(struct thermal_zone_device
*tz, int trip,
int trip_temp, int trip_hyst,
enum thermal_trip_type trip_type)
{
+ int trip_low_temp = trip_temp - trip_hyst;
+
if (tz->last_temperature == THERMAL_TEMP_INVALID)
return;
- if (tz->last_temperature < trip_temp &&
- tz->temperature >= trip_temp) {
- thermal_notify_tz_trip_up(tz->id, trip,
- tz->temperature);
- }
-
- if (tz->last_temperature >= trip_temp &&
- tz->temperature < (trip_temp - trip_hyst)) {
- thermal_notify_tz_trip_down(tz->id, trip,
- tz->temperature);
+ /*
+ * Due to the hysteresis, a third information is needed to
+ * detect when the temperature is wavering between the
+ * trip_low_temp and the trip_temp. A trip point is crossed
+ * the way up only if the temperature is above it while the
+ * previous temperature was below *and* we crossed the
+ * trip_temp_low before. The previous trip point give us the
+ * previous trip point transition. The similar problem exists
+ * when crossing the way down.
+ */
+ if (tz->last_temperature < trip_temp && tz->temperature >=
trip_temp &&
+ trip != tz->prev_trip) {
+ thermal_notify_tz_trip_up(tz->id, trip, tz-
temperature);
+ tz->prev_trip = trip;
+
+ } else if (tz->last_temperature >= trip_low_temp && tz-
temperature < trip_low_temp &&
+ trip == tz->prev_trip) {
+ thermal_notify_tz_trip_down(tz->id, trip, tz-
temperature);
+ tz->prev_trip = trip - 1;

Say, let's assume hysteresis is Zero,
When the temperature increases and we do thermal_notify_tz_trip_up()
for trip 0 and trip 1, tz->prev_trip is set to 1 in this case.
And then the temperature drops below trip 0, we don't have chance to do
thermal_notify_tz_trip_down() for trip 0, because we always handle the
trips in ascending order, and tz->prev_trip is 1 when we do
handle_thermal_trip(0).

Well I don't see how to handle this case, except accepting the detection will happen at the next temperature update, no ?


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog