[PATCH] add reg and stack dump to booke WD handler

[PATCH] add reg and stack dump to booke WD handler

[Dave Jiang]

Have the booke watchdog dump some useful information when triggered. Hopefully
that'll give the user some hint of what happened rather than just a mysterious
reboot. 

--

 arch/powerpc/kernel/traps.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index f786222..1af83b7 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1108,9 +1108,42 @@ void unrecoverable_exception(struct pt_regs *regs)
  */
 void __attribute__ ((weak)) WatchdogHandler(struct pt_regs *regs)
 {
+	unsigned long flags;
+
 	/* Generic WatchdogHandler, implement your own */
 	mtspr(SPRN_TCR, mfspr(SPRN_TCR)&(~TCR_WIE));
-	return;
+	oops_enter();
+
+	console_verbose();
+	bust_spinlocks(1);
+	local_save_flags(flags);
+
+	printk(KERN_ERR "PowerPC Book-E Watchdog detected LOCKUP\n");
+	printk(KERN_ERR "Watchdog exception at PC=%lx MSR=%lx\n",
+			regs->nip, regs->msr);
+#ifdef CONFIG_PREEMPT
+	printk(KERN_ERR "PREEMPT ");
+#endif
+#ifdef CONFIG_SMP
+	printk(KERN_ERR "SMP NR_CPUS=%d ", NR_CPUS);
+#endif
+#ifdef CONFIG_DEBUG_PAGEALLOC
+	printk(KERN_ERR "DEBUG_PAGEALLOC ");
+#endif
+#ifdef CONFIG_NUMA
+	printk(KERN_ERR "NUMA ");
+#endif
+	printk(KERN_ERR "%s\n", ppc_md.name ? ppc_md.name : "");
+
+	print_modules();
+	show_regs(regs);
+
+	bust_spinlocks(0);
+
+	printk(KERN_ERR "Wating for hardware watchdog reset...\n");
+	/* spin until hardware reset */
+	while(1)
+		cpu_relax();
 }
 
 void WatchdogException(struct pt_regs *regs)

Re: oops, forgot to sign off

[Dave Jiang]

Have the booke watchdog dump some useful information when triggered. Hopefully
that'll give the user some hint of what happened rather than just a mysterious
reboot.

Signed-off-by: Dave Jiang <djiang@mvista.com>

--

 arch/powerpc/kernel/traps.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index f786222..1af83b7 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1108,9 +1108,42 @@ void unrecoverable_exception(struct pt_regs *regs)
  */
 void __attribute__ ((weak)) WatchdogHandler(struct pt_regs *regs)
 {
+	unsigned long flags;
+
 	/* Generic WatchdogHandler, implement your own */
 	mtspr(SPRN_TCR, mfspr(SPRN_TCR)&(~TCR_WIE));
-	return;
+	oops_enter();
+
+	console_verbose();
+	bust_spinlocks(1);
+	local_save_flags(flags);
+
+	printk(KERN_ERR "PowerPC Book-E Watchdog detected LOCKUP\n");
+	printk(KERN_ERR "Watchdog exception at PC=%lx MSR=%lx\n",
+			regs->nip, regs->msr);
+#ifdef CONFIG_PREEMPT
+	printk(KERN_ERR "PREEMPT ");
+#endif
+#ifdef CONFIG_SMP
+	printk(KERN_ERR "SMP NR_CPUS=%d ", NR_CPUS);
+#endif
+#ifdef CONFIG_DEBUG_PAGEALLOC
+	printk(KERN_ERR "DEBUG_PAGEALLOC ");
+#endif
+#ifdef CONFIG_NUMA
+	printk(KERN_ERR "NUMA ");
+#endif
+	printk(KERN_ERR "%s\n", ppc_md.name ? ppc_md.name : "");
+
+	print_modules();
+	show_regs(regs);
+
+	bust_spinlocks(0);
+
+	printk(KERN_ERR "Wating for hardware watchdog reset...\n");
+	/* spin until hardware reset */
+	while(1)
+		cpu_relax();
 }
 
 void WatchdogException(struct pt_regs *regs)

Re: [PATCH] add reg and stack dump to booke WD handler

[Sergei Shtylyov]

