by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] thermal/acpi: Add ACPI trip point routines

On Fri, Jan 13, 2023 at 7:02 PM Daniel Lezcano
<[email protected]> wrote:
>
> The ACPI specification describes the trip points, the device tree
> bindings as well.
>
> The OF code uses the generic trip point structures.
>
> The ACPI has their own trip points structure and uses the get_trip_*
> ops to retrieve them.
>
> We can do the same as the OF code and create a set of ACPI functions
> to retrieve a trip point description. Having a common code for ACPI
> will help to cleanup the remaining Intel drivers and get rid of the
> get_trip_* functions.
>
> These changes add the ACPI thermal calls to retrieve the basic
> information we need to be reused in the thermal ACPI and Intel
> drivers.
>
> The different ACPI functions have the generic trip point structure
> passed as parameter where it is filled.
>
> This structure aims to be the one used by all the thermal drivers and
> the thermal framework.
>
> After this series, a couple of Intel drivers and the ACPI thermal
> driver will still have their own trip points definition but a new
> series on top of this one will finish the conversion to the generic
> trip point handling.
>
> This series depends on the generic trip point added to the thermal
> framework and available in the thermal/linux-next branch.
>
> https://lkml.org/lkml/2022/10/3/456
>
> It has been tested on a Intel i7-8650U - x280 with the INT3400, the
> PCH, ACPITZ, and x86_pkg_temp. No regression observed so far.

Some names of the functions defined below can be less cryptic IMO.
Please see the following comments.

> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/Kconfig | 4 +
> drivers/thermal/Makefile | 1 +
> drivers/thermal/thermal_acpi.c | 209 +++++++++++++++++++++++++++++++++
> include/linux/thermal.h | 8 ++
> 4 files changed, 222 insertions(+)
> create mode 100644 drivers/thermal/thermal_acpi.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index e052dae614eb..eaeb2b2ee6e9 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -76,6 +76,10 @@ config THERMAL_OF
> Say 'Y' here if you need to build thermal infrastructure
> based on device tree.
>
> +config THERMAL_ACPI
> + depends on ACPI
> + bool
> +
> config THERMAL_WRITABLE_TRIPS
> bool "Enable writable trip points"
> help
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 2506c6c8ca83..60f0dfa9aae2 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -13,6 +13,7 @@ thermal_sys-$(CONFIG_THERMAL_NETLINK) += thermal_netlink.o
> # interface to/from other layers providing sensors
> thermal_sys-$(CONFIG_THERMAL_HWMON) += thermal_hwmon.o
> thermal_sys-$(CONFIG_THERMAL_OF) += thermal_of.o
> +thermal_sys-$(CONFIG_THERMAL_ACPI) += thermal_acpi.o
>
> # governors
> thermal_sys-$(CONFIG_THERMAL_GOV_FAIR_SHARE) += gov_fair_share.o
> diff --git a/drivers/thermal/thermal_acpi.c b/drivers/thermal/thermal_acpi.c
> new file mode 100644
> index 000000000000..ef6f10713650
> --- /dev/null
> +++ b/drivers/thermal/thermal_acpi.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2022 Linaro Limited
> + *
> + * Author: Daniel Lezcano <[email protected]>
> + *
> + * ACPI thermal configuration
> + */
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/units.h>
> +#include <uapi/linux/thermal.h>
> +
> +#include "thermal_core.h"
> +
> +/*
> + * An hysteresis value below zero is invalid and we can consider a
> + * value greater than 20°K/°C is invalid too.
> + */
> +#define HYSTERESIS_MIN_DECIK 0
> +#define HYSTERESIS_MAX_DECIK 200

If the full word "hysteresis" is used here, it should also be used in
the hysteresis-related names below.

I would use the "hyst" abbreviation here and elsewhere where applicable.

> +
> +/*
> + * Minimum temperature for full military grade is 218°K (-55°C) and
> + * max temperature is 448°K (175°C). We can consider those values as
> + * the boundaries for the [trips] temperature returned by the
> + * firmware. Any values out of these boundaries can be considered
> + * bogus and we can assume the firmware has no data to provide.
> + */
> +#define TEMPERATURE_MIN_DECIK 2180
> +#define TEMPERATURE_MAX_DECIK 4480

