[PATCH v1 0/2] thermal: core: Two fixes for 6.12

[PATCH v1 0/2] thermal: core: Two fixes for 6.12

[Rafael J. Wysocki]

Hi Everyone,

These patches address two thermal core issues that should better be taken
care of in 6.12.

The first patch deals with the handling of polling delays.  It could be
6.11-rc material even, but it may change the behavior somewhat and it's
better to avoid regressing the kernel late in the cycle.

The other one adds some sanity checks for the temperature and hysteresis
of writable trips, to prevent trip low temperature from falling below
THERMAL_TEMP_INVALID due to an invalid temperature/hysteresis combination.

Thanks!

[PATCH v1 1/2] thermal: core: Fix rounding of delay jiffies

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Using round_jiffies() in thermal_set_delay_jiffies() is invalid because
its argument should be time in the future in absolute jiffies and it
computes the result with respect to the current jiffies value at the
invocation time.  Fortunately, in the majority of cases it does not
make any difference due to the time_is_after_jiffies() check in
round_jiffies_common().

While using round_jiffies_relative() instead of round_jiffies() might
reflect the intent a bit better, it still would not be defensible
because that function should be called when the timer is about to be
set and it is not suitable for pre-computation of delay values.

Accordingly, drop thermal_set_delay_jiffies() altogether, simply
convert polling_delay and passive_delay to jiffies during thermal
zone initialization and make thermal_zone_device_set_polling() call
round_jiffies_relative() on the delay if it is greather than 1 second.

Fixes: 17d399cd9c89 ("thermal/core: Precompute the delays from msecs to jiffies")
Fixes: e5f2cda61d06 ("thermal/core: Move thermal_set_delay_jiffies to static")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -323,11 +323,15 @@ static void thermal_zone_broken_disable(
 static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
 					    unsigned long delay)
 {
-	if (delay)
-		mod_delayed_work(system_freezable_power_efficient_wq,
-				 &tz->poll_queue, delay);
-	else
+	if (!delay) {
 		cancel_delayed_work(&tz->poll_queue);
+		return;
+	}
+
+	if (delay > HZ)
+		delay = round_jiffies_relative(delay);
+
+	mod_delayed_work(system_freezable_power_efficient_wq, &tz->poll_queue, delay);
 }
 
 static void thermal_zone_recheck(struct thermal_zone_device *tz, int error)
@@ -1312,13 +1316,6 @@ void thermal_cooling_device_unregister(s
 }
 EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
 
-static void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms)
-{
-	*delay_jiffies = msecs_to_jiffies(delay_ms);
-	if (delay_ms > 1000)
-		*delay_jiffies = round_jiffies(*delay_jiffies);
-}
-
 int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp)
 {
 	const struct thermal_trip_desc *td;
@@ -1465,8 +1462,8 @@ thermal_zone_device_register_with_trips(
 		td->threshold = INT_MAX;
 	}
 
-	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
-	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
+	tz->polling_delay_jiffies = msecs_to_jiffies(polling_delay);
+	tz->passive_delay_jiffies = msecs_to_jiffies(passive_delay);
 	tz->recheck_delay_jiffies = THERMAL_RECHECK_DELAY;
 
 	/* sys I/F */

Re: [PATCH v1 1/2] thermal: core: Fix rounding of delay jiffies

[Daniel Lezcano]

On 22/08/2024 21:47, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Using round_jiffies() in thermal_set_delay_jiffies() is invalid because
> its argument should be time in the future in absolute jiffies and it
> computes the result with respect to the current jiffies value at the
> invocation time.  Fortunately, in the majority of cases it does not
> make any difference due to the time_is_after_jiffies() check in
> round_jiffies_common().
> 
> While using round_jiffies_relative() instead of round_jiffies() might
> reflect the intent a bit better, it still would not be defensible
> because that function should be called when the timer is about to be
> set and it is not suitable for pre-computation of delay values.
> 
> Accordingly, drop thermal_set_delay_jiffies() altogether, simply
> convert polling_delay and passive_delay to jiffies during thermal
> zone initialization and make thermal_zone_device_set_polling() call
> round_jiffies_relative() on the delay if it is greather than 1 second.

For the record:

In the history, the code was:

+       if (delay > 1000)
+               schedule_delayed_work(&(tz->poll_queue),
+ 
round_jiffies(msecs_to_jiffies(delay)));
+       else
+               schedule_delayed_work(&(tz->poll_queue),
+                                     msecs_to_jiffies(delay));


And the initial commit 21bc42ab852549f4a547d18d77e0e4d1b24ffd96:

"ACPI: thermal: use round_jiffies when thermal zone polling is enabled"

Good catch !

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>


> Fixes: 17d399cd9c89 ("thermal/core: Precompute the delays from msecs to jiffies")
> Fixes: e5f2cda61d06 ("thermal/core: Move thermal_set_delay_jiffies to static")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.c |   23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -323,11 +323,15 @@ static void thermal_zone_broken_disable(
>   static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
>   					    unsigned long delay)
>   {
> -	if (delay)
> -		mod_delayed_work(system_freezable_power_efficient_wq,
> -				 &tz->poll_queue, delay);
> -	else
> +	if (!delay) {
>   		cancel_delayed_work(&tz->poll_queue);
> +		return;
> +	}
> +
> +	if (delay > HZ)
> +		delay = round_jiffies_relative(delay);
> +
> +	mod_delayed_work(system_freezable_power_efficient_wq, &tz->poll_queue, delay);
>   }
>   
>   static void thermal_zone_recheck(struct thermal_zone_device *tz, int error)
> @@ -1312,13 +1316,6 @@ void thermal_cooling_device_unregister(s
>   }
>   EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
>   
> -static void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms)
> -{
> -	*delay_jiffies = msecs_to_jiffies(delay_ms);
> -	if (delay_ms > 1000)
> -		*delay_jiffies = round_jiffies(*delay_jiffies);
> -}
> -
>   int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp)
>   {
>   	const struct thermal_trip_desc *td;
> @@ -1465,8 +1462,8 @@ thermal_zone_device_register_with_trips(
>   		td->threshold = INT_MAX;
>   	}
>   
> -	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
> -	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
> +	tz->polling_delay_jiffies = msecs_to_jiffies(polling_delay);
> +	tz->passive_delay_jiffies = msecs_to_jiffies(passive_delay);
>   	tz->recheck_delay_jiffies = THERMAL_RECHECK_DELAY;
>   
>   	/* sys I/F */
> 
> 
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH v1 2/2] 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 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) {
+		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

Re: [PATCH v1 2/2] thermal: sysfs: Add sanity checks for trip temperature and hysteresis

[Daniel Lezcano]

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
> 
> 
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 2/2] thermal: sysfs: Add sanity checks for trip temperature and hysteresis

[Rafael J. Wysocki]

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;
> > +     }

Re: [PATCH v1 2/2] thermal: sysfs: Add sanity checks for trip temperature and hysteresis

[Rafael J. Wysocki]

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;
> > > +     }