Date: Tue, 23 Jan 2024 11:58:02 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
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

Il 22/01/24 20:19, Daniel Lezcano ha scritto:
> 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;
> 

Agreed!

> 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 ?
> 

It's not the best, but the plan is to change the struct name in the second cycle,
as keeping the thermal_zone_params struct named as it is right now is essential
to avoid immutable branches.

window 1: Reorganization with temporary struct names -> no immutable branches
window 2: Cleanup and rename -> no immutable branches

Any change from the window 2 part brought to window 1 would need immutable
branches all around... so I really can't call it "thermal_zone_params" in
window 1.

Perhaps I can change the _platform_ name to something else, but still needs
to be different from "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
> 

Ok! :-)

>>      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 :)
> 
> 

Cool. It's time to write a good non-RFC series then!

Cheers,
Angelo

Powered by blists - more mailing lists