| Message ID | 12528772.O9o76ZdvQC@rjwysocki.net (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v2] thermal: sysfs: Add sanity checks for trip temperature and hysteresis | expand |
On 8/26/24 17:21, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Add sanity checks for new trip temperature and hysteresis values to > trip_point_temp_store() and trip_point_hyst_store() to prevent trip > point threshold from falling below THERMAL_TEMP_INVALID. > > However, still allow user space to pass THERMAL_TEMP_INVALID as the > new trip temperature value to invalidate the trip if necessary. > > Also allow the hysteresis to be updated when the temperature is invalid > to allow user space to avoid having to adjust hysteresis after a valid > temperature has been set, but in that case just change the value and do > nothing else. > > Fixes: be0a3600aa1e ("thermal: sysfs: Rework the handling of trip point updates") > Cc: 6.8+ <stable@vger.kernel.org> # 6.8+ > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Reorder the new check in trip_point_hyst_store() to be easier > to read (Daniel). > * Add a comment explaining the ordering of the new check in > trip_point_temp_store(). > * Allow the hysteresis to be updated when the temperature is invalid. > > This is an update of > > https://lore.kernel.org/linux-pm/7719509.EvYhyI6sBW@rjwysocki.net/ > > which is being sent separately because patch [1/2] from the original series > has been applied. > > --- > drivers/thermal/thermal_sysfs.c | 50 ++++++++++++++++++++++++++++++---------- > 1 file changed, 38 insertions(+), 12 deletions(-) > > Index: linux-pm/drivers/thermal/thermal_sysfs.c > =================================================================== > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c > +++ linux-pm/drivers/thermal/thermal_sysfs.c > @@ -111,18 +111,26 @@ trip_point_temp_store(struct device *dev > > mutex_lock(&tz->lock); > > - if (temp != trip->temperature) { > - if (tz->ops.set_trip_temp) { > - ret = tz->ops.set_trip_temp(tz, trip, temp); > - if (ret) > - goto unlock; > - } > + if (temp == trip->temperature) > + goto unlock; > > - thermal_zone_set_trip_temp(tz, trip, temp); > + /* Arrange the condition to avoid integer overflows. */ > + if (temp != THERMAL_TEMP_INVALID && > + temp <= trip->hysteresis + THERMAL_TEMP_INVALID) { > + ret = -EINVAL; > + goto unlock; > + } > > - __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); > + if (tz->ops.set_trip_temp) { > + ret = tz->ops.set_trip_temp(tz, trip, temp); > + if (ret) > + goto unlock; > } > > + thermal_zone_set_trip_temp(tz, trip, temp); > + > + __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); > + > unlock: > mutex_unlock(&tz->lock); > > @@ -152,15 +160,33 @@ trip_point_hyst_store(struct device *dev > > mutex_lock(&tz->lock); > > - if (hyst != trip->hysteresis) { > - thermal_zone_set_trip_hyst(tz, trip, hyst); > + if (hyst == trip->hysteresis) > + goto unlock; > + > + /* > + * Allow the hysteresis to be updated when the temperature is invalid > + * to allow user space to avoid having to adjust hysteresis after a > + * valid temperature has been set, but in that case just change the > + * value and do nothing else. > + */ > + if (trip->temperature == THERMAL_TEMP_INVALID) { > + WRITE_ONCE(trip->hysteresis, hyst); > + goto unlock; > + } > > - __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); > + if (trip->temperature - hyst <= THERMAL_TEMP_INVALID) { > + ret = -EINVAL; > + goto unlock; > } > > + thermal_zone_set_trip_hyst(tz, trip, hyst); > + > + __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); > + > +unlock: > mutex_unlock(&tz->lock); > > - return count; > + return ret ? ret : count; > } > > static ssize_t > > > Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Index: linux-pm/drivers/thermal/thermal_sysfs.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_sysfs.c +++ linux-pm/drivers/thermal/thermal_sysfs.c @@ -111,18 +111,26 @@ trip_point_temp_store(struct device *dev mutex_lock(&tz->lock); - if (temp != trip->temperature) { - if (tz->ops.set_trip_temp) { - ret = tz->ops.set_trip_temp(tz, trip, temp); - if (ret) - goto unlock; - } + if (temp == trip->temperature) + goto unlock; - thermal_zone_set_trip_temp(tz, trip, temp); + /* Arrange the condition to avoid integer overflows. */ + if (temp != THERMAL_TEMP_INVALID && + temp <= trip->hysteresis + THERMAL_TEMP_INVALID) { + ret = -EINVAL; + goto unlock; + } - __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); + if (tz->ops.set_trip_temp) { + ret = tz->ops.set_trip_temp(tz, trip, temp); + if (ret) + goto unlock; } + thermal_zone_set_trip_temp(tz, trip, temp); + + __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); + unlock: mutex_unlock(&tz->lock); @@ -152,15 +160,33 @@ trip_point_hyst_store(struct device *dev mutex_lock(&tz->lock); - if (hyst != trip->hysteresis) { - thermal_zone_set_trip_hyst(tz, trip, hyst); + if (hyst == trip->hysteresis) + goto unlock; + + /* + * Allow the hysteresis to be updated when the temperature is invalid + * to allow user space to avoid having to adjust hysteresis after a + * valid temperature has been set, but in that case just change the + * value and do nothing else. + */ + if (trip->temperature == THERMAL_TEMP_INVALID) { + WRITE_ONCE(trip->hysteresis, hyst); + goto unlock; + } - __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); + if (trip->temperature - hyst <= THERMAL_TEMP_INVALID) { + ret = -EINVAL; + goto unlock; } + thermal_zone_set_trip_hyst(tz, trip, hyst); + + __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); + +unlock: mutex_unlock(&tz->lock); - return count; + return ret ? ret : count; } static ssize_t