| 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
| ||
|
Date: Mon, 22 Jan 2024 20:19:20 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: rafael@...nel.org, rui.zhang@...el.com, lukasz.luba@....com,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [RFC PATCH 01/26] thermal: Introduce
thermal_zone_device_register() and params structure
On 18/01/2024 10:39, AngeloGioacchino Del Regno wrote:
> Il 16/01/24 10:58, AngeloGioacchino Del Regno ha scritto:
>> Il 15/01/24 13:39, Daniel Lezcano ha scritto:
>>> On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
>>>> In preparation for extending the thermal zone devices to actually have
>>>> a name and disambiguation of thermal zone types/names, introduce a new
>>>> thermal_zone_device_params structure which holds all of the parameters
>>>> that are necessary to register a thermal zone device, then add a new
>>>> function thermal_zone_device_register().
>>>>
>>>> The latter takes as parameter the newly introduced structure and is
>>>> made to eventually replace all usages of the now deprecated function
>>>> thermal_zone_device_register_with_trips() and of
>>>> thermal_tripless_zone_device_register().
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno
>>>> <angelogioacchino.delregno@...labora.com>
>>>> ---
>>>> drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
>>>> include/linux/thermal.h | 33 +++++++++++++++++++++++++++++++++
>>>> 2 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.c
>>>> b/drivers/thermal/thermal_core.c
>>>> index e5434cdbf23b..6be508eb2d72 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>>>> * whether trip points have been crossed (0 for interrupt
>>>> * driven systems)
>>>> *
>>>> + * This function is deprecated. See thermal_zone_device_register().
>>>> + *
>>>> * This interface function adds a new thermal zone device (sensor) to
>>>> * /sys/class/thermal folder as thermal_zone[0-*]. It tries to
>>>> bind all the
>>>> * thermal cooling devices registered at the same time.
>>>> @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const
>>>> char *type, struct thermal_trip *t
>>>> }
>>>> EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>>>> +/* This function is deprecated. See thermal_zone_device_register(). */
>>>> struct thermal_zone_device *thermal_tripless_zone_device_register(
>>>> const char *type,
>>>> void *devdata,
>>>> @@ -1420,6 +1423,30 @@ struct thermal_zone_device
>>>> *thermal_tripless_zone_device_register(
>>>> }
>>>> EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
>>>> +/**
>>>> + * thermal_zone_device_register() - register a new thermal zone device
>>>> + * @tzdp: Parameters of the new thermal zone device
>>>> + * See struct thermal_zone_device_register.
>>>> + *
>>>> + * This interface function adds a new thermal zone device (sensor) to
>>>> + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind
>>>> all the
>>>> + * thermal cooling devices registered at the same time.
>>>> + * thermal_zone_device_unregister() must be called when the device
>>>> is no
>>>> + * longer needed. The passive cooling depends on the .get_trend()
>>>> return value.
>>>> + *
>>>> + * Return: a pointer to the created struct thermal_zone_device or an
>>>> + * in case of error, an ERR_PTR. Caller must check return value with
>>>> + * IS_ERR*() helpers.
>>>> + */
>>>> +struct thermal_zone_device *thermal_zone_device_register(struct
>>>> thermal_zone_device_params *tzdp)
>>>> +{
>>>> + return thermal_zone_device_register_with_trips(tzdp->type,
>>>> tzdp->trips, tzdp->num_trips,
>>>> + tzdp->mask, tzdp->devdata, tzdp->ops,
>>>> + &tzdp->tzp, tzdp->passive_delay,
>>>> + tzdp->polling_delay);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>>>> +
>>>> void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
>>>> {
>>>> return tzd->devdata;
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index 98957bae08ff..c6ed33a7e468 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -258,6 +258,33 @@ struct thermal_zone_params {
>>>> int offset;
>>>> };
>>>> +/**
>>>> + * struct thermal_zone_device_params - parameters for a thermal
>>>> zone device
>>>> + * @type: the thermal zone device type
>>>> + * @tzp: thermal zone platform parameters
>>>> + * @ops: standard thermal zone device callbacks
>>>> + * @devdata: private device data
>>>> + * @trips: a pointer to an array of thermal trips, if any
>>>> + * @num_trips: the number of trip points the thermal zone
>>>> support
>>>> + * @mask: a bit string indicating the writeablility of trip
>>>> points
>>>> + * @passive_delay: number of milliseconds to wait between polls
>>>> when
>>>> + * performing passive cooling
>>>> + * @polling_delay: number of milliseconds to wait between polls
>>>> when checking
>>>> + * whether trip points have been crossed (0 for interrupt
>>>> + * driven systems)
>>>> + */
>>>> +struct thermal_zone_device_params {
>>>> + const char *type;
>>>> + struct thermal_zone_params tzp;
>>>> + struct thermal_zone_device_ops *ops;
>>>> + void *devdata;
>>>> + struct thermal_trip *trips;
>>>> + int num_trips;
>>>> + int mask;
>>>> + int passive_delay;
>>>> + int polling_delay;
>>>> +};
>>>
>>> From my POV, this "struct thermal_zone_params" has been always a
>>> inadequate and catch-all structure. It will confuse with
>>> thermal_zone_device_params
>>>
>>> I suggest to cleanup a bit that by sorting the parameters in the
>>> right structures where the result could be something like:
>>>
>>> eg.
>>>
>>> struct thermal_zone_params {
>>>
>>> const char *type;
>>> struct thermal_zone_device_ops *ops;
>>> struct thermal_trip *trips;
>>> int num_trips;
>>>
>>> int passive_delay;
>>> int polling_delay;
>>>
>>> void *devdata;
>>> bool no_hwmon;
>>> };
>>>
>>> struct thermal_governor_ipa_params {
>>> u32 sustainable_power;
>>> s32 k_po;
>>> s32 k_pu;
>>> s32 k_i;
>>> s32 k_d;
>>> s32 integral_cutoff;
>>> int slope;
>>> int offset;
>>> };
>>>
>>> struct thermal_governor_params {
>>> char governor_name[THERMAL_NAME_LENGTH];
>>> union {
>>> struct thermal_governor_ipa_params ipa_params;
>>> };
>>> };
>>>
>>> struct thermal_zone_device_params {
>>> struct thermal_zone_params *tzp;
>>> struct thermal_governor_params *tgp;
>>> }
>>>
>>> No functional changes just code reorg, being a series to be submitted
>>> before the rest on these RFC changes (2->26)
>>>
>>
>> Could work. It's true that thermal_zone_params is a catch-all
>> structure, and it's
>> not really the best... but I also haven't checked how complex and/or
>> how much time
>> would your proposed change take.
>>
>> Shouldn't take much as far as I can foresee, but I really have to
>> check a bit.
>> If I'm right as in it's not something huge, the next series will
>> directly have
>> this stuff sorted - if not, I'll reach to you.
>>
>
> So... I've checked the situation and coded some.
>
> I came to the conclusion that doing it as suggested is possible but
> realistically
> suboptimal, because that will need multiple immutable branches to
> actually end up
> in upstream as changes would otherwise break kernel build.
>
> Then, here I am suggesting a different way forward, while still
> performing this
> much needed cleanup and reorganization:
>
> First, we introduce thermal_zone_device_register() and params structure
> with
> this commit, which will have
>
> /* Structure to define Thermal Zone parameters */
> struct thermal_zone_params {
> /* Scheduled for removal - see struct thermal_zone_governor_params. */
> char governor_name[THERMAL_NAME_LENGTH];
Take the opportunity to introduce a patch before doing a change to:
const char *governor_name;
AFAICS, there is no place in the kernel where it is not a const char *
assignation which is by the way wrong:
char governor_name[THERMAL_NAME_LENGTH];
governor_name = "step_wise";
:)
> /* Scheduled for removal - see struct thermal_zone_governor_params. */
> bool no_hwmon;
>
> /*
> * Sustainable power (heat) that this thermal zone can dissipate in
> * mW
> */
> u32 sustainable_power;
>
> /*
> * Proportional parameter of the PID controller when
> * overshooting (i.e., when temperature is below the target)
> */
> s32 k_po;
>
> /*
> * Proportional parameter of the PID controller when
> * undershooting
> */
> s32 k_pu;
>
> /* Integral parameter of the PID controller */
> s32 k_i;
>
> /* Derivative parameter of the PID controller */
> s32 k_d;
>
> /* threshold below which the error is no longer accumulated */
> s32 integral_cutoff;
>
> /*
> * @slope: slope of a linear temperature adjustment curve.
> * Used by thermal zone drivers.
> */
> int slope;
> /*
> * @offset: offset of a linear temperature adjustment curve.
> * Used by thermal zone drivers (default 0).
> */
> int offset;
> };
>
> struct thermal_governor_params {
> char governor_name[THERMAL_NAME_LENGTH];
const char *governor_name;
> struct thermal_zone_params ipa_params;
> };
>
> struct thermal_zone_platform_params {
The name sounds inadequate.
May be just thermal_zone_params ?
> const char *type;
> struct thermal_zone_device_ops *ops;
Move the ops in the thermal_zone_device_params
> struct thermal_trip *trips;
> int num_trips;
> int mask;
>
> int passive_delay;
> int polling_delay;
>
> void *devdata;
And devdata also in the thermal_zone_device_params
> bool no_hwmon;
> };
>
>
> struct thermal_zone_device_params {
> struct thermal_zone_platform_params tzp;
> struct thermal_governor_params *tgp;
> };
>
> (Note that the `no_hwmon` and `governor_name` are *temporarily*
> duplicated, but
> there are good reasons to do that!)
>
> Drivers will follow with the migration to `thermal_zone_device_register()`,
> and that will be done *strictly* like so:
>
> struct thermal_zone_device_params tzdp = {
> .tzp = {
> .type = "acerhdf",
> .tzp = { .governor_name = "bang_bang" },
> .ops = &acerhdf_dev_ops,
> .trips = trips,
> .num_trips = ARRAY_SIZE(trips),
> .polling_delay = kernelmode ? interval * 1000 : 0,
> /* devdata, no_hwmon go here later in the code */
> },
> .tgp = { .governor_name = "bang_bang" }
> };
>
> Notice how in this case we're never *ever* referencing to any struct name,
> apart from struct thermal_zone_device_params (meaning, I'm never creating
> vars/pointers to struct thermal_zone_platform_params).
>
> If we follow this style, at least temporarily and at least during this
> cleanup,
> we will end up with a plan such that:
>
> 1. In the first merge window:
> - Drivers get migrated to thermal_zone_device_register()
> - Functions register_with_trips()/tripless get deprecated but not
> yet removed
>
> 2. In the next merge window (expecting all users updated from the first
> window):
> - Functions register_with_trips/tripless get removed (<- no more
> external refs
> to struct thermal_zone_params, which can be then safely renamed!)
> - Duplicated members governor_name and no_hwmon get removed from
> struct thermal_zone_params
> - Some structures get renamed to give them the proposed better names
> (which
> I also genuinely like)
> - Only Thermal API internal changes, as users (all the ones that are
> not in
> drivers/thermal/) won't need to get updated at all!
> ... and that's why I said "strictly like so" in the example up there.
>
> I think that this is the best strategy, for both ease of merging the
> changes and
> internal reorganization.
>
> Sure in the first merge window we'll be wasting a byte or two, but I am
> confident
> in that this is a very low price to pay - and even better, only
> temporarily - for
> other bigger benefits.
>
> What do you think?
Sounds like a plan :)
--
<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