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

[Zhang, Rui]

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