[PATCH v2] panic: Make panic_timeout configurable

[PATCH v2] panic: Make panic_timeout configurable

[Jason Baron]

The panic_timeout value can be set via the command line option 'panic=x', or via
/proc/sys/kernel/panic, however that is not sufficient when the panic occurs
before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
so that we can set the desired value from the .config.

The default panic_timeout value continues to be 0 - wait forever, except for
powerpc and mips, which have been defaulted to 180 and 5 respectively. This
is in keeping with the fact that these arches already set panic_timeout in
their arch init code. However, I found three exceptions- two in mips and one in
powerpc where the settings didn't match these default values. In those cases, I
left the arch code so it continues to override, in case the user has not changed
from the default. It would nice if these arches had one default value, or if we
could determine the correct setting at compile-time.

Signed-off-by: Jason Baron <jbaron@akamai.com>
---

-Restrict arch defaults to arch/ code
-add set_arch_panic_timeout(), in case .config specifies a non-default timeout

 arch/mips/Kconfig                      |  4 ++++
 arch/mips/ar7/setup.c                  |  2 +-
 arch/mips/emma/markeins/setup.c        |  2 +-
 arch/mips/include/asm/setup.h          |  2 ++
 arch/mips/netlogic/xlp/setup.c         |  1 -
 arch/mips/netlogic/xlr/setup.c         |  1 -
 arch/mips/sibyte/swarm/setup.c         |  2 --
 arch/powerpc/Kconfig                   |  4 ++++
 arch/powerpc/include/asm/setup.h       |  1 +
 arch/powerpc/kernel/setup_32.c         |  3 ---
 arch/powerpc/kernel/setup_64.c         |  3 ---
 arch/powerpc/platforms/pseries/setup.c |  2 +-
 include/linux/kernel.h                 |  9 +++++++++
 kernel/panic.c                         |  2 +-
 lib/Kconfig.debug                      | 11 +++++++++++
 15 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 650de39..0c21758 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -837,6 +837,10 @@ config SCHED_OMIT_FRAME_POINTER
 	bool
 	default y
 
+config PANIC_TIMEOUT
+	int
+	default 5
+
 #
 # Select some configuration options automatically based on user selections.
 #
diff --git a/arch/mips/ar7/setup.c b/arch/mips/ar7/setup.c
index 9a357ff..46e7abc 100644
--- a/arch/mips/ar7/setup.c
+++ b/arch/mips/ar7/setup.c
@@ -92,7 +92,7 @@ void __init plat_mem_setup(void)
 	_machine_restart = ar7_machine_restart;
 	_machine_halt = ar7_machine_halt;
 	pm_power_off = ar7_machine_power_off;
-	panic_timeout = 3;
+	set_arch_panic_timeout(3, ARCH_PANIC_TIMEOUT);
 
 	io_base = (unsigned long)ioremap(AR7_REGS_BASE, 0x10000);
 	if (!io_base)
diff --git a/arch/mips/emma/markeins/setup.c b/arch/mips/emma/markeins/setup.c
index d710058..03a47a7 100644
--- a/arch/mips/emma/markeins/setup.c
+++ b/arch/mips/emma/markeins/setup.c
@@ -112,7 +112,7 @@ void __init plat_mem_setup(void)
 	iomem_resource.end = EMMA2RH_ROM_BASE - 1;
 
 	/* Reboot on panic */
-	panic_timeout = 180;
+	set_arch_panic_timeout(180, ARCH_PANIC_TIMEOUT);
 
 	markeins_sio_setup();
 }
diff --git a/arch/mips/include/asm/setup.h b/arch/mips/include/asm/setup.h
index d7bfdeb..9d1b53c 100644
--- a/arch/mips/include/asm/setup.h
+++ b/arch/mips/include/asm/setup.h
@@ -24,4 +24,6 @@ extern unsigned long ebase;
 extern void per_cpu_trap_init(bool);
 extern void cpu_cache_init(void);
 
+#define ARCH_PANIC_TIMEOUT 5
+
 #endif /* __SETUP_H */
