Re: [PATCH 5/5] thermal/core: Sort the trip points when registering a thermal zone

From: "Zhang, Rui" <rui.zhang@intel.com>
To: "rafael@kernel.org" <rafael@kernel.org>,
	"daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>
Cc: "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"amitk@kernel.org" <amitk@kernel.org>
Subject: Re: [PATCH 5/5] thermal/core: Sort the trip points when registering a thermal zone
Date: Thu, 19 Jan 2023 07:22:59 +0000	[thread overview]
Message-ID: <f515876ea2e43e734b8d4ac7feda7f17ee04894f.camel@intel.com> (raw)
In-Reply-To: <20230118211123.111493-5-daniel.lezcano@linaro.org>

On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> Most of the drivers are converted to use the generic thermal trip
> points. They register a thermal zone with an array of trip points.
> 
> However, we don't have the guarantee the trip points are ordered. The
> main goal of moving to the generic trip points is to provide a common
> structure, ordered, so we can fix sanely how the trip points are
> crossed. As a reminder, the detection is fuzzy when the trip points
> are defined with hysteresis values, we can have duplicated or
> inconsistent trip events.
> 
> This change sorts the trip points array when it is registered with
> the
> thermal zone. The direction of the ordering is descending because
> when
> we browse the trip points, we want to check the highest trip points
> first as they are higher in temperature, thus higher in priority.
> 
> A pr_info() trace tells the trip points have been ordered if it
> appears they are not sorted initially.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c |  3 +++
>  drivers/thermal/thermal_core.h |  1 +
>  drivers/thermal/thermal_trip.c | 28 ++++++++++++++++++++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index d0577685085a..394770591771 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1255,6 +1255,9 @@ thermal_zone_device_register_with_trips(const
> char *type, struct thermal_trip *t
>  	if (num_trips > 0 && (!ops->get_trip_type || !ops-
> >get_trip_temp) && !trips)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (trips && thermal_trip_sort(trips, num_trips))
> +		pr_info("Thermal trips sorted for thermal zone '%s'\n",
> type);
> +	
>  	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
>  	if (!tz)
>  		return ERR_PTR(-ENOMEM);
> diff --git a/drivers/thermal/thermal_core.h
> b/drivers/thermal/thermal_core.h
> index 26350206a98d..4688107fda1d 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -117,6 +117,7 @@ void __thermal_zone_set_trips(struct
> thermal_zone_device *tz);
>  int __thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
>  			    struct thermal_trip *trip);
>  int __thermal_zone_get_temp(struct thermal_zone_device *tz, int
> *temp);
> +int thermal_trip_sort(struct thermal_trip *trips, int num_trips);
>  
>  /* sysfs I/F */
>  int thermal_zone_create_device_groups(struct thermal_zone_device *,
> int);
> diff --git a/drivers/thermal/thermal_trip.c
> b/drivers/thermal/thermal_trip.c
> index 2ef61ff7ffc3..924998f09a5a 100644
> --- a/drivers/thermal/thermal_trip.c
> +++ b/drivers/thermal/thermal_trip.c
> @@ -9,6 +9,34 @@
>   */
>  #include "thermal_core.h"
>  
> +/*
> + * The trip points must be ordered in the descending order so when
> we
> + * browse the trip points we will hit the critical, hot and then the
> + * passive/active trip points. The critical trip point being the
> first
> + * one to be handled.
> + */
> +int thermal_trip_sort(struct thermal_trip *trips, int num_trips)
> +{
> +	struct thermal_trip tt;
> +	int sorted = 0;
> +	int i, j;
> +
> +	for (i = 0; i < num_trips; i++) {
> +
> +		for (j = i + 1; j < num_trips; j++) {
> +
> +			if (trips[i].temperature <
> trips[j].temperature) {
> +				tt = trips[i];
> +				trips[i] = trips[j];
> +				trips[j] = tt;
> +				sorted++;
> +			}
> +		}
> + 	}
> +
> +	return sorted;
> +}
> +
When this happens, the index(trip_id) of each trip is changed, but we
pass the new trip_id to .get_trip_temp()/.set_trip_temp() callbacks.

This will confuse the drivers and update the wrong trips, right?

IMO, we need a map between thermal core trips and unsorted driver
trips.

thanks,
rui

>  int __for_each_thermal_trip(struct thermal_zone_device *tz,
>  			    int (*cb)(struct thermal_trip *,
>  				      int trip_id, void *),