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

[Rafael J. Wysocki]

Hi Daniel,

On Mon, Dec 4, 2023 at 5:55 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
>
> Hi Rafael,
>
> On 04/12/2023 15:50, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >
> > 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?