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

[Pan Xinhui <xinhui@linux.vnet.ibm.com>]

在 2017/2/16 18:57, Guilherme G. Piccoli 写道:
> On 16/02/2017 03:09, Michael Ellerman wrote:
>> Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> 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@linux.vnet.ibm.com>
>>> ---
>>>  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@linux.vnet.ibm.com>
---
  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
>>
>