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

[Zhang Rui <rui.zhang@intel.com>]

On Fri, 2022-07-15 at 23:09 +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 and we change the trip point
> browsing order depending on the temperature trend: in the ascending
> order when the temperature trend is raising, otherwise in the
> descending order.
> 
> 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.
> 
> Tested on a rk3399-rock960 with thermal stress and 4 trip points.
> Also
> tested with temperature emulation to create a temperature jump
> directly to the second trip point.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> V3:
> 
>   - Use the ordered indexes introduced in the previous patch as the
>     trip could be not ordered
> 
> V2:
>   - As spotted by Zhang Rui, the trip cross notification does not
>   work if the temperature drops and crosses two trip points in the
>   same update interval. In order to fix that, we browse the trip
> point
>   in the ascending order when the temperature trend is raising,
>   otherwise in the descending order.
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

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

> ---
>  drivers/thermal/thermal_core.c | 54 ++++++++++++++++++++++++--------
> --
>  include/linux/thermal.h        |  2 ++
>  2 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index f02f38b66445..a5c5f6f4e42b 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -354,30 +354,48 @@ static void handle_critical_trips(struct
> thermal_zone_device *tz,
>                 tz->ops->critical(tz);
>  }
>  
> -static void handle_thermal_trip_crossed(struct thermal_zone_device
> *tz, int trip,
> +static void handle_thermal_trip_crossed(struct thermal_zone_device
> *tz, int index,
>                                         int trip_temp, int trip_hyst,
>                                         enum thermal_trip_type
> trip_type)
>  {
> +       int trip_low_temp = trip_temp - trip_hyst;
> +       int trip = tz->trips_indexes[index];
> +       
>         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.
> +        *
> +        * Note the mechanism works only if the caller of the
> function
> +        * invoke the function with the trip point ascending or
> +        * descending regarding the temperature trend. A temperature
> +        * drop trend will browse the trip point in the descending
> +        * order
> +        */
> +       if (tz->last_temperature < trip_temp && tz->temperature >=
> trip_temp &&
> +           index != tz->prev_index) {
> +               thermal_notify_tz_trip_up(tz->id, trip, tz-
> >temperature);
> +               tz->prev_index = index;
> +       } else if (tz->last_temperature >= trip_low_temp && tz-
> >temperature < trip_low_temp &&
> +                  index == tz->prev_index) {
> +               thermal_notify_tz_trip_down(tz->id, trip, tz-
> >temperature);
> +               tz->prev_index--;
>         }
>  }
>  
> -static void handle_thermal_trip(struct thermal_zone_device *tz, int
> trip)
> +static void handle_thermal_trip(struct thermal_zone_device *tz, int
> index)
>  {
>         enum thermal_trip_type type;
>         int trip_temp, hyst = 0;
> +       int trip = tz->trips_indexes[index];
>  
>         /* Ignore disabled trip points */
>         if (test_bit(trip, &tz->trips_disabled))
> @@ -388,7 +406,7 @@ static void handle_thermal_trip(struct
> thermal_zone_device *tz, int trip)
>         if (tz->ops->get_trip_hyst)
>                 tz->ops->get_trip_hyst(tz, trip, &hyst);
>  
> -       handle_thermal_trip_crossed(tz, trip, trip_temp, hyst, type);
> +       handle_thermal_trip_crossed(tz, index, trip_temp, hyst,
> type);
>  
>         if (type == THERMAL_TRIP_CRITICAL || type ==
> THERMAL_TRIP_HOT)
>                 handle_critical_trips(tz, trip, trip_temp, type);
> @@ -428,6 +446,7 @@ static void thermal_zone_device_init(struct
> thermal_zone_device *tz)
>  {
>         struct thermal_instance *pos;
>         tz->temperature = THERMAL_TEMP_INVALID;
> +       tz->prev_index = -1;
>         tz->prev_low_trip = -INT_MAX;
>         tz->prev_high_trip = INT_MAX;
>         list_for_each_entry(pos, &tz->thermal_instances, tz_node)
> @@ -512,8 +531,13 @@ 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);
> +       if (tz->last_temperature <=  tz->temperature) {
> +               for (count = 0; count < tz->trips; count++)
> +                       handle_thermal_trip(tz, count);
> +       } else {
> +               for (count = tz->trips; count >= 0; count--)
> +                       handle_thermal_trip(tz, count);
> +       }
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_device_update);
>  
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 4c3b72536772..d512f21561f1 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -125,6 +125,7 @@ struct thermal_cooling_device {
>   * @last_temperature:  previous temperature read
>   * @emul_temperature:  emulated temperature when using
> CONFIG_THERMAL_EMULATION
>   * @passive:           1 if you've crossed a passive trip point, 0
> otherwise.
> + * @prev_index:                previous index pointing to the trip
> point the thermal zone was
>   * @prev_low_trip:     the low current temperature if you've crossed
> a passive
>                         trip point.
>   * @prev_high_trip:    the above current temperature if you've
> crossed a
> @@ -161,6 +162,7 @@ struct thermal_zone_device {
>         int last_temperature;
>         int emul_temperature;
>         int passive;
> +       int prev_index;
>         int prev_low_trip;
>         int prev_high_trip;
>         atomic_t need_update;