| Message ID | 1487290677-7200-3-git-send-email-gpiccoli@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
"Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> 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 <mpe@ellerman.id.au> writes: > "Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> 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" <gpiccoli@linux.vnet.ibm.com> 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. 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) > >> 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 <mpe@ellerman.id.au> writes: >> "Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> 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. 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 >
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;
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 <gpiccoli@linux.vnet.ibm.com> --- arch/powerpc/xmon/xmon.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)