Like the above, but regarding "temperature" and "temp" instead of
"hysteresis" and "hyst", respectively.

> +
> +static int thermal_acpi_get_temperature_object(struct acpi_device *adev,
> + char *object, int *temperature)

So this would become thermal_acpi_get_temp_object(). or even
thermal_acpi_get_temp() because it really returns the temperature
value.

I also don't particularly like returning values via pointers, which is
entirely avoidable here, because the temperature value obtained from
the ACPI control methods must be a positive number.

So I would make it

static int thermal_acpi_get_temp(struct acpi_device *adev, char *object_name)
{

which would be consistent with the definition of the hysteresis
function below (which returns the value directly).

> +{
> + unsigned long long temp;
> + acpi_status status;
> +
> + status = acpi_evaluate_integer(adev->handle, object, NULL, &temp);
> + if (ACPI_FAILURE(status)) {
> + acpi_handle_debug(adev->handle, "No temperature object '%s'\n", object);

This message may not be true, because status need not be AE_NOT_FOUND.

> + return -ENODEV;
> + }
> +
> + if (temp < TEMPERATURE_MIN_DECIK || temp >= TEMPERATURE_MAX_DECIK) {
> + acpi_handle_info(adev->handle, "Invalid temperature '%llu deci°K' for object '%s'\n",
> + temp, object);

I think that the message can be debug-level as well and I would just
say "Invalid value %llu returned by %s\n" in it.

> + return -ENODATA;

And I'm not sure if the difference between -ENODEV and -ENODATA
matters here. Maybe return -ENODATA in all error cases?

> + }
> +
> + *temperature = deci_kelvin_to_millicelsius(temp);
> +
> + return 0;
> +}
> +
> +/**
> + * thermal_acpi_trip_gtsh() - Get the global hysteresis value

thermal_acpi_global_hyst() please.

> + * @adev: the acpi device to get the description from
> + *
> + * Get the global hysteresis value for the trip points. If any call
> + * fail, we shall return a zero hysteresis value.
> + *
> + * Return: An integer between %HYSTERESIS_MIN_DECIK and %HYSTERESIS_MAX_DECIK
> + */
> +int thermal_acpi_trip_gtsh(struct acpi_device *adev)
> +{
> + unsigned long long hyst;
> + acpi_status status;
> +
> + status = acpi_evaluate_integer(adev->handle, "GTSH", NULL, &hyst);

GTSH is not a standard ACPI thing.

AFAICS, it's int3403 specific, so using it in a generic ACPI library
is questionable.

> + if (ACPI_FAILURE(status))
> + return 0;
> +
> + if (hyst < HYSTERESIS_MIN_DECIK || hyst >= HYSTERESIS_MAX_DECIK) {
> + acpi_handle_info(adev->handle, "Invalid hysteresis '%llu deci°K' for object 'GTSH'\n",
> + hyst);
> + return 0;
> + }
> +
> + return deci_kelvin_to_millicelsius(hyst);
> +}
> +EXPORT_SYMBOL_GPL(thermal_acpi_trip_gtsh);
> +
> +/**
> + * thermal_acpi_trip_act() - Get the specified active trip point

thermal_acpi_active_trip_temp, please.

> + * @adev: the acpi device to get the description from

Please spell ACPI in capitals in all comments.

And I would say

@adev: Thermal zone ACPI device object

> + * @trip: a &struct thermal_trip to be filled if the function succeed

@trip: Trip point structure to be populated on success

> + * @id: an integer speciyfing the active trip point id

@id: Active cooling level (0 - 9)

> + *
> + * The function calls the ACPI framework to get the "_ACTx" objects
> + * which describe the active trip points.

No, it doesn't do that. What it really does can be described as

"Evaluate the _ACx object for the thermal zone represented by @adev to
obtain the temperature of the active cooling trip point corresponding
to the active cooling level given by @id and initialize @trip as an
active trip point using that temperature value"

> The @id builds the "_ACTx"
> + * string with the numbered active trip point name. Then it fills the
> + * @trip structure with the information retrieved from those objects.
> + *
> + * Return:
> + * * 0 - Success
> + * * -ENODEV - Failed to retrieve the ACPI object
> + * * -ENODATA - The ACPI object value appears to be inconsistent
> + */
> +int thermal_acpi_trip_act(struct acpi_device *adev,
> + struct thermal_trip *trip, int id)
> +{
> + char name[5];

char name[] = "_AC0";

> + int ret;

int temp;

if (id < 0 || id > 9)
return -EINVAL;

name[3] += id;

> +
> + sprintf(name, "_AC%d", id);
> +
> + ret = thermal_acpi_get_temperature_object(adev, name, &trip->temperature);
> + if (ret)
> + return ret;

temp = thermal_acpi_get_temp(adev, name);
if (temp < 0)
return temp;

trip->temperature = temp;

And analogously below.

> +
> + trip->hysteresis = 0;
> + trip->type = THERMAL_TRIP_ACTIVE;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_acpi_trip_act);
> +
> +/**
> + * thermal_acpi_trip_psv() - Get the passive trip point

thermal_acpi_passive_trip_temp, please.

> + * @adev: the acpi device to get the description from
> + * @trip: a &struct thermal_trip to be filled if the function succeed

The same comments as above apply.

> + *
> + * The function calls the ACPI framework to get the "_PSV" object
> + * which describe the passive trip point. Then it fills the @trip
> + * structure with the information retrieved from those objects.

"Evaluate the _PSV object for the thermal zone represented by @adev to
obtain the temperature of the passive cooling trip point and
initialize @trip as a passive trip point using that temperature
value."

> + *
> + * Return:
> + * * 0 - Success
> + * * -ENODEV - Failed to retrieve the ACPI object
> + * * -ENODATA - The ACPI object value appears to be inconsistent
> + */
> +int thermal_acpi_trip_psv(struct acpi_device *adev, struct thermal_trip *trip)
> +{
> + int ret;
> +
> + ret = thermal_acpi_get_temperature_object(adev, "_PSV", &trip->temperature);
> + if (ret)
> + return ret;
> +
> + trip->hysteresis = 0;
> + trip->type = THERMAL_TRIP_PASSIVE;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_acpi_trip_psv);
> +
> +/**
> + * thermal_acpi_trip_hot() - Get the near critical trip point

thermal_acpi_hot_trip_temp, please.

> + * @adev: the acpi device to get the description from
> + * @trip: a &struct thermal_trip to be filled if the function succeed

The same comments as above apply.

> + *
> + * The function calls the ACPI framework to get the "_HOT" object
> + * which describe the hot trip point. Then it fills the @trip
> + * structure with the information retrieved from those objects.

"Evaluate the _HOT object for the thermal zone represented by @adev to
obtain the temperature of the trip point at which the system is
expected to be put into the S4 sleep state and initialize @trip as a
hot trip point using that temperature value."

> + *
> + * Return:
> + * * 0 - Success
> + * * -ENODEV - Failed to retrieve the ACPI object
> + * * -ENODATA - The ACPI object appears to be inconsistent
> + */
> +int thermal_acpi_trip_hot(struct acpi_device *adev, struct thermal_trip *trip)
> +{
> + int ret;
> +
> + ret = thermal_acpi_get_temperature_object(adev, "_HOT", &trip->temperature);
> + if (ret)
> + return ret;
> +
> + trip->hysteresis = 0;
> + trip->type = THERMAL_TRIP_HOT;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_acpi_trip_hot);
> +
> +/**
> + * thermal_acpi_trip_crit() - Get the critical trip point

thermal_acpi_crit_trip_temp, please.

> + * @adev: the acpi device to get the description from
> + * @trip: a &struct thermal_trip to be filled if the function succeed

The same comments as above apply.

> + *
> + * The function calls the ACPI framework to get the "_CRT" object
> + * which describe the critical trip point. Then it fills the @trip
> + * structure with the information retrieved from this object.

"Evaluate the _CRT object for the thermal zone represented by @adev to
obtain the temperature of the critical cooling trip point and
initialize @trip as a critical trip point using that temperature
value."

> + *
> + * Return:
> + * * 0 - Success
> + * * -ENODEV - Failed to retrieve the ACPI object
> + * * -ENODATA - The ACPI object value appears to be inconsistent
> + */
> +int thermal_acpi_trip_crit(struct acpi_device *adev, struct thermal_trip *trip)
> +{
> + int ret;
> +
> + ret = thermal_acpi_get_temperature_object(adev, "_CRT", &trip->temperature);
> + if (ret)
> + return ret;
> +
> + /*
> + * The hysteresis value has no sense here because critical
> + * trip point has no u-turn
> + */
> + trip->hysteresis = 0;
> + trip->type = THERMAL_TRIP_CRITICAL;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_acpi_trip_crit);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 30353e4b1424..ba2d5d4c23e2 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -334,6 +334,14 @@ static inline void devm_thermal_of_zone_unregister(struct device *dev,
> }
> #endif
>
> +#ifdef CONFIG_THERMAL_ACPI
> +int thermal_acpi_trip_gtsh(struct acpi_device *adev);
> +int thermal_acpi_trip_crit(struct acpi_device *adev, struct thermal_trip *trip);
> +int thermal_acpi_trip_hot(struct acpi_device *adev, struct thermal_trip *trip);
> +int thermal_acpi_trip_psv(struct acpi_device *adev, struct thermal_trip *trip);
> +int thermal_acpi_trip_act(struct acpi_device *adev, struct thermal_trip *trip, int id);
> +#endif
> +
> int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
> struct thermal_trip *trip);
> int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
> --

