| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d30068aa-b0df-4ec7-9a6b-9d5c6b49b74e@linaro.org>
Date: Thu, 4 Jul 2024 14:52:50 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Daniel Lezcano <daniel.lezcano@...aro.org>,
"Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Lukasz Luba <lukasz.luba@....com>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Zhang Rui <rui.zhang@...el.com>
Subject: Re: [PATCH v3] thermal: core: Call monitor_thermal_zone() if zone
temperature is invalid
Hi,
On 04/07/2024 14:49, Daniel Lezcano wrote:
> On 04/07/2024 13:46, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>>
>> Commit 202aa0d4bb53 ("thermal: core: Do not call handle_thermal_trip()
>> if zone temperature is invalid") caused __thermal_zone_device_update()
>> to return early if the current thermal zone temperature was invalid.
>>
>> This was done to avoid running handle_thermal_trip() and governor
>> callbacks in that case which led to confusion. However, it went too
>> far because monitor_thermal_zone() still needs to be called even when
>> the zone temperature is invalid to ensure that it will be updated
>> eventually in case thermal polling is enabled and the driver has no
>> other means to notify the core of zone temperature changes (for example,
>> it does not register an interrupt handler or ACPI notifier).
>>
>> Also if the .set_trips() zone callback is expected to set up monitoring
>> interrupts for a thermal zone, it needs to be provided with valid
>> boundaries and that can only be done if the zone temperature is known.
>>
>> Accordingly, to ensure that __thermal_zone_device_update() will
>> run again after a failing zone temperature check, make it call
>> monitor_thermal_zone() regardless of whether or not the zone
>> temperature is valid and make the latter schedule a thermal zone
>> temperature update if the zone temperature is invalid even if
>> polling is not enabled for the thermal zone (however, if this
>> continues to fail, give up after some time).
>
> Rafael,
>
> do we agree that we should fix somehow the current issue in this way because we are close to the merge window, but the proper fix is not doing that ?
I've tested this patch, but I have no opinion about it.
I sent https://lore.kernel.org/all/20240704-topic-sm8x50-upstream-fix-battmgr-temp-tz-warn-v1-1-9d66d6f6efde@linaro.org/ which
fixes the warning print, leaving the option for thermal core to update the tz once it becomes available,
which is the initial goal of this patchset.
Neil
>
>
>> Fixes: 202aa0d4bb53 ("thermal: core: Do not call handle_thermal_trip() if zone temperature is invalid")
>> Reported-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>> Link: https://lore.kernel.org/linux-pm/dc1e6cba-352b-4c78-93b5-94dd033fca16@linaro.org
>> Link: https://lore.kernel.org/linux-pm/2764814.mvXUDI8C0e@rjwysocki.net
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>> ---
>> drivers/thermal/thermal_core.c | 13 ++++++++++++-
>> drivers/thermal/thermal_core.h | 9 +++++++++
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> Index: linux-pm/drivers/thermal/thermal_core.c
>> ===================================================================
>> --- linux-pm.orig/drivers/thermal/thermal_core.c
>> +++ linux-pm/drivers/thermal/thermal_core.c
>> @@ -300,6 +300,14 @@ static void monitor_thermal_zone(struct
>> thermal_zone_device_set_polling(tz, tz->passive_delay_jiffies);
>> else if (tz->polling_delay_jiffies)
>> thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
>> + else if (tz->temperature == THERMAL_TEMP_INVALID &&
>> + tz->recheck_delay_jiffies <= THERMAL_MAX_RECHECK_DELAY) {
>> + thermal_zone_device_set_polling(tz, tz->recheck_delay_jiffies);
>> + /* Double the recheck delay for the next attempt. */
>> + tz->recheck_delay_jiffies += tz->recheck_delay_jiffies;
>> + if (tz->recheck_delay_jiffies > THERMAL_MAX_RECHECK_DELAY)
>> + dev_info(&tz->device, "Temperature unknown, giving up\n");
>> + }
>> }
>> static struct thermal_governor *thermal_get_tz_governor(struct thermal_zone_device *tz)
>> @@ -430,6 +438,7 @@ static void update_temperature(struct th
>> tz->last_temperature = tz->temperature;
>> tz->temperature = temp;
>> + tz->recheck_delay_jiffies = 1;
>> trace_thermal_temperature(tz);
>> @@ -514,7 +523,7 @@ void __thermal_zone_device_update(struct
>> update_temperature(tz);
>> if (tz->temperature == THERMAL_TEMP_INVALID)
>> - return;
>> + goto monitor;
>> tz->notify_event = event;
>> @@ -536,6 +545,7 @@ void __thermal_zone_device_update(struct
>> thermal_debug_update_trip_stats(tz);
>> +monitor:
>> monitor_thermal_zone(tz);
>> }
>> @@ -1438,6 +1448,7 @@ thermal_zone_device_register_with_trips(
>> thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
>> thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
>> + tz->recheck_delay_jiffies = 1;
>> /* sys I/F */
>> /* Add nodes that are always present via .groups */
>> Index: linux-pm/drivers/thermal/thermal_core.h
>> ===================================================================
>> --- linux-pm.orig/drivers/thermal/thermal_core.h
>> +++ linux-pm/drivers/thermal/thermal_core.h
>> @@ -67,6 +67,8 @@ struct thermal_governor {
>> * @polling_delay_jiffies: number of jiffies to wait between polls when
>> * checking whether trip points have been crossed (0 for
>> * interrupt driven systems)
>> + * @recheck_delay_jiffies: delay after a failed thermal zone temperature check
>> + * before attempting to check it again
>> * @temperature: current temperature. This is only for core code,
>> * drivers should use thermal_zone_get_temp() to get the
>> * current temperature
>> @@ -108,6 +110,7 @@ struct thermal_zone_device {
>> int num_trips;
>> unsigned long passive_delay_jiffies;
>> unsigned long polling_delay_jiffies;
>> + unsigned long recheck_delay_jiffies;
>> int temperature;
>> int last_temperature;
>> int emul_temperature;
>> @@ -133,6 +136,12 @@ struct thermal_zone_device {
>> struct thermal_trip_desc trips[] __counted_by(num_trips);
>> };
>> +/*
>> + * Maximum delay after a failing thermal zone temperature check before
>> + * attempting to check it again (in jiffies).
>> + */
>> +#define THERMAL_MAX_RECHECK_DELAY (30 * HZ)
>> +
>> /* Default Thermal Governor */
>> #if defined(CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE)
>> #define DEFAULT_THERMAL_GOVERNOR "step_wise"
>>
>>
>>
>
Powered by blists - more mailing lists