diff --git a/arch/mips/netlogic/xlp/setup.c b/arch/mips/netlogic/xlp/setup.c
index 6d981bb..54e75c7 100644
--- a/arch/mips/netlogic/xlp/setup.c
+++ b/arch/mips/netlogic/xlp/setup.c
@@ -92,7 +92,6 @@ static void __init xlp_init_mem_from_bars(void)
 
 void __init plat_mem_setup(void)
 {
-	panic_timeout	= 5;
 	_machine_restart = (void (*)(char *))nlm_linux_exit;
 	_machine_halt	= nlm_linux_exit;
 	pm_power_off	= nlm_linux_exit;
diff --git a/arch/mips/netlogic/xlr/setup.c b/arch/mips/netlogic/xlr/setup.c
index 214d123..921be5f 100644
--- a/arch/mips/netlogic/xlr/setup.c
+++ b/arch/mips/netlogic/xlr/setup.c
@@ -92,7 +92,6 @@ static void nlm_linux_exit(void)
 
 void __init plat_mem_setup(void)
 {
-	panic_timeout	= 5;
 	_machine_restart = (void (*)(char *))nlm_linux_exit;
 	_machine_halt	= nlm_linux_exit;
 	pm_power_off	= nlm_linux_exit;
diff --git a/arch/mips/sibyte/swarm/setup.c b/arch/mips/sibyte/swarm/setup.c
index 41707a2..3462c83 100644
--- a/arch/mips/sibyte/swarm/setup.c
+++ b/arch/mips/sibyte/swarm/setup.c
@@ -134,8 +134,6 @@ void __init plat_mem_setup(void)
 #error invalid SiByte board configuration
 #endif
 
-	panic_timeout = 5;  /* For debug.  */
-
 	board_be_handler = swarm_be_handler;
 
 	if (xicor_probe())
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index b44b52c..b2be8e8 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -147,6 +147,10 @@ config EARLY_PRINTK
 	bool
 	default y
 
+config PANIC_TIMEOUT
+	int
+	default 180
+
 config COMPAT
 	bool
 	default y if PPC64
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 703a841..11ba86e 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -26,6 +26,7 @@ extern void reloc_got2(unsigned long);
 void check_for_initrd(void);
 void do_init_bootmem(void);
 void setup_panic(void);
+#define ARCH_PANIC_TIMEOUT 180
 
 #endif /* !__ASSEMBLY__ */
 
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index b903dc5..2b0da27 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -296,9 +296,6 @@ void __init setup_arch(char **cmdline_p)
 	if (cpu_has_feature(CPU_FTR_UNIFIED_ID_CACHE))
 		ucache_bsize = icache_bsize = dcache_bsize;
 
-	/* reboot on panic */
-	panic_timeout = 180;
-
 	if (ppc_md.panic)
 		setup_panic();
 
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 4085aaa..856dd4e 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -588,9 +588,6 @@ void __init setup_arch(char **cmdline_p)
 	dcache_bsize = ppc64_caches.dline_size;
 	icache_bsize = ppc64_caches.iline_size;
 
-	/* reboot on panic */
-	panic_timeout = 180;
-
 	if (ppc_md.panic)
 		setup_panic();
 
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 1f97e2b..f5f710a 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -444,7 +444,7 @@ static void pSeries_machine_kexec(struct kimage *image)
 
 static void __init pSeries_setup_arch(void)
 {
-	panic_timeout = 10;
+	set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
 
 	/* Discover PIC type and setup ppc_md accordingly */
 	pseries_discover_pic();
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d4e98d1..2ac0277 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -393,6 +393,15 @@ extern int panic_on_oops;
 extern int panic_on_unrecovered_nmi;
 extern int panic_on_io_nmi;
 extern int sysctl_panic_on_stackoverflow;
+/*
+ * Only to be used by arch init code. If the user over-wrote the default
+ * CONFIG_PANIC_TIMEOUT, honor it.
+ */
+static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
+{
+	if (panic_timeout == arch_default_timeout)
+		panic_timeout = timeout;
+}
 extern const char *print_tainted(void);
 enum lockdep_ok {
 	LOCKDEP_STILL_OK,
diff --git a/kernel/panic.c b/kernel/panic.c
index c00b4ce..6d63003 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -33,7 +33,7 @@ static int pause_on_oops;
 static int pause_on_oops_flag;
 static DEFINE_SPINLOCK(pause_on_oops_lock);
 
-int panic_timeout;
+int panic_timeout = CONFIG_PANIC_TIMEOUT;
 EXPORT_SYMBOL_GPL(panic_timeout);
 
 ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index db25707..e3086d0 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -761,6 +761,17 @@ config PANIC_ON_OOPS_VALUE
 	default 0 if !PANIC_ON_OOPS
 	default 1 if PANIC_ON_OOPS
 
+config PANIC_TIMEOUT
+	int "panic timeout"
+	default 0
+	help
+	  Set the timeout value (in seconds) until a reboot occurs when the
+	  the kernel panics. If n = 0, then we wait forever. A timeout
+	  value n > 0 will wait n seconds before rebooting, while a timeout
+	  value n < 0 will reboot immediately. Note: If left at the default
+	  setting, some architectures (mips, powerpc) may override this setting
+	  in early boot.
+
 config SCHED_DEBUG
 	bool "Collect scheduler debugging info"
 	depends on DEBUG_KERNEL && PROC_FS
-- 
1.8.2

Re: [PATCH v2] panic: Make panic_timeout configurable

[Andrew Morton]

On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:

> The panic_timeout value can be set via the command line option 'panic=x', or via
> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
> so that we can set the desired value from the .config.
> 
> The default panic_timeout value continues to be 0 - wait forever, except for
> powerpc and mips, which have been defaulted to 180 and 5 respectively. This
> is in keeping with the fact that these arches already set panic_timeout in
> their arch init code. However, I found three exceptions- two in mips and one in
> powerpc where the settings didn't match these default values. In those cases, I
> left the arch code so it continues to override, in case the user has not changed
> from the default. It would nice if these arches had one default value, or if we
> could determine the correct setting at compile-time.

Felipe is proposing a simpler patch ("panic: setup panic_timeout
early") which switches to early_param().  Is that sufficient for the
(undescribed!) failure which you are presumably observing?

Re: [PATCH v2] panic: Make panic_timeout configurable

[Jason Baron]

On 11/18/2013 05:30 PM, Andrew Morton wrote:
> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
> 
>> The panic_timeout value can be set via the command line option 'panic=x', or via
>> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
>> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
>> so that we can set the desired value from the .config.
>>
>> The default panic_timeout value continues to be 0 - wait forever, except for
>> powerpc and mips, which have been defaulted to 180 and 5 respectively. This
>> is in keeping with the fact that these arches already set panic_timeout in
>> their arch init code. However, I found three exceptions- two in mips and one in
>> powerpc where the settings didn't match these default values. In those cases, I
>> left the arch code so it continues to override, in case the user has not changed
>> from the default. It would nice if these arches had one default value, or if we
>> could determine the correct setting at compile-time.
> 
> Felipe is proposing a simpler patch ("panic: setup panic_timeout
> early") which switches to early_param().  Is that sufficient for the
> (undescribed!) failure which you are presumably observing?
> 

No - that patch doesn't change the 'panic_timeout' value until the call
to 'parse_early_param()' is made. If there is a panic before that point, the
param doesn't do anything. The idea of this patch is to allow it to be configured
at build-time.

I've tested the patch by simply inserting a panic() call at the beginning of
'start_kernel()'. So, no I do not have a specific panic in mind for this.

Thanks,

-Jason

Re: [PATCH v2] panic: Make panic_timeout configurable

[Ingo Molnar]

* Jason Baron <jbaron@akamai.com> wrote:

> On 11/18/2013 05:30 PM, Andrew Morton wrote:
> > On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
> > 
> >> The panic_timeout value can be set via the command line option 'panic=x', or via
> >> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
> >> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
> >> so that we can set the desired value from the .config.
> >>
> >> The default panic_timeout value continues to be 0 - wait forever, 
> >> except for powerpc and mips, which have been defaulted to 180 and 
> >> 5 respectively. This is in keeping with the fact that these 
> >> arches already set panic_timeout in their arch init code. 
> >> However, I found three exceptions- two in mips and one in powerpc 
> >> where the settings didn't match these default values. In those 
> >> cases, I left the arch code so it continues to override, in case 
> >> the user has not changed from the default. It would nice if these 
> >> arches had one default value, or if we could determine the 
> >> correct setting at compile-time.
> > 
> > Felipe is proposing a simpler patch ("panic: setup panic_timeout 
> > early") which switches to early_param().  Is that sufficient for 
> > the (undescribed!) failure which you are presumably observing?
> > 
> 
> No - that patch doesn't change the 'panic_timeout' value until the 
> call to 'parse_early_param()' is made. If there is a panic before 
> that point, the param doesn't do anything. The idea of this patch is 
> to allow it to be configured at build-time.
> 
> I've tested the patch by simply inserting a panic() call at the 
> beginning of 'start_kernel()'. So, no I do not have a specific panic 
> in mind for this.

Would you be interested in picking up Felipe's patch/fix on top of 
yours? I was unable to communicate with him efficiently, but I'd take 
the patch if it's signed off by you.

Thanks,

	Ingo

Re: [PATCH v2] panic: Make panic_timeout configurable

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
> 
> > The panic_timeout value can be set via the command line option 'panic=x', or via
> > /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
> > before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
> > so that we can set the desired value from the .config.
> > 
> > The default panic_timeout value continues to be 0 - wait forever, except for
> > powerpc and mips, which have been defaulted to 180 and 5 respectively. This
> > is in keeping with the fact that these arches already set panic_timeout in
> > their arch init code. However, I found three exceptions- two in mips and one in
> > powerpc where the settings didn't match these default values. In those cases, I
> > left the arch code so it continues to override, in case the user has not changed
> > from the default. It would nice if these arches had one default value, or if we
> > could determine the correct setting at compile-time.
> 
> Felipe is proposing a simpler patch ("panic: setup panic_timeout
> early") which switches to early_param().  Is that sufficient for the
> (undescribed!) failure which you are presumably observing?

Also note that that patch is still incomplete: if panic_timeout is 
switched to early_param() then closely related functionality such as 
pause_on_oops should be moved early as well...

Thanks,

	Ingo

Re: [PATCH v2] panic: Make panic_timeout configurable

[Ralf Baechle]

On Mon, Nov 18, 2013 at 09:04:36PM +0000, Jason Baron wrote:

> The panic_timeout value can be set via the command line option 'panic=x', or via
> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
> so that we can set the desired value from the .config.
> 
> The default panic_timeout value continues to be 0 - wait forever, except for
> powerpc and mips, which have been defaulted to 180 and 5 respectively. This
> is in keeping with the fact that these arches already set panic_timeout in
> their arch init code. However, I found three exceptions- two in mips and one in
> powerpc where the settings didn't match these default values. In those cases, I
> left the arch code so it continues to override, in case the user has not changed
> from the default. It would nice if these arches had one default value, or if we
> could determine the correct setting at compile-time.

It's more complicated - MIPS was using the global default with five MIPS
platforms overriding the default.

I propose to kill these overrides for sanity unless somebody comes up
with a good argument.  Patch below.

  Ralf

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

 arch/mips/ar7/setup.c           | 1 -
 arch/mips/emma/markeins/setup.c | 3 ---
 arch/mips/netlogic/xlp/setup.c  | 1 -
 arch/mips/netlogic/xlr/setup.c  | 1 -
 arch/mips/sibyte/swarm/setup.c  | 2 --
 5 files changed, 8 deletions(-)

diff --git a/arch/mips/ar7/setup.c b/arch/mips/ar7/setup.c
index 9a357ff..820b7a3 100644
--- a/arch/mips/ar7/setup.c
+++ b/arch/mips/ar7/setup.c
@@ -92,7 +92,6 @@ void __init plat_mem_setup(void)
 	_machine_restart = ar7_machine_restart;
 	_machine_halt = ar7_machine_halt;
 	pm_power_off = ar7_machine_power_off;
-	panic_timeout = 3;
 
 	io_base = (unsigned long)ioremap(AR7_REGS_BASE, 0x10000);
 	if (!io_base)
diff --git a/arch/mips/emma/markeins/setup.c b/arch/mips/emma/markeins/setup.c
index d710058..9100122 100644
--- a/arch/mips/emma/markeins/setup.c
+++ b/arch/mips/emma/markeins/setup.c
@@ -111,9 +111,6 @@ void __init plat_mem_setup(void)
 	iomem_resource.start = EMMA2RH_IO_BASE;
 	iomem_resource.end = EMMA2RH_ROM_BASE - 1;
 
-	/* Reboot on panic */
-	panic_timeout = 180;
-
 	markeins_sio_setup();
 }
 
diff --git a/arch/mips/netlogic/xlp/setup.c b/arch/mips/netlogic/xlp/setup.c
index 6d981bb..54e75c7 100644
--- a/arch/mips/netlogic/xlp/setup.c
+++ b/arch/mips/netlogic/xlp/setup.c
@@ -92,7 +92,6 @@ static void __init xlp_init_mem_from_bars(void)
 
 void __init plat_mem_setup(void)
 {
-	panic_timeout	= 5;
 	_machine_restart = (void (*)(char *))nlm_linux_exit;
 	_machine_halt	= nlm_linux_exit;
 	pm_power_off	= nlm_linux_exit;
diff --git a/arch/mips/netlogic/xlr/setup.c b/arch/mips/netlogic/xlr/setup.c
index 214d123..921be5f 100644
--- a/arch/mips/netlogic/xlr/setup.c
+++ b/arch/mips/netlogic/xlr/setup.c
@@ -92,7 +92,6 @@ static void nlm_linux_exit(void)
 
 void __init plat_mem_setup(void)
 {
-	panic_timeout	= 5;
 	_machine_restart = (void (*)(char *))nlm_linux_exit;
 	_machine_halt	= nlm_linux_exit;
 	pm_power_off	= nlm_linux_exit;
diff --git a/arch/mips/sibyte/swarm/setup.c b/arch/mips/sibyte/swarm/setup.c
index 41707a2..3462c83 100644
--- a/arch/mips/sibyte/swarm/setup.c
+++ b/arch/mips/sibyte/swarm/setup.c
@@ -134,8 +134,6 @@ void __init plat_mem_setup(void)
 #error invalid SiByte board configuration
 #endif
 
-	panic_timeout = 5;  /* For debug.  */
-
 	board_be_handler = swarm_be_handler;
 
 	if (xicor_probe())

Re: [PATCH v2] panic: Make panic_timeout configurable

[Shinya Kuribayashi]

On 11/19/13 6:02 PM, Ralf Baechle wrote:> On Mon, Nov 18, 2013 at 09:04:36PM +0000, Jason Baron wrote:
> It's more complicated - MIPS was using the global default with five MIPS
> platforms overriding the default.
>
> I propose to kill these overrides for sanity unless somebody comes up
> with a good argument.  Patch below.
>
>    Ralf
>
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>
>   arch/mips/ar7/setup.c           | 1 -
>   arch/mips/emma/markeins/setup.c | 3 ---
>   arch/mips/netlogic/xlp/setup.c  | 1 -
>   arch/mips/netlogic/xlr/setup.c  | 1 -
>   arch/mips/sibyte/swarm/setup.c  | 2 --
>   5 files changed, 8 deletions(-)
[...]
> diff --git a/arch/mips/emma/markeins/setup.c b/arch/mips/emma/markeins/setup.c
> index d710058..9100122 100644
> --- a/arch/mips/emma/markeins/setup.c
> +++ b/arch/mips/emma/markeins/setup.c
> @@ -111,9 +111,6 @@ void __init plat_mem_setup(void)
>   	iomem_resource.start = EMMA2RH_IO_BASE;
>   	iomem_resource.end = EMMA2RH_ROM_BASE - 1;
>
> -	/* Reboot on panic */
> -	panic_timeout = 180;
> -
>   	markeins_sio_setup();
>   }
>

IIRC we had set it to 180 seconds for some historical reasons, but
I'm afraid nobody can recall the reason why it's set so in 2013...
Anyway I was thinking it too long and reduced to a few seconds locally
when debugging, so there shouldn't be a problem with this change.

FWIW, for EMMA2RH portion:

Acked-by: Shinya Kuribayashi <skuribay@pobox.com>

Thank you always for your help, Ralf.

Re: [PATCH v2] panic: Make panic_timeout configurable

[Ralf Baechle]

On Tue, Nov 19, 2013 at 11:51:27PM +0900, Shinya Kuribayashi wrote:

> IIRC we had set it to 180 seconds for some historical reasons, but
> I'm afraid nobody can recall the reason why it's set so in 2013...
> Anyway I was thinking it too long and reduced to a few seconds locally
> when debugging, so there shouldn't be a problem with this change.

I think one motivation was to have boards reboot automatically avoiding
the need to walk over to the board just powercycle or reset it - let
alone reseting by emailing/phoning an admin!

But that's a development-specific motivation and it shouldn't result
in a hardcoded reboot timeout for everybody.

  Ralf

Re: [PATCH v2] panic: Make panic_timeout configurable

[Jason Baron]

On 11/19/2013 04:02 AM, Ralf Baechle wrote:
> On Mon, Nov 18, 2013 at 09:04:36PM +0000, Jason Baron wrote:
> 
>> The panic_timeout value can be set via the command line option 'panic=x', or via
>> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
>> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
>> so that we can set the desired value from the .config.
>>
>> The default panic_timeout value continues to be 0 - wait forever, except for
>> powerpc and mips, which have been defaulted to 180 and 5 respectively. This
>> is in keeping with the fact that these arches already set panic_timeout in
>> their arch init code. However, I found three exceptions- two in mips and one in
>> powerpc where the settings didn't match these default values. In those cases, I
>> left the arch code so it continues to override, in case the user has not changed
>> from the default. It would nice if these arches had one default value, or if we
>> could determine the correct setting at compile-time.
> 
> It's more complicated - MIPS was using the global default with five MIPS
> platforms overriding the default.
> 
> I propose to kill these overrides for sanity unless somebody comes up
> with a good argument.  Patch below.
> 

And so have the mips default be 0? IE drop the arch/mips/Kconfig bits from
the patch I posted? (Which could of course be configured to a non-zero value
by the user, if desired.)

Thanks,

-Jason

Re: [PATCH v2] panic: Make panic_timeout configurable

[Ralf Baechle]

On Tue, Nov 19, 2013 at 12:11:29PM -0500, Jason Baron wrote:

> > I propose to kill these overrides for sanity unless somebody comes up
> > with a good argument.  Patch below.
> > 
> 
> And so have the mips default be 0? IE drop the arch/mips/Kconfig bits from
> the patch I posted? (Which could of course be configured to a non-zero value
> by the user, if desired.)

Yes.

  Ralf

Re: [PATCH v2] panic: Make panic_timeout configurable

[Jason Baron]

On 11/19/2013 02:09 AM, Ingo Molnar wrote:
> 
> * Jason Baron <jbaron@akamai.com> wrote:
> 
>> On 11/18/2013 05:30 PM, Andrew Morton wrote:
>>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
>>>
>>>> The panic_timeout value can be set via the command line option 'panic=x', or via
>>>> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
>>>> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
>>>> so that we can set the desired value from the .config.
>>>>
>>>> The default panic_timeout value continues to be 0 - wait forever, 
>>>> except for powerpc and mips, which have been defaulted to 180 and 
>>>> 5 respectively. This is in keeping with the fact that these 
>>>> arches already set panic_timeout in their arch init code. 
>>>> However, I found three exceptions- two in mips and one in powerpc 
>>>> where the settings didn't match these default values. In those 
>>>> cases, I left the arch code so it continues to override, in case 
>>>> the user has not changed from the default. It would nice if these 
>>>> arches had one default value, or if we could determine the 
>>>> correct setting at compile-time.
>>>
>>> Felipe is proposing a simpler patch ("panic: setup panic_timeout 
>>> early") which switches to early_param().  Is that sufficient for 
>>> the (undescribed!) failure which you are presumably observing?
>>>
>>
>> No - that patch doesn't change the 'panic_timeout' value until the 
>> call to 'parse_early_param()' is made. If there is a panic before 
>> that point, the param doesn't do anything. The idea of this patch is 
>> to allow it to be configured at build-time.
>>
>> I've tested the patch by simply inserting a panic() call at the 
>> beginning of 'start_kernel()'. So, no I do not have a specific panic 
>> in mind for this.
> 
> Would you be interested in picking up Felipe's patch/fix on top of 
> yours? I was unable to communicate with him efficiently, but I'd take 
> the patch if it's signed off by you.
> 
> Thanks,
> 
> 	Ingo
> 

Sure, I can round up all the related patches in this area that make
sense and re-submit as a series.

Felipe, would the CONFIG_PANIC_TIMEOUT=xx .config parameter work for your
needs, or would you still like to see the command-line processing moved
up?

I'd also like to hear from the PowerPC folks about the arch defaults
there. Now, that mips is ok with CONFIG_PANIC_TIMEOUT, PowerPC is the
only arch doing specific initialization of 'panic_timeout'.

Thanks,

-Jason

Re: [PATCH v2] panic: Make panic_timeout configurable

[Jayachandran C.]

On Tue, Nov 19, 2013 at 10:02:11AM +0100, Ralf Baechle wrote:
> On Mon, Nov 18, 2013 at 09:04:36PM +0000, Jason Baron wrote:
> 
> > The panic_timeout value can be set via the command line option 'panic=x', or via
> > /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
> > before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
> > so that we can set the desired value from the .config.
> > 
> > The default panic_timeout value continues to be 0 - wait forever, except for
> > powerpc and mips, which have been defaulted to 180 and 5 respectively. This
> > is in keeping with the fact that these arches already set panic_timeout in
> > their arch init code. However, I found three exceptions- two in mips and one in
> > powerpc where the settings didn't match these default values. In those cases, I
> > left the arch code so it continues to override, in case the user has not changed
> > from the default. It would nice if these arches had one default value, or if we
> > could determine the correct setting at compile-time.
> 
> It's more complicated - MIPS was using the global default with five MIPS
> platforms overriding the default.
> 
> I propose to kill these overrides for sanity unless somebody comes up
> with a good argument.  Patch below.
> 
>   Ralf
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
>  arch/mips/ar7/setup.c           | 1 -
>  arch/mips/emma/markeins/setup.c | 3 ---
>  arch/mips/netlogic/xlp/setup.c  | 1 -
>  arch/mips/netlogic/xlr/setup.c  | 1 -
>  arch/mips/sibyte/swarm/setup.c  | 2 --
>  5 files changed, 8 deletions(-)
> 
> diff --git a/arch/mips/ar7/setup.c b/arch/mips/ar7/setup.c
> index 9a357ff..820b7a3 100644
> --- a/arch/mips/ar7/setup.c
> +++ b/arch/mips/ar7/setup.c
> @@ -92,7 +92,6 @@ void __init plat_mem_setup(void)
>  	_machine_restart = ar7_machine_restart;
>  	_machine_halt = ar7_machine_halt;
>  	pm_power_off = ar7_machine_power_off;
> -	panic_timeout = 3;
>  
>  	io_base = (unsigned long)ioremap(AR7_REGS_BASE, 0x10000);
>  	if (!io_base)
> diff --git a/arch/mips/emma/markeins/setup.c b/arch/mips/emma/markeins/setup.c
> index d710058..9100122 100644
> --- a/arch/mips/emma/markeins/setup.c
> +++ b/arch/mips/emma/markeins/setup.c
> @@ -111,9 +111,6 @@ void __init plat_mem_setup(void)
>  	iomem_resource.start = EMMA2RH_IO_BASE;
>  	iomem_resource.end = EMMA2RH_ROM_BASE - 1;
>  
> -	/* Reboot on panic */
> -	panic_timeout = 180;
> -
>  	markeins_sio_setup();
>  }
>  
> diff --git a/arch/mips/netlogic/xlp/setup.c b/arch/mips/netlogic/xlp/setup.c
> index 6d981bb..54e75c7 100644
> --- a/arch/mips/netlogic/xlp/setup.c
> +++ b/arch/mips/netlogic/xlp/setup.c
> @@ -92,7 +92,6 @@ static void __init xlp_init_mem_from_bars(void)
>  
>  void __init plat_mem_setup(void)
>  {
> -	panic_timeout	= 5;
>  	_machine_restart = (void (*)(char *))nlm_linux_exit;
>  	_machine_halt	= nlm_linux_exit;
>  	pm_power_off	= nlm_linux_exit;
> diff --git a/arch/mips/netlogic/xlr/setup.c b/arch/mips/netlogic/xlr/setup.c
> index 214d123..921be5f 100644
> --- a/arch/mips/netlogic/xlr/setup.c
> +++ b/arch/mips/netlogic/xlr/setup.c
> @@ -92,7 +92,6 @@ static void nlm_linux_exit(void)
>  
>  void __init plat_mem_setup(void)
>  {
> -	panic_timeout	= 5;
>  	_machine_restart = (void (*)(char *))nlm_linux_exit;
>  	_machine_halt	= nlm_linux_exit;
>  	pm_power_off	= nlm_linux_exit;
> diff --git a/arch/mips/sibyte/swarm/setup.c b/arch/mips/sibyte/swarm/setup.c
> index 41707a2..3462c83 100644
> --- a/arch/mips/sibyte/swarm/setup.c
> +++ b/arch/mips/sibyte/swarm/setup.c
> @@ -134,8 +134,6 @@ void __init plat_mem_setup(void)
>  #error invalid SiByte board configuration
>  #endif
>  
> -	panic_timeout = 5;  /* For debug.  */
> -
>  	board_be_handler = swarm_be_handler;
>  
>  	if (xicor_probe())


Acked-by: Jayachandran C <jchandra@broadcom.com>

For the arch/mips/netlogic changes, thanks for fixing this.

JC.

Re: [PATCH v2] panic: Make panic_timeout configurable

[Michael Ellerman]

On Tue, Nov 19, 2013 at 05:04:14PM -0500, Jason Baron wrote:
> On 11/19/2013 02:09 AM, Ingo Molnar wrote:
> > 
> > * Jason Baron <jbaron@akamai.com> wrote:
> > 
> >> On 11/18/2013 05:30 PM, Andrew Morton wrote:
> >>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
> >>>
> >>>> The panic_timeout value can be set via the command line option 'panic=x', or via
> >>>> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
> >>>> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
> >>>> so that we can set the desired value from the .config.
> >>>>
> >>>> The default panic_timeout value continues to be 0 - wait forever, 
> >>>> except for powerpc and mips, which have been defaulted to 180 and 
> >>>> 5 respectively. This is in keeping with the fact that these 
> >>>> arches already set panic_timeout in their arch init code. 
> >>>> However, I found three exceptions- two in mips and one in powerpc 
> >>>> where the settings didn't match these default values. In those 
> >>>> cases, I left the arch code so it continues to override, in case 
> >>>> the user has not changed from the default. It would nice if these 
> >>>> arches had one default value, or if we could determine the 
> >>>> correct setting at compile-time.
...
> 
> Sure, I can round up all the related patches in this area that make
> sense and re-submit as a series.
> 
> Felipe, would the CONFIG_PANIC_TIMEOUT=xx .config parameter work for your
> needs, or would you still like to see the command-line processing moved
> up?
> 
> I'd also like to hear from the PowerPC folks about the arch defaults
> there. Now, that mips is ok with CONFIG_PANIC_TIMEOUT, PowerPC is the
> only arch doing specific initialization of 'panic_timeout'.

Hi Jason,

I think we'd like to choose the value at runtime, as we do now. The
powerpc arch supports a wide spread of different hardware, so it's nice
to be able to customise the value based on the platform. Also we build a
single kernel that boots on many platforms, and so we can't pick the
value at compile time.

cheers

Re: [PATCH v2] panic: Make panic_timeout configurable

[Jason Baron]

On 11/21/2013 06:16 AM, Michael Ellerman wrote:
> On Tue, Nov 19, 2013 at 05:04:14PM -0500, Jason Baron wrote:
>> On 11/19/2013 02:09 AM, Ingo Molnar wrote:
>>>
>>> * Jason Baron <jbaron@akamai.com> wrote:
>>>
>>>> On 11/18/2013 05:30 PM, Andrew Morton wrote:
>>>>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
>>>>>
>>>>>> The panic_timeout value can be set via the command line option 'panic=x', or via
>>>>>> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
>>>>>> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
>>>>>> so that we can set the desired value from the .config.
>>>>>>
>>>>>> The default panic_timeout value continues to be 0 - wait forever, 
>>>>>> except for powerpc and mips, which have been defaulted to 180 and 
>>>>>> 5 respectively. This is in keeping with the fact that these 
>>>>>> arches already set panic_timeout in their arch init code. 
>>>>>> However, I found three exceptions- two in mips and one in powerpc 
>>>>>> where the settings didn't match these default values. In those 
>>>>>> cases, I left the arch code so it continues to override, in case 
>>>>>> the user has not changed from the default. It would nice if these 
>>>>>> arches had one default value, or if we could determine the 
>>>>>> correct setting at compile-time.
> ...
>>
>> Sure, I can round up all the related patches in this area that make
>> sense and re-submit as a series.
>>
>> Felipe, would the CONFIG_PANIC_TIMEOUT=xx .config parameter work for your
>> needs, or would you still like to see the command-line processing moved
>> up?
>>
>> I'd also like to hear from the PowerPC folks about the arch defaults
>> there. Now, that mips is ok with CONFIG_PANIC_TIMEOUT, PowerPC is the
>> only arch doing specific initialization of 'panic_timeout'.
> 
> Hi Jason,
> 
> I think we'd like to choose the value at runtime, as we do now. The
> powerpc arch supports a wide spread of different hardware, so it's nice
> to be able to customise the value based on the platform. Also we build a
> single kernel that boots on many platforms, and so we can't pick the
> value at compile time.
> 
> cheers
> 

Hi,

Ok, So powerpc sets the timeout to '180' during setup_arch(), but then
overrides the value to '10' only for pSeries. The patch proposed in this
thread, sets the default built-in value for powerpc to 180 and then
continues to override in pSeries, if its still 180 (IE the user hasn't
requested another value). This allows the panic_timeout value to have
an effect before we reach the arch_init() code. Are you ok with this?
That is, are you ok with the proposed powerpc bits?

Or should we drop the powerpc default 180 Kconfig setting, such that
it continues to be 0 until the arch code is run? And in that case
only apply the arch defaults if still 0.

Thanks,

-Jason 

Re: [PATCH v2] panic: Make panic_timeout configurable

[Michael Ellerman]

On Thu, Nov 21, 2013 at 04:21:30PM -0500, Jason Baron wrote:
> On 11/21/2013 06:16 AM, Michael Ellerman wrote:
> > On Tue, Nov 19, 2013 at 05:04:14PM -0500, Jason Baron wrote:
> >> On 11/19/2013 02:09 AM, Ingo Molnar wrote:
> >>>
> >>> * Jason Baron <jbaron@akamai.com> wrote:
> >>>
> >>>> On 11/18/2013 05:30 PM, Andrew Morton wrote:
> >>>>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
> >>>>>
> >>>>>> The panic_timeout value can be set via the command line option 'panic=x', or via
> >>>>>> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
> >>>>>> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
> >>>>>> so that we can set the desired value from the .config.
> >>>>>>
> >>>>>> The default panic_timeout value continues to be 0 - wait forever, 
> >>>>>> except for powerpc and mips, which have been defaulted to 180 and 
> >>>>>> 5 respectively. This is in keeping with the fact that these 
> >>>>>> arches already set panic_timeout in their arch init code. 
> >>>>>> However, I found three exceptions- two in mips and one in powerpc 
> >>>>>> where the settings didn't match these default values. In those 
> >>>>>> cases, I left the arch code so it continues to override, in case 
> >>>>>> the user has not changed from the default. It would nice if these 
> >>>>>> arches had one default value, or if we could determine the 
> >>>>>> correct setting at compile-time.
> > ...
> >>
> >> Sure, I can round up all the related patches in this area that make
> >> sense and re-submit as a series.
> >>
> >> Felipe, would the CONFIG_PANIC_TIMEOUT=xx .config parameter work for your
> >> needs, or would you still like to see the command-line processing moved
> >> up?
> >>
> >> I'd also like to hear from the PowerPC folks about the arch defaults
> >> there. Now, that mips is ok with CONFIG_PANIC_TIMEOUT, PowerPC is the
> >> only arch doing specific initialization of 'panic_timeout'.
> > 
> > Hi Jason,
> > 
> > I think we'd like to choose the value at runtime, as we do now. The
> > powerpc arch supports a wide spread of different hardware, so it's nice
> > to be able to customise the value based on the platform. Also we build a
> > single kernel that boots on many platforms, and so we can't pick the
> > value at compile time.
> 
> Hi,
> 
> Ok, So powerpc sets the timeout to '180' during setup_arch(), but then
> overrides the value to '10' only for pSeries. 

Yep.

> The patch proposed in this thread, sets the default built-in value for
> powerpc to 180 and then continues to override in pSeries, if its
> still 180 (IE the user hasn't requested another value). This allows
> the panic_timeout value to have an effect before we reach the
> arch_init() code. Are you ok with this? That is, are you ok with the
> proposed powerpc bits?

Yes that looks OK to me.

It means we get a non-zero value during early boot, which is nice, the
user can override the value if they wish, and otherwise the existing
behaviour is unchanged.

cheers

Re: [PATCH v2] panic: Make panic_timeout configurable

[Felipe Contreras]

On Tue, Nov 19, 2013 at 1:15 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
>>
>> > The panic_timeout value can be set via the command line option 'panic=x', or via
>> > /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
>> > before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
>> > so that we can set the desired value from the .config.
>> >
>> > The default panic_timeout value continues to be 0 - wait forever, except for
>> > powerpc and mips, which have been defaulted to 180 and 5 respectively. This
>> > is in keeping with the fact that these arches already set panic_timeout in
>> > their arch init code. However, I found three exceptions- two in mips and one in
>> > powerpc where the settings didn't match these default values. In those cases, I
>> > left the arch code so it continues to override, in case the user has not changed
>> > from the default. It would nice if these arches had one default value, or if we
>> > could determine the correct setting at compile-time.
>>
>> Felipe is proposing a simpler patch ("panic: setup panic_timeout
>> early") which switches to early_param().  Is that sufficient for the
>> (undescribed!) failure which you are presumably observing?
>
> Also note that that patch is still incomplete: if panic_timeout is
> switched to early_param() then closely related functionality such as
> pause_on_oops should be moved early as well...

If that was the case, then whomever made the "oops" param generic sent
an incomplete patch because it's also early_param, and it's
interesting why you didn't have that problem with that patch, but you
do with this one.

-- 
Felipe Contreras