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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0gjz+QrwKmU3FVGVpW2OXNgXPmuT7nXmMxzOsf-ZfTOxQ@mail.gmail.com>
Date:   Tue, 24 Jan 2023 21:43:11 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     Linux PM <linux-pm@...r.kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux ACPI <linux-acpi@...r.kernel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v7 3/3] thermal: intel: int340x: Use generic trip points

On Mon, Jan 23, 2023 at 7:41 PM Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>
> From: Daniel Lezcano <daniel.lezcano@...aro.org>
>
> The thermal framework gives the possibility to register the trip
> points along with the thermal zone. When that is done, no get_trip_*
> callbacks are needed and they can be removed.
>
> Convert the existing callbacks content logic into generic trip points
> initialization code and register them along with the thermal zone.
>
> In order to consolidate the code, use ACPI trip library functions to
> populate generic trip points.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> Reviewed-by: Zhang Rui <rui.zhang@...el.com>
> [ rjw: Subject and changelog edits, rebase ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

This fix from Srinivas:

https://lore.kernel.org/linux-pm/20230123172110.376549-1-srinivas.pandruvada@linux.intel.com/

clearly shows that there are problems with this patch.

> ---
>  drivers/thermal/intel/int340x_thermal/Kconfig                |    1
>  drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c |  172 ++---------
>  drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h |   10
>  3 files changed, 48 insertions(+), 135 deletions(-)
>
> Index: linux-pm/drivers/thermal/intel/int340x_thermal/Kconfig
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/int340x_thermal/Kconfig
> +++ linux-pm/drivers/thermal/intel/int340x_thermal/Kconfig
> @@ -9,6 +9,7 @@ config INT340X_THERMAL
>         select THERMAL_GOV_USER_SPACE
>         select ACPI_THERMAL_REL
>         select ACPI_FAN
> +       select THERMAL_ACPI
>         select INTEL_SOC_DTS_IOSF_CORE
>         select INTEL_TCC
>         select PROC_THERMAL_MMIO_RAPL if POWERCAP
> Index: linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> +++ linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> @@ -37,65 +37,6 @@ static int int340x_thermal_get_zone_temp
>         return 0;
>  }
>
> -static int int340x_thermal_get_trip_temp(struct thermal_zone_device *zone,
> -                                        int trip, int *temp)
> -{
> -       struct int34x_thermal_zone *d = zone->devdata;
> -       int i;
> -
> -       if (trip < d->aux_trip_nr)
> -               *temp = d->aux_trips[trip];
> -       else if (trip == d->crt_trip_id)
> -               *temp = d->crt_temp;
> -       else if (trip == d->psv_trip_id)
> -               *temp = d->psv_temp;
> -       else if (trip == d->hot_trip_id)
> -               *temp = d->hot_temp;
> -       else {
> -               for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) {
> -                       if (d->act_trips[i].valid &&
> -                           d->act_trips[i].id == trip) {
> -                               *temp = d->act_trips[i].temp;
> -                               break;
> -                       }
> -               }
> -               if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT)
> -                       return -EINVAL;
> -       }
> -
> -       return 0;
> -}
> -
> -static int int340x_thermal_get_trip_type(struct thermal_zone_device *zone,
> -                                        int trip,
> -                                        enum thermal_trip_type *type)
> -{
> -       struct int34x_thermal_zone *d = zone->devdata;
> -       int i;
> -
> -       if (trip < d->aux_trip_nr)
> -               *type = THERMAL_TRIP_PASSIVE;
> -       else if (trip == d->crt_trip_id)
> -               *type = THERMAL_TRIP_CRITICAL;
> -       else if (trip == d->hot_trip_id)
> -               *type = THERMAL_TRIP_HOT;
> -       else if (trip == d->psv_trip_id)
> -               *type = THERMAL_TRIP_PASSIVE;
> -       else {
> -               for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) {
> -                       if (d->act_trips[i].valid &&
> -                           d->act_trips[i].id == trip) {
> -                               *type = THERMAL_TRIP_ACTIVE;
> -                               break;
> -                       }
> -               }
> -               if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT)
> -                       return -EINVAL;
> -       }
> -
> -       return 0;
> -}
> -
>  static int int340x_thermal_set_trip_temp(struct thermal_zone_device *zone,
>                                       int trip, int temp)
>  {
> @@ -109,20 +50,15 @@ static int int340x_thermal_set_trip_temp
>         if (ACPI_FAILURE(status))
>                 return -EIO;
>
> -       d->aux_trips[trip] = temp;
> -
>         return 0;
>  }
>
> -
> -static int int340x_thermal_get_trip_hyst(struct thermal_zone_device *zone,
> -               int trip, int *temp)
> +static int int340x_thermal_get_global_hyst(struct acpi_device *adev, int *temp)
>  {
> -       struct int34x_thermal_zone *d = zone->devdata;
>         acpi_status status;
>         unsigned long long hyst;
>
> -       status = acpi_evaluate_integer(d->adev->handle, "GTSH", NULL, &hyst);
> +       status = acpi_evaluate_integer(adev->handle, "GTSH", NULL, &hyst);
>         if (ACPI_FAILURE(status))
>                 *temp = 0;
>         else
> @@ -131,6 +67,7 @@ static int int340x_thermal_get_trip_hyst
>         return 0;
>  }
>
> +
>  static void int340x_thermal_critical(struct thermal_zone_device *zone)
>  {
>         dev_dbg(&zone->device, "%s: critical temperature reached\n", zone->type);
> @@ -138,58 +75,36 @@ static void int340x_thermal_critical(str
>
>  static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
>         .get_temp       = int340x_thermal_get_zone_temp,
> -       .get_trip_temp  = int340x_thermal_get_trip_temp,
> -       .get_trip_type  = int340x_thermal_get_trip_type,
>         .set_trip_temp  = int340x_thermal_set_trip_temp,
> -       .get_trip_hyst =  int340x_thermal_get_trip_hyst,
>         .critical       = int340x_thermal_critical,
>  };
>
> -static int int340x_thermal_get_trip_config(acpi_handle handle, char *name,
> -                                     int *temp)
> -{
> -       unsigned long long r;
> -       acpi_status status;
> -
> -       status = acpi_evaluate_integer(handle, name, NULL, &r);
> -       if (ACPI_FAILURE(status))
> -               return -EIO;
> -
> -       *temp = deci_kelvin_to_millicelsius(r);
> -
> -       return 0;
> -}
> -
>  int int340x_thermal_read_trips(struct int34x_thermal_zone *int34x_zone)

First of all, this function can be invoked from int3403_notify() to
update the trip points in the case of a firmware notification.

This, of course, can be racing with the thermal core's use of the trip
points and I don't think that there is a way to synchronize that right
now.

Second, the hysteresis value set by int340x_thermal_zone_add() will be
overwritten with zero by the new code below.

I'm dropping this one for now (and the fix on top of it) and we'll
need to revisit it.

>  {
> -       int trip_cnt = int34x_zone->aux_trip_nr;
> -       int i;
> +       int trip_cnt;
> +       int i, ret;
>
> -       int34x_zone->crt_trip_id = -1;
> -       if (!int340x_thermal_get_trip_config(int34x_zone->adev->handle, "_CRT",
> -                                            &int34x_zone->crt_temp))
> -               int34x_zone->crt_trip_id = trip_cnt++;
> -
> -       int34x_zone->hot_trip_id = -1;
> -       if (!int340x_thermal_get_trip_config(int34x_zone->adev->handle, "_HOT",
> -                                            &int34x_zone->hot_temp))
> -               int34x_zone->hot_trip_id = trip_cnt++;
> -
> -       int34x_zone->psv_trip_id = -1;
> -       if (!int340x_thermal_get_trip_config(int34x_zone->adev->handle, "_PSV",
> -                                            &int34x_zone->psv_temp))
> -               int34x_zone->psv_trip_id = trip_cnt++;
> +       trip_cnt = int34x_zone->aux_trip_nr;
> +
> +       ret = thermal_acpi_trip_critical(int34x_zone->adev, &int34x_zone->trips[trip_cnt]);
> +       if (!ret)
> +               trip_cnt++;
> +
> +       ret = thermal_acpi_trip_hot(int34x_zone->adev, &int34x_zone->trips[trip_cnt]);
> +       if (!ret)
> +               trip_cnt++;
> +
> +       ret = thermal_acpi_trip_passive(int34x_zone->adev, &int34x_zone->trips[trip_cnt]);
> +       if (!ret)
> +               trip_cnt++;
>
>         for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) {
> -               char name[5] = { '_', 'A', 'C', '0' + i, '\0' };
>
> -               if (int340x_thermal_get_trip_config(int34x_zone->adev->handle,
> -                                       name,
> -                                       &int34x_zone->act_trips[i].temp))
> +               ret = thermal_acpi_trip_active(int34x_zone->adev, i, &int34x_zone->trips[trip_cnt]);
> +               if (ret)
>                         break;
>
> -               int34x_zone->act_trips[i].id = trip_cnt++;
> -               int34x_zone->act_trips[i].valid = true;
> +               trip_cnt++;
>         }
>
>         return trip_cnt;
> @@ -208,7 +123,7 @@ struct int34x_thermal_zone *int340x_ther
>         acpi_status status;
>         unsigned long long trip_cnt;
>         int trip_mask = 0;
> -       int ret;
> +       int i, ret;
>
>         int34x_thermal_zone = kzalloc(sizeof(*int34x_thermal_zone),
>                                       GFP_KERNEL);
> @@ -228,32 +143,35 @@ struct int34x_thermal_zone *int340x_ther
>                 int34x_thermal_zone->ops->get_temp = get_temp;
>
>         status = acpi_evaluate_integer(adev->handle, "PATC", NULL, &trip_cnt);
> -       if (ACPI_FAILURE(status))
> -               trip_cnt = 0;
> -       else {
> -               int i;
> -
> -               int34x_thermal_zone->aux_trips =
> -                       kcalloc(trip_cnt,
> -                               sizeof(*int34x_thermal_zone->aux_trips),
> -                               GFP_KERNEL);
> -               if (!int34x_thermal_zone->aux_trips) {
> -                       ret = -ENOMEM;
> -                       goto err_trip_alloc;
> -               }
> -               trip_mask = BIT(trip_cnt) - 1;
> +       if (!ACPI_FAILURE(status)) {
>                 int34x_thermal_zone->aux_trip_nr = trip_cnt;
> -               for (i = 0; i < trip_cnt; ++i)
> -                       int34x_thermal_zone->aux_trips[i] = THERMAL_TEMP_INVALID;
> +               trip_mask = BIT(trip_cnt) - 1;
> +       }
> +
> +       int34x_thermal_zone->trips = kzalloc(sizeof(*int34x_thermal_zone->trips) *
> +                                            (INT340X_THERMAL_MAX_TRIP_COUNT + trip_cnt),
> +                                             GFP_KERNEL);
> +       if (!int34x_thermal_zone->trips) {
> +               ret = -ENOMEM;
> +               goto err_trips_alloc;
>         }
>
>         trip_cnt = int340x_thermal_read_trips(int34x_thermal_zone);
>
> +       for (i = 0; i < trip_cnt; ++i)
> +               int340x_thermal_get_global_hyst(adev, &int34x_thermal_zone->trips[i].hysteresis);
> +
> +       for (i = 0; i < int34x_thermal_zone->aux_trip_nr; i++) {
> +               int34x_thermal_zone->trips[i].type = THERMAL_TRIP_PASSIVE;
> +               int34x_thermal_zone->trips[i].temperature = THERMAL_TEMP_INVALID;
> +       }
> +
>         int34x_thermal_zone->lpat_table = acpi_lpat_get_conversion_table(
>                                                                 adev->handle);
>
> -       int34x_thermal_zone->zone = thermal_zone_device_register(
> +       int34x_thermal_zone->zone = thermal_zone_device_register_with_trips(
>                                                 acpi_device_bid(adev),
> +                                               int34x_thermal_zone->trips,
>                                                 trip_cnt,
>                                                 trip_mask, int34x_thermal_zone,
>                                                 int34x_thermal_zone->ops,
> @@ -272,9 +190,9 @@ struct int34x_thermal_zone *int340x_ther
>  err_enable:
>         thermal_zone_device_unregister(int34x_thermal_zone->zone);
>  err_thermal_zone:
> +       kfree(int34x_thermal_zone->trips);
>         acpi_lpat_free_conversion_table(int34x_thermal_zone->lpat_table);
> -       kfree(int34x_thermal_zone->aux_trips);
> -err_trip_alloc:
> +err_trips_alloc:
>         kfree(int34x_thermal_zone->ops);
>  err_ops_alloc:
>         kfree(int34x_thermal_zone);
> @@ -287,7 +205,7 @@ void int340x_thermal_zone_remove(struct
>  {
>         thermal_zone_device_unregister(int34x_thermal_zone->zone);
>         acpi_lpat_free_conversion_table(int34x_thermal_zone->lpat_table);
> -       kfree(int34x_thermal_zone->aux_trips);
> +       kfree(int34x_thermal_zone->trips);
>         kfree(int34x_thermal_zone->ops);
>         kfree(int34x_thermal_zone);
>  }
> Index: linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> +++ linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h
> @@ -10,6 +10,7 @@
>  #include <acpi/acpi_lpat.h>
>
>  #define INT340X_THERMAL_MAX_ACT_TRIP_COUNT     10
> +#define INT340X_THERMAL_MAX_TRIP_COUNT INT340X_THERMAL_MAX_ACT_TRIP_COUNT + 3
>
>  struct active_trip {
>         int temp;
> @@ -19,15 +20,8 @@ struct active_trip {
>
>  struct int34x_thermal_zone {
>         struct acpi_device *adev;
> -       struct active_trip act_trips[INT340X_THERMAL_MAX_ACT_TRIP_COUNT];
> -       unsigned long *aux_trips;
> +       struct thermal_trip *trips;
>         int aux_trip_nr;
> -       int psv_temp;
> -       int psv_trip_id;
> -       int crt_temp;
> -       int crt_trip_id;
> -       int hot_temp;
> -       int hot_trip_id;
>         struct thermal_zone_device *zone;
>         struct thermal_zone_device_ops *ops;
>         void *priv_data;
>
>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