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

Message ID 20200430161741.1832050-1-daniel.thompson@linaro.org
State Accepted
Commit a4912303ac6fede434acf5c23a9108cdaf79844a
Headers show
Series
  • [v2] serial: kgdboc: Allow earlycon initialization to be deferred
Related show

Commit Message

Daniel Thompson April 30, 2020, 4:17 p.m.
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(-)


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

Comments

Doug Anderson April 30, 2020, 4:47 p.m. | #1
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>
Doug Anderson May 21, 2020, 5:18 p.m. | #2
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
Daniel Thompson May 22, 2020, 3:30 p.m. | #3
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.

Patch

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