[PATCH] serial: kgdboc: Allow earlycon initialization to be deferred

[PATCH] serial: kgdboc: Allow earlycon initialization to be deferred

[Daniel Thompson]

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(-)

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)

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

Re: [PATCH] serial: kgdboc: Allow earlycon initialization to be deferred

[Doug Anderson]

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

Re: [PATCH] serial: kgdboc: Allow earlycon initialization to be deferred

[Daniel Thompson]

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.

[PATCH v2] serial: kgdboc: Allow earlycon initialization to be deferred

[Daniel Thompson]

Currently there is no guarantee that an earlycon will be initialized
before kgdboc tries to adopt it. Almost the opposite: on systems
with ACPI then if earlycon has no arguments then it is guaranteed that
earlycon will not be initialized.

This patch mitigates the problem by giving kgdboc_earlycon a second
chance during console_init(). This isn't quite as good as stopping during
early parameter parsing but it is still early in the kernel boot.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---

Notes:
    v2: Simplified, more robust, runs earlier, still has Doug's
        recent patchset as a prerequisite. What's not to like?
    
    More specifically, based on feedback from Doug Anderson, I
    have replaced the initial hacky implementation with a console
    initcall.
    
    I also made it defer more aggressively after realizing that both
    earlycon and kgdboc_earlycon are handled as early parameters
    (meaning I think the current approach relies on the ordering
    of drivers/tty/serial/Makefile to ensure the earlycon is enabled
    before kgdboc tries to adopt it).
    
    Finally, my apologies to Jason and kgdb ML folks, who are seeing
    this patch for the first time. I copied the original circulation
    list from a patch that wasn't kgdb related and forgot to update.

 drivers/tty/serial/kgdboc.c | 41 +++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 7aca0a67fc0b..596213272ec3 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -509,6 +509,10 @@ static struct kgdb_io kgdboc_earlycon_io_ops = {
 	.is_console		= true,
 };

+#define MAX_CONSOLE_NAME_LEN (sizeof((struct console *) 0)->name)
+static char kgdboc_earlycon_param[MAX_CONSOLE_NAME_LEN] __initdata;
+static bool kgdboc_earlycon_late_enable __initdata;
+
 static int __init kgdboc_earlycon_init(char *opt)
 {
 	struct console *con;
@@ -529,7 +533,24 @@ static int __init kgdboc_earlycon_init(char *opt)
 	console_unlock();

 	if (!con) {
-		pr_info("Couldn't find kgdb earlycon\n");
+		/*
+		 * Both earlycon and kgdboc_earlycon are initialized during
+		 * early parameter parsing. We cannot guarantee earlycon gets
+		 * in first and, in any case, on ACPI systems earlycon may
+		 * defer its own initialization (usually to somewhere within
+		 * setup_arch() ). To cope with either of these situations
+		 * we can defer our own initialization to a little later in
+		 * the boot.
+		 */
+		if (!kgdboc_earlycon_late_enable) {
+			pr_info("No suitable earlycon yet, will try later\n");
+			if (opt)
+				strscpy(kgdboc_earlycon_param, opt,
+					sizeof(kgdboc_earlycon_param));
+			kgdboc_earlycon_late_enable = true;
+		} else {
+			pr_info("Couldn't find kgdb earlycon\n");
+		}
 		return 0;
 	}

@@ -543,8 +564,24 @@ static int __init kgdboc_earlycon_init(char *opt)

 	return 0;
 }
-
 early_param("kgdboc_earlycon", kgdboc_earlycon_init);
+
+/*
+ * This is only intended for the late adoption of an early console.
+ *
+ * It is not a reliable way to adopt regular consoles because
+ * we can not control what order console initcalls are made and
+ * many regular consoles are registered much later in the boot
+ * process than the console initcalls!
+ */
+static int __init kgdboc_earlycon_late_init(void)
+{
+	if (kgdboc_earlycon_late_enable)
+		kgdboc_earlycon_init(kgdboc_earlycon_param);
+	return 0;
+}
+console_initcall(kgdboc_earlycon_late_init);
+
 #endif /* CONFIG_KGDB_SERIAL_CONSOLE */

 module_init(init_kgdboc);

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

Re: [PATCH v2] serial: kgdboc: Allow earlycon initialization to be deferred

[Doug Anderson]

Hi,

