| Message ID | 7719509.EvYhyI6sBW@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | thermal: core: Two fixes for 6.12 | expand |
On 22/08/2024 21:48, 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 thresholds 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. > > 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> > --- > drivers/thermal/thermal_sysfs.c | 38 ++++++++++++++++++++++++++------------ > 1 file changed, 26 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,25 @@ 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); > + if (temp != THERMAL_TEMP_INVALID && > + temp <= trip->hysteresis + THERMAL_TEMP_INVALID) { It seems to me the condition is hard to understand. temp <= trip->hysteresis + THERMAL_TEMP_INVALID ==> temp - trip->hysteresis <= THERMAL_TEMP_INVALID Could be the test below simpler to understand ? if (trip->hysteresis && temp - trip->hysteresis < THERMAL_TEMP_INVALID)) I think more sanity check may be needed also. if (temp < 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 +159,22 @@ 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; > > - __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); > + if (hyst + THERMAL_TEMP_INVALID >= trip->temperature) { > + 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 > > >
On Fri, Aug 23, 2024 at 5:26 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 22/08/2024 21:48, 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 thresholds 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. > > > > 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> > > --- > > drivers/thermal/thermal_sysfs.c | 38 ++++++++++++++++++++++++++------------ > > 1 file changed, 26 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,25 @@ 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); > > + if (temp != THERMAL_TEMP_INVALID && > > + temp <= trip->hysteresis + THERMAL_TEMP_INVALID) { > > It seems to me the condition is hard to understand. That's not the key consideration here though. > > temp <= trip->hysteresis + THERMAL_TEMP_INVALID This cannot overflow because trip->hysteresis is non-negative. > > ==> > > temp - trip->hysteresis <= THERMAL_TEMP_INVALID But this can. > > > Could be the test below simpler to understand ? > > if (trip->hysteresis && > temp - trip->hysteresis < THERMAL_TEMP_INVALID)) > > I think more sanity check may be needed also. > > if (temp < THERMAL_TEMP_INVALID) With my version of the check above this is not necessary (unless I'm missing something}. > > + ret = -EINVAL; > > + goto unlock; > > + }
On Fri, Aug 23, 2024 at 6:39 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Aug 23, 2024 at 5:26 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: > > > > On 22/08/2024 21:48, 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 thresholds 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. > > > > > > 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> > > > --- > > > drivers/thermal/thermal_sysfs.c | 38 ++++++++++++++++++++++++++------------ > > > 1 file changed, 26 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,25 @@ 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); > > > + if (temp != THERMAL_TEMP_INVALID && > > > + temp <= trip->hysteresis + THERMAL_TEMP_INVALID) { > > > > It seems to me the condition is hard to understand. > > That's not the key consideration here though. > > > > > temp <= trip->hysteresis + THERMAL_TEMP_INVALID > > This cannot overflow because trip->hysteresis is non-negative. > > > > > ==> > > > > temp - trip->hysteresis <= THERMAL_TEMP_INVALID > > But this can. Well, I think I should add a comment there to point that out or people will try to "clean it up". Also note that in the hysteresis case the condition can be if (trip->temperature - hyst <= THERMAL_TEMP_INVALID) { because trip->temperature is never below THERMAL_TEMP_INVALID there. Moreover, setting the hysteresis when the temperature is THERMAL_TRIP_INVALID does not make much sense. I'll send a v2. > > > > > > Could be the test below simpler to understand ? > > > > if (trip->hysteresis && > > temp - trip->hysteresis < THERMAL_TEMP_INVALID)) > > > > I think more sanity check may be needed also. > > > > if (temp < THERMAL_TEMP_INVALID) > > With my version of the check above this is not necessary (unless I'm > missing something}. > > > > + ret = -EINVAL; > > > + goto unlock; > > > + }
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,25 @@ 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); + 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 +159,22 @@ 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; - __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); + if (hyst + THERMAL_TEMP_INVALID >= trip->temperature) { + 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