Message-ID: <9783d2a6-7395-4516-9fd1-d7c42ea35d07@collabora.com>
Date: Thu, 18 Jan 2024 10:39:30 +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 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];

	/* 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];
	struct thermal_zone_params ipa_params;
};

struct thermal_zone_platform_params {
	const char *type;
	struct thermal_zone_device_ops *ops;
	struct thermal_trip *trips;
	int num_trips;
	int mask;

	int passive_delay;
	int polling_delay;

	void *devdata;
	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?

Cheers!
Angelo

Powered by blists - more mailing lists