Re: [PATCH v3 5/6] thermal: core: Sort trip point crossing notifications by temperature

[Lukasz Luba]

On 4/2/24 20:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> If multiple trip points are crossed in one go and the trips table in
> the thermal zone device object is not sorted, the corresponding trip
> point crossing notifications sent to user space will not be ordered
> either.
>
> Moreover, if the trips table is sorted by trip temperature in ascending
> order, the trip crossing notifications on the way up will be sent in that
> order too, but the trip crossing notifications on the way down will be
> sent in the reverse order.
>
> This is generally confusing and it is better to make the kernel send the
> notifications in the order of growing (on the way up) or falling (on the
> way down) trip temperature.
>
> To achieve that, instead of sending a trip crossing notification and
> recording a trip crossing event in the statistics right away from
> handle_thermal_trip(), put the trip in question on a list that will be
> sorted by __thermal_zone_device_update() after processing all of the trips
> and before sending the notifications and recording trip crossing events.
>
> Link: https://lore.kernel.org/linux-pm/20240306085428.88011-1-daniel.lezcano@xxxxxxxxxx/
> Reported-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>
> v2 -> v3: Rebase, update changelog, add notify_temp to struct thermal_trip_desc
>
> v1 -> v2: New patch
>
> ---
>   drivers/thermal/thermal_core.c |   41 +++++++++++++++++++++++++++++++++++------
>   drivers/thermal/thermal_core.h |    2 ++
>   2 files changed, 37 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -15,6 +15,7 @@
>   #include <linux/slab.h>
>   #include <linux/kdev_t.h>
>   #include <linux/idr.h>
> +#include <linux/list_sort.h>
>   #include <linux/thermal.h>
>   #include <linux/reboot.h>
>   #include <linux/string.h>
> @@ -361,7 +362,9 @@ static void handle_critical_trips(struct
>   }
>   
>   static void handle_thermal_trip(struct thermal_zone_device *tz,
> -				struct thermal_trip_desc *td)
> +				struct thermal_trip_desc *td,
> +				struct list_head *way_up_list,
> +				struct list_head *way_down_list)
>   {
>   	const struct thermal_trip *trip = &td->trip;
>   	int old_threshold;
> @@ -387,8 +390,8 @@ static void handle_thermal_trip(struct t
>   		 * In that case, the trip temperature becomes the new threshold.
>   		 */
>   		if (tz->temperature < trip->temperature - trip->hysteresis) {
> -			thermal_notify_tz_trip_down(tz, trip);
> -			thermal_debug_tz_trip_down(tz, trip);
> +			list_add(&td->notify_list_node, way_down_list);
> +			td->notify_temp = trip->temperature - trip->hysteresis;
>   		} else {
>   			td->threshold -= trip->hysteresis;
>   		}
> @@ -398,8 +401,8 @@ static void handle_thermal_trip(struct t
>   		 * if the zone temperature exceeds the trip one.  The new
>   		 * threshold is then set to the low temperature of the trip.
>   		 */
> -		thermal_notify_tz_trip_up(tz, trip);
> -		thermal_debug_tz_trip_up(tz, trip);
> +		list_add_tail(&td->notify_list_node, way_up_list);
> +		td->notify_temp = trip->temperature;
>   		td->threshold -= trip->hysteresis;
>   	}
>   
> @@ -452,10 +455,24 @@ static void thermal_zone_device_init(str
>   		pos->initialized = false;
>   }
>   
> +static int thermal_trip_notify_cmp(void *ascending, const struct list_head *a,
> +				   const struct list_head *b)
> +{
> +	struct thermal_trip_desc *tda = container_of(a, struct thermal_trip_desc,
> +						     notify_list_node);
> +	struct thermal_trip_desc *tdb = container_of(b, struct thermal_trip_desc,
> +						     notify_list_node);
> +	int ret = tdb->notify_temp - tda->notify_temp;
> +
> +	return ascending ? ret : -ret;
> +}
> +
>   void __thermal_zone_device_update(struct thermal_zone_device *tz,
>   				  enum thermal_notify_event event)
>   {
>   	struct thermal_trip_desc *td;
> +	LIST_HEAD(way_down_list);
> +	LIST_HEAD(way_up_list);
>   
>   	if (tz->suspended)
>   		return;
> @@ -470,7 +487,19 @@ void __thermal_zone_device_update(struct
>   	tz->notify_event = event;
>   
>   	for_each_trip_desc(tz, td)
> -		handle_thermal_trip(tz, td);
> +		handle_thermal_trip(tz, td, &way_up_list, &way_down_list);
> +
> +	list_sort(&way_up_list, &way_up_list, thermal_trip_notify_cmp);
> +	list_for_each_entry(td, &way_up_list, notify_list_node) {
> +		thermal_notify_tz_trip_up(tz, &td->trip);
> +		thermal_debug_tz_trip_up(tz, &td->trip);
> +	}
> +
> +	list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
> +	list_for_each_entry(td, &way_down_list, notify_list_node) {
> +		thermal_notify_tz_trip_down(tz, &td->trip);
> +		thermal_debug_tz_trip_down(tz, &td->trip);
> +	}
>   
>   	monitor_thermal_zone(tz);
>   }
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -17,6 +17,8 @@
>   
>   struct thermal_trip_desc {
>   	struct thermal_trip trip;
> +	struct list_head notify_list_node;
> +	int notify_temp;
>   	int threshold;
>   };
>   


Reviewed-by: Lukasz Luba <lukasz.luba@xxxxxxx>