2018-09-21 13:07:12

by Willy Wolff

[permalink] [raw]
Subject: [PATCH] thermal: power_allocator: remove early break

If a trip of type critical is defined before any trip of passive
or active type, power_allocator governor will not switch on.

Signed-off-by: Willy Wolff <[email protected]>
---
drivers/thermal/power_allocator.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 3055f9a12a17..6544b68e3ebf 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -496,8 +496,6 @@ static void get_governor_trips(struct thermal_zone_device *tz,
}
} else if (type == THERMAL_TRIP_ACTIVE) {
last_active = i;
- } else {
- break;
}
}

--
2.11.0



2018-09-24 16:46:35

by Eduardo Valentin

[permalink] [raw]
Subject: Re: [PATCH] thermal: power_allocator: remove early break

Hey,

On Fri, Sep 21, 2018 at 02:06:09PM +0100, Willy Wolff wrote:
> If a trip of type critical is defined before any trip of passive
> or active type, power_allocator governor will not switch on.


But, if a critical trip is reached, the expectation is to perform a
thermal shutdown. Why would you expect to have power allocator
to be activated during the process of a shutdown?

>
> Signed-off-by: Willy Wolff <[email protected]>
> ---
> drivers/thermal/power_allocator.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> index 3055f9a12a17..6544b68e3ebf 100644
> --- a/drivers/thermal/power_allocator.c
> +++ b/drivers/thermal/power_allocator.c
> @@ -496,8 +496,6 @@ static void get_governor_trips(struct thermal_zone_device *tz,
> }
> } else if (type == THERMAL_TRIP_ACTIVE) {
> last_active = i;
> - } else {
> - break;
> }
> }
>
> --
> 2.11.0
>

2018-09-26 22:25:35

by Willy Wolff

[permalink] [raw]
Subject: Re: [PATCH] thermal: power_allocator: remove early break

This function is called at initialization when power_allocator
governor is set, and used to parametrize how the thermal
manager shoud work.
By reading its documentation, this function fix a switch_on
trip point (i.e. first passive trip point seen during a scan of
all trip points for that thermal_zone_device) and a
max_desired_temperature trip point (i. e. last seen passive
or active trip point during the scan). At last, if there is neither
a passive nor an active trip point, the governor does nothing.

If a critical trip point is defined before a passive trip point, the
governor will not work, even if there is a
passive or active trip point declared after the critical point.
For instance, on an Odroid-XU3/4 with a Samsung Exynos
5422 CPU, trip points are set as "active" first, "critical", then
"passive". https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi?h=linux-4.18.y#n53
There is no documentation constraining to an order in the
definition of trip points neither in the thermal manager nor in
power_allocator.
Thus, dts contributors could choose any order to define a
thermal-zones, and set the "critical" trip point first, breaking this
thermal governor.
On Mon, 24 Sep 2018 at 17:45, Eduardo Valentin <[email protected]> wrote:
>
> Hey,
>
> On Fri, Sep 21, 2018 at 02:06:09PM +0100, Willy Wolff wrote:
> > If a trip of type critical is defined before any trip of passive
> > or active type, power_allocator governor will not switch on.
>
>
> But, if a critical trip is reached, the expectation is to perform a
> thermal shutdown. Why would you expect to have power allocator
> to be activated during the process of a shutdown?
>
> >
> > Signed-off-by: Willy Wolff <[email protected]>
> > ---
> > drivers/thermal/power_allocator.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> > index 3055f9a12a17..6544b68e3ebf 100644
> > --- a/drivers/thermal/power_allocator.c
> > +++ b/drivers/thermal/power_allocator.c
> > @@ -496,8 +496,6 @@ static void get_governor_trips(struct thermal_zone_device *tz,
> > }
> > } else if (type == THERMAL_TRIP_ACTIVE) {
> > last_active = i;
> > - } else {
> > - break;
> > }
> > }
> >
> > --
> > 2.11.0
> >