Re: [PATCH v1 6/7] ACPI: thermal: Rework thermal_get_trend()

[Rafael J. Wysocki]

On Tue, Jul 18, 2023 at 8:21 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Rework the ACPI thermal driver's .get_trend() callback function,
> thermal_get_trend(), to use trip point data stored in the generic
> trip structures instead of calling thermal_get_trip_type() and
> thermal_get_trip_temp() and make it hold thermal_check_lock to
> protect against possible races against trip point updates.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>  drivers/acpi/thermal.c |  107 +++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 78 insertions(+), 29 deletions(-)
>
> Index: linux-pm/drivers/acpi/thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/thermal.c
> +++ linux-pm/drivers/acpi/thermal.c
> @@ -572,47 +572,96 @@ static int thermal_get_crit_temp(struct
>         return -EINVAL;
>  }
>
> +static struct thermal_trip *acpi_thermal_get_trip(struct acpi_thermal *tz,
> +                                          int trip_index)
> +{
> +       struct thermal_trip *trip;
> +       int i;
> +
> +       if (!tz || trip_index < 0)
> +               return NULL;
> +
> +       trip = tz->trips.critical.trip_ref.trip;
> +       if (trip) {
> +               if (!trip_index)
> +                       return trip;
> +
> +               trip_index--;
> +       }
> +
> +       trip = tz->trips.hot.trip_ref.trip;
> +       if (trip) {
> +               if (!trip_index)
> +                       return trip;
> +
> +               trip_index--;
> +       }
> +
> +       trip = tz->trips.passive.trip_ref.trip;
> +       if (trip) {
> +               if (!trip_index)
> +                       return trip;
> +
> +               trip_index--;
> +       }
> +
> +       for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
> +               trip = tz->trips.active[i].trip_ref.trip;
> +               if (trip) {
> +                       if (!trip_index)
> +                               return trip;
> +
> +                       trip_index--;
> +               }
> +       }
> +
> +       return NULL;
> +}
> +
>  static int thermal_get_trend(struct thermal_zone_device *thermal,
> -                            int trip, enum thermal_trend *trend)
> +                            int trip_index, enum thermal_trend *trend)
>  {
>         struct acpi_thermal *tz = thermal_zone_device_priv(thermal);
> -       enum thermal_trip_type type;
> -       int i;
> +       struct thermal_trip *trip;
> +       int ret = 0;
>
> -       if (thermal_get_trip_type(thermal, trip, &type))
> -               return -EINVAL;
> +       mutex_lock(&tz->thermal_check_lock);
>
> -       if (type == THERMAL_TRIP_ACTIVE) {
> -               int trip_temp;
> +       trip = acpi_thermal_get_trip(tz, trip_index);
> +       if (!trip) {

This should also return an error for trips with invalid temperature.

Moreover, an error should be returned for the critical and hot trips,
because it doesn't make sense to deal with them here.

It looks like a new version of this patch is needed.

> +               ret = -EINVAL;
> +               goto out;
> +       }
> +       if (trip->type == THERMAL_TRIP_ACTIVE) {
>                 int temp = deci_kelvin_to_millicelsius_with_offset(
>                                         tz->temperature, tz->kelvin_offset);
> -               if (thermal_get_trip_temp(thermal, trip, &trip_temp))
> -                       return -EINVAL;
>
> -               if (temp > trip_temp) {
> +               if (temp > trip->temperature)
>                         *trend = THERMAL_TREND_RAISING;
> -                       return 0;
> -               } else {
> -                       /* Fall back on default trend */
> -                       return -EINVAL;
> -               }
> +               else /* Fall back on default trend */
> +                       ret = -EINVAL;
> +       } else {
> +               /*
> +                * tz->temperature has already been updated by generic thermal
> +                * layer, before this callback being invoked.
> +                */
> +               int i = tz->trips.passive.tc1 * (tz->temperature -
> +                               tz->last_temperature) +
> +                       tz->trips.passive.tc2 * (tz->temperature -
> +                               tz->trips.passive.temperature);
> +
> +               if (i > 0)
> +                       *trend = THERMAL_TREND_RAISING;
> +               else if (i < 0)
> +                       *trend = THERMAL_TREND_DROPPING;
> +               else
> +                       *trend = THERMAL_TREND_STABLE;
>         }
>
> -       /*
> -        * tz->temperature has already been updated by generic thermal layer,
> -        * before this callback being invoked
> -        */
> -       i = tz->trips.passive.tc1 * (tz->temperature - tz->last_temperature) +
> -           tz->trips.passive.tc2 * (tz->temperature - tz->trips.passive.temperature);
> -
> -       if (i > 0)
> -               *trend = THERMAL_TREND_RAISING;
> -       else if (i < 0)
> -               *trend = THERMAL_TREND_DROPPING;
> -       else
> -               *trend = THERMAL_TREND_STABLE;
> +out:
> +       mutex_unlock(&tz->thermal_check_lock);
>
> -       return 0;
> +       return ret;
>  }
>
>  static void acpi_thermal_zone_device_hot(struct thermal_zone_device *thermal)
>
>
>