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

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

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