Message ID | 8466dffc-d75c-d83a-2a4f-d567721db7d5@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
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
在 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 >
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 >> >
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 --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); }