Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <13318886.uLZWGnKmhe@kreacher> <9147669.CDJkKcVGEf@kreacher>
In-Reply-To: <9147669.CDJkKcVGEf@kreacher>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 19 Jul 2023 20:50:01 +0200
Message-ID: <CAJZ5v0jWQHq-YSQtsOPeriH1_7-3gFQvNHawv7J9D8rzxXichQ@mail.gmail.com>
Subject: Re: [PATCH v1 6/7] ACPI: thermal: Rework thermal_get_trend()
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc:     Linux ACPI <linux-acpi@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Michal Wilczynski <michal.wilczynski@intel.com>,
        Zhang Rui <rui.zhang@intel.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, Jul 18, 2023 at 8:21 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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@intel.com>
> ---
>  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)
>
>
>