[PATCH 0/3] powerpc/xmon: improvements and fixes

This series contains some improvements and fixes to xmon:

1) Pan Xinhui fixed a long-term bug, in which the xmon debugger got
stuck enabled after user invoked it using sysrq, regardless of its
state set in the kernel command-line.

2) A debugfs entry was added in order to allow user to enable/disable
xmon without need a kernel reload.

3) The nobt option was dropped and some minor issues were fixed, like
a misplacement of __initdata.

@Pan Xinhui: I did a minor modification in your commit message, feel
free to improve it if you think it isn't ok.

Thanks!


Guilherme G. Piccoli (2):
  powerpc/xmon: drop the nobt option from xmon, yet keeping the
    functionality
  powerpc/xmon: add debugfs entry for xmon

Pan Xinhui (1):
  powerpc/xmon: Fix an unexpected xmon on/off state change

 arch/powerpc/xmon/xmon.c | 61 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 12 deletions(-)

-- 
2.7.4

The xmon parameter nobt was added long time ago, by commit 26c8af5f01df
("[POWERPC] print backtrace when entering xmon"). The problem that time
was that during a crash in a machine with USB keyboard, xmon wouldn't
respond to commands from the keyboard, so printing the backtrace wouldn't
be possible.

Idea then was to show automatically the backtrace on xmon crash for the
first time it's invoked (if it recovers, next time xmon won't show
backtrace automatically). The nobt parameter was added _only_ to prevent
this automatic trace show. Seems long time ago USB keyboards didn't work
that well!

We don't need it anymore, but the feature of auto showing the backtrace
on first crash seems interesting (imagine a case of auto-reboot script),
so this patch keeps the functionality, yet removes the nobt parameter.
Also, this patch fixes __initdata placement on xmon_early and replaces
__initcall() with modern device_initcall() on sysrq handler.

Signed-off-by: Guilherme G. Piccoli <redacted>
---
 arch/powerpc/xmon/xmon.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index f6e5c3d..03f5d65 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c

@@ -185,7 +185,7 @@ static void dump_tlb_44x(void);
 static void dump_tlb_book3e(void);
 #endif
 
-static int xmon_no_auto_backtrace;
+static bool xmon_no_auto_backtrace;
 
 #ifdef CONFIG_PPC64
 #define REG		"%.16lx"

@@ -882,7 +882,7 @@ cmds(struct pt_regs *excp)
 	xmon_regs = excp;
 
 	if (!xmon_no_auto_backtrace) {
-		xmon_no_auto_backtrace = 1;
+		xmon_no_auto_backtrace = true;
 		xmon_show_stack(excp->gpr[1], excp->link, excp->nip);
 	}
 

@@ -3248,11 +3248,17 @@ static void xmon_init(int enable)
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handle_xmon(int key)
 {
+	bool autobt_state = xmon_no_auto_backtrace;
+
+	xmon_no_auto_backtrace = true;
 	/* ensure xmon is enabled */
 	xmon_init(1);
+
 	debugger(get_irq_regs());
 	if (xmon_off)
 		xmon_init(0);
+
+	xmon_no_auto_backtrace = autobt_state;
 }
 
 static struct sysrq_key_op sysrq_xmon_op = {

@@ -3266,10 +3272,10 @@ static int __init setup_xmon_sysrq(void)
 	register_sysrq_key('x', &sysrq_xmon_op);
 	return 0;
 }
-__initcall(setup_xmon_sysrq);
+device_initcall(setup_xmon_sysrq);
 #endif /* CONFIG_MAGIC_SYSRQ */
 
