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

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: "Zhang, Rui" <rui.zhang@intel.com>,
	"rafael@kernel.org" <rafael@kernel.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 11:25:30 +0100	[thread overview]
Message-ID: <d852675e-fdae-ec02-a4d1-4f3c7c8f64d7@linaro.org> (raw)
In-Reply-To: <f515876ea2e43e734b8d4ac7feda7f17ee04894f.camel@intel.com>

Hi Rui,

On 19/01/2023 08:22, Zhang, Rui wrote:
> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:

[ ... ]

>> +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.

If we pass the thermal trips to the 
thermal_zone_device_register_with_trips(), .get_trip_temp, 
.get_trip_hyst and .get_trip_type are not used.

.set_trip_temp is called from sysfs, where the trip_id is read from the 
file name. This trip_id will be correct in this case, as the files are 
created after sorting the array.

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

No, because at the moment we use the generic trip structure, it is 
handled by the thermal framework.

The drivers do not have to deal with the trip id or assuming its value 
given a trip point after registering the thermal zone. If it does, we 
should fix the driver as the trip_id is a framework internal value.

The trip_id is just an index to be passed around, so whatever the value, 
it should point to the right trip point.

For instance, the device tree describes the trip point and they could be 
in any order, all the DT based drivers are agnostic of the trip_id.

If there is an update of the trip points, we read the trip points 
definition again and do an update of all of them.

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

That what I proposed several months ago but we concluded that would 
another extra level of complexity. So we decided to replace all the 
.get_trip_* by a generic trip point structure handled by the thermal 
framework itself.

Hopefully, soon we will have all .get_trip* gone with a nice cleanup of 
the drivers and the core framework.

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