[PATCH 0/3] thermal: hook in with reboot and crash

[PATCH 0/3] thermal: hook in with reboot and crash

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

Hello,

This small series of changes teaches thermal core about
reboot and crash callbacks. The intention is to have the core
to get notified and the pass in the event to thermal governors
that are willing to perform actions during reboot or crash events.
The thermal workers will be teared down in the process too.

There is no code dependency this series was built on top of:
https://lkml.org/lkml/2023/5/18/1207

Separate governor changes will be sent in another series.

BR,

Eduardo Valentin (3):
  thermal: core: introduce governor .reboot_prepare()
  thermal: core: register reboot nb
  thermal: core: register a crash callback

 drivers/thermal/thermal_core.c | 54 ++++++++++++++++++++++++++++++++++
 include/linux/thermal.h        |  4 +++
 2 files changed, 58 insertions(+)

-- 
2.34.1

[PATCH 1/3] thermal: core: introduce governor .reboot_prepare()

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

This callback is used to notify the governors that
reboot is coming up. Upon this event, this callback
gives governors the opportunity to leave the thermal
zones in a sane state prior to reboot.

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 include/linux/thermal.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 82c8e09a63e0..d3c8af928522 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -201,6 +201,9 @@ struct thermal_zone_device {
  *		below the trip point temperature
  * @check_error:	callback called whenever temperature updates fail.
  *		Opportunity for the governor to react on errors.
+ * @reboot_prepare:	callback called upon system restart.
+ *		Opportunity for the governor to tear down zones or at least
+ *		leave them in a safe state.
  * @governor_list:	node in thermal_governor_list (in thermal_core.c)
  */
 struct thermal_governor {
@@ -209,6 +212,7 @@ struct thermal_governor {
 	void (*unbind_from_tz)(struct thermal_zone_device *tz);
 	int (*throttle)(struct thermal_zone_device *tz, int trip);
 	void (*check_error)(struct thermal_zone_device *tz, int error);
+	void (*reboot_prepare)(struct thermal_zone_device *tz);
 	struct list_head	governor_list;
 };
 
-- 
2.34.1

[PATCH 2/3] thermal: core: register reboot nb

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

This commit will register a reboot notifier block
callback for giving opportunity tearing down
thermal zones, e.g. remove their workqueues, and
for giving the governors the opportunity to leave
the hardware in a known safe state.

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 drivers/thermal/thermal_core.c | 39 ++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 359e7b2ff0e3..66a255fb650b 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -319,6 +319,12 @@ static void handle_error_temperature(struct thermal_zone_device *tz, int error)
 		tz->governor->check_error(tz, error);
 }
 
+static void handle_reboot_prepare(struct thermal_zone_device *tz)
+{
+	if (tz->governor && tz->governor->reboot_prepare)
+		tz->governor->reboot_prepare(tz);
+}
+
 void thermal_zone_device_critical(struct thermal_zone_device *tz)
 {
 	/*
@@ -1508,6 +1514,30 @@ struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name)
 }
 EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
 
+/* Best effort attempt to leave thermal zones at a safe state across reboots */
+static void thermal_reboot_prepare(void)
+{
+	struct thermal_zone_device *tz;
+
+	list_for_each_entry(tz, &thermal_tz_list, node) {
+		cancel_delayed_work(&tz->poll_queue);
+		handle_reboot_prepare(tz);
+	}
+}
+
+static int thermal_reboot_notify(struct notifier_block *nb,
+				 unsigned long mode, void *_unused)
+{
+	switch (mode) {
+	case SYS_RESTART:
+		thermal_reboot_prepare();
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
 static int thermal_pm_notify(struct notifier_block *nb,
 			     unsigned long mode, void *_unused)
 {
@@ -1539,6 +1569,10 @@ static struct notifier_block thermal_pm_nb = {
 	.notifier_call = thermal_pm_notify,
 };
 
+static struct notifier_block thermal_reboot_nb = {
+	.notifier_call = thermal_reboot_notify,
+};
+
 static int __init thermal_init(void)
 {
 	int result;
@@ -1572,6 +1606,11 @@ static int __init thermal_init(void)
 		pr_warn("Thermal: Can not register suspend notifier, return %d\n",
 			result);
 
+	result = register_reboot_notifier(&thermal_reboot_nb);
+	if (result)
+		pr_warn("Thermal: Can not register reboot notifier, return %d\n",
+			result);
+
 	return 0;
 
 unregister_governors:
-- 
2.34.1

[PATCH 3/3] thermal: core: register a crash callback

[Eduardo Valentin]

From: Eduardo Valentin <eduval@amazon.com>

This commit will register a crash callback for
the thermal subsystem, this way the thermal
core can tear down the thermal zones and ask
governors to leave the hardware in a known
safe state prior upon the event of a crash.

Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-pm@vger.kernel.org (open list:THERMAL)
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)

Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 drivers/thermal/thermal_core.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 66a255fb650b..38b168b9245e 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -20,6 +20,7 @@
 #include <linux/string.h>
 #include <linux/of.h>
 #include <linux/suspend.h>
+#include <linux/panic_notifier.h>
 
 #define CREATE_TRACE_POINTS
 #include "thermal_trace.h"
@@ -1525,6 +1526,14 @@ static void thermal_reboot_prepare(void)
 	}
 }
 
+static int thermal_crash_notify(struct notifier_block *nb,
+				unsigned long mode, void *_unused)
+{
+	thermal_reboot_prepare();
+
+	return 0;
+}
+
 static int thermal_reboot_notify(struct notifier_block *nb,
 				 unsigned long mode, void *_unused)
 {
@@ -1569,6 +1578,10 @@ static struct notifier_block thermal_pm_nb = {
 	.notifier_call = thermal_pm_notify,
 };
 
+static struct notifier_block thermal_crash_nb = {
+	.notifier_call = thermal_crash_notify,
+};
+
 static struct notifier_block thermal_reboot_nb = {
 	.notifier_call = thermal_reboot_notify,
 };
@@ -1611,6 +1624,8 @@ static int __init thermal_init(void)
 		pr_warn("Thermal: Can not register reboot notifier, return %d\n",
 			result);
 
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &thermal_crash_nb);
 	return 0;
 
 unregister_governors:
-- 
2.34.1

Re: [PATCH 0/3] thermal: hook in with reboot and crash

[Daniel Lezcano]

Hi,

On 25/05/2023 23:16, Eduardo Valentin wrote:
> From: Eduardo Valentin <eduval@amazon.com>
> 
> Hello,
> 
> This small series of changes teaches thermal core about
> reboot and crash callbacks. The intention is to have the core
> to get notified and the pass in the event to thermal governors
> that are willing to perform actions during reboot or crash events.
> The thermal workers will be teared down in the process too.

What problem does it solve?


> There is no code dependency this series was built on top of:
> https://lkml.org/lkml/2023/5/18/1207
> 
> Separate governor changes will be sent in another series.
> 
> BR,
> 
> Eduardo Valentin (3):
>    thermal: core: introduce governor .reboot_prepare()
>    thermal: core: register reboot nb
>    thermal: core: register a crash callback
> 
>   drivers/thermal/thermal_core.c | 54 ++++++++++++++++++++++++++++++++++
>   include/linux/thermal.h        |  4 +++
>   2 files changed, 58 insertions(+)
> 

-- 
<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: [EXTERNAL][PATCH 0/3] thermal: hook in with reboot and crash

[Eduardo Valentin]

On Fri, May 26, 2023 at 10:27:39AM +0200, Daniel Lezcano wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Hi,
> 
> On 25/05/2023 23:16, Eduardo Valentin wrote:
> > From: Eduardo Valentin <eduval@amazon.com>
> > 
> > Hello,
> > 
> > This small series of changes teaches thermal core about
> > reboot and crash callbacks. The intention is to have the core
> > to get notified and the pass in the event to thermal governors
> > that are willing to perform actions during reboot or crash events.
> > The thermal workers will be teared down in the process too.
> 
> What problem does it solve?

This cover letter was not clear enough. In fact, the context for
all patches I am sending now and will be sending in near future
is when the thermal subsystem is configured to control temperature
of a target device. The thermal subsystem is configured to have
cooling devices that will act on the target system, and has
input, temperature sensors, to have visibility to the target system
temperature.  In this case, the problem is when the controlling system
becomes unresponsive upon reboot or crash, therefore losing
control of temperature of the target system. This series solves the
problem by giving knowledge to the governors of such events, allowing
the governors to have opportunity to act before the actual event happens. 

> 
> 
> > There is no code dependency this series was built on top of:
> > https://lkml.org/lkml/2023/5/18/1207
> > 
> > Separate governor changes will be sent in another series.
> > 
> > BR,
> > 
> > Eduardo Valentin (3):
> >    thermal: core: introduce governor .reboot_prepare()
> >    thermal: core: register reboot nb
> >    thermal: core: register a crash callback
> > 
> >   drivers/thermal/thermal_core.c | 54 ++++++++++++++++++++++++++++++++++
> >   include/linux/thermal.h        |  4 +++
> >   2 files changed, 58 insertions(+)
> > 
> 
> --
> <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
> 

-- 
All the best,
Eduardo Valentin

Re: [PATCH 0/3] thermal: hook in with reboot and crash

[Eduardo Valentin]

On Mon, Jun 05, 2023 at 04:27:55PM -0700, Eduardo Valentin wrote:
> 
> 
> 
> On Fri, May 26, 2023 at 10:27:39AM +0200, Daniel Lezcano wrote:
> >
> >
> >
> > Hi,
> >
> > On 25/05/2023 23:16, Eduardo Valentin wrote:
> > > From: Eduardo Valentin <eduval@amazon.com>
> > >
> > > Hello,
> > >
> > > This small series of changes teaches thermal core about
> > > reboot and crash callbacks. The intention is to have the core
> > > to get notified and the pass in the event to thermal governors
> > > that are willing to perform actions during reboot or crash events.
> > > The thermal workers will be teared down in the process too.
> >
> > What problem does it solve?
> 
> This cover letter was not clear enough. In fact, the context for
> all patches I am sending now and will be sending in near future
> is when the thermal subsystem is configured to control temperature
> of a target device. The thermal subsystem is configured to have
> cooling devices that will act on the target system, and has
> input, temperature sensors, to have visibility to the target system
> temperature.  In this case, the problem is when the controlling system
> becomes unresponsive upon reboot or crash, therefore losing
> control of temperature of the target system. This series solves the
> problem by giving knowledge to the governors of such events, allowing
> the governors to have opportunity to act before the actual event happens.


Again, this is a different situation than a emergence shutdown due to 
temperature/overheat on the typical application of the thermal subsystem.
Where it runs in the same system it controls the temperature of.

Here we want to reduce the likelihood of loosing control of temperature of a target systems
upon events where the controlling system is unavailable.



-- 
All the best,
Eduardo Valentin

Re: [PATCH 0/3] thermal: hook in with reboot and crash

[Rafael J. Wysocki]

On Tue, Jun 6, 2023 at 1:32 AM Eduardo Valentin <evalenti@kernel.org> wrote:
>
> On Mon, Jun 05, 2023 at 04:27:55PM -0700, Eduardo Valentin wrote:
> >
> >
> >
> > On Fri, May 26, 2023 at 10:27:39AM +0200, Daniel Lezcano wrote:
> > >
> > >
> > >
> > > Hi,
> > >
> > > On 25/05/2023 23:16, Eduardo Valentin wrote:
> > > > From: Eduardo Valentin <eduval@amazon.com>
> > > >
> > > > Hello,
> > > >
> > > > This small series of changes teaches thermal core about
> > > > reboot and crash callbacks. The intention is to have the core
> > > > to get notified and the pass in the event to thermal governors
> > > > that are willing to perform actions during reboot or crash events.
> > > > The thermal workers will be teared down in the process too.
> > >
> > > What problem does it solve?
> >
> > This cover letter was not clear enough. In fact, the context for
> > all patches I am sending now and will be sending in near future
> > is when the thermal subsystem is configured to control temperature
> > of a target device. The thermal subsystem is configured to have
> > cooling devices that will act on the target system, and has
> > input, temperature sensors, to have visibility to the target system
> > temperature.  In this case, the problem is when the controlling system
> > becomes unresponsive upon reboot or crash, therefore losing
> > control of temperature of the target system. This series solves the
> > problem by giving knowledge to the governors of such events, allowing
> > the governors to have opportunity to act before the actual event happens.
>
>
> Again, this is a different situation than a emergence shutdown due to
> temperature/overheat on the typical application of the thermal subsystem.
> Where it runs in the same system it controls the temperature of.
>
> Here we want to reduce the likelihood of loosing control of temperature of a target systems
> upon events where the controlling system is unavailable.

So the use case for this seems to be a BMC running Linux that is
responsible for the thermal control of a host system.

It kind of escapes me why you want a thermal governor in the BMC's
kernel to be part of this.

[PATCH 0/3] thermal: hook in with reboot and crash

[Eduardo Valentin]

Hey Rafael,

On Thu, Jun 29, 2023 at 09:07:36PM +0200, Rafael J. Wysocki wrote:
> 
> 
> 
> On Tue, Jun 6, 2023 at 1:32 AM Eduardo Valentin <evalenti@kernel.org> wrote:
> >
> > On Mon, Jun 05, 2023 at 04:27:55PM -0700, Eduardo Valentin wrote:
> > >
> > >
> > >
> > > On Fri, May 26, 2023 at 10:27:39AM +0200, Daniel Lezcano wrote:
> > > >
> > > >
> > > >
> > > > Hi,
> > > >
> > > > On 25/05/2023 23:16, Eduardo Valentin wrote:
> > > > > From: Eduardo Valentin <eduval@amazon.com>
> > > > >
> > > > > Hello,
> > > > >
> > > > > This small series of changes teaches thermal core about
> > > > > reboot and crash callbacks. The intention is to have the core
> > > > > to get notified and the pass in the event to thermal governors
> > > > > that are willing to perform actions during reboot or crash events.
> > > > > The thermal workers will be teared down in the process too.
> > > >
> > > > What problem does it solve?
> > >
> > > This cover letter was not clear enough. In fact, the context for
> > > all patches I am sending now and will be sending in near future
> > > is when the thermal subsystem is configured to control temperature
> > > of a target device. The thermal subsystem is configured to have
> > > cooling devices that will act on the target system, and has
> > > input, temperature sensors, to have visibility to the target system
> > > temperature.  In this case, the problem is when the controlling system
> > > becomes unresponsive upon reboot or crash, therefore losing
> > > control of temperature of the target system. This series solves the
> > > problem by giving knowledge to the governors of such events, allowing
> > > the governors to have opportunity to act before the actual event happens.
> >
> >
> > Again, this is a different situation than a emergence shutdown due to
> > temperature/overheat on the typical application of the thermal subsystem.
> > Where it runs in the same system it controls the temperature of.
> >
> > Here we want to reduce the likelihood of loosing control of temperature of a target systems
> > upon events where the controlling system is unavailable.
> 
> So the use case for this seems to be a BMC running Linux that is
> responsible for the thermal control of a host system.

BMC is a great example here, yes. But not limited to it.

BMC is responsible of thermal control of not only host though,
but the entire server thermal, considering not only
host temperature sensor as inputs, but plenty of other sensing
in the motherboard, for instance.

The changes are not limited to BMC controlling a host temperature though.
Any system controlling a target device temperature will fit.

> 
> It kind of escapes me why you want a thermal governor in the BMC's
> kernel to be part of this.

Yeah, I understand the typical BMC design would have its control
in userland. But as I said this not necessarily only about BMCs
controlling host temperature.

Primary reason to have in kernel is reliability compared to a
userspace based control, avoiding downtime of the control,
having the control available and stable during the entire
life time of the controlling system, e.g from
early boot, and avoiding unnecessary down time of the control
due to application crashes. Sure the kernel can crash too :-),
that is why this patch was written in the first place, but
it is a more rare case.

Userspace may still be involved here where user wants to have,
for example a fixed override cooling state. But in that case
the user would need to disable the in kernel control and
take care of the cooling state and corner scenarios, like
crash and reboot.

I also do not see anything that would prevent a control
of the controlling system, like a BMC, be put in the kernel,
given all the abstractions needed, cooling devices, governor,
sensors, are available there. Mechanically and physically
the thermal zone is still something part of the hardware
where the controlling system, say a BMC, is responsible for.


-- 
All the best,
Eduardo Valentin