All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-13 12:33 ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-13 12:33 UTC (permalink / raw)
  To: linux-kernel
  Cc: edubezval, grygorii.strashko, j-keerthy, nm, linux-pm,
	linux-omap, joel, akpm, linux-arm-kernel, mingo, peterz, dyoung,
	josh, mpe

orderly_poweroff is triggered when a graceful shutdown
of system is desired. This may be used in many critical states of the
kernel such as when subsystems detects conditions such as critical
temperature conditions. However, in certain conditions in system
boot up sequences like those in the middle of driver probes being
initiated, userspace will be unable to power off the system in a clean
manner and leaves the system in a critical state. In cases like these,
the /sbin/poweroff will return success (having forked off to attempt
powering off the system. However, the system overall will fail to
completely poweroff (since other modules will be probed) and the system
is still functional with no userspace (since that would have shut itself
off).

However, there is no clean way of detecting such failure of userspace
powering off the system. In such scenarios, it is necessary for a backup
workqueue to be able to force a shutdown of the system when orderly
shutdown is not successful after a configurable time period.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Suggested-by: Eduardo Valentin <edubezval@gmail.com> 
Reported-by: Nishanth Menon <nm@ti.com>
---
Links to previous discussion can be found here:

http://www.spinics.net/lists/linux-omap/msg124925.html

Boot tested on DRA7.

changes in v2:

	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS

 arch/Kconfig    |  7 +++++++
 kernel/reboot.c | 23 ++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

Index: linux/arch/Kconfig
===================================================================
--- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
+++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
@@ -37,6 +37,18 @@
 	def_bool y
 	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
 
+config SHUTDOWN_BACKUP_DELAY_MS
+	int "Backup shutdown delay in milli-seconds"
+	default 0
+	help
+	  The number of milliseconds to delay before backup workqueue
+	  executes attempting to poweroff the system after the
+	  orderly_poweroff function has failed to complete.
+
+	  If set to 0, the backup workqueue is not active. The value
+	  should be conservatively configured based on userspace latencies
+	  expected for a given system.
+
 config KPROBES
 	bool "Kprobes"
 	depends on MODULES
Index: linux/kernel/reboot.c
===================================================================
--- linux.orig/kernel/reboot.c	2016-01-11 15:26:07.732173131 +0530
+++ linux/kernel/reboot.c	2016-01-11 15:38:33.502341511 +0530
@@ -424,6 +424,38 @@
 	return ret;
 }
 
+#if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
+
+/**
+ * shutdown_backup_func - shutdown backup work after a known delay
+ * @work: work_struct associated with the backup shutdown function
+ *
+ * In system bootup sequences like those in the middle of driver probes being
+ * initiated, userspace will be unable to power off the system in a clean
+ * manner and leaves the system in a critical state.
+ * In such scenarios, this backup workqueue forces a shutdown of the system
+ * when orderly shutdown is not successful after a configurable time period.
+ */
+static void shutdown_backup_func(struct work_struct *work)
+{
+	pr_warn("Orderly_poweroff has failed! Attempting kernel_power_off\n");
+	kernel_power_off();
+
+	pr_warn("kernel_power_off has failed! Attempting emergency_restart\n");
+	emergency_restart();
+}
+
+static DECLARE_DELAYED_WORK(bkup_shutdown_work, shutdown_backup_func);
+
+static inline void setup_backup_shutdown_workqueue(void)
+{
+	schedule_delayed_work(&bkup_shutdown_work,
+		      msecs_to_jiffies(CONFIG_SHUTDOWN_BACKUP_DELAY_MS));
+}
+#else
+static inline void setup_backup_shutdown_workqueue(void) { }
+#endif
+
 static int __orderly_poweroff(bool force)
 {
 	int ret;
@@ -442,6 +474,12 @@
 		kernel_power_off();
 	}
 
+	/*
+	 * Schedule a backup work function to execute after a known time
+	 * when orderly shutdown fails.
+	 */
+	setup_backup_shutdown_workqueue();
+
 	return ret;
 }
 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-13 12:33 ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-13 12:33 UTC (permalink / raw)
  To: linux-kernel
  Cc: edubezval, grygorii.strashko, j-keerthy, nm, linux-pm,
	linux-omap, joel, akpm, linux-arm-kernel, mingo, peterz, dyoung,
	josh, mpe

orderly_poweroff is triggered when a graceful shutdown
of system is desired. This may be used in many critical states of the
kernel such as when subsystems detects conditions such as critical
temperature conditions. However, in certain conditions in system
boot up sequences like those in the middle of driver probes being
initiated, userspace will be unable to power off the system in a clean
manner and leaves the system in a critical state. In cases like these,
the /sbin/poweroff will return success (having forked off to attempt
powering off the system. However, the system overall will fail to
completely poweroff (since other modules will be probed) and the system
is still functional with no userspace (since that would have shut itself
off).

However, there is no clean way of detecting such failure of userspace
powering off the system. In such scenarios, it is necessary for a backup
workqueue to be able to force a shutdown of the system when orderly
shutdown is not successful after a configurable time period.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Suggested-by: Eduardo Valentin <edubezval@gmail.com> 
Reported-by: Nishanth Menon <nm@ti.com>
---
Links to previous discussion can be found here:

http://www.spinics.net/lists/linux-omap/msg124925.html

Boot tested on DRA7.

changes in v2:

	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS

 arch/Kconfig    |  7 +++++++
 kernel/reboot.c | 23 ++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

Index: linux/arch/Kconfig
===================================================================
--- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
+++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
@@ -37,6 +37,18 @@
 	def_bool y
 	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
 
+config SHUTDOWN_BACKUP_DELAY_MS
+	int "Backup shutdown delay in milli-seconds"
+	default 0
+	help
+	  The number of milliseconds to delay before backup workqueue
+	  executes attempting to poweroff the system after the
+	  orderly_poweroff function has failed to complete.
+
+	  If set to 0, the backup workqueue is not active. The value
+	  should be conservatively configured based on userspace latencies
+	  expected for a given system.
+
 config KPROBES
 	bool "Kprobes"
 	depends on MODULES
Index: linux/kernel/reboot.c
===================================================================
--- linux.orig/kernel/reboot.c	2016-01-11 15:26:07.732173131 +0530
+++ linux/kernel/reboot.c	2016-01-11 15:38:33.502341511 +0530
@@ -424,6 +424,38 @@
 	return ret;
 }
 
+#if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
+
+/**
+ * shutdown_backup_func - shutdown backup work after a known delay
+ * @work: work_struct associated with the backup shutdown function
+ *
+ * In system bootup sequences like those in the middle of driver probes being
+ * initiated, userspace will be unable to power off the system in a clean
+ * manner and leaves the system in a critical state.
+ * In such scenarios, this backup workqueue forces a shutdown of the system
+ * when orderly shutdown is not successful after a configurable time period.
+ */
+static void shutdown_backup_func(struct work_struct *work)
+{
+	pr_warn("Orderly_poweroff has failed! Attempting kernel_power_off\n");
+	kernel_power_off();
+
+	pr_warn("kernel_power_off has failed! Attempting emergency_restart\n");
+	emergency_restart();
+}
+
+static DECLARE_DELAYED_WORK(bkup_shutdown_work, shutdown_backup_func);
+
+static inline void setup_backup_shutdown_workqueue(void)
+{
+	schedule_delayed_work(&bkup_shutdown_work,
+		      msecs_to_jiffies(CONFIG_SHUTDOWN_BACKUP_DELAY_MS));
+}
+#else
+static inline void setup_backup_shutdown_workqueue(void) { }
+#endif
+
 static int __orderly_poweroff(bool force)
 {
 	int ret;
@@ -442,6 +474,12 @@
 		kernel_power_off();
 	}
 
+	/*
+	 * Schedule a backup work function to execute after a known time
+	 * when orderly shutdown fails.
+	 */
+	setup_backup_shutdown_workqueue();
+
 	return ret;
 }
 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-13 12:33 ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-13 12:33 UTC (permalink / raw)
  To: linux-arm-kernel

orderly_poweroff is triggered when a graceful shutdown
of system is desired. This may be used in many critical states of the
kernel such as when subsystems detects conditions such as critical
temperature conditions. However, in certain conditions in system
boot up sequences like those in the middle of driver probes being
initiated, userspace will be unable to power off the system in a clean
manner and leaves the system in a critical state. In cases like these,
the /sbin/poweroff will return success (having forked off to attempt
powering off the system. However, the system overall will fail to
completely poweroff (since other modules will be probed) and the system
is still functional with no userspace (since that would have shut itself
off).

However, there is no clean way of detecting such failure of userspace
powering off the system. In such scenarios, it is necessary for a backup
workqueue to be able to force a shutdown of the system when orderly
shutdown is not successful after a configurable time period.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Suggested-by: Eduardo Valentin <edubezval@gmail.com> 
Reported-by: Nishanth Menon <nm@ti.com>
---
Links to previous discussion can be found here:

http://www.spinics.net/lists/linux-omap/msg124925.html

Boot tested on DRA7.

changes in v2:

	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS

 arch/Kconfig    |  7 +++++++
 kernel/reboot.c | 23 ++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

Index: linux/arch/Kconfig
===================================================================
--- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
+++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
@@ -37,6 +37,18 @@
 	def_bool y
 	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
 
+config SHUTDOWN_BACKUP_DELAY_MS
+	int "Backup shutdown delay in milli-seconds"
+	default 0
+	help
+	  The number of milliseconds to delay before backup workqueue
+	  executes attempting to poweroff the system after the
+	  orderly_poweroff function has failed to complete.
+
+	  If set to 0, the backup workqueue is not active. The value
+	  should be conservatively configured based on userspace latencies
+	  expected for a given system.
+
 config KPROBES
 	bool "Kprobes"
 	depends on MODULES
Index: linux/kernel/reboot.c
===================================================================
--- linux.orig/kernel/reboot.c	2016-01-11 15:26:07.732173131 +0530
+++ linux/kernel/reboot.c	2016-01-11 15:38:33.502341511 +0530
@@ -424,6 +424,38 @@
 	return ret;
 }
 
+#if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
+
+/**
+ * shutdown_backup_func - shutdown backup work after a known delay
+ * @work: work_struct associated with the backup shutdown function
+ *
+ * In system bootup sequences like those in the middle of driver probes being
+ * initiated, userspace will be unable to power off the system in a clean
+ * manner and leaves the system in a critical state.
+ * In such scenarios, this backup workqueue forces a shutdown of the system
+ * when orderly shutdown is not successful after a configurable time period.
+ */
+static void shutdown_backup_func(struct work_struct *work)
+{
+	pr_warn("Orderly_poweroff has failed! Attempting kernel_power_off\n");
+	kernel_power_off();
+
+	pr_warn("kernel_power_off has failed! Attempting emergency_restart\n");
+	emergency_restart();
+}
+
+static DECLARE_DELAYED_WORK(bkup_shutdown_work, shutdown_backup_func);
+
+static inline void setup_backup_shutdown_workqueue(void)
+{
+	schedule_delayed_work(&bkup_shutdown_work,
+		      msecs_to_jiffies(CONFIG_SHUTDOWN_BACKUP_DELAY_MS));
+}
+#else
+static inline void setup_backup_shutdown_workqueue(void) { }
+#endif
+
 static int __orderly_poweroff(bool force)
 {
 	int ret;
@@ -442,6 +474,12 @@
 		kernel_power_off();
 	}
 
+	/*
+	 * Schedule a backup work function to execute after a known time
+	 * when orderly shutdown fails.
+	 */
+	setup_backup_shutdown_workqueue();
+
 	return ret;
 }
 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-13 12:33 ` Keerthy
@ 2016-01-14  9:05   ` Ingo Molnar
  -1 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-14  9:05 UTC (permalink / raw)
  To: Keerthy
  Cc: linux-kernel, edubezval, grygorii.strashko, nm, linux-pm,
	linux-omap, joel, akpm, linux-arm-kernel, peterz, dyoung, josh,
	mpe, Thomas Gleixner, Peter Zijlstra


