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