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 16:50:34 +0000	[thread overview]
Message-ID: <546fcd7a262395efeafc93c6cbd0736efab5eace.camel@intel.com> (raw)
In-Reply-To: <d852675e-fdae-ec02-a4d1-4f3c7c8f64d7@linaro.org>

On Thu, 2023-01-19 at 11:25 +0100, Daniel Lezcano wrote:
> 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.

agreed.

> 
> .set_trip_temp is called from sysfs, where the trip_id is read from
> the 
> file name.

yes.

>  This trip_id will be correct in this case, as the files are 
> created after sorting the array.

yes, the trip_id from sysfs matches its index in tz->trips[].

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

I didn't quite follow this.
Please correct me if my understanding is wrong,

Say, driver supports two writable trip point A and B, B has higher
temperature but it is set in trips[0] when the driver registers the
thermal device.

After thermal_trip_sort(), B becomes trips[1], and its sysfs attribute
is shown as trip_point_1_xxx, right?

When setting the trip B temperature, trip_id is 1, and we invoke
	tz->ops->set_trip_temp(tz, trip_id, trip->temperature);

In the driver, the .set_trip_temp() callback updates trip A instead of
trip B because trip_id == 1 stands for trip A from the drivers
perspective of view, right?

You can refer to the .set_trip_temp() callback of x86_pkg_temp_thermal.
c which handles two trip points.

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

If the problem is valid, maybe we can add an 'orig_id' to struct
thermal_trip for the driver to reference?

thanks,
rui