Messages in this thread | ![/](/images/icornerl.gif) | | From | "Zhang, Rui" <> | 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 |
| |
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
| ![\](/images/icornerr.gif) |