-static int __initdata xmon_early;
+static int xmon_early __initdata;
 
 static int __init early_parse_xmon(char *p)
 {

@@ -3281,8 +3287,6 @@ static int __init early_parse_xmon(char *p)
 		xmon_off = 0;
 	else if (strncmp(p, "off", 3) == 0)
 		xmon_off = 1;
-	else if (strncmp(p, "nobt", 4) == 0)
-		xmon_no_auto_backtrace = 1;
 	else
 		return 1;
 

-- 
2.7.4

Currently the xmon debugger is set only via kernel boot command-line.
It's disabled by default, and can be enabled with "xmon=on" on the
command-line. Also, xmon may be accessed via sysrq mechanism.
But we cannot enable/disable xmon in runtime, it needs kernel reload.

This patch introduces a debugfs entry for xmon, allowing user to query
its current state and change it if desired. Basically, the "xmon" file
to read from/write to is under the debugfs mount point, on powerpc
directory. It's a simple attribute, value 0 meaning xmon is disabled
and value 1 the opposite. Writing these states to the file will take
immediate effect on the debugger.

Signed-off-by: Guilherme G. Piccoli <redacted>
---
v2: dropped the custom parser by using simple attributes [mpe suggestion].

 arch/powerpc/xmon/xmon.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 03f5d65..5f03b3c 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c

@@ -29,6 +29,10 @@
 #include <linux/nmi.h>
 #include <linux/ctype.h>
 
+#ifdef CONFIG_DEBUG_FS
+#include <linux/debugfs.h>
+#endif
+
 #include <asm/ptrace.h>
 #include <asm/string.h>
 #include <asm/prom.h>

@@ -3275,6 +3279,33 @@ static int __init setup_xmon_sysrq(void)
 device_initcall(setup_xmon_sysrq);
 #endif /* CONFIG_MAGIC_SYSRQ */
 
+#ifdef CONFIG_DEBUG_FS
+static int xmon_dbgfs_set(void *data, u64 val)
+{
+	xmon_off = !val;
+	xmon_init(!xmon_off);
+
+	return 0;
+}
+
+static int xmon_dbgfs_get(void *data, u64 *val)
+{
+	*val = !xmon_off;
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(xmon_dbgfs_ops, xmon_dbgfs_get,
+			xmon_dbgfs_set, "%llu\n");
+
+static int __init setup_xmon_dbgfs(void)
+{
+	debugfs_create_file("xmon", 0600, powerpc_debugfs_root, NULL,
+				&xmon_dbgfs_ops);
+	return 0;
+}
+device_initcall(setup_xmon_dbgfs);
+#endif /* CONFIG_DEBUG_FS */
+
 static int xmon_early __initdata;
 
 static int __init early_parse_xmon(char *p)

-- 
2.7.4

From: Pan Xinhui <redacted>

Once xmon is triggered by sysrq-x, it is enabled always afterwards even
if it is disabled during boot. This will cause a system reset interrupt
fail to dump. So keep xmon in its original state after exit.

We have several ways to set xmon on or off.
1) by a build config CONFIG_XMON_DEFAULT.
2) by a boot cmdline with xmon or xmon=early or xmon=on to enable xmon
and xmon=off to disable xmon. This value will override that in step 1.
3) by a debugfs interface, as proposed in this patchset.
And this value can override those in step 1 and 2.

Signed-off-by: Pan Xinhui <redacted>
Signed-off-by: Guilherme G. Piccoli <redacted>
---
 arch/powerpc/xmon/xmon.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 9c0e17c..f6e5c3d 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c

@@ -76,6 +76,7 @@ static int xmon_gate;
 #endif /* CONFIG_SMP */
 
 static unsigned long in_xmon __read_mostly = 0;
+static int xmon_off = !IS_ENABLED(CONFIG_XMON_DEFAULT);
 
 static unsigned long adrs;
 static int size = 1;

@@ -3250,6 +3251,8 @@ static void sysrq_handle_xmon(int key)
 	/* ensure xmon is enabled */
 	xmon_init(1);
 	debugger(get_irq_regs());