Dave Jiang wrote:
> Have the booke watchdog dump some useful information when triggered. Hopefully
> that'll give the user some hint of what happened rather than just a mysterious
> reboot. 

   Why not do this in drivers/watchdog/booke_wdt.c -- WatchdogHandler() is a weak symbol?
 
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index f786222..1af83b7 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1108,9 +1108,42 @@ void unrecoverable_exception(struct pt_regs *regs)
>   */
>  void __attribute__ ((weak)) WatchdogHandler(struct pt_regs *regs)
>  {
> +	unsigned long flags;
> +
>  	/* Generic WatchdogHandler, implement your own */
>  	mtspr(SPRN_TCR, mfspr(SPRN_TCR)&(~TCR_WIE));
> -	return;
> +	oops_enter();
> +
> +	console_verbose();
> +	bust_spinlocks(1);
> +	local_save_flags(flags);
> +
> +	printk(KERN_ERR "PowerPC Book-E Watchdog detected LOCKUP\n");
> +	printk(KERN_ERR "Watchdog exception at PC=%lx MSR=%lx\n",

   This partly duplicates printk() in WatchdogException().

> +			regs->nip, regs->msr);

  These regs are later dumped by show_regs() anyway.

> +#ifdef CONFIG_PREEMPT
> +	printk(KERN_ERR "PREEMPT ");

   Does specifying loglevel on non '\n' terminated lines really make sense?  I suspect all what you'll get will be ugly output with <3> insterted here and there.

> +#endif
> +#ifdef CONFIG_SMP
> +	printk(KERN_ERR "SMP NR_CPUS=%d ", NR_CPUS);
> +#endif
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> +	printk(KERN_ERR "DEBUG_PAGEALLOC ");
> +#endif
> +#ifdef CONFIG_NUMA
> +	printk(KERN_ERR "NUMA ");
> +#endif
> +	printk(KERN_ERR "%s\n", ppc_md.name ? ppc_md.name : "");
> +
> +	print_modules();
> +	show_regs(regs);
> +
> +	bust_spinlocks(0);
> +
> +	printk(KERN_ERR "Wating for hardware watchdog reset...\n");

   A typo here.

> +	/* spin until hardware reset */
> +	while(1)
> +		cpu_relax();

   Not even giving it a chance? That's cruel. :-)
   Seriously, this seems too specific watchdog behavior to be here.

WBR, Sergei

Re: [PATCH] add reg and stack dump to booke WD handler

[Dave Jiang]

Have the booke watchdog dump some useful information when triggered. Hopefully
that'll give the user some hint of what happened rather than just a mysterious
reboot.

--

 Ok here's try 2. Modified the watchdog driver instead per Sergei's suggestion.

 drivers/char/watchdog/booke_wdt.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/drivers/char/watchdog/booke_wdt.c b/drivers/char/watchdog/booke_wdt.c
index 0e23f29..90e5301 100644
--- a/drivers/char/watchdog/booke_wdt.c
+++ b/drivers/char/watchdog/booke_wdt.c
@@ -23,6 +23,7 @@
 #include <asm/reg_booke.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>
+#include <asm/machdep.h>
 
 /* If the kernel parameter wdt_enable=1, the watchdog will be enabled at boot.
  * Also, the wdt_period sets the watchdog timer period timeout.
@@ -47,6 +48,42 @@ u32 booke_wdt_period = WDT_PERIOD_DEFAULT;
 #define WDTP(x)		(TCR_WP(x))
 #endif
 
+void WatchdogHandler(struct pt_regs *regs)
+{
+	unsigned long flags;
+
+	mtspr(SPRN_TCR, mfspr(SPRN_TCR) & ~TCR_WIE);
+	oops_enter();
+
+	console_verbose();
+	bust_spinlocks(1);
+	local_save_flags(flags);
+
+#ifdef CONFIG_PREEMPT
+	printk("PREEMPT ");
+#endif
+#ifdef CONFIG_SMP
+	printk("SMP NR_CPUS=%d ", NR_CPUS);
+#endif
+#ifdef CONFIG_DEBUG_PAGEALLOC
+	printk("DEBUG_PAGEALLOC ");
+#endif
+#ifdef CONFIG_NUMA
+	printk("NUMA ");
+#endif
+	printk("%s\n", ppc_md.name ? ppc_md.name : "");
+
+	print_modules();
+	show_regs(regs);
+
+	bust_spinlocks(0);
+
+	printk(KERN_EMERG "Waiting for hardware watchdog reset...\n");
+	/* spin until hardware reset */
+	while(1)
+		cpu_relax();
+}
+
 /*
  * booke_wdt_ping:
  */

Re: [PATCH] add reg and stack dump to booke WD handler

[Kumar Gala]

On Apr 13, 2007, at 3:06 PM, Dave Jiang wrote:

> Have the booke watchdog dump some useful information when  
> triggered. Hopefully
> that'll give the user some hint of what happened rather than just a  
> mysterious
> reboot.

Forgot the signed-off-by again (git-commit -s)

> --
>
>  Ok here's try 2. Modified the watchdog driver instead per Sergei's  
> suggestion.

I'm against having a WatchdogHandler in booke_wdt.c.  The idea behind  
the weak symbol in traps.c is to allow platform code to override the  
behavior.  I'm ok if we put the spew in the generic case.

Also, how are you testing this?

- k

