Re: [PATCH v2 2/2] thermal: trip: Rework thermal_zone_set_trip() and its callers

[Rafael J. Wysocki]

On Tue, Nov 28, 2023 at 10:42 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
>
>
>
> On 11/28/23 13:58, 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, some checks done by them both need not go under the thermal
> > zone lock and code duplication between them can be reduced quite a bit
> > by moving the majority of logic into thermal_zone_set_trip().
> >
> > Rework all of the above functions to address the above.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >
> > v1 -> v2:
> >     * Fix 2 typos in the changelog (Lukasz).
> >     * Split one change into the [1/2].
> >
> > ---
> >   drivers/thermal/thermal_core.h  |    9 ++++++
> >   drivers/thermal/thermal_sysfs.c |   52 ++++++++--------------------------
> >   drivers/thermal/thermal_trip.c  |   60 +++++++++++++++++++++++++++-------------
> >   include/linux/thermal.h         |    3 --
> >   4 files changed, 62 insertions(+), 62 deletions(-)
> >
>
> [snip]
>
> > Index: linux-pm/drivers/thermal/thermal_trip.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_trip.c
> > +++ linux-pm/drivers/thermal/thermal_trip.c
> > @@ -148,39 +148,61 @@ int thermal_zone_get_trip(struct thermal
> >   EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
> >
> >   int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> > -                       const struct thermal_trip *trip)
> > +                       enum thermal_set_trip_target what, const char *buf)
> >   {
> > -     struct thermal_trip t;
> > -     int ret;
> > +     struct thermal_trip *trip;
> > +     int val, ret = 0;
> >
> > -     ret = __thermal_zone_get_trip(tz, trip_id, &t);
> > +     if (trip_id < 0 || trip_id >= tz->num_trips)
> > +             ret = -EINVAL;
>
> That shouldn't progress forward IMO, but simply 'return -EINVAL;'...

Good catch, thank you!

> > +
> > +     ret = kstrtoint(buf, 10, &val);
> >       if (ret)
> >               return ret;
> >
> > -     if (t.type != trip->type)
> > -             return -EINVAL;
> > +     mutex_lock(&tz->lock);
> >
> > -     if (t.temperature != trip->temperature && tz->ops->set_trip_temp) {
> > -             ret = tz->ops->set_trip_temp(tz, trip_id, trip->temperature);
> > -             if (ret)
> > -                     return ret;
> > -     }
> > +     trip = &tz->trips[trip_id];
>
> ... because here we might get an issue.

Right.