Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Wed, 29 Nov 2023 13:16:51 +0100 | Subject | Re: [PATCH v2 2/2] thermal: trip: Rework thermal_zone_set_trip() and its callers |
| |
On Tue, Nov 28, 2023 at 10:42 PM Lukasz Luba <lukasz.luba@arm.com> wrote: > > > > On 11/28/23 13:58, 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, 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@intel.com> > > --- > > > > 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.
| |