Date:   Tue, 3 Jan 2023 11:44:27 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     srinivas.pandruvada@...ux.intel.com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        rui.zhang@...el.com, Amit Kucheria <amitk@...nel.org>
Subject: Re: [PATCH v2 1/3] thermal/acpi: Add ACPI trip point routines

On 02/01/2023 19:22, Rafael J. Wysocki wrote:
> On Mon, Jan 2, 2023 at 7:01 PM Daniel Lezcano <daniel.lezcano@...aro.org> wrote:
>>
>> From: Daniel Lezcano <daniel.lezcano@...aro.org>
>>
>> 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.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...nel.org>
>> ---
>>   drivers/thermal/Kconfig        |  13 ++
>>   drivers/thermal/Makefile       |   1 +
>>   drivers/thermal/thermal_acpi.c | 279 +++++++++++++++++++++++++++++++++
>>   include/linux/thermal.h        |  16 ++
>>   4 files changed, 309 insertions(+)
>>   create mode 100644 drivers/thermal/thermal_acpi.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index e052dae614eb..2c19bccd1223 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -76,6 +76,19 @@ config THERMAL_OF
>>            Say 'Y' here if you need to build thermal infrastructure
>>            based on device tree.
>>
>> +config THERMAL_ACPI
> 
> Not needed.
> 
> Or if you want it to be built only if there are any users, call it
> ACPI_THERMAL_LIB and do
> 
> config ACPI_THERMAL_LIB
>      depends on ACPI_THERMAL
>      bool
> 
> and let the users select it.

Yes, I think it makes more sense to not provide any option and just 
compile the wrappers when ACPI_THERMAL is set.


>> 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..28c629b4d814
>> --- /dev/null
>> +++ b/drivers/thermal/thermal_acpi.c
>> @@ -0,0 +1,279 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2022 Linaro Limited
>> + *
>> + * Author: Daniel Lezcano <daniel.lezcano@...aro.org>
>> + *
>> + * 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"
>> +
>> +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);
>> +       if (ACPI_FAILURE(status))
>> +               return 0;
>> +
>> +       return (int)(hyst * 100);
> 
> What if the result is larger than INT_MAX?

That would mean ACPI is returning more than 4 billions degree hysteresis 
value.

What strategy should we use in these functions? Trust the values 
returned by ACPI or double check if they are consistent ?


>> +}
>> +EXPORT_SYMBOL_GPL(thermal_acpi_trip_gtsh);
>> +
>> +int thermal_acpi_get_tzd(struct acpi_device *adev, struct acpi_handle_list *devices)
>> +{
>> +       acpi_status status;
>> +
>> +       /*
>> +        * _TZD (Thermal zone device): This optional object evaluates
>> +        * to a package of device names. Each name corresponds to a
>> +        * device in the ACPI namespace that is associated with the
>> +        * thermal zone. The temperature reported by the thermal zone
>> +        * is roughly correspondent to that of each of the devices.
>> +        */
> 
> I don't think that the comment is necessary.
> 
> The spec already contains a definition of this object.

Yes, this comment is the description from the spec. I put them there to 
save time for those who are reading the code so they don't have to go 
back and forth between the documentation and the code.

Do you really want me to remove all of them ?

[ ... ]


> Overall, I'm not sure about simple wrappers around
> acpi_evaluate_integer/reference() that effectively discard the return
> value and don't even bother to sanitize the return value before
> returning it to the caller.

Ok, so we don't want to trust the values returned by ACPI.

If all the sanity checks are done in the functions, would it make more 
sense then ?


> The ones that initialize a trip point make more sense IMO.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists