Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 4 May 2020 15:13:15 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Douglas Anderson <dianders@chromium.org>
Cc:     jason.wessel@windriver.com, gregkh@linuxfoundation.org,
        agross@kernel.org, kgdb-bugreport@lists.sourceforge.net,
        catalin.marinas@arm.com, linux-serial@vger.kernel.org,
        sumit.garg@linaro.org, corbet@lwn.net, mingo@redhat.com,
        will@kernel.org, hpa@zytor.com, tglx@linutronix.de,
        frowand.list@gmail.com, bp@alien8.de, bjorn.andersson@linaro.org,
        jslaby@suse.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 07/11] kgdboc: Add kgdboc_earlycon to support early
 kgdb using boot consoles
Message-ID: <20200504141315.oo7uxb6y75l6tv32@holly.lan>
References: <20200428211351.85055-1-dianders@chromium.org>
 <20200428141218.v3.7.I8fba5961bf452ab92350654aa61957f23ecf0100@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200428141218.v3.7.I8fba5961bf452ab92350654aa61957f23ecf0100@changeid>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@chromium.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Sumit Garg <sumit.garg@linaro.org>
> ---
> 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.