Re: [PATCH v1 1/6] thermal: core: Store zone trips table in struct thermal_zone_device

Stanislaw Gruszka
2024-02-08T08:22:15Z

On Mon, Feb 05, 2024 at 10:14:31PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The current code requires thermal zone creators to pass a pointer to a
> writable trips table to thermal_zone_device_register_with_trips() and
> that trips table is then used by the thermal core going forward.
>
> Consequently, the callers of thermal_zone_device_register_with_trips()
> are required to hold on to the trips table passed to it until the given
> thermal zone is unregistered, at which point the trips table can be
> freed, but at the same time they are not allowed to access the cells in
> that table directly. This is both error prone and confusing.
>
> To address it, turn the trips table pointer in struct thermal_zone_device
> into a flex array (counted by its num_trips field), allocate it during
> thermal zone device allocation and copy the contents of the trips table
> supplied by the zone creator (which can be const now) into it.
>
> This allows the callers of thermal_zone_device_register_with_trips() to
> drop their trip tables right after the zone registration.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
> drivers/thermal/thermal_core.c | 16 +++++++++-------
> include/linux/thermal.h | 10 +++++-----
> 2 files changed, 14 insertions(+), 12 deletions(-)
>
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -130,7 +130,6 @@ struct thermal_cooling_device {
> * @trip_hyst_attrs: attributes for trip points for sysfs: trip hysteresis
> * @mode: current mode of this thermal zone
> * @devdata: private pointer for device private data
> - * @trips: an array of struct thermal_trip
> * @num_trips: number of trip points the thermal zone supports
> * @passive_delay_jiffies: number of jiffies to wait between polls when
> * performing passive cooling.
> @@ -160,6 +159,7 @@ struct thermal_cooling_device {
> * @poll_queue: delayed work for polling
> * @notify_event: Last notification event
> * @suspended: thermal zone suspend indicator
> + * @trips: array of struct thermal_trip objects
> */
> struct thermal_zone_device {
> int id;
> @@ -172,7 +172,6 @@ struct thermal_zone_device {
> struct thermal_attr *trip_hyst_attrs;
> enum thermal_device_mode mode;
> void *devdata;
> - struct thermal_trip *trips;
> int num_trips;
> unsigned long passive_delay_jiffies;
> unsigned long polling_delay_jiffies;
> @@ -193,10 +192,11 @@ struct thermal_zone_device {
> struct list_head node;
> struct delayed_work poll_queue;
> enum thermal_notify_event notify_event;
> + bool suspended;
> #ifdef CONFIG_THERMAL_DEBUGFS
> struct thermal_debugfs *debugfs;
> #endif
> - bool suspended;
> + struct thermal_trip trips[] __counted_by(num_trips);
> };
>
> /**
> @@ -315,7 +315,7 @@ int thermal_zone_get_crit_temp(struct th
> #ifdef CONFIG_THERMAL
> struct thermal_zone_device *thermal_zone_device_register_with_trips(
> const char *type,
> - struct thermal_trip *trips,
> + const struct thermal_trip *trips,
> int num_trips, int mask,
> void *devdata,
> struct thermal_zone_device_ops *ops,
> @@ -375,7 +375,7 @@ void thermal_zone_device_critical(struct
> #else
> static inline struct thermal_zone_device *thermal_zone_device_register_with_trips(
> const char *type,
> - struct thermal_trip *trips,
> + const struct thermal_trip *trips,
> int num_trips, int mask,
> void *devdata,
> struct thermal_zone_device_ops *ops,
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1272,10 +1272,13 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_
> * IS_ERR*() helpers.
> */
> struct thermal_zone_device *
> -thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int num_trips, int mask,
> - void *devdata, struct thermal_zone_device_ops *ops,
> - const struct thermal_zone_params *tzp, int passive_delay,
> - int polling_delay)
> +thermal_zone_device_register_with_trips(const char *type,
> + const struct thermal_trip *trips,
> + int num_trips, int mask,
> + void *devdata,
> + struct thermal_zone_device_ops *ops,
> + const struct thermal_zone_params *tzp,
> + int passive_delay, int polling_delay)
> {
> struct thermal_zone_device *tz;
> int id;
> @@ -1322,7 +1325,7 @@ thermal_zone_device_register_with_trips(
> if (!thermal_class)
> return ERR_PTR(-ENODEV);
>
> - tz = kzalloc(sizeof(*tz), GFP_KERNEL);
> + tz = kzalloc(struct_size(tz, trips, num_trips), GFP_KERNEL);
> if (!tz)
> return ERR_PTR(-ENOMEM);
>
> @@ -1344,7 +1347,6 @@ thermal_zone_device_register_with_trips(
> result = id;
> goto free_tzp;
> }
> -
> tz->id = id;
> strscpy(tz->type, type, sizeof(tz->type));
>
> @@ -1354,7 +1356,7 @@ thermal_zone_device_register_with_trips(
> tz->ops = ops;
> tz->device.class = thermal_class;
> tz->devdata = devdata;
> - tz->trips = trips;
> + memcpy(tz->trips, trips, num_trips * sizeof(trips[0]));
> tz->num_trips = num_trips;
>
> thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
>
>
>
>