| Message ID | 20200429170804.880720-1-daniel.thompson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | serial: kgdboc: Allow earlycon initialization to be deferred | expand |
Hi, On Wed, Apr 29, 2020 at 10:08 AM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > As described in the big comment in the patch, earlycon initialization > can be deferred if, a) earlycon was supplied without arguments and, b) > the ACPI SPCR table hasn't yet been parsed. > > Unfortunately, if deferred, then the earlycon is not ready during early > parameter parsing so kgdboc cannot use it. This patch mitigates the > problem by giving kgdboc_earlycon a second chance during > dbg_late_init(). Adding a special purpose interface slightly increase > the intimacy between kgdboc and debug-core but this seems better than > adding kgdb specific hooks into the arch code (and much, much better > than faking non-intimacy with function pointers). > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > --- > > Notes: > Hi Doug, > > This patch extends your patch set to make it easier to deploy on ACPI > systems[1]: > earlycon kgdboc_earlycon kgdboc=ttyAMA0 > > I have mixed feeling about it because it adds calls from debug-core > into kgdboc and I don't think there are other examples of this. > However earlycon auto-configuration is so awesome I'd like to > be able to keep using it and this is the best I have come up with > so far ;-). It's a little gross, but it's OK with me. I guess the other option would be to have "kgdboc_earlycon" try again at various different initcall levels... Speaking of which, I wonder if you could just make kgdboc register to run at "console_initcall" level. If I'm reading it properly: start_kernel() - setup_arch(): ACPI stuff is done by the end of this, right? - console_init(): It would be easy to get called here, I think. - dbg_late_init(): Where you're hooking in now. I didn't put printouts in any code and test it out, but if the above is right then you'll actually get called _earlier_ and with less hackiness if you just have kgdboc try again at console initlevel. > Daniel. > > > [1] And also on DT based arm64 systems that have ACPI support > enabled at compile time because such systems don't decide > whether to adopt DT or ACPI until after early parameter > parsing. > > drivers/tty/serial/kgdboc.c | 26 +++++++++++++++++++++++++- > include/linux/kgdb.h | 2 ++ > kernel/debug/debug_core.c | 4 ++++ > 3 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c > index 7aca0a67fc0b..a7a079ce2c5d 100644 > --- a/drivers/tty/serial/kgdboc.c > +++ b/drivers/tty/serial/kgdboc.c > @@ -509,6 +509,8 @@ static struct kgdb_io kgdboc_earlycon_io_ops = { > .is_console = true, > }; > > +static bool kgdboc_earlycon_late_enable __initdata; > + > static int __init kgdboc_earlycon_init(char *opt) > { > struct console *con; > @@ -529,7 +531,23 @@ static int __init kgdboc_earlycon_init(char *opt) > console_unlock(); > > if (!con) { > - pr_info("Couldn't find kgdb earlycon\n"); > + /* > + * If earlycon deferred its initialization then we also need to > + * do that since there is no console at this point. We will > + * only defer ourselves when kgdboc_earlycon has no arguments. > + * This is because earlycon init is only deferred if there are > + * no arguments to earlycon (we assume that a user who doesn't > + * specify an earlycon driver won't know the right console name > + * to put into kgdboc_earlycon and will let that auto-configure > + * too). > + */ > + if (!kgdboc_earlycon_late_enable && > + earlycon_acpi_spcr_enable && (!opt || !opt[0])) { > + earlycon_kgdboc_late_enable = true; > + pr_info("No suitable earlycon yet, will try later\n"); > + } else { > + pr_info("Couldn't find kgdb earlycon\n"); > + } Personally I'd rather take all the caveats out and just make it generic. Stash the name of the console in a string (you can make it initdata so it doesn't waste any space) and just always retry later if we didn't find the console. Then you don't need to be quite so fragile and if someone else finds another reason to delay earlycon we'll still work. Speaking of which, if we build kgdboc as a module won't you get an error accessing "earlycon_acpi_spcr_enable"? > return 0; > } > > @@ -545,6 +563,12 @@ static int __init kgdboc_earlycon_init(char *opt) > } > > early_param("kgdboc_earlycon", kgdboc_earlycon_init); > + > +void __init kgdb_earlycon_late_init(void) > +{ > + if (kgdboc_earlycon_late_enable) > + earlycon_kgdboc_init(NULL); > +} > #endif /* CONFIG_KGDB_SERIAL_CONSOLE */ > > module_init(init_kgdboc); > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h > index 77a3c519478a..02867a2f0eb4 100644 > --- a/include/linux/kgdb.h > +++ b/include/linux/kgdb.h > @@ -227,6 +227,8 @@ extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt); > extern void kgdb_arch_late(void); > > > +extern void __init kgdb_earlycon_late_init(void); > + It's not required to add "__init" for declarations, is it? > /** > * struct kgdb_arch - Describe architecture specific values. > * @gdb_bpt_instr: The instruction to trigger a breakpoint. > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c > index 2d74dcbca477..f066ef2bc615 100644 > --- a/kernel/debug/debug_core.c > +++ b/kernel/debug/debug_core.c > @@ -963,11 +963,15 @@ void __weak kgdb_arch_late(void) > { > } > > +void __init __weak kgdb_earlycon_late_init(void) > + I assume the above is because "kgdboc" can be compiled as a module and you need to essentially no-op your call in that case? If so, could you add a comment about it? I also would have thought you'd actually need to define the weak function implementation, not just declare it. Maybe I'm confused, though. > void __init dbg_late_init(void) > { > dbg_is_early = false; > if (kgdb_io_module_registered) > kgdb_arch_late(); > + else > + kgdb_earlycon_late_init(); > kdb_init(KDB_INIT_FULL); It feels like it'd be better not to make yourself an "else" but rather to add a 2nd "if" test either at the beginning or the end of this function. I'm 99% sure it makes no difference, but it makes my brain hurt a little trying to prove it because you've added another flow of control to analyze / keep working. Specifically you've now got a case where you're running a bunch of the "debug_core" code where "dbg_is_early = false" but you haven't yet run "KDB_INIT_FULL". Anyway, I don't feel that strongly about it, so if you really like it the way it is that's fine... -Doug
On Wed, Apr 29, 2020 at 05:32:01PM -0700, Doug Anderson wrote: > Hi, > > On Wed, Apr 29, 2020 at 10:08 AM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > As described in the big comment in the patch, earlycon initialization > > can be deferred if, a) earlycon was supplied without arguments and, b) > > the ACPI SPCR table hasn't yet been parsed. > > > > Unfortunately, if deferred, then the earlycon is not ready during early > > parameter parsing so kgdboc cannot use it. This patch mitigates the > > problem by giving kgdboc_earlycon a second chance during > > dbg_late_init(). Adding a special purpose interface slightly increase > > the intimacy between kgdboc and debug-core but this seems better than > > adding kgdb specific hooks into the arch code (and much, much better > > than faking non-intimacy with function pointers). > > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > > --- > > > > Notes: > > Hi Doug, > > > > This patch extends your patch set to make it easier to deploy on ACPI > > systems[1]: > > earlycon kgdboc_earlycon kgdboc=ttyAMA0 > > > > I have mixed feeling about it because it adds calls from debug-core > > into kgdboc and I don't think there are other examples of this. > > However earlycon auto-configuration is so awesome I'd like to > > be able to keep using it and this is the best I have come up with > > so far ;-). > > It's a little gross, but it's OK with me. I guess the other option > would be to have "kgdboc_earlycon" try again at various different > initcall levels... > > Speaking of which, I wonder if you could just make kgdboc register to > run at "console_initcall" level. If I'm reading it properly: > > start_kernel() > - setup_arch(): ACPI stuff is done by the end of this, right? > - console_init(): It would be easy to get called here, I think. > - dbg_late_init(): Where you're hooking in now. > > I didn't put printouts in any code and test it out, but if the above > is right then you'll actually get called _earlier_ and with less > hackiness if you just have kgdboc try again at console initlevel. Thanks, I'll take a look at this. I had a nagging feeling I must be missing something when I gave up and wrote the hack found in this patch. Sounds like I should have paid that feeling closer attention! > > @@ -529,7 +531,23 @@ static int __init kgdboc_earlycon_init(char *opt) > > console_unlock(); > > > > if (!con) { > > - pr_info("Couldn't find kgdb earlycon\n"); > > + /* > > + * If earlycon deferred its initialization then we also need to > > + * do that since there is no console at this point. We will > > + * only defer ourselves when kgdboc_earlycon has no arguments. > > + * This is because earlycon init is only deferred if there are > > + * no arguments to earlycon (we assume that a user who doesn't > > + * specify an earlycon driver won't know the right console name > > + * to put into kgdboc_earlycon and will let that auto-configure > > + * too). > > + */ > > + if (!kgdboc_earlycon_late_enable && > > + earlycon_acpi_spcr_enable && (!opt || !opt[0])) { > > + earlycon_kgdboc_late_enable = true; > > + pr_info("No suitable earlycon yet, will try later\n"); > > + } else { > > + pr_info("Couldn't find kgdb earlycon\n"); > > + } > > Personally I'd rather take all the caveats out and just make it > generic. Stash the name of the console in a string (you can make it > initdata so it doesn't waste any space) and just always retry later if > we didn't find the console. Then you don't need to be quite so > fragile and if someone else finds another reason to delay earlycon > we'll still work. Will do. > Speaking of which, if we build kgdboc as a module won't you get an > error accessing "earlycon_acpi_spcr_enable"? Very likely. I have a note to test this as a module but was curious whether having kgdb_earlycon_late_init() was the right approach anyway. > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h > > index 77a3c519478a..02867a2f0eb4 100644 > > --- a/include/linux/kgdb.h > > +++ b/include/linux/kgdb.h > > @@ -227,6 +227,8 @@ extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt); > > extern void kgdb_arch_late(void); > > > > > > +extern void __init kgdb_earlycon_late_init(void); > > + > > It's not required to add "__init" for declarations, is it? This is just matching styles with the rest of the file (like the extern). Maybe I'll put polishing the header a little on my TODO list. > > * struct kgdb_arch - Describe architecture specific values. > > * @gdb_bpt_instr: The instruction to trigger a breakpoint. > > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c > > index 2d74dcbca477..f066ef2bc615 100644 > > --- a/kernel/debug/debug_core.c > > +++ b/kernel/debug/debug_core.c > > @@ -963,11 +963,15 @@ void __weak kgdb_arch_late(void) > > { > > } > > > > +void __init __weak kgdb_earlycon_late_init(void) > > + > > I assume the above is because "kgdboc" can be compiled as a module and > you need to essentially no-op your call in that case? If so, could > you add a comment about it? I also would have thought you'd actually > need to define the weak function implementation, not just declare it. > Maybe I'm confused, though. Ah... When I rebased this patch on your most recent patchset I did most of the fix ups during the merge. The final few problems I caught *after* the merge and it looks like I neglected to commit them. Sorry... and I'm just relieved you didn't try and compile test this patch! > > void __init dbg_late_init(void) > > { > > dbg_is_early = false; > > if (kgdb_io_module_registered) > > kgdb_arch_late(); > > + else > > + kgdb_earlycon_late_init(); > > kdb_init(KDB_INIT_FULL); > > It feels like it'd be better not to make yourself an "else" but rather > to add a 2nd "if" test either at the beginning or the end of this > function. I'm 99% sure it makes no difference, but it makes my brain > hurt a little trying to prove it because you've added another flow of > control to analyze / keep working. Specifically you've now got a case > where you're running a bunch of the "debug_core" code where > "dbg_is_early = false" but you haven't yet run "KDB_INIT_FULL". > > Anyway, I don't feel that strongly about it, so if you really like it > the way it is that's fine... It is done this way to prevent kgdb_arch_late() being called twice (because I don't want to have to mandate that kgdb_arch_late() is idempotent on every architecture). However I guess a simple alternative would be to call kgdb_earlycon_late_init() *before* setting dbg_is_early to false. Anyhow, I hope you early review comments mean this issue can become irrelevant anyway! Daniel.
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c index 7aca0a67fc0b..a7a079ce2c5d 100644 --- a/drivers/tty/serial/kgdboc.c +++ b/drivers/tty/serial/kgdboc.c @@ -509,6 +509,8 @@ static struct kgdb_io kgdboc_earlycon_io_ops = { .is_console = true, }; +static bool kgdboc_earlycon_late_enable __initdata; + static int __init kgdboc_earlycon_init(char *opt) { struct console *con; @@ -529,7 +531,23 @@ static int __init kgdboc_earlycon_init(char *opt) console_unlock(); if (!con) { - pr_info("Couldn't find kgdb earlycon\n"); + /* + * If earlycon deferred its initialization then we also need to + * do that since there is no console at this point. We will + * only defer ourselves when kgdboc_earlycon has no arguments. + * This is because earlycon init is only deferred if there are + * no arguments to earlycon (we assume that a user who doesn't + * specify an earlycon driver won't know the right console name + * to put into kgdboc_earlycon and will let that auto-configure + * too). + */ + if (!kgdboc_earlycon_late_enable && + earlycon_acpi_spcr_enable && (!opt || !opt[0])) { + earlycon_kgdboc_late_enable = true; + pr_info("No suitable earlycon yet, will try later\n"); + } else { + pr_info("Couldn't find kgdb earlycon\n"); + } return 0; } @@ -545,6 +563,12 @@ static int __init kgdboc_earlycon_init(char *opt) } early_param("kgdboc_earlycon", kgdboc_earlycon_init); + +void __init kgdb_earlycon_late_init(void) +{ + if (kgdboc_earlycon_late_enable) + earlycon_kgdboc_init(NULL); +} #endif /* CONFIG_KGDB_SERIAL_CONSOLE */ module_init(init_kgdboc); diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h index 77a3c519478a..02867a2f0eb4 100644 --- a/include/linux/kgdb.h +++ b/include/linux/kgdb.h @@ -227,6 +227,8 @@ extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt); extern void kgdb_arch_late(void); +extern void __init kgdb_earlycon_late_init(void); + /** * struct kgdb_arch - Describe architecture specific values. * @gdb_bpt_instr: The instruction to trigger a breakpoint. diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c index 2d74dcbca477..f066ef2bc615 100644 --- a/kernel/debug/debug_core.c +++ b/kernel/debug/debug_core.c @@ -963,11 +963,15 @@ void __weak kgdb_arch_late(void) { } +void __init __weak kgdb_earlycon_late_init(void) + void __init dbg_late_init(void) { dbg_is_early = false; if (kgdb_io_module_registered) kgdb_arch_late(); + else + kgdb_earlycon_late_init(); kdb_init(KDB_INIT_FULL); if (kgdb_io_module_registered && kgdb_break_asap)
As described in the big comment in the patch, earlycon initialization can be deferred if, a) earlycon was supplied without arguments and, b) the ACPI SPCR table hasn't yet been parsed. Unfortunately, if deferred, then the earlycon is not ready during early parameter parsing so kgdboc cannot use it. This patch mitigates the problem by giving kgdboc_earlycon a second chance during dbg_late_init(). Adding a special purpose interface slightly increase the intimacy between kgdboc and debug-core but this seems better than adding kgdb specific hooks into the arch code (and much, much better than faking non-intimacy with function pointers). Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> --- Notes: Hi Doug, This patch extends your patch set to make it easier to deploy on ACPI systems[1]: earlycon kgdboc_earlycon kgdboc=ttyAMA0 I have mixed feeling about it because it adds calls from debug-core into kgdboc and I don't think there are other examples of this. However earlycon auto-configuration is so awesome I'd like to be able to keep using it and this is the best I have come up with so far ;-). Daniel. [1] And also on DT based arm64 systems that have ACPI support enabled at compile time because such systems don't decide whether to adopt DT or ACPI until after early parameter parsing. drivers/tty/serial/kgdboc.c | 26 +++++++++++++++++++++++++- include/linux/kgdb.h | 2 ++ kernel/debug/debug_core.c | 4 ++++ 3 files changed, 31 insertions(+), 1 deletion(-) base-commit: 6a8b55ed4056ea5559ebe4f6a4b247f627870d4c prerequisite-patch-id: cbaa70eb783f1f34aec7f5839d1fecbc7616a9f6 prerequisite-patch-id: d7543cdd19fb194ded3361d52818970083efdb06 prerequisite-patch-id: 2238d976451dac9e3ee1bf02a077d633e342aa0c prerequisite-patch-id: 9e4296261b608ee172060d04b3de431a5e370096 prerequisite-patch-id: 2b008e0e14a212072874ecb483d9c6844d161b08 prerequisite-patch-id: f5b692b89c997d828832e3ab27fffb8f770d7b6f prerequisite-patch-id: 851d6f4874aa24540db9d765275ae736e8b2955b prerequisite-patch-id: d3969c2fb7cd320eafebe63d7da270dac5a82fc9 prerequisite-patch-id: e1fc1478b7f75094d263ffc64a9f3528151831cf prerequisite-patch-id: 45fb53996a9f5993e03673c10eebf2834c58307f prerequisite-patch-id: 50ac1ddb52c3cce8b712036f212fdd67d7493112 -- 2.25.1