2023-01-19 17:11:44

On Fri, Jan 20, 2023 at 7:27 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 20/01/2023 19:15, Rafael J. Wysocki wrote:
> > On Fri, Jan 20, 2023 at 7:08 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >>
> >> Hi Rafael,
> >>
> >>
> >> On 19/01/2023 14:15, Rafael J. Wysocki wrote:
> >>
> >> [ ... ]
> >>
> >>>> +static int thermal_acpi_get_temperature_object(struct acpi_device *adev,
> >>>> + char *object, int *temperature)
> >>>
> >>> So this would become thermal_acpi_get_temp_object(). or even
> >>> thermal_acpi_get_temp() because it really returns the temperature
> >>> value.
> >>>
> >>> I also don't particularly like returning values via pointers, which is
> >>> entirely avoidable here, because the temperature value obtained from
> >>> the ACPI control methods must be a positive number.
> >>>
> >>> So I would make it
> >>>
> >>> static int thermal_acpi_get_temp(struct acpi_device *adev, char *object_name)
> >>> {
> >>
> >> We are converting decikelvin -> millicelsius. Even it is very unlikely,
> >> the result could be less than zero (eg. -1°C). We won't be able to
> >> differentiate -ENODATA with a negative value, no ?
> >>
> >> In the future, it is possible we will have to deal with cold trip points
> >> in order to warm a board. May be we should don't care for now ?
> >
> > My point is that the ACPI specification mandates that the return
> > values be in deciK and so always non-negative.
>
> I understand that but the code does:
>
> static int thermal_acpi_get_temp(struct acpi_device *adev, char
> *object_name)
> {
> ...
>
> return deci_kelvin_to_millicelsius(temp);
> }
>
> All the callers do:
>
> ...
>
> ret = thermal_acpi_get_temp(adev, name);
> if (ret < 0)
> return ret;
> /* This could be an error
> * or negative millicelsius temperature
> */
>
> /* here we already have millicelsius */
> trip->temperature = ret;
> ...
>
> So I guess we want to do:
>
> ...
>
> ret = thermal_acpi_get_temp(adev, name);
> if (ret < 0)
> return ret;
>
> /* we convert here instead in thermal_acpi_get_temp() */
> trip->temperature = deci_kelvin_to_millicelsius(ret);
> ...
>
> Sounds good ?

Yes, it does. Convert when it is known to be valid.