linux-kernel - Re: [PATCH 1/2] thermal: intel: int340x: Add performance control for platform temperature control

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: <CAJZ5v0hgK+B1j6=M+Sx1+jC4d1n3GZfcpLtZJ_j58xdMXweTPA@mail.gmail.com>
Date: Wed, 4 Jun 2025 22:55:43 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc: rui.zhang@...el.com, daniel.lezcano@...aro.org, rafael@...nel.org, 
	lukasz.luba@....com, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 1/2] thermal: intel: int340x: Add performance control for
 platform temperature control

On Wed, Jun 4, 2025 at 10:35 PM Srinivas Pandruvada
<srinivas.pandruvada@...ux.intel.com> wrote:
>
> Add additional attribute to control performance of platform temperature
> control feature. Two attributes are added:

It would be good to explain why they are needed.

> gain: 0-7 levels, with 0 being most aggressive.
>         7 – graceful, favors performance at the expense of temperature
>         overshoots
>         0 – aggressive, favors tight regulation over performance
>
> min_performance_level: A value from 0-255. Specifies minimum Performance
>         level below which the there will be no throttling.
>         0 - all levels of throttling allowed including survivability
>         actions.
>         255 - no throttling allowed.

The description of min_performance_level above doesn't seem to be
consistent to me.  Specifically, the descriptions of the 0 and 255
values appear to indicate that this really is about what kind of
throttling can be applied.

Also, I gather that the units of this number are arbitrary and it is
not tied to anything specific.  I mean, 127 doesn't mean 50% of CPU
performance, for instance.

> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> ---
>  Documentation/driver-api/thermal/intel_dptf.rst   | 10 ++++++++++
>  .../platform_temperature_control.c                | 15 ++++++++++++++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/thermal/intel_dptf.rst b/Documentation/driver-api/thermal/intel_dptf.rst
> index ec5769accae0..794f5cce548e 100644
> --- a/Documentation/driver-api/thermal/intel_dptf.rst
> +++ b/Documentation/driver-api/thermal/intel_dptf.rst
> @@ -206,6 +206,16 @@ All these controls needs admin privilege to update.
>         Update a new temperature target in milli degree celsius for hardware to
>         use for the temperature control.
>
> +``gain`` (RW)
> +       A value in the range 0-7. Sets the aggressiveness of control loop.
> +       7 – graceful, favors performance at the expense of temperature overshoots.
> +       0 – aggressive, favors tight regulation over performance.
> +
> +``min_performance_level`` (RW)
> +       Minimum Performance level below which the there will be no throttling.
> +       0 - all levels of throttling allowed including survivability actions.
> +       256 - no throttling allowed.
> +
>  Given that this is platform temperature control, it is expected that a
>  single user-level manager owns and manages the controls. If multiple
>  user-level software applications attempt to write different targets, it
> diff --git a/drivers/thermal/intel/int340x_thermal/platform_temperature_control.c b/drivers/thermal/intel/int340x_thermal/platform_temperature_control.c
> index 2d6504514893..6cd05783a52d 100644
> --- a/drivers/thermal/intel/int340x_thermal/platform_temperature_control.c
> +++ b/drivers/thermal/intel/int340x_thermal/platform_temperature_control.c
> @@ -49,7 +49,7 @@ struct mmio_reg {
>  };
>
>  #define MAX_ATTR_GROUP_NAME_LEN        32
> -#define PTC_MAX_ATTRS          3
> +#define PTC_MAX_ATTRS          5
>
>  struct ptc_data {
>         u32 offset;
> @@ -57,6 +57,8 @@ struct ptc_data {
>         struct attribute *ptc_attrs[PTC_MAX_ATTRS];
>         struct device_attribute temperature_target_attr;
>         struct device_attribute enable_attr;
> +       struct device_attribute gain_attr;
> +       struct device_attribute min_performance_level_attr;
>         char group_name[MAX_ATTR_GROUP_NAME_LEN];
>  };
>
> @@ -78,6 +80,8 @@ static u32 ptc_offsets[PTC_MAX_INSTANCES] = {0x5B20, 0x5B28, 0x5B30};
>  static const char * const ptc_strings[] = {
>         "temperature_target",
>         "enable",
> +       "gain",
> +       "min_performance_level",
>         NULL
>  };
>
> @@ -177,6 +181,11 @@ PTC_SHOW(temperature_target);
>  PTC_STORE(temperature_target);
>  PTC_SHOW(enable);
>  PTC_STORE(enable);
> +PTC_SHOW(gain);
> +PTC_STORE(gain);
> +PTC_SHOW(min_performance_level);
> +PTC_STORE(min_performance_level);
> +
>
>  #define ptc_init_attribute(_name)\
>         do {\
> @@ -193,9 +202,13 @@ static int ptc_create_groups(struct pci_dev *pdev, int instance, struct ptc_data
>
>         ptc_init_attribute(temperature_target);
>         ptc_init_attribute(enable);
> +       ptc_init_attribute(gain);
> +       ptc_init_attribute(min_performance_level);
>
>         data->ptc_attrs[index++] = &data->temperature_target_attr.attr;
>         data->ptc_attrs[index++] = &data->enable_attr.attr;
> +       data->ptc_attrs[index++] = &data->gain_attr.attr;
> +       data->ptc_attrs[index++] = &data->min_performance_level_attr.attr;
>         data->ptc_attrs[index] = NULL;
>
>         snprintf(data->group_name, MAX_ATTR_GROUP_NAME_LEN,
> --
> 2.49.0
>