[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <546fcd7a262395efeafc93c6cbd0736efab5eace.camel@intel.com>
Date: Thu, 19 Jan 2023 16:50:34 +0000
From: "Zhang, Rui" <rui.zhang@...el.com>
To: "rafael@...nel.org" <rafael@...nel.org>,
"daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>
CC: "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"amitk@...nel.org" <amitk@...nel.org>
Subject: Re: [PATCH 5/5] thermal/core: Sort the trip points when registering a
thermal zone
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
Powered by blists - more mailing lists