Re: [PATCH] powerpc/xmon: Fix an unexpected xmon onoff state change

[Pan Xinhui]

å 2017/2/16 18:57, Guilherme G. Piccoli åé:
> On 16/02/2017 03:09, Michael Ellerman wrote:
> > Pan Xinhui <xinhui.pan@xxxxxxxxxxxxxxxxxx> writes:
> >
> > > 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 interrut
> > > fail to dump. So keep xmon in its original state after exit.
> > >
> > > Signed-off-by: Pan Xinhui <xinhui.pan@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  arch/powerpc/xmon/xmon.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > > index 9c0e17c..721212f 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 = 0;
> > >
> > >  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);
> > >  }
> >
> > I don't think this is right.
> >
> > xmon_off is only true if you boot with xmon=off on the command line.
> >
> > So if you boot with CONFIG_XMON_DEFAULT=n, and nothing on the command
> > line, then enter xmon via sysrq, then exit, xmon will still be enabled.
>
> Agreed, noticed it after some work in V2 of my patch. I'm addressing it
> there, so maybe no harm in keeping this way here..
Hi, mpe
	I cooked a new patch. And as Paul mentioned in slack, we need keep xmon on too if xmon=early is set in cmdline.

hi, Guilherme
	feel free to include it in your new patchset. :)

thanks
xinhui

--------patch-----
powerpc/xmon: Fix an unexpected xmon onoff state change

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. We need someone implement it in the future.
And this value can override those in step 1 and 2.

Signed-off-by: Pan Xinhui <xinhui.pan@xxxxxxxxxxxxxxxxxx>
---
 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.9.3



> Thanks,
>
>
> Guilherme
>
>
>
> > cheers