+	if (xmon_off)
+		xmon_init(0);
 }
 
 static struct sysrq_key_op sysrq_xmon_op = {

@@ -3266,16 +3269,16 @@ static int __init setup_xmon_sysrq(void)
 __initcall(setup_xmon_sysrq);
 #endif /* CONFIG_MAGIC_SYSRQ */
 
-static int __initdata xmon_early, xmon_off;
+static int __initdata xmon_early;
 
 static int __init early_parse_xmon(char *p)
 {
 	if (!p || strncmp(p, "early", 5) == 0) {
 		/* just "xmon" is equivalent to "xmon=early" */
-		xmon_init(1);
 		xmon_early = 1;
+		xmon_off = 0;
 	} else if (strncmp(p, "on", 2) == 0)
-		xmon_init(1);
+		xmon_off = 0;
 	else if (strncmp(p, "off", 3) == 0)
 		xmon_off = 1;
 	else if (strncmp(p, "nobt", 4) == 0)

@@ -3289,10 +3292,9 @@ early_param("xmon", early_parse_xmon);
 
 void __init xmon_setup(void)
 {
-#ifdef CONFIG_XMON_DEFAULT
-	if (!xmon_off)
-		xmon_init(1);
-#endif
+	if (xmon_off)
+		return;
+	xmon_init(1);
 	if (xmon_early)
 		debugger(NULL);
 }

-- 
2.7.4

New version of the series was sent, please ignore this one.

Thanks,


Guilherme

"Guilherme G. Piccoli" [off-list ref] writes:

Subject: Re: [PATCH 2/3] powerpc/xmon: drop the nobt option from xmon plus minor fixes

In future please use the same version number for all patches of a
series.

ie. This should include a v2, like the rest of the patches in the series.

It confuses the tools to have "v2 1/3" "2/3" "v2 3/3".

I realise that might seem a little odd when a patch is new to the
series, but the version is the version *of the series*, not the
individual patches.

For a new patch you can just add after the change log:

---
v2: New for v2 of the series.


For example.

The xmon parameter nobt was added long time ago, by commit 26c8af5f01df
("[POWERPC] print backtrace when entering xmon"). The problem that time
was that during a crash in a machine with USB keyboard, xmon wouldn't
respond to commands from the keyboard, so printing the backtrace wouldn't
be possible.

Idea then was to show automatically the backtrace on xmon crash for the
first time it's invoked (if it recovers, next time xmon won't show
backtrace automatically). The nobt parameter was added _only_ to prevent
this automatic trace show. Seems long time ago USB keyboards didn't work
that well!

We don't need it anymore, but the feature of auto showing the backtrace
on first crash seems interesting (imagine a case of auto-reboot script),
so this patch keeps the functionality, yet removes the nobt parameter.

I'm going to take this as-is, because I want to get it in for v4.11.

But I don't think we need the auto back trace logic at all. If anything
it's an anti-feature IMHO.

Imagine you're debugging a machine and you drop into xmon to check
something, then drop out again.

Then you go away and leave the box, and it crashes into xmon, but xmon
doesn't print a backtrace because you've already been in xmon. Usually
you can just get on the console and hit 't', but sometimes the machine
crashes so hard that xmon doesn't take input - in which case you now
have no backtrace. :sadface:

So I'll send a follow-up patch to remove the auto backtrace stuff
completely and see if anyone objects.

cheers

Michael Ellerman [off-list ref] writes:

"Guilherme G. Piccoli" [off-list ref] writes:

...

Imagine you're debugging a machine and you drop into xmon to check
something, then drop out again.

Then you go away and leave the box, and it crashes into xmon, but xmon
doesn't print a backtrace because you've already been in xmon. Usually
you can just get on the console and hit 't', but sometimes the machine
crashes so hard that xmon doesn't take input - in which case you now
have no backtrace. :sadface:

OK I read your patch wrong, the above won't happen.

But I still don't think we need any of the auto back trace suppression.

cheers

On 02/21/2017 02:16 AM, Michael Ellerman wrote:

"Guilherme G. Piccoli" [off-list ref] writes:

quoted

Subject: Re: [PATCH 2/3] powerpc/xmon: drop the nobt option from xmon plus minor fixes

In future please use the same version number for all patches of a
series.

ie. This should include a v2, like the rest of the patches in the series.

It confuses the tools to have "v2 1/3" "2/3" "v2 3/3".

I realise that might seem a little odd when a patch is new to the
series, but the version is the version *of the series*, not the
individual patches.

For a new patch you can just add after the change log:

---
v2: New for v2 of the series.


For example.

Sure, thanks for the hint and for explain very well how I should
proceed! Unfortunately...as you probably already noticed, I'm only
seeing this after sent the v2 of the series heheh
Sorry, next time I'll follow your suggestion (TBH I thought of it before
sending this, but I got more inclined in mess with the series numbering
heheh)

quoted

The xmon parameter nobt was added long time ago, by commit 26c8af5f01df
("[POWERPC] print backtrace when entering xmon"). The problem that time
was that during a crash in a machine with USB keyboard, xmon wouldn't
respond to commands from the keyboard, so printing the backtrace wouldn't
be possible.

Idea then was to show automatically the backtrace on xmon crash for the
first time it's invoked (if it recovers, next time xmon won't show
backtrace automatically). The nobt parameter was added _only_ to prevent
this automatic trace show. Seems long time ago USB keyboards didn't work
that well!

We don't need it anymore, but the feature of auto showing the backtrace
on first crash seems interesting (imagine a case of auto-reboot script),
so this patch keeps the functionality, yet removes the nobt parameter.

I'm going to take this as-is, because I want to get it in for v4.11.

But I don't think we need the auto back trace logic at all. If anything
it's an anti-feature IMHO.

Imagine you're debugging a machine and you drop into xmon to check
something, then drop out again.

Then you go away and leave the box, and it crashes into xmon, but xmon
doesn't print a backtrace because you've already been in xmon. Usually
you can just get on the console and hit 't', but sometimes the machine
crashes so hard that xmon doesn't take input - in which case you now
have no backtrace. :sadface:

So I'll send a follow-up patch to remove the auto backtrace stuff
completely and see if anyone objects.

OK, guess you noticed in your next message I kept the trace
behavior...let's discuss there =)

Cheers,


Guilherme

cheers

On 02/21/2017 02:33 AM, Michael Ellerman wrote:

Michael Ellerman [off-list ref] writes:

quoted

"Guilherme G. Piccoli" [off-list ref] writes:

...

quoted

Imagine you're debugging a machine and you drop into xmon to check
something, then drop out again.

Then you go away and leave the box, and it crashes into xmon, but xmon
doesn't print a backtrace because you've already been in xmon. Usually
you can just get on the console and hit 't', but sometimes the machine
crashes so hard that xmon doesn't take input - in which case you now
have no backtrace. :sadface:

OK I read your patch wrong, the above won't happen.

But I still don't think we need any of the auto back trace suppression.

Just to clarify, so do you think we always should print the backtrace
automatically, correctly? I can change it and resend the patch if you want.

Thanks,


Guilherme

cheers