Re: [PATCH v3 1/8] thermal: core: Add mechanism for connecting trips with driver data

[Daniel Lezcano]

On 25/07/2023 14:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Some drivers need to update trip point data (temperature and/or
> hysteresis) upon notifications from the platform firmware or they
> may need to reprogram hardware when trip point parameters are changed
> via sysfs.  For those purposes, they need to connect struct thermal_trip
> to a private data set associated with the trip or the other way around
> and using a trip point index for that may not always work, because the
> core may need to reorder the trips during thermal zone registration (in
> particular, they may need to be sorted).
>
> To allow that to be done without using a trip point index, introduce
> a new field in struct thermal_trip that can be pointed by the driver
> to its own data structure containing a trip pointer to be initialized
> by the core during thermal zone registration.  That pointer will then
> have to be updated by the core every time the location of the given
> trip point object in memory changes.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>
> v2 -> v3: No changes.
>
> v1 -> v2: No changes.
>
> ---
>   drivers/thermal/thermal_core.c |   20 +++++++++++++++++---
>   include/linux/thermal.h        |   13 +++++++++++++
>   2 files changed, 30 insertions(+), 3 deletions(-)
>
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -76,16 +76,29 @@ struct thermal_zone_device_ops {
>   	void (*critical)(struct thermal_zone_device *);
>   };
>   
> +struct thermal_trip_ref {
> +	struct thermal_trip *trip;
> +};

That introduces a circular dependency. That should be avoided.

>   /**
>    * struct thermal_trip - representation of a point in temperature domain
>    * @temperature: temperature value in miliCelsius
>    * @hysteresis: relative hysteresis in miliCelsius
>    * @type: trip point type
> + * @driver_ref: driver's reference to this trip point
> + *
> + * If @driver_ref is not NULL, the trip pointer in the object pointed to by it
> + * will be initialized by the core during thermal zone registration and updated
> + * whenever the location of the given trip object changes.  This allows the
> + * driver to access the trip point data without knowing the relative ordering
> + * of trips within the trip table used by the core and, given a trip pointer,
> + * to get back to its private data associated with the given trip.
>    */
>   struct thermal_trip {
>   	int temperature;
>   	int hysteresis;
>   	enum thermal_trip_type type;
> +	struct thermal_trip_ref *driver_ref;
>   };

Why not use void *priv ?

AFAICT, the ACPI driver is the only one where when we reorder the trip 
points, the trip id is no longer matching the definition provided by the 
ACPI description.

It is possible to have the driver *specific* code to define its own 
structure with the id and use it instead of the trip_id.

So we end up with the ACPI driver registering the trip points with a 
data structure containing a private trip id.

The thermal framework is not supposed to have to deal with this kind of 
driver issues and from a higher perspective, any driver specific thing 
must stay in the driver.

eg.

struct acpi_thermal_trip_data {
	int id;
	... other info
};

struct acpi_thermal_trip_data attd[NRTRIPS] = { .id = 0 }, { .id = 1 }, ...

struct thermal_trip trips[NRTRIPS];

trips[i].priv = &attd[i];


The drivers with another kind of specific trip data can use this field.


>   struct thermal_cooling_device_ops {
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1306,14 +1306,28 @@ thermal_zone_device_register_with_trips(
>   	if (result)
>   		goto release_device;
>   
> +	mutex_lock(&tz->lock);
> +
>   	for (count = 0; count < num_trips; count++) {
> -		struct thermal_trip trip;
> +		int temperature = 0;
> +
> +		if (trips) {
> +			temperature = trips[count].temperature;
> +			if (trips[count].driver_ref)
> +				trips[count].driver_ref->trip = &trips[count];
> +		} else {
> +			struct thermal_trip trip;

As mentioned above, that should not appear in the thermal core code.


> -		result = thermal_zone_get_trip(tz, count, &trip);
> -		if (result || !trip.temperature)
> +			result = __thermal_zone_get_trip(tz, count, &trip);
> +			if (!result)
> +				temperature = trip.temperature;
> +		}
> +		if (!temperature)
>   			set_bit(count, &tz->trips_disabled);
>   	}
>   
> +	mutex_unlock(&tz->lock);
> +
>   	/* Update 'this' zone's governor information */
>   	mutex_lock(&thermal_governor_lock);
>   

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog