[PATCH v2] thermal: sysfs: Add sanity checks for trip temperature and hysteresis

[PATCH v2] thermal: sysfs: Add sanity checks for trip temperature and hysteresis

[Rafael J. Wysocki]

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

Re: [PATCH v2] thermal: sysfs: Add sanity checks for trip temperature and hysteresis

[Lukasz Luba]

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>