From: Daniel Thompson <daniel.thompson@linaro.org>
To: Douglas Anderson <dianders@chromium.org>
Cc: jason.wessel@windriver.com, gregkh@linuxfoundation.org,
kgdb-bugreport@lists.sourceforge.net, mingo@redhat.com,
hpa@zytor.com, bp@alien8.de, linux-serial@vger.kernel.org,
agross@kernel.org, tglx@linutronix.de, frowand.list@gmail.com,
bjorn.andersson@linaro.org, jslaby@suse.com,
catalin.marinas@arm.com, corbet@lwn.net, will@kernel.org,
Arnd Bergmann <arnd@arndb.de>,
linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH v2 6/9] kgdboc: Add earlycon_kgdboc to support early kgdb using boot consoles
Date: Mon, 27 Apr 2020 17:36:44 +0100 [thread overview]
Message-ID: <20200427163644.pyolyoxdxo3u5w6e@holly.lan> (raw)
In-Reply-To: <20200421141234.v2.6.I8fba5961bf452ab92350654aa61957f23ecf0100@changeid>
On Tue, Apr 21, 2020 at 02:14:44PM -0700, Douglas Anderson wrote:
> We want to enable kgdb to debug the early parts of the kernel.
> Unfortunately kgdb normally is a client of the tty API in the kernel
> and serial drivers don't register to the tty layer until fairly late
> in the boot process.
>
> Serial drivers do, however, commonly register a boot console. Let's
> enable the kgdboc driver to work with boot consoles to provide early
> debugging.
>
> This change co-opts the existing read() function pointer that's part
> of "struct console". It's assumed that if a boot console (with the
> flag CON_BOOT) has implemented read() that both the read() and write()
> function are polling functions. That means they work without
> interrupts and read() will return immediately (with 0 bytes read) if
> there's nothing to read. This should be a safe assumption since it
> appears that no current boot consoles implement read() right now and
> there seems no reason to do so unless they wanted to support
> "earlycon_kgdboc".
>
> The console API isn't really intended to have clients work with it
> like we're doing. Specifically there doesn't appear to be any way for
> clients to be notified about a boot console being unregistered. We'll
> work around this by checking that our console is still valid before
> using it. We'll also try to transition off of the boot console and
> onto the "tty" API as quickly as possible.
>
> The normal/expected way to make all this work is to use
> "earlycon_kgdboc" and "kgdboc" together. You should point them both
> to the same physical serial connection. At boot time, as the system
> transitions from the boot console to the normal console, kgdb will
> switch over. If you don't use things in the normal/expected way it's
> a bit of a buyer-beware situation. Things thought about:
>
> - If you specify only "earlycon_kgdboc" but not "kgdboc" you still
> might end up dropping into kgdb upon a crash/sysrq but you may not
> be able to type.
> - If you use "keep_bootcon" (which is already a bit of a buyer-beware
> option) and specify "earlycon_kgdboc" but not "kgdboc" we'll keep
> trying to use your boot console for kgdb.
> - If your "earlycon_kgdboc" and "kgdboc" devices are not the same
> device things should work OK, but it'll be your job to switch over
> which device you're monitoring (including figuring out how to switch
> over gdb in-flight if you're using it).
>
> When trying to enable "earlycon_kgdboc" it should be noted that the
> names that are registered through the boot console layer and the tty
> layer are not the same for the same port. For example when debugging
> on one board I'd need to pass "earlycon_kgdboc=qcom_geni
> kgdboc=ttyMSM0" to enable things properly. Since digging up the boot
> console name is a pain and there will rarely be more than one boot
> console enabled, you can provide the "earlycon_kgdboc" parameter
> without specifying the name of the boot console. In this case we'll
> just pick the first boot that implements read() that we find.
>
> This new "earlycon_kgdboc" parameter should be contrasted to the
> existing "ekgdboc" parameter. While both provide a way to debug very
> early, the usage and mechanisms are quite different. Specifically
> "earlycon_kgdboc" is meant to be used in tandem with "kgdboc" and
> there is a transition from one to the other. The "ekgdboc" parameter,
> on the other hand, replaces the "kgdboc" parameter. It runs the same
> logic as the "kgdboc" parameter but just relies on your TTY driver
> being present super early. The only known usage of the old "ekgdboc"
> parameter is documented as "ekgdboc=kbd earlyprintk=vga". It should
> be noted that "kbd" has special treatment allowing it to init early as
> a tty device.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Again, very happy with the overall approach, just a few quibbles.
> ---
> This patch touches files in several different subsystems, but it
> touches a single line and that line is related to kgdb. I'm assuming
> this can all go through the kgdb tree, but if needed I can always
> introduce a new API call instead of modifying the old one and then
> just have the old API call be a thin wrapper on the new one.
Funny you should say that!
I don't really like that extra argument although it is nothing to do
with simplifying merges...
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 519d8cfbfbed..2f526f2d2bea 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -409,6 +465,80 @@ static int __init kgdboc_early_init(char *opt)
> }
>
> early_param("ekgdboc", kgdboc_early_init);
> +
> +static int earlycon_kgdboc_get_char(void)
> +{
> + char c;
> +
> + if (earlycon_neutered || !earlycon->read(earlycon, &c, 1))
> + return NO_POLL_CHAR;
> +
> + return c;
> +}
> +
> +static void earlycon_kgdboc_put_char(u8 chr)
> +{
> + if (!earlycon_neutered)
> + earlycon->write(earlycon, &chr, 1);
> +}
> +
> +static void earlycon_kgdboc_pre_exp_handler(void)
> +{
> + /*
> + * We don't get notified when the boot console is unregistered.
> + * Double-check when we enter the debugger. Unfortunately we
> + * can't really unregister ourselves now, but at least don't crash.
> + */
> + if (earlycon && !earlycon_neutered && !is_earlycon_still_valid()) {
> + pr_warn("Neutering kgdb since boot console vanished\n");
> + earlycon_neutered = true;
This is, IMHO, too subtle.
I don't think this is merely a warning with a gentle message about
neutering. IIUC the system is (or will shortly be) dead in the water.
After diligently stopping all the CPUs the debug-core will then start
waiting for a character that cannot possibly come!
I think this might be one of those vanishingly rare places where
panicing might actually the right thing to do... although only after
neutering" the kgdb panic handler first ;-).
> + }
> +}
> +
> +static struct kgdb_io earlycon_kgdboc_io_ops = {
> + .name = "earlycon_kgdboc",
> + .read_char = earlycon_kgdboc_get_char,
> + .write_char = earlycon_kgdboc_put_char,
> + .pre_exception = earlycon_kgdboc_pre_exp_handler,
> + .is_console = true,
> +};
> +
> +static int __init earlycon_kgdboc_init(char *opt)
> +{
> + struct console *con;
> +
> + kdb_init(KDB_INIT_EARLY);
This is normally taken care of by debug-core.c . Could this be
integrated into kgdb_register_io_module() ?
> +
> + /*
> + * Look for a matching console, or if the name was left blank just
> + * pick the first one we find.
> + */
> + console_lock();
> + for_each_console(con) {
> + if (con->write && con->read &&
> + (con->flags & (CON_BOOT | CON_ENABLED)) &&
> + (!opt || !opt[0] || strcmp(con->name, opt) == 0))
> + break;
> + }
> + console_unlock();
> +
> + if (!con) {
> + pr_info("Couldn't find kgdb earlycon\n");
> + return 0;
> + }
> +
> + earlycon = con;
> + pr_info("Going to register kgdb with earlycon '%s'\n", con->name);
> + if (kgdb_register_io_module(&earlycon_kgdboc_io_ops, false) != 0) {
> + earlycon = NULL;
> + pr_info("Failed to register kgdb with earlycon\n");
> + return 0;
> + }
> +
> + return 0;
> +}
> +
> +early_param("earlycon_kgdboc", earlycon_kgdboc_init);
> #endif /* CONFIG_KGDB_SERIAL_CONSOLE */
>
> module_init(init_kgdboc);
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 8f178239856d..1b5435c6d92a 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -1074,16 +1074,21 @@ EXPORT_SYMBOL_GPL(kgdb_schedule_breakpoint);
> /**
> * kgdb_register_io_module - register KGDB IO module
> * @new_dbg_io_ops: the io ops vector
> + * @replace: If true it's OK if there were old ops. This is used
> + * to transition from early kgdb to normal kgdb. It's
> + * assumed these are the same device so kgdb can continue.
> *
> * Register it with the KGDB core.
> */
> -int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops)
> +int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops, bool replace)
As I said I'm not a big fan of the extra argument. It makes the call
sites harder to read.
Could earlycon_kgdboc be registered with a boolean flag set so that
a subsequent register will automatically replace the old one
(maybe "is_replaceable" or "is_temporary")?
For bonus marks the core could also enforce that a replaceable io ops
table must have init set to null (because there is no deinit).
Daniel.
next prev parent reply other threads:[~2020-04-27 16:36 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-21 21:14 [PATCH v2 0/9] kgdb: Support late serial drivers; enable early debug w/ boot consoles Douglas Anderson
2020-04-21 21:14 ` [PATCH v2 1/9] kgdb: Disable WARN_CONSOLE_UNLOCKED for all kgdb Douglas Anderson
2020-04-27 13:40 ` Daniel Thompson
2020-04-21 21:14 ` [PATCH v2 2/9] Revert "kgdboc: disable the console lock when in kgdb" Douglas Anderson
2020-04-27 13:41 ` Daniel Thompson
2020-04-21 21:14 ` [PATCH v2 3/9] kgdboc: Use a platform device to handle tty drivers showing up late Douglas Anderson
2020-04-27 14:19 ` Daniel Thompson
2020-04-21 21:14 ` [PATCH v2 4/9] kgdb: Delay "kgdbwait" to dbg_late_init() by default Douglas Anderson
2020-04-27 15:42 ` Daniel Thompson
2020-04-21 21:14 ` [PATCH v2 5/9] arm64: Add call_break_hook() to early_brk64() for early kgdb Douglas Anderson
2020-04-27 16:52 ` Daniel Thompson
2020-04-21 21:14 ` [PATCH v2 6/9] kgdboc: Add earlycon_kgdboc to support early kgdb using boot consoles Douglas Anderson
2020-04-27 16:36 ` Daniel Thompson [this message]
2020-04-28 21:32 ` Doug Anderson
2020-04-21 21:14 ` [PATCH v2 7/9] Documentation: kgdboc: Document new earlycon_kgdboc parameter Douglas Anderson
2020-04-27 16:46 ` Daniel Thompson
2020-04-28 21:32 ` Doug Anderson
2020-04-21 21:14 ` [PATCH v2 8/9] serial: qcom_geni_serial: Support earlycon_kgdboc Douglas Anderson
2020-04-27 16:48 ` Daniel Thompson
2020-04-21 21:14 ` [PATCH v2 9/9] serial: 8250_early: " Douglas Anderson
2020-04-27 16:50 ` Daniel Thompson
2020-04-23 13:50 ` [PATCH v2 0/9] kgdb: Support late serial drivers; enable early debug w/ boot consoles Greg KH
2020-04-24 8:32 ` Sumit Garg
2020-04-24 10:13 ` Daniel Thompson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200427163644.pyolyoxdxo3u5w6e@holly.lan \
--to=daniel.thompson@linaro.org \
--cc=agross@kernel.org \
--cc=arnd@arndb.de \
--cc=bjorn.andersson@linaro.org \
--cc=bp@alien8.de \
--cc=catalin.marinas@arm.com \
--cc=corbet@lwn.net \
--cc=dianders@chromium.org \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=hpa@zytor.com \
--cc=jason.wessel@windriver.com \
--cc=jslaby@suse.com \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).