diff mbox

powerpc/xmon: Fix an unexpected xmon onoff state change

Message ID 8466dffc-d75c-d83a-2a4f-d567721db7d5@linux.vnet.ibm.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

xinhui Feb. 16, 2017, 11:51 a.m. UTC 
在 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(-)

Comments

Michael Ellerman Feb. 17, 2017, 6:05 a.m. UTC | #1 
Pan Xinhui <xinhui@linux.vnet.ibm.com> writes:
> 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.

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

cheers
xinhui Feb. 17, 2017, 9:30 a.m. UTC | #2 
在 2017/2/17 14:05, Michael Ellerman 写道:
> Pan Xinhui <xinhui@linux.vnet.ibm.com> writes:
>> 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.

>> @@ -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
>
Guilherme G. Piccoli Feb. 17, 2017, 12:25 p.m. UTC | #3 
On 02/17/2017 07:30 AM, Pan Xinhui wrote:
> 
> 
> 在 2017/2/17 14:05, Michael Ellerman 写道:
>> Pan Xinhui <xinhui@linux.vnet.ibm.com> writes:
>>> 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.
> 
>>> @@ -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

> 
>> cheers
>>
>
Michael Ellerman Feb. 20, 2017, 4:46 a.m. UTC | #4 
Pan Xinhui <xinhui@linux.vnet.ibm.com> writes:

> 在 2017/2/17 14:05, Michael Ellerman 写道:
>> Pan Xinhui <xinhui@linux.vnet.ibm.com> writes:
>>> 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.
>
>>> @@ -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.

I hope you're joking! :)

cheers
diff mbox

Patch

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