| 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: <CAJZ5v0gRoy0+LD0EgwbS0NL8kHhas4T6itgawGjE8iNf57MOcg@mail.gmail.com>
Date: Thu, 4 Jul 2024 16:21:23 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, Lukasz Luba <lukasz.luba@....com>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>, Zhang Rui <rui.zhang@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH v3] thermal: core: Call monitor_thermal_zone() if zone
temperature is invalid
On Thu, Jul 4, 2024 at 2:49 PM Daniel Lezcano <daniel.lezcano@...aro.org> 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,
Yes.
> but the proper fix is not doing that ?
We need to decide what to do in general when __thermal_zone_get_temp()
returns an error. A proper fix would result from that, but it would
require more time than is available IMV. We can properly fix this in
6.11.
For 6.10 I see two options:
1. Apply the v2 of this patch:
https://lore.kernel.org/linux-pm/2764814.mvXUDI8C0e@rjwysocki.net/
I slightly prefer it because it is simpler and doesn't change the size
of struct thermal_zone_device. However, the clear disadvantage of it
is that it will poke at dead thermal zones indefinitely.
The THERMAL_RECHECK_DELAY_MS value in it can be adjusted. Maybe 250
ms would be a better choice?
2. Apply this patch (ie. v3)
It is nicer to thermal zones that never become operational, but it may
miss thermal zones that become operational very late.
> > 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"
> >
> >
> >
>
> --
> <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
>
>
Powered by blists - more mailing lists