Re: [PATCH v1] thermal: core: Move passive polling management to the core

Lukasz Luba
2024-04-30T14:58:30Z

On 4/30/24 14:48, Rafael J. Wysocki wrote:
> Hi Lukasz,
>
> On Mon, Apr 29, 2024 at 11:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Rafael,
>>
>> On 4/25/24 15:11, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Passive polling is enabled by setting the 'passive' field in
>>> struct thermal_zone_device to a positive value so long as the
>>> 'passive_delay_jiffies' field is greater than zero. It causes
>>> the thermal core to actively check the thermal zone temperature
>>> periodically which in theory should be done after crossing a
>>> passive trip point on the way up in order to allow governors to
>>> react more rapidly to temperature changes and adjust mitigation
>>> more precisely.
>>>
>>> However, the 'passive' field in struct thermal_zone_device is currently
>>> managed by governors which is quite problematic. First of all, only
>>> two governors, Step-Wise and Power Allocator, update that field at
>>> all, so the other governors do not benefit from passive polling,
>>> although in principle they should. Moreover, if the zone governor is
>>> changed from, say, Step-Wise to Fair-Share after 'passive' has been
>>> incremented by the former, it is not going to be reset back to zero by
>>> the latter even if the zone temperature falls down below all passive
>>> trip points.
>>>
>>> For this reason, make handle_thermal_trip() increment 'passive'
>>> to enable passive polling for the given thermal zone whenever a
>>> passive trip point is crossed on the way up and decrement it
>>> whenever a passive trip point is crossed on the way down. Also
>>> remove the 'passive' field updates from governors and additionally
>>> clear it in thermal_zone_device_init() to prevent passive polling
>>> from being enabled after a system resume just beacuse it was enabled
>>> before suspending the system.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>
>>> This has been mentioned here:
>>>
>>> https://lore.kernel.org/linux-pm/61560bc6-d453-4b0c-a4ea-b375d547b143@linaro.org/
>>>
>>> and I need someone to double check if the Power Allocator governor does not
>>> need to be adjusted more for this change.
>>>
>>> ---
>>> drivers/thermal/gov_power_allocator.c | 12 +++++++-----
>>> drivers/thermal/gov_step_wise.c | 10 ----------
>>> drivers/thermal/thermal_core.c | 10 ++++++++--
>>> 3 files changed, 15 insertions(+), 17 deletions(-)
>>>
>>> Index: linux-pm/drivers/thermal/thermal_core.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/thermal/thermal_core.c
>>> +++ linux-pm/drivers/thermal/thermal_core.c
>>> @@ -389,6 +389,9 @@ static void handle_thermal_trip(struct t
>>> if (tz->temperature < trip->temperature - trip->hysteresis) {
>>> list_add(&td->notify_list_node, way_down_list);
>>> td->notify_temp = trip->temperature - trip->hysteresis;
>>> +
>>> + if (trip->type == THERMAL_TRIP_PASSIVE)
>>> + tz->passive--;
>>
>> This gets negative values and than the core switches to fast 'polling'
>> mode. The values is decremented from 0 each time the
>> thermal_zone_device_enable() is called.
>
> Interesting.
>
> This shouldn't happen because it means that the passive trip has been
> crossed on the way down, but it wasn't crossed on the way up.
>
> It looks like an initialization issue to me.
>
>> Then IPA is actually called every 100ms after boot w/ low temp,
>> while it should 1s.
>>
>> Please see the logs below:
>> 'short log' after boot
>> ----------------------------------------------
>>
>> [ 1.632670] thermal_sys: TZ: tz_id=0 passive-- = -1
>> [ 1.637984] thermal_sys: TZ: tz_id=0 passive-- = -2
>> [ 1.643641] thermal_sys: TZ: tz_id=1 passive-- = -1
>> ----------------------------------------------
>>
>> long log with call stack dumped
>> ----------------------------------------------
>>
>> [ 1.632973] thermal_sys: TZ: tz_id=0 passive-- = -1
>> [ 1.638295] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 6.9.0-rc5+ #28
>> [ 1.645409] Hardware name: Radxa ROCK 4SE (DT)
>> [ 1.650376] Call trace:
>> [ 1.653109] dump_backtrace+0x9c/0x100
>> [ 1.657309] show_stack+0x20/0x38
>> [ 1.661017] dump_stack_lvl+0xc0/0xd0
>> [ 1.665119] dump_stack+0x18/0x28
>> [ 1.668828] __thermal_zone_device_update+0x1fc/0x550
>> [ 1.674484] thermal_zone_device_set_mode+0x64/0xc0
>> [ 1.679943] thermal_zone_device_enable+0x1c/0x30
>> [ 1.685206] thermal_of_zone_register+0x34c/0x590
>
> So let's see.
>
> thermal_of_zone_register() calls
> thermal_zone_device_register_with_trips() which calls
> thermal_zone_device_update() for the first time, but
> __thermal_zone_device_update() sees that
> thermal_zone_device_is_enabled() returns false, so it does nothing.
>
> This is right after thermal_zone_device_init() has been called, so
> tz->temperature == THERMAL_TEMP_INVALID and tz->passive == 0.
>
> Next, thermal_zone_device_enable() is called by
> thermal_of_zone_register() and it calls __thermal_zone_device_update()
> via thermal_zone_device_set_mode().
>
> This time thermal_zone_device_is_enabled() returns true, so
> update_temperature() is called and, unless __thermal_zone_get_temp()
> returns an error, it sets tz->last_temperature to THERMAL_TEMP_INVALID
> and tz->temperature to the current zone temperature.
>
> Next, handle_thermal_trip() is called for all trips and it sees that
> tz->last_temperature == THERMAL_TEMP_INVALID, so it skips the branch
> in which tz->passive is decremented.
>
> The only case I can see in which something else can happen in when
> __thermal_zone_get_temp() called by update_temperature() returns an
> error code (and if it is -EAGAIN, it won't even trigger a warning
> message) in which case the error is silently discarded and
> __thermal_zone_device_update() happily proceeds with tz->temperature
> == THERMAL_TEMP_INVALID and tz->last_temperature == 0.

That correct.

>
> This can lead to many surprises down the road, so IMV
> __thermal_zone_device_update() should abort if it sees tz->temperature
> == THERMAL_TEMP_INVALID past calling update_temperature().

agree

>
> So I'm wondering if the patch below (modulo white-space damage from
> GMail) helps.
>
>> [ 1.690473] devm_thermal_of_zone_register+0x6c/0xc0
>> [ 1.696031] rockchip_thermal_probe+0x238/0x5e8
>> [ 1.701106] platform_probe+0x70/0xe8
>> [ 1.705208] really_probe+0xc4/0x278
>> [ 1.709205] __driver_probe_device+0x80/0x140
>> [ 1.714078] driver_probe_device+0x48/0x130
>> [ 1.718756] __driver_attach+0x7c/0x138
>> [ 1.723045] bus_for_each_dev+0x80/0xf0
>> [ 1.727342] driver_attach+0x2c/0x40
>> [ 1.731340] bus_add_driver+0xec/0x1f8
>> [ 1.735539] driver_register+0x68/0x138
>> [ 1.739828] __platform_driver_register+0x30/0x48
>> [ 1.745093] rockchip_thermal_driver_init+0x24/0x38
>> [ 1.750551] do_one_initcall+0x50/0x2d8
>> [ 1.754844] kernel_init_freeable+0x204/0x440
>> [ 1.759722] kernel_init+0x28/0x140
>> [ 1.763631] ret_from_fork+0x10/0x20
>> [ 1.767802] thermal_sys: TZ: tz_id=0 passive-- = -2
>
> ---
> drivers/thermal/thermal_core.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -496,6 +496,9 @@ void __thermal_zone_device_update(struct
>
> update_temperature(tz);
>
> + if (tz->temperature == THERMAL_TEMP_INVALID)
> + return;
> +
> tz->notify_event = event;
>
> for_each_trip_desc(tz, td)

Yes, it prevents that previous situation. I have added a debug print
before your return in the code above and the it's in the log:

[ 1.632520] thermal_sys: TZ: THERMAL_TEMP_INVALID, return
[ 1.638899] thermal_sys: TZ: THERMAL_TEMP_INVALID, return

The tracing also shows that we have only 1s slow polling.
It also works properly in IPA when crossing 2 trip points
and coming back to low temp.

So, that code above should be OK. Thanks!