>
>  drivers/char/watchdog/booke_wdt.c |   37 ++++++++++++++++++++++++++ 
> +++++++++++
>  1 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/char/watchdog/booke_wdt.c b/drivers/char/ 
> watchdog/booke_wdt.c
> index 0e23f29..90e5301 100644
> --- a/drivers/char/watchdog/booke_wdt.c
> +++ b/drivers/char/watchdog/booke_wdt.c
> @@ -23,6 +23,7 @@
>  #include <asm/reg_booke.h>
>  #include <asm/uaccess.h>
>  #include <asm/system.h>
> +#include <asm/machdep.h>
>
>  /* If the kernel parameter wdt_enable=1, the watchdog will be  
> enabled at boot.
>   * Also, the wdt_period sets the watchdog timer period timeout.
> @@ -47,6 +48,42 @@ u32 booke_wdt_period = WDT_PERIOD_DEFAULT;
>  #define WDTP(x)		(TCR_WP(x))
>  #endif
>
> +void WatchdogHandler(struct pt_regs *regs)
> +{
> +	unsigned long flags;
> +
> +	mtspr(SPRN_TCR, mfspr(SPRN_TCR) & ~TCR_WIE);
> +	oops_enter();
> +
> +	console_verbose();
> +	bust_spinlocks(1);
> +	local_save_flags(flags);
> +
> +#ifdef CONFIG_PREEMPT
> +	printk("PREEMPT ");
> +#endif
> +#ifdef CONFIG_SMP
> +	printk("SMP NR_CPUS=%d ", NR_CPUS);
> +#endif
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> +	printk("DEBUG_PAGEALLOC ");
> +#endif
> +#ifdef CONFIG_NUMA
> +	printk("NUMA ");
> +#endif
> +	printk("%s\n", ppc_md.name ? ppc_md.name : "");
> +
> +	print_modules();
> +	show_regs(regs);
> +
> +	bust_spinlocks(0);
> +
> +	printk(KERN_EMERG "Waiting for hardware watchdog reset...\n");
> +	/* spin until hardware reset */
> +	while(1)
> +		cpu_relax();
> +}
> +
>  /*
>   * booke_wdt_ping:
>   */

Re: [PATCH] add reg and stack dump to booke WD handler

[Dave Jiang]

Kumar Gala wrote:
>
> On Apr 13, 2007, at 3:06 PM, Dave Jiang wrote:
>
>> Have the booke watchdog dump some useful information when triggered. 
>> Hopefully
>> that'll give the user some hint of what happened rather than just a 
>> mysterious
>> reboot.
>
> Forgot the signed-off-by again (git-commit -s)
>
I need to stop doing that! Grrrrr....

>> -- 
>>
>>  Ok here's try 2. Modified the watchdog driver instead per Sergei's 
>> suggestion.
>
> I'm against having a WatchdogHandler in booke_wdt.c.  The idea behind 
> the weak symbol in traps.c is to allow platform code to override the 
> behavior.  I'm ok if we put the spew in the generic case.
>
Ok, I will put it back in the generic handler.

> Also, how are you testing this?
I just stop the userland watchdog daemon and the watchdog timeout kicks 
in. Of course I could also write a kernel module that takes a spinlock 
and spin.... I tested on the MPC8548 and seems to work as expected.

Re: [PATCH] add reg and stack dump to booke WD handler

[Sergei Shtylyov]

Hello.

Kumar Gala wrote:

>> Have the booke watchdog dump some useful information when  triggered. 
>> Hopefully
>> that'll give the user some hint of what happened rather than just a  
>> mysterious
>> reboot.
 
>>  Ok here's try 2. Modified the watchdog driver instead per Sergei's  
>> suggestion.
 
> I'm against having a WatchdogHandler in booke_wdt.c.  The idea behind  
> the weak symbol in traps.c is to allow platform code to override the  
> behavior.

   Hmm, to do what?

>  I'm ok if we put the spew in the generic case.

   Well, I was mostly against putting there while(1) cpu_relax();

> Also, how are you testing this?

   By enabling the watchdog and not then pinging it, I guess...