* Keerthy <j-keerthy@ti.com> wrote:

> orderly_poweroff is triggered when a graceful shutdown
> of system is desired. This may be used in many critical states of the
> kernel such as when subsystems detects conditions such as critical
> temperature conditions. However, in certain conditions in system
> boot up sequences like those in the middle of driver probes being
> initiated, userspace will be unable to power off the system in a clean
> manner and leaves the system in a critical state. In cases like these,
> the /sbin/poweroff will return success (having forked off to attempt
> powering off the system. However, the system overall will fail to
> completely poweroff (since other modules will be probed) and the system
> is still functional with no userspace (since that would have shut itself
> off).
> 
> However, there is no clean way of detecting such failure of userspace
> powering off the system. In such scenarios, it is necessary for a backup
> workqueue to be able to force a shutdown of the system when orderly
> shutdown is not successful after a configurable time period.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> Suggested-by: Eduardo Valentin <edubezval@gmail.com> 
> Reported-by: Nishanth Menon <nm@ti.com>
> ---
> Links to previous discussion can be found here:
> 
> http://www.spinics.net/lists/linux-omap/msg124925.html
> 
> Boot tested on DRA7.
> 
> changes in v2:
> 
> 	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
> 
>  arch/Kconfig    |  7 +++++++
>  kernel/reboot.c | 23 ++++++++++++++++++-----
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> Index: linux/arch/Kconfig
> ===================================================================
> --- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
> +++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
> @@ -37,6 +37,18 @@
>  	def_bool y
>  	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
>  
> +config SHUTDOWN_BACKUP_DELAY_MS
> +	int "Backup shutdown delay in milli-seconds"
> +	default 0
> +	help
> +	  The number of milliseconds to delay before backup workqueue
> +	  executes attempting to poweroff the system after the
> +	  orderly_poweroff function has failed to complete.
> +
> +	  If set to 0, the backup workqueue is not active. The value
> +	  should be conservatively configured based on userspace latencies
> +	  expected for a given system.

I don't really understand this. In what circumstances can a reboot fail?

I think that is what should be fixed: a reboot should never fail, instead of 
introducing some sort of fragile timeout based method.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14  9:05   ` Ingo Molnar
  0 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-14  9:05 UTC (permalink / raw)
  To: linux-arm-kernel


* Keerthy <j-keerthy@ti.com> wrote:

> orderly_poweroff is triggered when a graceful shutdown
> of system is desired. This may be used in many critical states of the
> kernel such as when subsystems detects conditions such as critical
> temperature conditions. However, in certain conditions in system
> boot up sequences like those in the middle of driver probes being
> initiated, userspace will be unable to power off the system in a clean
> manner and leaves the system in a critical state. In cases like these,
> the /sbin/poweroff will return success (having forked off to attempt
> powering off the system. However, the system overall will fail to
> completely poweroff (since other modules will be probed) and the system
> is still functional with no userspace (since that would have shut itself
> off).
> 
> However, there is no clean way of detecting such failure of userspace
> powering off the system. In such scenarios, it is necessary for a backup
> workqueue to be able to force a shutdown of the system when orderly
> shutdown is not successful after a configurable time period.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> Suggested-by: Eduardo Valentin <edubezval@gmail.com> 
> Reported-by: Nishanth Menon <nm@ti.com>
> ---
> Links to previous discussion can be found here:
> 
> http://www.spinics.net/lists/linux-omap/msg124925.html
> 
> Boot tested on DRA7.
> 
> changes in v2:
> 
> 	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
> 
>  arch/Kconfig    |  7 +++++++
>  kernel/reboot.c | 23 ++++++++++++++++++-----
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> Index: linux/arch/Kconfig
> ===================================================================
> --- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
> +++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
> @@ -37,6 +37,18 @@
>  	def_bool y
>  	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
>  
> +config SHUTDOWN_BACKUP_DELAY_MS
> +	int "Backup shutdown delay in milli-seconds"
> +	default 0
> +	help
> +	  The number of milliseconds to delay before backup workqueue
> +	  executes attempting to poweroff the system after the
> +	  orderly_poweroff function has failed to complete.
> +
> +	  If set to 0, the backup workqueue is not active. The value
> +	  should be conservatively configured based on userspace latencies
> +	  expected for a given system.

I don't really understand this. In what circumstances can a reboot fail?

I think that is what should be fixed: a reboot should never fail, instead of 
introducing some sort of fragile timeout based method.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-14  9:05   ` Ingo Molnar
  (?)
@ 2016-01-14  9:18     ` Keerthy
  -1 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-14  9:18 UTC (permalink / raw)
  To: Ingo Molnar, Keerthy
  Cc: linux-kernel, edubezval, grygorii.strashko, nm, linux-pm,
	linux-omap, joel, akpm, linux-arm-kernel, peterz, dyoung, josh,
	mpe, Thomas Gleixner, Peter Zijlstra

Hi Ingo,

On Thursday 14 January 2016 02:35 PM, Ingo Molnar wrote:
>
> * Keerthy <j-keerthy@ti.com> wrote:
>
>> orderly_poweroff is triggered when a graceful shutdown
>> of system is desired. This may be used in many critical states of the
>> kernel such as when subsystems detects conditions such as critical
>> temperature conditions. However, in certain conditions in system
>> boot up sequences like those in the middle of driver probes being
>> initiated, userspace will be unable to power off the system in a clean
>> manner and leaves the system in a critical state. In cases like these,
>> the /sbin/poweroff will return success (having forked off to attempt
>> powering off the system. However, the system overall will fail to
>> completely poweroff (since other modules will be probed) and the system
>> is still functional with no userspace (since that would have shut itself
>> off).
>>
>> However, there is no clean way of detecting such failure of userspace
>> powering off the system. In such scenarios, it is necessary for a backup
>> workqueue to be able to force a shutdown of the system when orderly
>> shutdown is not successful after a configurable time period.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> Suggested-by: Eduardo Valentin <edubezval@gmail.com>
>> Reported-by: Nishanth Menon <nm@ti.com>
>> ---
>> Links to previous discussion can be found here:
>>
>> http://www.spinics.net/lists/linux-omap/msg124925.html
>>
>> Boot tested on DRA7.
>>
>> changes in v2:
>>
>> 	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
>>
>>   arch/Kconfig    |  7 +++++++
>>   kernel/reboot.c | 23 ++++++++++++++++++-----
>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> Index: linux/arch/Kconfig
>> ===================================================================
>> --- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
>> +++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
>> @@ -37,6 +37,18 @@
>>   	def_bool y
>>   	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
>>
>> +config SHUTDOWN_BACKUP_DELAY_MS
>> +	int "Backup shutdown delay in milli-seconds"
>> +	default 0
>> +	help
>> +	  The number of milliseconds to delay before backup workqueue
>> +	  executes attempting to poweroff the system after the
>> +	  orderly_poweroff function has failed to complete.
>> +
>> +	  If set to 0, the backup workqueue is not active. The value
>> +	  should be conservatively configured based on userspace latencies
>> +	  expected for a given system.
>
> I don't really understand this. In what circumstances can a reboot fail?
>
> I think that is what should be fixed: a reboot should never fail, instead of
> introducing some sort of fragile timeout based method.

Here is the complete description of the scenario which was reported by 
Nishanth who encountered the issue. The link has bootlogs and 
description of the exact case which led to this patch.

http://www.spinics.net/lists/linux-omap/msg124923.html

Regards,
Keerthy
>
> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14  9:18     ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-14  9:18 UTC (permalink / raw)
  To: Ingo Molnar, Keerthy
  Cc: linux-kernel, edubezval, grygorii.strashko, nm, linux-pm,
	linux-omap, joel, akpm, linux-arm-kernel, peterz, dyoung, josh,
	mpe, Thomas Gleixner, Peter Zijlstra

Hi Ingo,

On Thursday 14 January 2016 02:35 PM, Ingo Molnar wrote:
>
> * Keerthy <j-keerthy@ti.com> wrote:
>
>> orderly_poweroff is triggered when a graceful shutdown
>> of system is desired. This may be used in many critical states of the
>> kernel such as when subsystems detects conditions such as critical
>> temperature conditions. However, in certain conditions in system
>> boot up sequences like those in the middle of driver probes being
>> initiated, userspace will be unable to power off the system in a clean
>> manner and leaves the system in a critical state. In cases like these,
>> the /sbin/poweroff will return success (having forked off to attempt
>> powering off the system. However, the system overall will fail to
>> completely poweroff (since other modules will be probed) and the system
>> is still functional with no userspace (since that would have shut itself
>> off).
>>
>> However, there is no clean way of detecting such failure of userspace
>> powering off the system. In such scenarios, it is necessary for a backup
>> workqueue to be able to force a shutdown of the system when orderly
>> shutdown is not successful after a configurable time period.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> Suggested-by: Eduardo Valentin <edubezval@gmail.com>
>> Reported-by: Nishanth Menon <nm@ti.com>
>> ---
>> Links to previous discussion can be found here:
>>
>> http://www.spinics.net/lists/linux-omap/msg124925.html
>>
>> Boot tested on DRA7.
>>
>> changes in v2:
>>
>> 	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
>>
>>   arch/Kconfig    |  7 +++++++
>>   kernel/reboot.c | 23 ++++++++++++++++++-----
>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> Index: linux/arch/Kconfig
>> ===================================================================
>> --- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
>> +++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
>> @@ -37,6 +37,18 @@
>>   	def_bool y
>>   	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
>>
>> +config SHUTDOWN_BACKUP_DELAY_MS
>> +	int "Backup shutdown delay in milli-seconds"
>> +	default 0
>> +	help
>> +	  The number of milliseconds to delay before backup workqueue
>> +	  executes attempting to poweroff the system after the
>> +	  orderly_poweroff function has failed to complete.
>> +
>> +	  If set to 0, the backup workqueue is not active. The value
>> +	  should be conservatively configured based on userspace latencies
>> +	  expected for a given system.
>
> I don't really understand this. In what circumstances can a reboot fail?
>
> I think that is what should be fixed: a reboot should never fail, instead of
> introducing some sort of fragile timeout based method.

Here is the complete description of the scenario which was reported by 
Nishanth who encountered the issue. The link has bootlogs and 
description of the exact case which led to this patch.

http://www.spinics.net/lists/linux-omap/msg124923.html

Regards,
Keerthy
>
> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14  9:18     ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-14  9:18 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Ingo,

On Thursday 14 January 2016 02:35 PM, Ingo Molnar wrote:
>
> * Keerthy <j-keerthy@ti.com> wrote:
>
>> orderly_poweroff is triggered when a graceful shutdown
>> of system is desired. This may be used in many critical states of the
>> kernel such as when subsystems detects conditions such as critical
>> temperature conditions. However, in certain conditions in system
>> boot up sequences like those in the middle of driver probes being
>> initiated, userspace will be unable to power off the system in a clean
>> manner and leaves the system in a critical state. In cases like these,
>> the /sbin/poweroff will return success (having forked off to attempt
>> powering off the system. However, the system overall will fail to
>> completely poweroff (since other modules will be probed) and the system
>> is still functional with no userspace (since that would have shut itself
>> off).
>>
>> However, there is no clean way of detecting such failure of userspace
>> powering off the system. In such scenarios, it is necessary for a backup
>> workqueue to be able to force a shutdown of the system when orderly
>> shutdown is not successful after a configurable time period.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> Suggested-by: Eduardo Valentin <edubezval@gmail.com>
>> Reported-by: Nishanth Menon <nm@ti.com>
>> ---
>> Links to previous discussion can be found here:
>>
>> http://www.spinics.net/lists/linux-omap/msg124925.html
>>
>> Boot tested on DRA7.
>>
>> changes in v2:
>>
>> 	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
>>
>>   arch/Kconfig    |  7 +++++++
>>   kernel/reboot.c | 23 ++++++++++++++++++-----
>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> Index: linux/arch/Kconfig
>> ===================================================================
>> --- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
>> +++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
>> @@ -37,6 +37,18 @@
>>   	def_bool y
>>   	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
>>
>> +config SHUTDOWN_BACKUP_DELAY_MS
>> +	int "Backup shutdown delay in milli-seconds"
>> +	default 0
>> +	help
>> +	  The number of milliseconds to delay before backup workqueue
>> +	  executes attempting to poweroff the system after the
>> +	  orderly_poweroff function has failed to complete.
>> +
>> +	  If set to 0, the backup workqueue is not active. The value
>> +	  should be conservatively configured based on userspace latencies
>> +	  expected for a given system.
>
> I don't really understand this. In what circumstances can a reboot fail?
>
> I think that is what should be fixed: a reboot should never fail, instead of
> introducing some sort of fragile timeout based method.

Here is the complete description of the scenario which was reported by 
Nishanth who encountered the issue. The link has bootlogs and 
description of the exact case which led to this patch.

http://www.spinics.net/lists/linux-omap/msg124923.html

Regards,
Keerthy
>
> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-14  9:18     ` Keerthy
@ 2016-01-14 10:09       ` Ingo Molnar
  -1 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-14 10:09 UTC (permalink / raw)
  To: Keerthy
  Cc: Keerthy, linux-kernel, edubezval, grygorii.strashko, nm,
	linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra


* Keerthy <a0393675@ti.com> wrote:

> Hi Ingo,
> 
> On Thursday 14 January 2016 02:35 PM, Ingo Molnar wrote:
> >
> >* Keerthy <j-keerthy@ti.com> wrote:
> >
> >>orderly_poweroff is triggered when a graceful shutdown
> >>of system is desired. This may be used in many critical states of the
> >>kernel such as when subsystems detects conditions such as critical
> >>temperature conditions. However, in certain conditions in system
> >>boot up sequences like those in the middle of driver probes being
> >>initiated, userspace will be unable to power off the system in a clean
> >>manner and leaves the system in a critical state. In cases like these,
> >>the /sbin/poweroff will return success (having forked off to attempt
> >>powering off the system. However, the system overall will fail to
> >>completely poweroff (since other modules will be probed) and the system
> >>is still functional with no userspace (since that would have shut itself
> >>off).
> >>
> >>However, there is no clean way of detecting such failure of userspace
> >>powering off the system. In such scenarios, it is necessary for a backup
> >>workqueue to be able to force a shutdown of the system when orderly
> >>shutdown is not successful after a configurable time period.
> >>
> >>Signed-off-by: Keerthy <j-keerthy@ti.com>
> >>Suggested-by: Eduardo Valentin <edubezval@gmail.com>
> >>Reported-by: Nishanth Menon <nm@ti.com>
> >>---
> >>Links to previous discussion can be found here:
> >>
> >>http://www.spinics.net/lists/linux-omap/msg124925.html
> >>
> >>Boot tested on DRA7.
> >>
> >>changes in v2:
> >>
> >>	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
> >>
> >>  arch/Kconfig    |  7 +++++++
> >>  kernel/reboot.c | 23 ++++++++++++++++++-----
> >>  2 files changed, 25 insertions(+), 5 deletions(-)
> >>
> >>Index: linux/arch/Kconfig
> >>===================================================================
> >>--- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
> >>+++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
> >>@@ -37,6 +37,18 @@
> >>  	def_bool y
> >>  	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
> >>
> >>+config SHUTDOWN_BACKUP_DELAY_MS
> >>+	int "Backup shutdown delay in milli-seconds"
> >>+	default 0
> >>+	help
> >>+	  The number of milliseconds to delay before backup workqueue
> >>+	  executes attempting to poweroff the system after the
> >>+	  orderly_poweroff function has failed to complete.
> >>+
> >>+	  If set to 0, the backup workqueue is not active. The value
> >>+	  should be conservatively configured based on userspace latencies
> >>+	  expected for a given system.
> >
> >I don't really understand this. In what circumstances can a reboot fail?
> >
> >I think that is what should be fixed: a reboot should never fail, instead of
> >introducing some sort of fragile timeout based method.
> 
> Here is the complete description of the scenario which was reported by Nishanth 
> who encountered the issue. The link has bootlogs and description of the exact 
> case which led to this patch.
> 
> http://www.spinics.net/lists/linux-omap/msg124923.html

it's a reply in the middle of a discussion ...

What I managed to decode is that this:

static int __orderly_poweroff(bool force)
{
        int ret;

        ret = run_cmd(poweroff_cmd);

        if (ret && force) {
                pr_warn("Failed to start orderly shutdown: forcing the issue\n");

                /*
                 * I guess this should try to kick off some daemon to sync and
                 * poweroff asap.  Or not even bother syncing if we're doing an
                 * emergency shutdown?
                 */
                emergency_sync();
                kernel_power_off();
        }

        return ret;
}

could fail to actually power the system off, if the run_cmd(poweroff_cmd) 
'succeeds', but due to a user-space bug it does not actually call the real 
poweroff system call?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14 10:09       ` Ingo Molnar
  0 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-14 10:09 UTC (permalink / raw)
  To: linux-arm-kernel


* Keerthy <a0393675@ti.com> wrote:

> Hi Ingo,
> 
> On Thursday 14 January 2016 02:35 PM, Ingo Molnar wrote:
> >
> >* Keerthy <j-keerthy@ti.com> wrote:
> >
> >>orderly_poweroff is triggered when a graceful shutdown
> >>of system is desired. This may be used in many critical states of the
> >>kernel such as when subsystems detects conditions such as critical
> >>temperature conditions. However, in certain conditions in system
> >>boot up sequences like those in the middle of driver probes being
> >>initiated, userspace will be unable to power off the system in a clean
> >>manner and leaves the system in a critical state. In cases like these,
> >>the /sbin/poweroff will return success (having forked off to attempt
> >>powering off the system. However, the system overall will fail to
> >>completely poweroff (since other modules will be probed) and the system
> >>is still functional with no userspace (since that would have shut itself
> >>off).
> >>
> >>However, there is no clean way of detecting such failure of userspace
> >>powering off the system. In such scenarios, it is necessary for a backup
> >>workqueue to be able to force a shutdown of the system when orderly
> >>shutdown is not successful after a configurable time period.
> >>
> >>Signed-off-by: Keerthy <j-keerthy@ti.com>
> >>Suggested-by: Eduardo Valentin <edubezval@gmail.com>
> >>Reported-by: Nishanth Menon <nm@ti.com>
> >>---
> >>Links to previous discussion can be found here:
> >>
> >>http://www.spinics.net/lists/linux-omap/msg124925.html
> >>
> >>Boot tested on DRA7.
> >>
> >>changes in v2:
> >>
> >>	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
> >>
> >>  arch/Kconfig    |  7 +++++++
> >>  kernel/reboot.c | 23 ++++++++++++++++++-----
> >>  2 files changed, 25 insertions(+), 5 deletions(-)
> >>
> >>Index: linux/arch/Kconfig
> >>===================================================================
> >>--- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
> >>+++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
> >>@@ -37,6 +37,18 @@
> >>  	def_bool y
> >>  	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
> >>
> >>+config SHUTDOWN_BACKUP_DELAY_MS
> >>+	int "Backup shutdown delay in milli-seconds"
> >>+	default 0
> >>+	help
> >>+	  The number of milliseconds to delay before backup workqueue
> >>+	  executes attempting to poweroff the system after the
> >>+	  orderly_poweroff function has failed to complete.
> >>+
> >>+	  If set to 0, the backup workqueue is not active. The value
> >>+	  should be conservatively configured based on userspace latencies
> >>+	  expected for a given system.
> >
> >I don't really understand this. In what circumstances can a reboot fail?
> >
> >I think that is what should be fixed: a reboot should never fail, instead of
> >introducing some sort of fragile timeout based method.
> 
> Here is the complete description of the scenario which was reported by Nishanth 
> who encountered the issue. The link has bootlogs and description of the exact 
> case which led to this patch.
> 
> http://www.spinics.net/lists/linux-omap/msg124923.html

it's a reply in the middle of a discussion ...

What I managed to decode is that this:

static int __orderly_poweroff(bool force)
{
        int ret;

        ret = run_cmd(poweroff_cmd);

        if (ret && force) {
                pr_warn("Failed to start orderly shutdown: forcing the issue\n");

                /*
                 * I guess this should try to kick off some daemon to sync and
                 * poweroff asap.  Or not even bother syncing if we're doing an
                 * emergency shutdown?
                 */
                emergency_sync();
                kernel_power_off();
        }

        return ret;
}

could fail to actually power the system off, if the run_cmd(poweroff_cmd) 
'succeeds', but due to a user-space bug it does not actually call the real 
poweroff system call?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-14 10:09       ` Ingo Molnar
  (?)
@ 2016-01-14 10:42         ` Keerthy
  -1 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-14 10:42 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Keerthy, linux-kernel, edubezval, grygorii.strashko, nm,
	linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra

Hi Ingo,

On Thursday 14 January 2016 03:39 PM, Ingo Molnar wrote:
>
> * Keerthy <a0393675@ti.com> wrote:
>
>> Hi Ingo,
>>
>> On Thursday 14 January 2016 02:35 PM, Ingo Molnar wrote:
>>>
>>> * Keerthy <j-keerthy@ti.com> wrote:
>>>
>>>> orderly_poweroff is triggered when a graceful shutdown
>>>> of system is desired. This may be used in many critical states of the
>>>> kernel such as when subsystems detects conditions such as critical
>>>> temperature conditions. However, in certain conditions in system
>>>> boot up sequences like those in the middle of driver probes being
>>>> initiated, userspace will be unable to power off the system in a clean
>>>> manner and leaves the system in a critical state. In cases like these,
>>>> the /sbin/poweroff will return success (having forked off to attempt
>>>> powering off the system. However, the system overall will fail to
>>>> completely poweroff (since other modules will be probed) and the system
>>>> is still functional with no userspace (since that would have shut itself
>>>> off).
>>>>
>>>> However, there is no clean way of detecting such failure of userspace
>>>> powering off the system. In such scenarios, it is necessary for a backup
>>>> workqueue to be able to force a shutdown of the system when orderly
>>>> shutdown is not successful after a configurable time period.
>>>>
>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>> Suggested-by: Eduardo Valentin <edubezval@gmail.com>
>>>> Reported-by: Nishanth Menon <nm@ti.com>
>>>> ---
>>>> Links to previous discussion can be found here:
>>>>
>>>> http://www.spinics.net/lists/linux-omap/msg124925.html
>>>>
>>>> Boot tested on DRA7.
>>>>
>>>> changes in v2:
>>>>
>>>> 	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
>>>>
>>>>   arch/Kconfig    |  7 +++++++
>>>>   kernel/reboot.c | 23 ++++++++++++++++++-----
>>>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>>>
>>>> Index: linux/arch/Kconfig
>>>> ===================================================================
>>>> --- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
>>>> +++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
>>>> @@ -37,6 +37,18 @@
>>>>   	def_bool y
>>>>   	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
>>>>
>>>> +config SHUTDOWN_BACKUP_DELAY_MS
>>>> +	int "Backup shutdown delay in milli-seconds"
>>>> +	default 0
>>>> +	help
>>>> +	  The number of milliseconds to delay before backup workqueue
>>>> +	  executes attempting to poweroff the system after the
>>>> +	  orderly_poweroff function has failed to complete.
>>>> +
>>>> +	  If set to 0, the backup workqueue is not active. The value
>>>> +	  should be conservatively configured based on userspace latencies
>>>> +	  expected for a given system.
>>>
>>> I don't really understand this. In what circumstances can a reboot fail?
>>>
>>> I think that is what should be fixed: a reboot should never fail, instead of
>>> introducing some sort of fragile timeout based method.
>>
>> Here is the complete description of the scenario which was reported by Nishanth
>> who encountered the issue. The link has bootlogs and description of the exact
>> case which led to this patch.
>>
>> http://www.spinics.net/lists/linux-omap/msg124923.html
>
> it's a reply in the middle of a discussion ...
>
> What I managed to decode is that this:
>
> static int __orderly_poweroff(bool force)
> {
>          int ret;
>
>          ret = run_cmd(poweroff_cmd);
>
>          if (ret && force) {
>                  pr_warn("Failed to start orderly shutdown: forcing the issue\n");
>
>                  /*
>                   * I guess this should try to kick off some daemon to sync and
>                   * poweroff asap.  Or not even bother syncing if we're doing an
>                   * emergency shutdown?
>                   */
>                  emergency_sync();
>                  kernel_power_off();
>          }
>
>          return ret;
> }
>
> could fail to actually power the system off, if the run_cmd(poweroff_cmd)
> 'succeeds', but due to a user-space bug it does not actually call the real
> poweroff system call?
>

I tried to simulate the issue.

In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
ti_bandgap_probe i call

orderly_poweroff(true);

This is while driver probes are still on going. I observe that
ret = run_cmd(poweroff_cmd);

ret is a non-zero value and we enter the if condition:

Even after the

emergency_sync();
kernel_power_off();

calls

the console remained active in weird state.


> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14 10:42         ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-14 10:42 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Keerthy, linux-kernel, edubezval, grygorii.strashko, nm,
	linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra

Hi Ingo,

On Thursday 14 January 2016 03:39 PM, Ingo Molnar wrote:
>
> * Keerthy <a0393675@ti.com> wrote:
>
>> Hi Ingo,
>>
>> On Thursday 14 January 2016 02:35 PM, Ingo Molnar wrote:
>>>
>>> * Keerthy <j-keerthy@ti.com> wrote:
>>>
>>>> orderly_poweroff is triggered when a graceful shutdown
>>>> of system is desired. This may be used in many critical states of the
>>>> kernel such as when subsystems detects conditions such as critical
>>>> temperature conditions. However, in certain conditions in system
>>>> boot up sequences like those in the middle of driver probes being
>>>> initiated, userspace will be unable to power off the system in a clean
>>>> manner and leaves the system in a critical state. In cases like these,
>>>> the /sbin/poweroff will return success (having forked off to attempt
>>>> powering off the system. However, the system overall will fail to
>>>> completely poweroff (since other modules will be probed) and the system
>>>> is still functional with no userspace (since that would have shut itself
>>>> off).
>>>>
>>>> However, there is no clean way of detecting such failure of userspace
>>>> powering off the system. In such scenarios, it is necessary for a backup
>>>> workqueue to be able to force a shutdown of the system when orderly
>>>> shutdown is not successful after a configurable time period.
>>>>
>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>> Suggested-by: Eduardo Valentin <edubezval@gmail.com>
>>>> Reported-by: Nishanth Menon <nm@ti.com>
>>>> ---
>>>> Links to previous discussion can be found here:
>>>>
>>>> http://www.spinics.net/lists/linux-omap/msg124925.html
>>>>
>>>> Boot tested on DRA7.
>>>>
>>>> changes in v2:
>>>>
>>>> 	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
>>>>
>>>>   arch/Kconfig    |  7 +++++++
>>>>   kernel/reboot.c | 23 ++++++++++++++++++-----
>>>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>>>
>>>> Index: linux/arch/Kconfig
>>>> ===================================================================
>>>> --- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
>>>> +++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
>>>> @@ -37,6 +37,18 @@
>>>>   	def_bool y
>>>>   	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
>>>>
>>>> +config SHUTDOWN_BACKUP_DELAY_MS
>>>> +	int "Backup shutdown delay in milli-seconds"
>>>> +	default 0
>>>> +	help
>>>> +	  The number of milliseconds to delay before backup workqueue
>>>> +	  executes attempting to poweroff the system after the
>>>> +	  orderly_poweroff function has failed to complete.
>>>> +
>>>> +	  If set to 0, the backup workqueue is not active. The value
>>>> +	  should be conservatively configured based on userspace latencies
>>>> +	  expected for a given system.
>>>
>>> I don't really understand this. In what circumstances can a reboot fail?
>>>
>>> I think that is what should be fixed: a reboot should never fail, instead of
>>> introducing some sort of fragile timeout based method.
>>
>> Here is the complete description of the scenario which was reported by Nishanth
>> who encountered the issue. The link has bootlogs and description of the exact
>> case which led to this patch.
>>
>> http://www.spinics.net/lists/linux-omap/msg124923.html
>
> it's a reply in the middle of a discussion ...
>
> What I managed to decode is that this:
>
> static int __orderly_poweroff(bool force)
> {
>          int ret;
>
>          ret = run_cmd(poweroff_cmd);
>
>          if (ret && force) {
>                  pr_warn("Failed to start orderly shutdown: forcing the issue\n");
>
>                  /*
>                   * I guess this should try to kick off some daemon to sync and
>                   * poweroff asap.  Or not even bother syncing if we're doing an
>                   * emergency shutdown?
>                   */
>                  emergency_sync();
>                  kernel_power_off();
>          }
>
>          return ret;
> }
>
> could fail to actually power the system off, if the run_cmd(poweroff_cmd)
> 'succeeds', but due to a user-space bug it does not actually call the real
> poweroff system call?
>

I tried to simulate the issue.

In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
ti_bandgap_probe i call

orderly_poweroff(true);

This is while driver probes are still on going. I observe that
ret = run_cmd(poweroff_cmd);

ret is a non-zero value and we enter the if condition:

Even after the

emergency_sync();
kernel_power_off();

calls

the console remained active in weird state.


> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14 10:42         ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-14 10:42 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Ingo,

On Thursday 14 January 2016 03:39 PM, Ingo Molnar wrote:
>
> * Keerthy <a0393675@ti.com> wrote:
>
>> Hi Ingo,
>>
>> On Thursday 14 January 2016 02:35 PM, Ingo Molnar wrote:
>>>
>>> * Keerthy <j-keerthy@ti.com> wrote:
>>>
>>>> orderly_poweroff is triggered when a graceful shutdown
>>>> of system is desired. This may be used in many critical states of the
>>>> kernel such as when subsystems detects conditions such as critical
>>>> temperature conditions. However, in certain conditions in system
>>>> boot up sequences like those in the middle of driver probes being
>>>> initiated, userspace will be unable to power off the system in a clean
>>>> manner and leaves the system in a critical state. In cases like these,
>>>> the /sbin/poweroff will return success (having forked off to attempt
>>>> powering off the system. However, the system overall will fail to
>>>> completely poweroff (since other modules will be probed) and the system
>>>> is still functional with no userspace (since that would have shut itself
>>>> off).
>>>>
>>>> However, there is no clean way of detecting such failure of userspace
>>>> powering off the system. In such scenarios, it is necessary for a backup
>>>> workqueue to be able to force a shutdown of the system when orderly
>>>> shutdown is not successful after a configurable time period.
>>>>
>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>> Suggested-by: Eduardo Valentin <edubezval@gmail.com>
>>>> Reported-by: Nishanth Menon <nm@ti.com>
>>>> ---
>>>> Links to previous discussion can be found here:
>>>>
>>>> http://www.spinics.net/lists/linux-omap/msg124925.html
>>>>
>>>> Boot tested on DRA7.
>>>>
>>>> changes in v2:
>>>>
>>>> 	* Changed #ifdef to #if CONFIG_SHUTDOWN_BACKUP_DELAY_MS
>>>>
>>>>   arch/Kconfig    |  7 +++++++
>>>>   kernel/reboot.c | 23 ++++++++++++++++++-----
>>>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>>>
>>>> Index: linux/arch/Kconfig
>>>> ===================================================================
>>>> --- linux.orig/arch/Kconfig	2016-01-11 15:26:07.732173131 +0530
>>>> +++ linux/arch/Kconfig	2016-01-11 15:26:07.728173205 +0530
>>>> @@ -37,6 +37,18 @@
>>>>   	def_bool y
>>>>   	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !PPC64
>>>>
>>>> +config SHUTDOWN_BACKUP_DELAY_MS
>>>> +	int "Backup shutdown delay in milli-seconds"
>>>> +	default 0
>>>> +	help
>>>> +	  The number of milliseconds to delay before backup workqueue
>>>> +	  executes attempting to poweroff the system after the
>>>> +	  orderly_poweroff function has failed to complete.
>>>> +
>>>> +	  If set to 0, the backup workqueue is not active. The value
>>>> +	  should be conservatively configured based on userspace latencies
>>>> +	  expected for a given system.
>>>
>>> I don't really understand this. In what circumstances can a reboot fail?
>>>
>>> I think that is what should be fixed: a reboot should never fail, instead of
>>> introducing some sort of fragile timeout based method.
>>
>> Here is the complete description of the scenario which was reported by Nishanth
>> who encountered the issue. The link has bootlogs and description of the exact
>> case which led to this patch.
>>
>> http://www.spinics.net/lists/linux-omap/msg124923.html
>
> it's a reply in the middle of a discussion ...
>
> What I managed to decode is that this:
>
> static int __orderly_poweroff(bool force)
> {
>          int ret;
>
>          ret = run_cmd(poweroff_cmd);
>
>          if (ret && force) {
>                  pr_warn("Failed to start orderly shutdown: forcing the issue\n");
>
>                  /*
>                   * I guess this should try to kick off some daemon to sync and
>                   * poweroff asap.  Or not even bother syncing if we're doing an
>                   * emergency shutdown?
>                   */
>                  emergency_sync();
>                  kernel_power_off();
>          }
>
>          return ret;
> }
>
> could fail to actually power the system off, if the run_cmd(poweroff_cmd)
> 'succeeds', but due to a user-space bug it does not actually call the real
> poweroff system call?
>

I tried to simulate the issue.

In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
ti_bandgap_probe i call

orderly_poweroff(true);

This is while driver probes are still on going. I observe that
ret = run_cmd(poweroff_cmd);

ret is a non-zero value and we enter the if condition:

Even after the

emergency_sync();
kernel_power_off();

calls

the console remained active in weird state.


> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-14 10:42         ` Keerthy
@ 2016-01-14 11:23           ` Ingo Molnar
  -1 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-14 11:23 UTC (permalink / raw)
  To: Keerthy
  Cc: Keerthy, linux-kernel, edubezval, grygorii.strashko, nm,
	linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra


* Keerthy <a0393675@ti.com> wrote:

> I tried to simulate the issue.
> 
> In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
> ti_bandgap_probe i call
> 
> orderly_poweroff(true);
> 
> This is while driver probes are still on going. I observe that
> ret = run_cmd(poweroff_cmd);
> 
> ret is a non-zero value and we enter the if condition:
> 
> Even after the
> 
> emergency_sync();
> kernel_power_off();
> 
> calls
> 
> the console remained active in weird state.

Now _that_ is clearly an architecture bug that should not be papered over ...

If kernel_power_off() is called then the system should power off. No ifs and 
whens.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14 11:23           ` Ingo Molnar
  0 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-14 11:23 UTC (permalink / raw)
  To: linux-arm-kernel


* Keerthy <a0393675@ti.com> wrote:

> I tried to simulate the issue.
> 
> In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
> ti_bandgap_probe i call
> 
> orderly_poweroff(true);
> 
> This is while driver probes are still on going. I observe that
> ret = run_cmd(poweroff_cmd);
> 
> ret is a non-zero value and we enter the if condition:
> 
> Even after the
> 
> emergency_sync();
> kernel_power_off();
> 
> calls
> 
> the console remained active in weird state.

Now _that_ is clearly an architecture bug that should not be papered over ...

If kernel_power_off() is called then the system should power off. No ifs and 
whens.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-14 11:23           ` Ingo Molnar
@ 2016-01-14 13:25             ` One Thousand Gnomes
  -1 siblings, 0 replies; 35+ messages in thread
From: One Thousand Gnomes @ 2016-01-14 13:25 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Keerthy, Keerthy, linux-kernel, edubezval, grygorii.strashko, nm,
	linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra

> 
> If kernel_power_off() is called then the system should power off. No ifs and 
> whens.

Even if it doesn't the watchdog should kill it. 

That is broken on some platforms on the watchdog side as the
watchdog shuts down during our power off callbacks - because the system
firmware is too stupid to reset the watchdog as it powers back up (so
keeps rebooting).

If you watchdog and firmware function properly you shouldn't even have to
care if you crash during the kernel power off.

Alan

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14 13:25             ` One Thousand Gnomes
  0 siblings, 0 replies; 35+ messages in thread
From: One Thousand Gnomes @ 2016-01-14 13:25 UTC (permalink / raw)
  To: linux-arm-kernel

> 
> If kernel_power_off() is called then the system should power off. No ifs and 
> whens.

Even if it doesn't the watchdog should kill it. 

That is broken on some platforms on the watchdog side as the
watchdog shuts down during our power off callbacks - because the system
firmware is too stupid to reset the watchdog as it powers back up (so
keeps rebooting).

If you watchdog and firmware function properly you shouldn't even have to
care if you crash during the kernel power off.

Alan

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-14 11:23           ` Ingo Molnar
@ 2016-01-14 14:22             ` Russell King - ARM Linux
  -1 siblings, 0 replies; 35+ messages in thread
From: Russell King - ARM Linux @ 2016-01-14 14:22 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Keerthy, nm, grygorii.strashko, Peter Zijlstra, linux-pm, peterz,
	Keerthy, linux-kernel, josh, edubezval, joel, mpe, akpm,
	linux-omap, dyoung, Thomas Gleixner, linux-arm-kernel

On Thu, Jan 14, 2016 at 12:23:54PM +0100, Ingo Molnar wrote:
> * Keerthy <a0393675@ti.com> wrote:
> > I tried to simulate the issue.
> > 
> > In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
> > ti_bandgap_probe i call
> > 
> > orderly_poweroff(true);
> > 
> > This is while driver probes are still on going. I observe that
> > ret = run_cmd(poweroff_cmd);
> > 
> > ret is a non-zero value and we enter the if condition:
> > 
> > Even after the
> > 
> > emergency_sync();
> > kernel_power_off();
> > 
> > calls
> > 
> > the console remained active in weird state.
> 
> Now _that_ is clearly an architecture bug that should not be papered over ...

No, it's not an architecture bug - it's a platform bug.  The ARM
architecture has no standard way to control CPU reset or system
power, all that is up to the platform.

> If kernel_power_off() is called then the system should power off. No
> ifs and whens.

There definitely are ifs and whens.  Only if the platform has support,
and when that support works.

If the platform does not provide such support, or that support is
broken, there's nothing that can be done at the architecture level.

Looking at the log given via the message that was referred to in a
previous message in this thread, it looks like userspace fails to get
to the point of calling into the kernel:

159.636627] rc              S c0756104     0   527      1 0x00000000
159.643027] [<c0756104>] (__schedule) from [<c0756778>] (schedule+0x40/0x98)
159.650108] [<c0756778>] (schedule) from [<c0193c50>] (pipe_wait+0x60/0x9c)
159.657102] [<c0193c50>] (pipe_wait) from [<c0193cc0>] (wait_for_partner+0x34/0x
159.664792] [<c0193cc0>] (wait_for_partner) from [<c01947fc>] (fifo_open+0x1a4/0
159.672658] [<c01947fc>] (fifo_open) from [<c018a5c0>] (do_dentry_open+0x1c8/0x3
159.680351] [<c018a5c0>] (do_dentry_open) from [<c019855c>] (do_last+0x64c/0xce0
159.687867] [<c019855c>] (do_last) from [<c019adf8>] (path_openat+0x80/0x608)
159.695036] [<c019adf8>] (path_openat) from [<c019be34>] (do_filp_open+0x2c/0x88
159.702555] [<c019be34>] (do_filp_open) from [<c018b94c>] (do_sys_open+0xfc/0x1c
159.710158] [<c018b94c>] (do_sys_open) from [<c00102a0>] (ret_fast_syscall+0x0/0

and later on, it's still there:

219.253041] rc              S c0756104     0   527      1 0x00000000
219.259443] [<c0756104>] (__schedule) from [<c0756778>] (schedule+0x40/0x98)
219.266524] [<c0756778>] (schedule) from [<c0193c50>] (pipe_wait+0x60/0x9c)
219.273516] [<c0193c50>] (pipe_wait) from [<c0193cc0>] (wait_for_partner+0x34/0x
219.281206] [<c0193cc0>] (wait_for_partner) from [<c01947fc>] (fifo_open+0x1a4/0
219.289071] [<c01947fc>] (fifo_open) from [<c018a5c0>] (do_dentry_open+0x1c8/0x3
219.296762] [<c018a5c0>] (do_dentry_open) from [<c019855c>] (do_last+0x64c/0xce0

The 'rc' script in sysvinit/upstart is normally responsible for walking
through /etc/rc?.d/* running the scripts in order.  It looks like this
has wedged, and so it's not getting anywhere near to asking the kernel
to shut down.

That's even more confirmed by there being no "Power down" message in
the log, which is printed by kernel_power_off().  So I'm not convinced
that the pointed to log is actually an illustration of the problem
that its being discussed here: it looks to me like some other failure,
and it looks like 'rc' is stuck trying to open a fifo.

It would be nice to see an example of a log where we have proof that
kernel_power_off() was called (via the "Power down" message being in
the log.)

In any case, at the architecture level, if a platform code fails to
reboot, we print "Reboot failed -- System halted", disable IRQs and
spin.

We don't print anything if a platform hasn't provided a "pm_power_off()"
hook, or that hook fails though - we just fall back to the generic
code which does a do_exit(0) for the caller.

I think some people have their power off/reboot stuff as part of their
watchdog driver, which can be a loadable module - if the module isn't
loaded, then these facilities are not available.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-14 14:22             ` Russell King - ARM Linux
  0 siblings, 0 replies; 35+ messages in thread
From: Russell King - ARM Linux @ 2016-01-14 14:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jan 14, 2016 at 12:23:54PM +0100, Ingo Molnar wrote:
> * Keerthy <a0393675@ti.com> wrote:
> > I tried to simulate the issue.
> > 
> > In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
> > ti_bandgap_probe i call
> > 
> > orderly_poweroff(true);
> > 
> > This is while driver probes are still on going. I observe that
> > ret = run_cmd(poweroff_cmd);
> > 
> > ret is a non-zero value and we enter the if condition:
> > 
> > Even after the
> > 
> > emergency_sync();
> > kernel_power_off();
> > 
> > calls
> > 
> > the console remained active in weird state.
> 
> Now _that_ is clearly an architecture bug that should not be papered over ...

No, it's not an architecture bug - it's a platform bug.  The ARM
architecture has no standard way to control CPU reset or system
power, all that is up to the platform.

> If kernel_power_off() is called then the system should power off. No
> ifs and whens.

There definitely are ifs and whens.  Only if the platform has support,
and when that support works.

If the platform does not provide such support, or that support is
broken, there's nothing that can be done at the architecture level.

Looking at the log given via the message that was referred to in a
previous message in this thread, it looks like userspace fails to get
to the point of calling into the kernel:

159.636627] rc              S c0756104     0   527      1 0x00000000
159.643027] [<c0756104>] (__schedule) from [<c0756778>] (schedule+0x40/0x98)
159.650108] [<c0756778>] (schedule) from [<c0193c50>] (pipe_wait+0x60/0x9c)
159.657102] [<c0193c50>] (pipe_wait) from [<c0193cc0>] (wait_for_partner+0x34/0x
159.664792] [<c0193cc0>] (wait_for_partner) from [<c01947fc>] (fifo_open+0x1a4/0
159.672658] [<c01947fc>] (fifo_open) from [<c018a5c0>] (do_dentry_open+0x1c8/0x3
159.680351] [<c018a5c0>] (do_dentry_open) from [<c019855c>] (do_last+0x64c/0xce0
159.687867] [<c019855c>] (do_last) from [<c019adf8>] (path_openat+0x80/0x608)
159.695036] [<c019adf8>] (path_openat) from [<c019be34>] (do_filp_open+0x2c/0x88
159.702555] [<c019be34>] (do_filp_open) from [<c018b94c>] (do_sys_open+0xfc/0x1c
159.710158] [<c018b94c>] (do_sys_open) from [<c00102a0>] (ret_fast_syscall+0x0/0

and later on, it's still there:

219.253041] rc              S c0756104     0   527      1 0x00000000
219.259443] [<c0756104>] (__schedule) from [<c0756778>] (schedule+0x40/0x98)
219.266524] [<c0756778>] (schedule) from [<c0193c50>] (pipe_wait+0x60/0x9c)
219.273516] [<c0193c50>] (pipe_wait) from [<c0193cc0>] (wait_for_partner+0x34/0x
219.281206] [<c0193cc0>] (wait_for_partner) from [<c01947fc>] (fifo_open+0x1a4/0
219.289071] [<c01947fc>] (fifo_open) from [<c018a5c0>] (do_dentry_open+0x1c8/0x3
219.296762] [<c018a5c0>] (do_dentry_open) from [<c019855c>] (do_last+0x64c/0xce0

The 'rc' script in sysvinit/upstart is normally responsible for walking
through /etc/rc?.d/* running the scripts in order.  It looks like this
has wedged, and so it's not getting anywhere near to asking the kernel
to shut down.

That's even more confirmed by there being no "Power down" message in
the log, which is printed by kernel_power_off().  So I'm not convinced
that the pointed to log is actually an illustration of the problem
that its being discussed here: it looks to me like some other failure,
and it looks like 'rc' is stuck trying to open a fifo.

It would be nice to see an example of a log where we have proof that
kernel_power_off() was called (via the "Power down" message being in
the log.)

In any case, at the architecture level, if a platform code fails to
reboot, we print "Reboot failed -- System halted", disable IRQs and
spin.

We don't print anything if a platform hasn't provided a "pm_power_off()"
hook, or that hook fails though - we just fall back to the generic
code which does a do_exit(0) for the caller.

I think some people have their power off/reboot stuff as part of their
watchdog driver, which can be a loadable module - if the module isn't
loaded, then these facilities are not available.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-14 14:22             ` Russell King - ARM Linux
@ 2016-01-15 10:13               ` Ingo Molnar
  -1 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-15 10:13 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Keerthy, nm, grygorii.strashko, Peter Zijlstra, linux-pm, peterz,
	Keerthy, linux-kernel, josh, edubezval, joel, mpe, akpm,
	linux-omap, dyoung, Thomas Gleixner, linux-arm-kernel


* Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Jan 14, 2016 at 12:23:54PM +0100, Ingo Molnar wrote:
> > * Keerthy <a0393675@ti.com> wrote:
> > > I tried to simulate the issue.
> > > 
> > > In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
> > > ti_bandgap_probe i call
> > > 
> > > orderly_poweroff(true);
> > > 
> > > This is while driver probes are still on going. I observe that
> > > ret = run_cmd(poweroff_cmd);
> > > 
> > > ret is a non-zero value and we enter the if condition:
> > > 
> > > Even after the
> > > 
> > > emergency_sync();
> > > kernel_power_off();
> > > 
> > > calls
> > > 
> > > the console remained active in weird state.
> > 
> > Now _that_ is clearly an architecture bug that should not be papered over ...
> 
> No, it's not an architecture bug - it's a platform bug. [...]

It's an 'architecture bug' in Linux kernel speak: all stuff that is traditionally 
under arch/*. The 'arch' in that directory name derives from 'architecture'.

kernel_power_off() is a traditionally architecture level (not core kernel level 
and not driver level) code.

> [...] The ARM architecture has no standard way to control CPU reset or system 
> power, all that is up to the platform.

... and platform code is typically part of arch/ as well.

FYI, you are making an unnecessarily obtuse argument by insisting on the 
architecture != platform triviality and you also injected an uncalled for 
patronizing tone into this discussion by pretending that I don't know that 
distinction. It's sad.

> > If kernel_power_off() is called then the system should power off. No ifs and 
> > whens.
> 
> There definitely are ifs and whens.  Only if the platform has support, and when 
> that support works.

And that is precisely what I meant: in a correctly working kernel, with correctly 
working hardware, in a correctly working universe, the core kernel expects 
kernel_power_off() to never 'fail'.

As the name suggests.

Yes, bugs in user-space, kernel-space, hardware and designed buggy hardware might 
prevent a reboot - as usual.

I.e. I NAK this patch from a core kernel perspective, we don't add such 
workarounds without a lot more information about why it's the right thing to do.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-15 10:13               ` Ingo Molnar
  0 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-15 10:13 UTC (permalink / raw)
  To: linux-arm-kernel


* Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Jan 14, 2016 at 12:23:54PM +0100, Ingo Molnar wrote:
> > * Keerthy <a0393675@ti.com> wrote:
> > > I tried to simulate the issue.
> > > 
> > > In the probe function of drivers/thermal/ti-soc-thermal/ti-bandgap.c
> > > ti_bandgap_probe i call
> > > 
> > > orderly_poweroff(true);
> > > 
> > > This is while driver probes are still on going. I observe that
> > > ret = run_cmd(poweroff_cmd);
> > > 
> > > ret is a non-zero value and we enter the if condition:
> > > 
> > > Even after the
> > > 
> > > emergency_sync();
> > > kernel_power_off();
> > > 
> > > calls
> > > 
> > > the console remained active in weird state.
> > 
> > Now _that_ is clearly an architecture bug that should not be papered over ...
> 
> No, it's not an architecture bug - it's a platform bug. [...]

It's an 'architecture bug' in Linux kernel speak: all stuff that is traditionally 
under arch/*. The 'arch' in that directory name derives from 'architecture'.

kernel_power_off() is a traditionally architecture level (not core kernel level 
and not driver level) code.

> [...] The ARM architecture has no standard way to control CPU reset or system 
> power, all that is up to the platform.

... and platform code is typically part of arch/ as well.

FYI, you are making an unnecessarily obtuse argument by insisting on the 
architecture != platform triviality and you also injected an uncalled for 
patronizing tone into this discussion by pretending that I don't know that 
distinction. It's sad.

> > If kernel_power_off() is called then the system should power off. No ifs and 
> > whens.
> 
> There definitely are ifs and whens.  Only if the platform has support, and when 
> that support works.

And that is precisely what I meant: in a correctly working kernel, with correctly 
working hardware, in a correctly working universe, the core kernel expects 
kernel_power_off() to never 'fail'.

As the name suggests.

Yes, bugs in user-space, kernel-space, hardware and designed buggy hardware might 
prevent a reboot - as usual.

I.e. I NAK this patch from a core kernel perspective, we don't add such 
workarounds without a lot more information about why it's the right thing to do.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-14 13:25             ` One Thousand Gnomes
@ 2016-01-15 10:14               ` Ingo Molnar
  -1 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-15 10:14 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Keerthy, Keerthy, linux-kernel, edubezval, grygorii.strashko, nm,
	linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra


* One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:

> > If kernel_power_off() is called then the system should power off. No ifs and 
> > whens.
> 
> Even if it doesn't the watchdog should kill it. 
> 
> That is broken on some platforms on the watchdog side as the
> watchdog shuts down during our power off callbacks - because the system
> firmware is too stupid to reset the watchdog as it powers back up (so
> keeps rebooting).
> 
> If you watchdog and firmware function properly you shouldn't even have to
> care if you crash during the kernel power off.

That's a good point as well - if the system is 'stuck' for some notion of stuck, 
then watchdog drivers can help.

Here it's unclear whether user-space even called the sys_reboot() system call.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-15 10:14               ` Ingo Molnar
  0 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-15 10:14 UTC (permalink / raw)
  To: linux-arm-kernel


* One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:

> > If kernel_power_off() is called then the system should power off. No ifs and 
> > whens.
> 
> Even if it doesn't the watchdog should kill it. 
> 
> That is broken on some platforms on the watchdog side as the
> watchdog shuts down during our power off callbacks - because the system
> firmware is too stupid to reset the watchdog as it powers back up (so
> keeps rebooting).
> 
> If you watchdog and firmware function properly you shouldn't even have to
> care if you crash during the kernel power off.

That's a good point as well - if the system is 'stuck' for some notion of stuck, 
then watchdog drivers can help.

Here it's unclear whether user-space even called the sys_reboot() system call.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-15 10:13               ` Ingo Molnar
@ 2016-01-15 11:05                 ` Russell King - ARM Linux
  -1 siblings, 0 replies; 35+ messages in thread
From: Russell King - ARM Linux @ 2016-01-15 11:05 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Keerthy, nm, grygorii.strashko, Peter Zijlstra, linux-pm, peterz,
	Keerthy, linux-kernel, josh, edubezval, joel, mpe, akpm,
	linux-omap, dyoung, Thomas Gleixner, linux-arm-kernel

On Fri, Jan 15, 2016 at 11:13:12AM +0100, Ingo Molnar wrote:
> FYI, you are making an unnecessarily obtuse argument by insisting on the 
> architecture != platform triviality and you also injected an uncalled for 
> patronizing tone into this discussion by pretending that I don't know that 
> distinction. It's sad.

I was trying to be helpful, but your attitude seems to always one of
finding some way to turn a reasonable discussion into an ad hominem
attack.  You've just proven that you're totally impossible to work
with.

I won't make the mistake of replying to one of your messages again.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-15 11:05                 ` Russell King - ARM Linux
  0 siblings, 0 replies; 35+ messages in thread
From: Russell King - ARM Linux @ 2016-01-15 11:05 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Jan 15, 2016 at 11:13:12AM +0100, Ingo Molnar wrote:
> FYI, you are making an unnecessarily obtuse argument by insisting on the 
> architecture != platform triviality and you also injected an uncalled for 
> patronizing tone into this discussion by pretending that I don't know that 
> distinction. It's sad.

I was trying to be helpful, but your attitude seems to always one of
finding some way to turn a reasonable discussion into an ad hominem
attack.  You've just proven that you're totally impossible to work
with.

I won't make the mistake of replying to one of your messages again.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-15 10:14               ` Ingo Molnar
  (?)
@ 2016-01-15 13:29                 ` Grygorii Strashko
  -1 siblings, 0 replies; 35+ messages in thread
From: Grygorii Strashko @ 2016-01-15 13:29 UTC (permalink / raw)
  To: Ingo Molnar, One Thousand Gnomes
  Cc: Keerthy, Keerthy, linux-kernel, edubezval, nm, linux-pm,
	linux-omap, joel, akpm, linux-arm-kernel, peterz, dyoung, josh,
	mpe, Thomas Gleixner, Peter Zijlstra

On 01/15/2016 12:14 PM, Ingo Molnar wrote:
> 
> * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
>>> If kernel_power_off() is called then the system should power off. No ifs and
>>> whens.
>>
>> Even if it doesn't the watchdog should kill it.
>>
>> That is broken on some platforms on the watchdog side as the
>> watchdog shuts down during our power off callbacks - because the system
>> firmware is too stupid to reset the watchdog as it powers back up (so
>> keeps rebooting).
>>
>> If you watchdog and firmware function properly you shouldn't even have to
>> care if you crash during the kernel power off.
> 
> That's a good point as well - if the system is 'stuck' for some notion of stuck,
> then watchdog drivers can help.
> 

Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
not too much chances for Watchdog to fire.
void machine_power_off(void)
{
	local_irq_disable();
	smp_send_stop();

	if (pm_power_off)
		pm_power_off();

	--- endless loop ?
	--- or restart ?
}
[and even if it will be there - 20-30sec is usual timeout for Watchdog and this
enough time to burn the system in case of thermal emergency poweroff :(]

> Here it's unclear whether user-space even called the sys_reboot() system call.
> 

That's true - original log [1] has 
Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
[...]
Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst 
Nov 30 11:19:24 INIT: Switching to runlevel: 0
Nov 30 11:19:24 INIT: Sending processes the TERM signal

and there are no
[  220.004522] reboot: Power down


Also, It's not the first time this part of code is discussed (thermal emergency poweroff) [2],
so the good question, as for me, is it really required and safe to use orderly_poweroff() in
case of thermal emergency poweroff ([3] as example)?

In general, this kind of use case can be simulated using SysRq on any arch
- [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
  INIT: version 2.88 booting
  Starting udev
^^ The issue most probably might happens when system in the process of loading modules
So, once modules loading process is started - fire Sysrq "poweroff(o)"

[1] http://pastebin.ubuntu.com/14326688/
[2] https://lkml.org/lkml/2012/9/18/577
[3] http://review.omapzoom.org/#/c/34898/
-- 
regards,
-grygorii

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-15 13:29                 ` Grygorii Strashko
  0 siblings, 0 replies; 35+ messages in thread
From: Grygorii Strashko @ 2016-01-15 13:29 UTC (permalink / raw)
  To: Ingo Molnar, One Thousand Gnomes
  Cc: Keerthy, Keerthy, linux-kernel, edubezval, nm, linux-pm,
	linux-omap, joel, akpm, linux-arm-kernel, peterz, dyoung, josh,
	mpe, Thomas Gleixner, Peter Zijlstra

On 01/15/2016 12:14 PM, Ingo Molnar wrote:
> 
> * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
>>> If kernel_power_off() is called then the system should power off. No ifs and
>>> whens.
>>
>> Even if it doesn't the watchdog should kill it.
>>
>> That is broken on some platforms on the watchdog side as the
>> watchdog shuts down during our power off callbacks - because the system
>> firmware is too stupid to reset the watchdog as it powers back up (so
>> keeps rebooting).
>>
>> If you watchdog and firmware function properly you shouldn't even have to
>> care if you crash during the kernel power off.
> 
> That's a good point as well - if the system is 'stuck' for some notion of stuck,
> then watchdog drivers can help.
> 

Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
not too much chances for Watchdog to fire.
void machine_power_off(void)
{
	local_irq_disable();
	smp_send_stop();

	if (pm_power_off)
		pm_power_off();

	--- endless loop ?
	--- or restart ?
}
[and even if it will be there - 20-30sec is usual timeout for Watchdog and this
enough time to burn the system in case of thermal emergency poweroff :(]

> Here it's unclear whether user-space even called the sys_reboot() system call.
> 

That's true - original log [1] has 
Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
[...]
Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst 
Nov 30 11:19:24 INIT: Switching to runlevel: 0
Nov 30 11:19:24 INIT: Sending processes the TERM signal

and there are no
[  220.004522] reboot: Power down


Also, It's not the first time this part of code is discussed (thermal emergency poweroff) [2],
so the good question, as for me, is it really required and safe to use orderly_poweroff() in
case of thermal emergency poweroff ([3] as example)?

In general, this kind of use case can be simulated using SysRq on any arch
- [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
  INIT: version 2.88 booting
  Starting udev
^^ The issue most probably might happens when system in the process of loading modules
So, once modules loading process is started - fire Sysrq "poweroff(o)"

[1] http://pastebin.ubuntu.com/14326688/
[2] https://lkml.org/lkml/2012/9/18/577
[3] http://review.omapzoom.org/#/c/34898/
-- 
regards,
-grygorii

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-15 13:29                 ` Grygorii Strashko
  0 siblings, 0 replies; 35+ messages in thread
From: Grygorii Strashko @ 2016-01-15 13:29 UTC (permalink / raw)
  To: linux-arm-kernel

On 01/15/2016 12:14 PM, Ingo Molnar wrote:
> 
> * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
>>> If kernel_power_off() is called then the system should power off. No ifs and
>>> whens.
>>
>> Even if it doesn't the watchdog should kill it.
>>
>> That is broken on some platforms on the watchdog side as the
>> watchdog shuts down during our power off callbacks - because the system
>> firmware is too stupid to reset the watchdog as it powers back up (so
>> keeps rebooting).
>>
>> If you watchdog and firmware function properly you shouldn't even have to
>> care if you crash during the kernel power off.
> 
> That's a good point as well - if the system is 'stuck' for some notion of stuck,
> then watchdog drivers can help.
> 

Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
not too much chances for Watchdog to fire.
void machine_power_off(void)
{
	local_irq_disable();
	smp_send_stop();

	if (pm_power_off)
		pm_power_off();

	--- endless loop ?
	--- or restart ?
}
[and even if it will be there - 20-30sec is usual timeout for Watchdog and this
enough time to burn the system in case of thermal emergency poweroff :(]

> Here it's unclear whether user-space even called the sys_reboot() system call.
> 

That's true - original log [1] has 
Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
[...]
Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst 
Nov 30 11:19:24 INIT: Switching to runlevel: 0
Nov 30 11:19:24 INIT: Sending processes the TERM signal

and there are no
[  220.004522] reboot: Power down


Also, It's not the first time this part of code is discussed (thermal emergency poweroff) [2],
so the good question, as for me, is it really required and safe to use orderly_poweroff() in
case of thermal emergency poweroff ([3] as example)?

In general, this kind of use case can be simulated using SysRq on any arch
- [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
  INIT: version 2.88 booting
  Starting udev
^^ The issue most probably might happens when system in the process of loading modules
So, once modules loading process is started - fire Sysrq "poweroff(o)"

[1] http://pastebin.ubuntu.com/14326688/
[2] https://lkml.org/lkml/2012/9/18/577
[3] http://review.omapzoom.org/#/c/34898/
-- 
regards,
-grygorii

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-15 13:29                 ` Grygorii Strashko
@ 2016-01-15 14:12                   ` Russell King - ARM Linux
  -1 siblings, 0 replies; 35+ messages in thread
From: Russell King - ARM Linux @ 2016-01-15 14:12 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: One Thousand Gnomes, nm, Keerthy, Peter Zijlstra, linux-pm,
	peterz, Keerthy, linux-kernel, josh, edubezval, joel, mpe, akpm,
	linux-omap, dyoung, Thomas Gleixner, linux-arm-kernel

On Fri, Jan 15, 2016 at 03:29:04PM +0200, Grygorii Strashko wrote:
> Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
> not too much chances for Watchdog to fire.
> void machine_power_off(void)
> {
> 	local_irq_disable();
> 	smp_send_stop();
> 
> 	if (pm_power_off)
> 		pm_power_off();
> 
> 	--- endless loop ?
> 	--- or restart ?
> }
> [and even if it will be there - 20-30sec is usual timeout for Watchdog
> and this enough time to burn the system in case of thermal emergency
> poweroff :(]

I covered this in my reply to Ingo yesterday.  The result is that a
failed or unimplemented call drops through to do_exit(0) on behalf of
the calling process, terminating that process.  However, as I said
in that same email, I don't think you're getting anywhere near this
code.

> That's true - original log [1] has 
> Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
> [...]
> Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst 
> Nov 30 11:19:24 INIT: Switching to runlevel: 0
> Nov 30 11:19:24 INIT: Sending processes the TERM signal
> 
> and there are no
> [  220.004522] reboot: Power down

Right, so things are stuck in userspace, which means the system is still
in an active runnable state.

As I mentioned (again) in my email, the issue appears to be that the 'rc'
script is stuck waiting on a FIFO.

The init daemon is trying to do an orderly shutdown.  As part of that,
it's executing the 'rc' script, which in systems I've seen, runs through
a set of scripts in the /etc/rc?.d directory in order, which normally
bring up or take down services and perform other sequenced actions.

If this script hangs (as it seems to be doing) it won't get to running
/sbin/poweroff or similar, and that means machine_power_off() won't be
called.

> In general, this kind of use case can be simulated using SysRq on any arch
> - [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
>   INIT: version 2.88 booting
>   Starting udev
> ^^ The issue most probably might happens when system in the process of
> loading modules
> So, once modules loading process is started - fire Sysrq "poweroff(o)"

This suggests it could be a udev issue - but without knowing what's
happening inside sysvinit's scripts, it's hard to know for certain.
Adding some debug to the 'rc' script (make sure it works without
rebooting or changing the run level, or have a way of restoring the
file if it fails to boot) so that it's possible to see what it's doing
may be a good idea - the simplest approach may be to just add

set -x

towards the top of the file - which will make it very noisy.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-15 14:12                   ` Russell King - ARM Linux
  0 siblings, 0 replies; 35+ messages in thread
From: Russell King - ARM Linux @ 2016-01-15 14:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Jan 15, 2016 at 03:29:04PM +0200, Grygorii Strashko wrote:
> Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
> not too much chances for Watchdog to fire.
> void machine_power_off(void)
> {
> 	local_irq_disable();
> 	smp_send_stop();
> 
> 	if (pm_power_off)
> 		pm_power_off();
> 
> 	--- endless loop ?
> 	--- or restart ?
> }
> [and even if it will be there - 20-30sec is usual timeout for Watchdog
> and this enough time to burn the system in case of thermal emergency
> poweroff :(]

I covered this in my reply to Ingo yesterday.  The result is that a
failed or unimplemented call drops through to do_exit(0) on behalf of
the calling process, terminating that process.  However, as I said
in that same email, I don't think you're getting anywhere near this
code.

> That's true - original log [1] has 
> Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
> [...]
> Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst 
> Nov 30 11:19:24 INIT: Switching to runlevel: 0
> Nov 30 11:19:24 INIT: Sending processes the TERM signal
> 
> and there are no
> [  220.004522] reboot: Power down

Right, so things are stuck in userspace, which means the system is still
in an active runnable state.

As I mentioned (again) in my email, the issue appears to be that the 'rc'
script is stuck waiting on a FIFO.

The init daemon is trying to do an orderly shutdown.  As part of that,
it's executing the 'rc' script, which in systems I've seen, runs through
a set of scripts in the /etc/rc?.d directory in order, which normally
bring up or take down services and perform other sequenced actions.

If this script hangs (as it seems to be doing) it won't get to running
/sbin/poweroff or similar, and that means machine_power_off() won't be
called.

> In general, this kind of use case can be simulated using SysRq on any arch
> - [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
>   INIT: version 2.88 booting
>   Starting udev
> ^^ The issue most probably might happens when system in the process of
> loading modules
> So, once modules loading process is started - fire Sysrq "poweroff(o)"

This suggests it could be a udev issue - but without knowing what's
happening inside sysvinit's scripts, it's hard to know for certain.
Adding some debug to the 'rc' script (make sure it works without
rebooting or changing the run level, or have a way of restoring the
file if it fails to boot) so that it's possible to see what it's doing
may be a good idea - the simplest approach may be to just add

set -x

towards the top of the file - which will make it very noisy.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-15 13:29                 ` Grygorii Strashko
@ 2016-01-19  9:06                   ` Ingo Molnar
  -1 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-19  9:06 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: One Thousand Gnomes, Keerthy, Keerthy, linux-kernel, edubezval,
	nm, linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra


* Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 01/15/2016 12:14 PM, Ingo Molnar wrote:
> > 
> > * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> > 
> >>> If kernel_power_off() is called then the system should power off. No ifs and
> >>> whens.
> >>
> >> Even if it doesn't the watchdog should kill it.
> >>
> >> That is broken on some platforms on the watchdog side as the
> >> watchdog shuts down during our power off callbacks - because the system
> >> firmware is too stupid to reset the watchdog as it powers back up (so
> >> keeps rebooting).
> >>
> >> If you watchdog and firmware function properly you shouldn't even have to
> >> care if you crash during the kernel power off.
> > 
> > That's a good point as well - if the system is 'stuck' for some notion of stuck,
> > then watchdog drivers can help.
> > 
> 
> Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
> not too much chances for Watchdog to fire.
> void machine_power_off(void)
> {
> 	local_irq_disable();
> 	smp_send_stop();
> 
> 	if (pm_power_off)
> 		pm_power_off();
> 
> 	--- endless loop ?
> 	--- or restart ?
> }
> [and even if it will be there - 20-30sec is usual timeout for Watchdog and this
> enough time to burn the system in case of thermal emergency poweroff :(]
> 
> > Here it's unclear whether user-space even called the sys_reboot() system call.
> > 
> 
> That's true - original log [1] has 
> Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
> [...]
> Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst 
> Nov 30 11:19:24 INIT: Switching to runlevel: 0
> Nov 30 11:19:24 INIT: Sending processes the TERM signal
> 
> and there are no
> [  220.004522] reboot: Power down
> 
> 
> Also, It's not the first time this part of code is discussed (thermal emergency poweroff) [2],
> so the good question, as for me, is it really required and safe to use orderly_poweroff() in
> case of thermal emergency poweroff ([3] as example)?
> 
> In general, this kind of use case can be simulated using SysRq on any arch
> - [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
>   INIT: version 2.88 booting
>   Starting udev
> ^^ The issue most probably might happens when system in the process of loading modules
> So, once modules loading process is started - fire Sysrq "poweroff(o)"

So I'd say emergency poweroff should be named accordingly - and the 
orderly_poweroff() name suggest anything but an emergency, right?

So I'd be fine with the following:

 - introduce a poweroff_emergency() core kernel function call

 - use it in drivers where it's justified

 - poweroff_emergency() has a configurable timeout value. If the timeout value is
   set to 0 then it powers the system off immediately.

Functionally it would be mostly equivalent to your current patch (except the '0' 
immediate poweroff functionality).

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-19  9:06                   ` Ingo Molnar
  0 siblings, 0 replies; 35+ messages in thread
From: Ingo Molnar @ 2016-01-19  9:06 UTC (permalink / raw)
  To: linux-arm-kernel


* Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 01/15/2016 12:14 PM, Ingo Molnar wrote:
> > 
> > * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> > 
> >>> If kernel_power_off() is called then the system should power off. No ifs and
> >>> whens.
> >>
> >> Even if it doesn't the watchdog should kill it.
> >>
> >> That is broken on some platforms on the watchdog side as the
> >> watchdog shuts down during our power off callbacks - because the system
> >> firmware is too stupid to reset the watchdog as it powers back up (so
> >> keeps rebooting).
> >>
> >> If you watchdog and firmware function properly you shouldn't even have to
> >> care if you crash during the kernel power off.
> > 
> > That's a good point as well - if the system is 'stuck' for some notion of stuck,
> > then watchdog drivers can help.
> > 
> 
> Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
> not too much chances for Watchdog to fire.
> void machine_power_off(void)
> {
> 	local_irq_disable();
> 	smp_send_stop();
> 
> 	if (pm_power_off)
> 		pm_power_off();
> 
> 	--- endless loop ?
> 	--- or restart ?
> }
> [and even if it will be there - 20-30sec is usual timeout for Watchdog and this
> enough time to burn the system in case of thermal emergency poweroff :(]
> 
> > Here it's unclear whether user-space even called the sys_reboot() system call.
> > 
> 
> That's true - original log [1] has 
> Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
> [...]
> Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst 
> Nov 30 11:19:24 INIT: Switching to runlevel: 0
> Nov 30 11:19:24 INIT: Sending processes the TERM signal
> 
> and there are no
> [  220.004522] reboot: Power down
> 
> 
> Also, It's not the first time this part of code is discussed (thermal emergency poweroff) [2],
> so the good question, as for me, is it really required and safe to use orderly_poweroff() in
> case of thermal emergency poweroff ([3] as example)?
> 
> In general, this kind of use case can be simulated using SysRq on any arch
> - [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
>   INIT: version 2.88 booting
>   Starting udev
> ^^ The issue most probably might happens when system in the process of loading modules
> So, once modules loading process is started - fire Sysrq "poweroff(o)"

So I'd say emergency poweroff should be named accordingly - and the 
orderly_poweroff() name suggest anything but an emergency, right?

So I'd be fine with the following:

 - introduce a poweroff_emergency() core kernel function call

 - use it in drivers where it's justified

 - poweroff_emergency() has a configurable timeout value. If the timeout value is
   set to 0 then it powers the system off immediately.

Functionally it would be mostly equivalent to your current patch (except the '0' 
immediate poweroff functionality).

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
  2016-01-19  9:06                   ` Ingo Molnar
  (?)
@ 2016-01-19 10:32                     ` Keerthy
  -1 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-19 10:32 UTC (permalink / raw)
  To: Ingo Molnar, Grygorii Strashko
  Cc: One Thousand Gnomes, Keerthy, linux-kernel, edubezval, nm,
	linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra

Hi Ingo,

On Tuesday 19 January 2016 02:36 PM, Ingo Molnar wrote:
>
> * Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 01/15/2016 12:14 PM, Ingo Molnar wrote:
>>>
>>> * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>>>
>>>>> If kernel_power_off() is called then the system should power off. No ifs and
>>>>> whens.
>>>>
>>>> Even if it doesn't the watchdog should kill it.
>>>>
>>>> That is broken on some platforms on the watchdog side as the
>>>> watchdog shuts down during our power off callbacks - because the system
>>>> firmware is too stupid to reset the watchdog as it powers back up (so
>>>> keeps rebooting).
>>>>
>>>> If you watchdog and firmware function properly you shouldn't even have to
>>>> care if you crash during the kernel power off.
>>>
>>> That's a good point as well - if the system is 'stuck' for some notion of stuck,
>>> then watchdog drivers can help.
>>>
>>
>> Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
>> not too much chances for Watchdog to fire.
>> void machine_power_off(void)
>> {
>> 	local_irq_disable();
>> 	smp_send_stop();
>>
>> 	if (pm_power_off)
>> 		pm_power_off();
>>
>> 	--- endless loop ?
>> 	--- or restart ?
>> }
>> [and even if it will be there - 20-30sec is usual timeout for Watchdog and this
>> enough time to burn the system in case of thermal emergency poweroff :(]
>>
>>> Here it's unclear whether user-space even called the sys_reboot() system call.
>>>
>>
>> That's true - original log [1] has
>> Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
>> [...]
>> Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst
>> Nov 30 11:19:24 INIT: Switching to runlevel: 0
>> Nov 30 11:19:24 INIT: Sending processes the TERM signal
>>
>> and there are no
>> [  220.004522] reboot: Power down
>>
>>
>> Also, It's not the first time this part of code is discussed (thermal emergency poweroff) [2],
>> so the good question, as for me, is it really required and safe to use orderly_poweroff() in
>> case of thermal emergency poweroff ([3] as example)?
>>
>> In general, this kind of use case can be simulated using SysRq on any arch
>> - [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
>>    INIT: version 2.88 booting
>>    Starting udev
>> ^^ The issue most probably might happens when system in the process of loading modules
>> So, once modules loading process is started - fire Sysrq "poweroff(o)"
>
> So I'd say emergency poweroff should be named accordingly - and the
> orderly_poweroff() name suggest anything but an emergency, right?
>
> So I'd be fine with the following:
>
>   - introduce a poweroff_emergency() core kernel function call
>
>   - use it in drivers where it's justified
>
>   - poweroff_emergency() has a configurable timeout value. If the timeout value is
>     set to 0 then it powers the system off immediately.
>
> Functionally it would be mostly equivalent to your current patch (except the '0'
> immediate poweroff functionality).

Thanks for the suggestion. I will work on this and get back.

Best Regards,
Keerthy

>
> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-19 10:32                     ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-19 10:32 UTC (permalink / raw)
  To: Ingo Molnar, Grygorii Strashko
  Cc: One Thousand Gnomes, Keerthy, linux-kernel, edubezval, nm,
	linux-pm, linux-omap, joel, akpm, linux-arm-kernel, peterz,
	dyoung, josh, mpe, Thomas Gleixner, Peter Zijlstra

Hi Ingo,

On Tuesday 19 January 2016 02:36 PM, Ingo Molnar wrote:
>
> * Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 01/15/2016 12:14 PM, Ingo Molnar wrote:
>>>
>>> * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>>>
>>>>> If kernel_power_off() is called then the system should power off. No ifs and
>>>>> whens.
>>>>
>>>> Even if it doesn't the watchdog should kill it.
>>>>
>>>> That is broken on some platforms on the watchdog side as the
>>>> watchdog shuts down during our power off callbacks - because the system
>>>> firmware is too stupid to reset the watchdog as it powers back up (so
>>>> keeps rebooting).
>>>>
>>>> If you watchdog and firmware function properly you shouldn't even have to
>>>> care if you crash during the kernel power off.
>>>
>>> That's a good point as well - if the system is 'stuck' for some notion of stuck,
>>> then watchdog drivers can help.
>>>
>>
>> Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
>> not too much chances for Watchdog to fire.
>> void machine_power_off(void)
>> {
>> 	local_irq_disable();
>> 	smp_send_stop();
>>
>> 	if (pm_power_off)
>> 		pm_power_off();
>>
>> 	--- endless loop ?
>> 	--- or restart ?
>> }
>> [and even if it will be there - 20-30sec is usual timeout for Watchdog and this
>> enough time to burn the system in case of thermal emergency poweroff :(]
>>
>>> Here it's unclear whether user-space even called the sys_reboot() system call.
>>>
>>
>> That's true - original log [1] has
>> Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
>> [...]
>> Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst
>> Nov 30 11:19:24 INIT: Switching to runlevel: 0
>> Nov 30 11:19:24 INIT: Sending processes the TERM signal
>>
>> and there are no
>> [  220.004522] reboot: Power down
>>
>>
>> Also, It's not the first time this part of code is discussed (thermal emergency poweroff) [2],
>> so the good question, as for me, is it really required and safe to use orderly_poweroff() in
>> case of thermal emergency poweroff ([3] as example)?
>>
>> In general, this kind of use case can be simulated using SysRq on any arch
>> - [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
>>    INIT: version 2.88 booting
>>    Starting udev
>> ^^ The issue most probably might happens when system in the process of loading modules
>> So, once modules loading process is started - fire Sysrq "poweroff(o)"
>
> So I'd say emergency poweroff should be named accordingly - and the
> orderly_poweroff() name suggest anything but an emergency, right?
>
> So I'd be fine with the following:
>
>   - introduce a poweroff_emergency() core kernel function call
>
>   - use it in drivers where it's justified
>
>   - poweroff_emergency() has a configurable timeout value. If the timeout value is
>     set to 0 then it powers the system off immediately.
>
> Functionally it would be mostly equivalent to your current patch (except the '0'
> immediate poweroff functionality).

Thanks for the suggestion. I will work on this and get back.

Best Regards,
Keerthy

>
> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2] reboot: Backup orderly_poweroff
@ 2016-01-19 10:32                     ` Keerthy
  0 siblings, 0 replies; 35+ messages in thread
From: Keerthy @ 2016-01-19 10:32 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Ingo,

On Tuesday 19 January 2016 02:36 PM, Ingo Molnar wrote:
>
> * Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 01/15/2016 12:14 PM, Ingo Molnar wrote:
>>>
>>> * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>>>
>>>>> If kernel_power_off() is called then the system should power off. No ifs and
>>>>> whens.
>>>>
>>>> Even if it doesn't the watchdog should kill it.
>>>>
>>>> That is broken on some platforms on the watchdog side as the
>>>> watchdog shuts down during our power off callbacks - because the system
>>>> firmware is too stupid to reset the watchdog as it powers back up (so
>>>> keeps rebooting).
>>>>
>>>> If you watchdog and firmware function properly you shouldn't even have to
>>>> care if you crash during the kernel power off.
>>>
>>> That's a good point as well - if the system is 'stuck' for some notion of stuck,
>>> then watchdog drivers can help.
>>>
>>
>> Seems ARM doesn't have endless loop implemented in machine_power_off() - so,
>> not too much chances for Watchdog to fire.
>> void machine_power_off(void)
>> {
>> 	local_irq_disable();
>> 	smp_send_stop();
>>
>> 	if (pm_power_off)
>> 		pm_power_off();
>>
>> 	--- endless loop ?
>> 	--- or restart ?
>> }
>> [and even if it will be there - 20-30sec is usual timeout for Watchdog and this
>> enough time to burn the system in case of thermal emergency poweroff :(]
>>
>>> Here it's unclear whether user-space even called the sys_reboot() system call.
>>>
>>
>> That's true - original log [1] has
>> Nov 30 11:19:22 [    5.942769] thermal thermal_zone3: critical temperature reached(108 C),shutting down
>> [...]
>> Nov 30 11:19:24 [    7.387900] ahci 4a140000.sata: flags: 64bit ncq sntf stag pm led clo only pmp pio slum part ccc apst
>> Nov 30 11:19:24 INIT: Switching to runlevel: 0
>> Nov 30 11:19:24 INIT: Sending processes the TERM signal
>>
>> and there are no
>> [  220.004522] reboot: Power down
>>
>>
>> Also, It's not the first time this part of code is discussed (thermal emergency poweroff) [2],
>> so the good question, as for me, is it really required and safe to use orderly_poweroff() in
>> case of thermal emergency poweroff ([3] as example)?
>>
>> In general, this kind of use case can be simulated using SysRq on any arch
>> - [3.290034] Freeing unused kernel memory: 492K (c0a67000 - c0ae2000)
>>    INIT: version 2.88 booting
>>    Starting udev
>> ^^ The issue most probably might happens when system in the process of loading modules
>> So, once modules loading process is started - fire Sysrq "poweroff(o)"
>
> So I'd say emergency poweroff should be named accordingly - and the
> orderly_poweroff() name suggest anything but an emergency, right?
>
> So I'd be fine with the following:
>
>   - introduce a poweroff_emergency() core kernel function call
>
>   - use it in drivers where it's justified
>
>   - poweroff_emergency() has a configurable timeout value. If the timeout value is
>     set to 0 then it powers the system off immediately.
>
> Functionally it would be mostly equivalent to your current patch (except the '0'
> immediate poweroff functionality).

Thanks for the suggestion. I will work on this and get back.

Best Regards,
Keerthy

>
> Thanks,
>
> 	Ingo
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2016-01-19 10:34 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-13 12:33 [PATCH v2] reboot: Backup orderly_poweroff Keerthy
2016-01-13 12:33 ` Keerthy
2016-01-13 12:33 ` Keerthy
2016-01-14  9:05 ` Ingo Molnar
2016-01-14  9:05   ` Ingo Molnar
2016-01-14  9:18   ` Keerthy
2016-01-14  9:18     ` Keerthy
2016-01-14  9:18     ` Keerthy
2016-01-14 10:09     ` Ingo Molnar
2016-01-14 10:09       ` Ingo Molnar
2016-01-14 10:42       ` Keerthy
2016-01-14 10:42         ` Keerthy
2016-01-14 10:42         ` Keerthy
2016-01-14 11:23         ` Ingo Molnar
2016-01-14 11:23           ` Ingo Molnar
2016-01-14 13:25           ` One Thousand Gnomes
2016-01-14 13:25             ` One Thousand Gnomes
2016-01-15 10:14             ` Ingo Molnar
2016-01-15 10:14               ` Ingo Molnar
2016-01-15 13:29               ` Grygorii Strashko
2016-01-15 13:29                 ` Grygorii Strashko
2016-01-15 13:29                 ` Grygorii Strashko
2016-01-15 14:12                 ` Russell King - ARM Linux
2016-01-15 14:12                   ` Russell King - ARM Linux
2016-01-19  9:06                 ` Ingo Molnar
2016-01-19  9:06                   ` Ingo Molnar
2016-01-19 10:32                   ` Keerthy
2016-01-19 10:32                     ` Keerthy
2016-01-19 10:32                     ` Keerthy
2016-01-14 14:22           ` Russell King - ARM Linux
2016-01-14 14:22             ` Russell King - ARM Linux
2016-01-15 10:13             ` Ingo Molnar
2016-01-15 10:13               ` Ingo Molnar
2016-01-15 11:05               ` Russell King - ARM Linux
2016-01-15 11:05                 ` Russell King - ARM Linux

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.