On Tue, Apr 28, 2020 at 02:13:47PM -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
> "kgdboc_earlycon".
>
> 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
> "kgdboc_earlycon" 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 "kgdboc_earlycon" but not "kgdboc" and the boot
> console vanishes at a weird time we'll panic if someone tries to
> drop into kgdb.
> - If you use "keep_bootcon" (which is already a bit of a buyer-beware
> option) and specify "kgdboc_earlycon" but not "kgdboc" we'll keep
> trying to use your boot console for kgdb.
> - If your "kgdboc_earlycon" 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).
As mentioned in other threads. If we are changing the way we manage the
lifetime of the consoles I think it would be good to squash that change
down and simplify some of these cases.
> When trying to enable "kgdboc_earlycon" 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 "kgdboc_earlycon=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 "kgdboc_earlycon" 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 "kgdboc_earlycon" 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
> "kgdboc_earlycon" 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@xxxxxxxxxxxx>
> Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Tested-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> ---
> I have kept Greg's Reviewed-by and Sumit's Tested-by tags on this
> commit despite changes that aren't totally trivial. Please yell if
> you disagree with this. Reasons:
> - Greg's Reviewed-by seemed more an overall acknowledgment that the
> series wasn't totally insane rather than a detailed review. I don't
> think the changes from v2 to v3 change that.
> - Sumit's Tested-by seemed useful as confirmation that someone else
> made this work on a machine that wasn't mine. I don't believe that
> the changes from v2 to v3 should affect anything here.
>
> Changes in v3:
> - Add deinit() to I/O ops to know a driver can be replaced.
> - Don't just neuter input, panic if earlycon vanishes.
> - No extra param to kgdb_register_io_module().
> - Renamed earlycon_kgdboc to kgdboc_earlycon.
> - Simplify earlycon_kgdb deinit by using the deinit() function.
>
> Changes in v2:
> - Assumes we have ("kgdb: Disable WARN_CONSOLE_UNLOCKED for all kgdb")
> - Fix kgdbts, tty/mips_ejtag_fdc, and usb/early/ehci-dbgp
>
> drivers/tty/serial/kgdboc.c | 136 ++++++++++++++++++++++++++++++++++++
> include/linux/kgdb.h | 4 ++
> kernel/debug/debug_core.c | 23 ++++--
> 3 files changed, 159 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 519d8cfbfbed..7aca0a67fc0b 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -21,6 +21,7 @@
> #include <linux/input.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> +#include <linux/serial_core.h>
>
> #define MAX_CONFIG_LEN 40
>
> @@ -42,6 +43,13 @@ static int kgdb_tty_line;
>
> static struct platform_device *kgdboc_pdev;
>
> +#ifdef CONFIG_KGDB_SERIAL_CONSOLE
Isn't this always set for this file (see Makefile)?
I think all the instances of this check (and the diligent
#else clauses are redundant).
> +static struct kgdb_io kgdboc_earlycon_io_ops;
> +struct console *earlycon;
static?
> <snip>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index b072aeb1fd78..77a3c519478a 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -1075,15 +1075,21 @@ EXPORT_SYMBOL_GPL(kgdb_schedule_breakpoint);
> */
> int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops)
> {
> + struct kgdb_io *old_dbg_io_ops;
> int err;
>
> spin_lock(&kgdb_registration_lock);
>
> - if (dbg_io_ops) {
> - spin_unlock(&kgdb_registration_lock);
> + old_dbg_io_ops = dbg_io_ops;
> + if (old_dbg_io_ops) {
> + if (!old_dbg_io_ops->deinit) {
> + spin_unlock(&kgdb_registration_lock);
>
> - pr_err("Another I/O driver is already registered with KGDB\n");
> - return -EBUSY;
> + pr_err("KGDB I/O driver %s can't replace %s.\n",
> + new_dbg_io_ops->name, old_dbg_io_ops->name);
> + return -EBUSY;
> + }
> + old_dbg_io_ops->deinit();
> }
>
> if (new_dbg_io_ops->init) {
> @@ -1098,6 +1104,12 @@ int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops)
>
> spin_unlock(&kgdb_registration_lock);
>
> + if (old_dbg_io_ops) {
> + pr_info("Replaced I/O driver %s with %s\n",
> + old_dbg_io_ops->name, new_dbg_io_ops->name);
I know that causes no trouble for the current deinit() method does but
I'd be more comfortable if the core printed this before calling deinit()?
Daniel.
