Re: [PATCH v3 1/2] thermal: sysfs: Rework the handling of trip point updates

Rafael J. Wysocki
2023-12-04T17:47:16Z

Hi Daniel,

On Mon, Dec 4, 2023 at 5:55 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
>
> Hi Rafael,
>
> On 04/12/2023 15:50, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Both trip_point_temp_store() and trip_point_hyst_store() use
> > thermal_zone_set_trip() to update a given trip point, but none of them
> > actually needs to change more than one field in struct thermal_trip
> > representing it. However, each of them effectively calls
> > __thermal_zone_get_trip() twice in a row for the same trip index value,
> > once directly and once via thermal_zone_set_trip(), which is not
> > particularly efficient, and the way in which thermal_zone_set_trip()
> > carries out the update is not particularly straightforward.
> >
> > Moreover, input processing need not be done under the thermal zone lock
> > in any of these functions.
> >
> > Rework trip_point_temp_store() and trip_point_hyst_store() to address
> > the above, move the part of thermal_zone_set_trip() that is still
> > useful to a new function called thermal_zone_trip_updated() and drop
> > the rest of it.
> >
> > While at it, make trip_point_hyst_store() reject negative hysteresis
> > values.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > v2 -> v3: No changes
> >
> > v1 -> v2: Still check device_is_registered() under the zone lock
> >
> > ---
> > drivers/thermal/thermal_core.h | 2 +
> > drivers/thermal/thermal_sysfs.c | 75 ++++++++++++++++++++++++++++------------
> > drivers/thermal/thermal_trip.c | 45 ++++--------------------
> > include/linux/thermal.h | 4 --
> > 4 files changed, 64 insertions(+), 62 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_sysfs.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> > +++ linux-pm/drivers/thermal/thermal_sysfs.c
> > @@ -78,6 +78,19 @@ mode_store(struct device *dev, struct de
> > return count;
> > }
> >
> > +static int check_thermal_zone_and_trip_id(struct device *dev,
> > + struct thermal_zone_device *tz,
> > + int trip_id)
> > +{
> > + if (!device_is_registered(dev))
> > + return -ENODEV;
> > +
> > + if (trip_id < 0 || trip_id >= tz->num_trips)
> > + return -EINVAL;
>
> I'm not sure if this check is useful. The function is called from
> trip_point_*_store() which is providing the trip id from the file name
> parsing which is in turn built from an existing trip id. There is no
> reason the trip id is going to be wrong.

I can drop the check just fine.

Do you have any other comments on this patch?