[v2,0/6] thermal: Store trips table and ops in thermal_zone_device

Hi Everyone,

This is an update of

https://lore.kernel.org/linux-pm/2728491.mvXUDI8C0e@kreacher/

that has been rebased on top of

https://lore.kernel.org/linux-pm/6017196.lOV4Wx5bFT@kreacher/

and includes some bug fixes.

The original series description still applies:

"This series changes the PM core to copy the trips and zone ops directly
 into struct thermal_zone_device so as to allow the callers of the zone
 registration function to discard their own copies of those things after
 zone registration and/or possibly allocate them as read-only.

 The first patch makes the thermal core create a copy of the trips table which
 is declared as a flex array to enable additional bounds checking on it.  The
 next two patches update the ACPI thermal driver and Intel thermal drivers to
 take benefit of that change.

 In a similar pattern, patch [4/6] makes the thermal core create an internal
 copy of the zone ops supplied by the zone creator, so as to allow the
 original ops structure to be discarded after zone registration or allocated
 as read-only, and the next two patches update the ACPI thermal driver and Intel
 thermal drivers to actually do that.

 The other thermal drivers need not be changed, although in principle they may
 be simplified a bit too in the future.

 As usual, please refer to the individual patch changelogs for details."

However, the imx thermal driver is modified by patch [1/6], because it uses its
local trips table and expects it to contain the current passive trip temperature
value (as set via sysfs), and the thermal_of driver is modified by patch [4/6],
because it uses the ops pointer from the thermal zone device to free the ops.

Thanks!

On 14/02/2024 13:28, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The current code expects 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 expected to access 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, which
> will allow the callers of thermal_zone_device_register_with_trips() to
> drop their trip tables right after the zone registration.
> 
> This requires the imx thermal driver to be adjusted to store the new
> temperature in its internal trips table in imx_set_trip_temp(), because
> it will be separate from the core's trips table now and it has to be
> explicitly kept in sync with the latter.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>

On 14/02/2024 13:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because the thermal core creates and uses its own copy of the trips
> table passed to thermal_zone_device_register_with_trips(), it is not
> necessary to hold on to a local copy of it any more after the given
> thermal zone has been registered.
> 
> Accordingly, modify Intel thermal drivers to discard the trips tables
> passed to thermal_zone_device_register_with_trips() after thermal zone
> registration, for example by storing them in local variables which are
> automatically discarded when the zone registration is complete.
> 
> Also make some additional code simplifications unlocked by the above
> changes.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

On 14/02/2024 13:48, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because thermal zone operations are now stored directly in struct
> thermal_zone_device, acpi_thermal_zone_ops need not be modified by
> the thermal core and so it can be const.
> 
> Adjust the code accordingly.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

On 22/02/2024 14:10, Rafael J. Wysocki wrote:

[ ... ]

> Index: linux-pm/drivers/thermal/thermal_of.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_of.c
> +++ linux-pm/drivers/thermal/thermal_of.c
> @@ -440,12 +440,10 @@ static int thermal_of_unbind(struct ther
>    */
>   static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
>   {
> -	struct thermal_trip *trips = tz->trips;
>   	struct thermal_zone_device_ops *ops = tz->ops;
>   
>   	thermal_zone_device_disable(tz);
>   	thermal_zone_device_unregister(tz);
> -	kfree(trips);

thermal_of_zone_register() must free the allocated trip points after 
registering the thermal zone.

>   	kfree(ops);
>   }

On Thu, Feb 22, 2024 at 2:38 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 22/02/2024 14:10, Rafael J. Wysocki wrote:
>
> [ ... ]
>
> > Index: linux-pm/drivers/thermal/thermal_of.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_of.c
> > +++ linux-pm/drivers/thermal/thermal_of.c
> > @@ -440,12 +440,10 @@ static int thermal_of_unbind(struct ther
> >    */
> >   static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
> >   {
> > -     struct thermal_trip *trips = tz->trips;
> >       struct thermal_zone_device_ops *ops = tz->ops;
> >
> >       thermal_zone_device_disable(tz);
> >       thermal_zone_device_unregister(tz);
> > -     kfree(trips);
>
> thermal_of_zone_register() must free the allocated trip points after
> registering the thermal zone.
>
> >       kfree(ops);
> >   }

Good point.

Let me send another update, then.