[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 interrut
fail to dump. So keep xmon in its original state after exit.

Signed-off-by: Pan Xinhui <redacted>
---
 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);
 }
 
 static struct sysrq_key_op sysrq_xmon_op = {

@@ -3266,7 +3269,7 @@ 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)
 {

-- 
2.4.11

On 15/02/2017 05:49, Pan Xinhui wrote:

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 <redacted>

Patch is fine - minor typo: interrut => interrupt

Feel free to add my:
Tested-by: Guilherme G. Piccoli <redacted>

Thanks,


Guilherme

Pan Xinhui [off-list ref] writes:

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.

cheers

On 16/02/2017 03:09, Michael Ellerman wrote:

Pan Xinhui [off-list ref] writes:

quoted

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 <redacted>
---
 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..

Thanks,


Guilherme

cheers

在 2017/2/16 18:57, Guilherme G. Piccoli 写道:

On 16/02/2017 03:09, Michael Ellerman wrote:

quoted

Pan Xinhui [off-list ref] writes:

quoted

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 <redacted>
---
 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 <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.9.3



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

Pan Xinhui [off-list ref] writes:

I think the logic would probably clearer if we invert this to become
xmon_on.

You've just changed the timing of when xmon gets enabled for the above
two cases, from here which is called very early, to xmon_setup() which
is called much later in boot.

That effectively disables xmon for most of the boot, which we do not
want to do.

cheers

在 2017/2/17 14:05, Michael Ellerman 写道:

Pan Xinhui [off-list ref] writes:

quoted

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);

I think the logic would probably clearer if we invert this to become
xmon_on.

yep, make sense.

quoted

@@ -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;

You've just changed the timing of when xmon gets enabled for the above
two cases, from here which is called very early, to xmon_setup() which
is called much later in boot.

That effectively disables xmon for most of the boot, which we do not
want to do.

Although it is not often that kernel got stucked during boot. Yes, the behavior changed anyway. Will fix that in v3.

cheers

On 02/17/2017 07:30 AM, Pan Xinhui wrote:

在 2017/2/17 14:05, Michael Ellerman 写道:

quoted

Pan Xinhui [off-list ref] writes:

quoted

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);

I think the logic would probably clearer if we invert this to become
xmon_on.

yep, make sense.

quoted

quoted

@@ -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;

You've just changed the timing of when xmon gets enabled for the above
two cases, from here which is called very early, to xmon_setup() which
is called much later in boot.

That effectively disables xmon for most of the boot, which we do not
want to do.

Although it is not often that kernel got stucked during boot. Yes, the behavior changed anyway. Will fix that in v3.

Pan/Michael, I'm working my patches on top of Pan's. So, I sent his V2
on my series, as patch #1.

Guess the workflow is better/easier if we can work the patches on the
series exclusively, since each time Pan's patch is changed, I need to
refactor my patches.

Pan, if possible send your V3 to me, I'll refactor my series on top of
it and send again on the list. Or if you or Michael have better
suggestions of workflow, let me know.

Thanks,


Guilherme

quoted

cheers

Pan Xinhui [off-list ref] writes:

=E5=9C=A8 2017/2/17 14:05, Michael Ellerman =E5=86=99=E9=81=93:

quoted

Pan Xinhui [off-list ref] writes:

quoted

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 =3D 0;
+static int xmon_off =3D !IS_ENABLED(CONFIG_XMON_DEFAULT);

I think the logic would probably clearer if we invert this to become
xmon_on.

yep, make sense.

quoted

quoted

@@ -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) =3D=3D 0) {
  		/* just "xmon" is equivalent to "xmon=3Dearly" */
-		xmon_init(1);
  		xmon_early =3D 1;
+		xmon_off =3D 0;
  	} else if (strncmp(p, "on", 2) =3D=3D 0)
-		xmon_init(1);
+		xmon_off =3D 0;

You've just changed the timing of when xmon gets enabled for the above
two cases, from here which is called very early, to xmon_setup() which
is called much later in boot.

That effectively disables xmon for most of the boot, which we do not
want to do.

Although it is not often that kernel got stucked during boot.

I hope you're joking! :)

cheers