| Message ID | 1887866.tdWV9SEqCh@kreacher |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thermal: trip: Rework thermal_zone_set_trip() and its callers | expand |
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;'... > + > + 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.
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.
Index: linux-pm/drivers/thermal/thermal_core.h =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.h +++ linux-pm/drivers/thermal/thermal_core.h @@ -122,6 +122,15 @@ void __thermal_zone_device_update(struct void __thermal_zone_set_trips(struct thermal_zone_device *tz); int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id, struct thermal_trip *trip); + +enum thermal_set_trip_target { + THERMAL_TRIP_SET_TEMP, + THERMAL_TRIP_SET_HYST, +}; + +int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id, + enum thermal_set_trip_target what, const char *buf); + int thermal_zone_trip_id(struct thermal_zone_device *tz, const struct thermal_trip *trip); int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp); Index: linux-pm/drivers/thermal/thermal_sysfs.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_sysfs.c +++ linux-pm/drivers/thermal/thermal_sysfs.c @@ -120,31 +120,17 @@ trip_point_temp_store(struct device *dev const char *buf, size_t count) { struct thermal_zone_device *tz = to_thermal_zone(dev); - struct thermal_trip trip; - int trip_id, ret; + int trip_id; + int ret; + + if (!device_is_registered(dev)) + return -ENODEV; if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1) return -EINVAL; - mutex_lock(&tz->lock); - - if (!device_is_registered(dev)) { - ret = -ENODEV; - goto unlock; - } - - ret = __thermal_zone_get_trip(tz, trip_id, &trip); - if (ret) - goto unlock; - - ret = kstrtoint(buf, 10, &trip.temperature); - if (ret) - goto unlock; + ret = thermal_zone_set_trip(tz, trip_id, THERMAL_TRIP_SET_TEMP, buf); - ret = thermal_zone_set_trip(tz, trip_id, &trip); -unlock: - mutex_unlock(&tz->lock); - return ret ? ret : count; } @@ -179,30 +165,16 @@ trip_point_hyst_store(struct device *dev const char *buf, size_t count) { struct thermal_zone_device *tz = to_thermal_zone(dev); - struct thermal_trip trip; - int trip_id, ret; + int trip_id; + int ret; + + if (!device_is_registered(dev)) + return -ENODEV; if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1) return -EINVAL; - mutex_lock(&tz->lock); - - if (!device_is_registered(dev)) { - ret = -ENODEV; - goto unlock; - } - - ret = __thermal_zone_get_trip(tz, trip_id, &trip); - if (ret) - goto unlock; - - ret = kstrtoint(buf, 10, &trip.hysteresis); - if (ret) - goto unlock; - - ret = thermal_zone_set_trip(tz, trip_id, &trip); -unlock: - mutex_unlock(&tz->lock); + ret = thermal_zone_set_trip(tz, trip_id, THERMAL_TRIP_SET_HYST, buf); return ret ? ret : count; } 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; + + 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]; - if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) { - ret = tz->ops->set_trip_hyst(tz, trip_id, trip->hysteresis); - if (ret) - return ret; + switch (what) { + case THERMAL_TRIP_SET_TEMP: + if (val == trip->temperature) + goto unlock; + + if (tz->ops->set_trip_temp) { + ret = tz->ops->set_trip_temp(tz, trip_id, val); + if (ret) + goto unlock; + } + trip->temperature = val; + break; + + case THERMAL_TRIP_SET_HYST: + if (val == trip->hysteresis) + goto unlock; + + if (tz->ops->set_trip_hyst) { + ret = tz->ops->set_trip_hyst(tz, trip_id, val); + if (ret) + goto unlock; + } + trip->hysteresis = val; + break; + + default: + ret = -EINVAL; + goto unlock; } - if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis)) - tz->trips[trip_id] = *trip; - thermal_notify_tz_trip_change(tz->id, trip_id, trip->type, trip->temperature, trip->hysteresis); __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); - return 0; +unlock: + mutex_unlock(&tz->lock); + + return ret; } int thermal_zone_trip_id(struct thermal_zone_device *tz, Index: linux-pm/include/linux/thermal.h =================================================================== --- linux-pm.orig/include/linux/thermal.h +++ linux-pm/include/linux/thermal.h @@ -283,9 +283,6 @@ int __thermal_zone_get_trip(struct therm int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id, struct thermal_trip *trip); -int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id, - const struct thermal_trip *trip); - int for_each_thermal_trip(struct thermal_zone_device *tz, int (*cb)(struct thermal_trip *, void *), void *data);