On Thu, Apr 30, 2020 at 9:18 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> Currently there is no guarantee that an earlycon will be initialized
> before kgdboc tries to adopt it. Almost the opposite: on systems
> with ACPI then if earlycon has no arguments then it is guaranteed that
> earlycon will not be initialized.
>
> This patch mitigates the problem by giving kgdboc_earlycon a second
> chance during console_init(). This isn't quite as good as stopping during
> early parameter parsing but it is still early in the kernel boot.
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>
> Notes:
>     v2: Simplified, more robust, runs earlier, still has Doug's
>         recent patchset as a prerequisite. What's not to like?
>
>     More specifically, based on feedback from Doug Anderson, I
>     have replaced the initial hacky implementation with a console
>     initcall.
>
>     I also made it defer more aggressively after realizing that both
>     earlycon and kgdboc_earlycon are handled as early parameters
>     (meaning I think the current approach relies on the ordering
>     of drivers/tty/serial/Makefile to ensure the earlycon is enabled
>     before kgdboc tries to adopt it).
>
>     Finally, my apologies to Jason and kgdb ML folks, who are seeing
>     this patch for the first time. I copied the original circulation
>     list from a patch that wasn't kgdb related and forgot to update.
>
>  drivers/tty/serial/kgdboc.c | 41 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)

Thanks, this looks great!

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2] serial: kgdboc: Allow earlycon initialization to be deferred

[Doug Anderson]

Hi,

On Thu, Apr 30, 2020 at 9:47 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Apr 30, 2020 at 9:18 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > Currently there is no guarantee that an earlycon will be initialized
> > before kgdboc tries to adopt it. Almost the opposite: on systems
> > with ACPI then if earlycon has no arguments then it is guaranteed that
> > earlycon will not be initialized.
> >
> > This patch mitigates the problem by giving kgdboc_earlycon a second
> > chance during console_init(). This isn't quite as good as stopping during
> > early parameter parsing but it is still early in the kernel boot.
> >
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > ---
> >
> > Notes:
> >     v2: Simplified, more robust, runs earlier, still has Doug's
> >         recent patchset as a prerequisite. What's not to like?
> >
> >     More specifically, based on feedback from Doug Anderson, I
> >     have replaced the initial hacky implementation with a console
> >     initcall.
> >
> >     I also made it defer more aggressively after realizing that both
> >     earlycon and kgdboc_earlycon are handled as early parameters
> >     (meaning I think the current approach relies on the ordering
> >     of drivers/tty/serial/Makefile to ensure the earlycon is enabled
> >     before kgdboc tries to adopt it).
> >
> >     Finally, my apologies to Jason and kgdb ML folks, who are seeing
> >     this patch for the first time. I copied the original circulation
> >     list from a patch that wasn't kgdb related and forgot to update.
> >
> >  drivers/tty/serial/kgdboc.c | 41 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 39 insertions(+), 2 deletions(-)
>
> Thanks, this looks great!
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Are you planning to rebase this patch atop what landed?  It seems like
a useful feature.  If you want me to give a shot a rebasing, let me
know!

-Doug

Re: [PATCH v2] serial: kgdboc: Allow earlycon initialization to be deferred

[Daniel Thompson]

On Thu, May 21, 2020 at 10:18:10AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 30, 2020 at 9:47 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Thu, Apr 30, 2020 at 9:18 AM Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > >
> > > Currently there is no guarantee that an earlycon will be initialized
> > > before kgdboc tries to adopt it. Almost the opposite: on systems
> > > with ACPI then if earlycon has no arguments then it is guaranteed that
> > > earlycon will not be initialized.
> > >
> > > This patch mitigates the problem by giving kgdboc_earlycon a second
> > > chance during console_init(). This isn't quite as good as stopping during
> > > early parameter parsing but it is still early in the kernel boot.
> > >
> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > ---
> > >
> > > Notes:
> > >     v2: Simplified, more robust, runs earlier, still has Doug's
> > >         recent patchset as a prerequisite. What's not to like?
> > >
> > >     More specifically, based on feedback from Doug Anderson, I
> > >     have replaced the initial hacky implementation with a console
> > >     initcall.
> > >
> > >     I also made it defer more aggressively after realizing that both
> > >     earlycon and kgdboc_earlycon are handled as early parameters
> > >     (meaning I think the current approach relies on the ordering
> > >     of drivers/tty/serial/Makefile to ensure the earlycon is enabled
> > >     before kgdboc tries to adopt it).
> > >
> > >     Finally, my apologies to Jason and kgdb ML folks, who are seeing
> > >     this patch for the first time. I copied the original circulation
> > >     list from a patch that wasn't kgdb related and forgot to update.
> > >
> > >  drivers/tty/serial/kgdboc.c | 41 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 39 insertions(+), 2 deletions(-)
> >
> > Thanks, this looks great!
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> Are you planning to rebase this patch atop what landed?  It seems like
> a useful feature.  If you want me to give a shot a rebasing, let me
> know!

I've got it on it's way...


Daniel.