>>  drivers/char/watchdog/booke_wdt.c |   37 ++++++++++++++++++++++++++ 
>> +++++++++++
>>  1 files changed, 37 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/char/watchdog/booke_wdt.c b/drivers/char/ 
>> watchdog/booke_wdt.c
>> index 0e23f29..90e5301 100644
>> --- a/drivers/char/watchdog/booke_wdt.c
>> +++ b/drivers/char/watchdog/booke_wdt.c
>> @@ -23,6 +23,7 @@
>>  #include <asm/reg_booke.h>
>>  #include <asm/uaccess.h>
>>  #include <asm/system.h>
>> +#include <asm/machdep.h>
>>
>>  /* If the kernel parameter wdt_enable=1, the watchdog will be  
>> enabled at boot.
>>   * Also, the wdt_period sets the watchdog timer period timeout.
>> @@ -47,6 +48,42 @@ u32 booke_wdt_period = WDT_PERIOD_DEFAULT;
>>  #define WDTP(x)        (TCR_WP(x))
>>  #endif
>>
>> +void WatchdogHandler(struct pt_regs *regs)
>> +{
>> +    unsigned long flags;
>> +
>> +    mtspr(SPRN_TCR, mfspr(SPRN_TCR) & ~TCR_WIE);
>> +    oops_enter();
>> +
>> +    console_verbose();
>> +    bust_spinlocks(1);
>> +    local_save_flags(flags);
>> +
>> +#ifdef CONFIG_PREEMPT
>> +    printk("PREEMPT ");
>> +#endif
>> +#ifdef CONFIG_SMP
>> +    printk("SMP NR_CPUS=%d ", NR_CPUS);
>> +#endif
>> +#ifdef CONFIG_DEBUG_PAGEALLOC
>> +    printk("DEBUG_PAGEALLOC ");
>> +#endif
>> +#ifdef CONFIG_NUMA
>> +    printk("NUMA ");
>> +#endif
>> +    printk("%s\n", ppc_md.name ? ppc_md.name : "");
>> +
>> +    print_modules();
>> +    show_regs(regs);
>> +
>> +    bust_spinlocks(0);
>> +
>> +    printk(KERN_EMERG "Waiting for hardware watchdog reset...\n");
>> +    /* spin until hardware reset */
>> +    while(1)
>> +        cpu_relax();
>> +}
>> +
>>  /*
>>   * booke_wdt_ping:
>>   */

WBR, Sergei

Re: [PATCH] add reg and stack dump to booke WD handler

[Dave Jiang]

Have the booke watchdog dump some useful information when triggered.
Hopefully that'll give the user some hint of what happened rather than just a
mysterious reboot.

Signed-off-by: Dave Jiang <djiang@mvista.com>

---

 arch/powerpc/kernel/traps.c |   32 +++++++++++++++++++++++++++++++-
 1 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index f786222..e4d6186 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1108,9 +1108,39 @@ void unrecoverable_exception(struct pt_regs *regs)
  */
 void __attribute__ ((weak)) WatchdogHandler(struct pt_regs *regs)
 {
+	unsigned long flags;
+
 	/* Generic WatchdogHandler, implement your own */
 	mtspr(SPRN_TCR, mfspr(SPRN_TCR)&(~TCR_WIE));
-	return;
+	oops_enter();
+
+	console_verbose();
+	bust_spinlocks(1);
+	local_save_flags(flags);
+
+#ifdef CONFIG_PREEMPT
+	printk("PREEMPT ");
+#endif
+#ifdef CONFIG_SMP
+	printk("SMP NR_CPUS=%d ", NR_CPUS);
+#endif
+#ifdef CONFIG_DEBUG_PAGEALLOC
+	printk("DEBUG_PAGEALLOC ");
+#endif
+#ifdef CONFIG_NUMA
+	printk("NUMA ");
+#endif
+	printk("%s\n", ppc_md.name ? ppc_md.name : "");
+
+	print_modules();
+	show_regs(regs);
+
+	bust_spinlocks(0);
+
+	printk(KERN_EMERG "Waiting for hardware watchdog reset...\n");
+	/* spin until hardware reset */
+	while(1)
+		cpu_relax();
 }
 
 void WatchdogException(struct pt_regs *regs)

Re: [PATCH] add reg and stack dump to booke WD handler

[Paul Mackerras]

Dave Jiang writes:

> Have the booke watchdog dump some useful information when triggered.
> Hopefully that'll give the user some hint of what happened rather than just a
> mysterious reboot.

Why not call either die() or panic()?  Won't they do what you want?

Paul.

Re: [PATCH] add reg and stack dump to booke WD handler

[Dave Jiang]

Paul Mackerras wrote:
> Dave Jiang wrote:
> > Have the booke watchdog dump some useful information when triggered.
> > Hopefully that'll give the user some hint of what happened rather than just a
> > mysterious reboot.
>
> Why not call either die() or panic()?  Won't they do what you want?

I think the main thing I'm worried about is kexec. Both panic and die handler
can potentially call kexec. The watchdog cannot be killed unless a hardware
reset happens. So it is possible that during the boot of the kexec'd kernel the
watchdog forces a hardware reset. In fact, the watchdog can actually make kdump
rather tricky since the current implementation makes it being kept alive via
userland.

I'm thinking that the only way to get around that is to ping the watchdog
consistently in something such as the timer interrupt. But then we probably
have to move the initialization of the driver into core kernel code and not a
char driver. Do we want to do that?

-- 

------------------------------------------------------
Dave Jiang
Software Engineer
MontaVista Software, Inc.
http://www.mvista.com
------------------------------------------------------