linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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
Date: Mon, 4 May 2020 15:13:15 +0100	[thread overview]
Message-ID: <20200504141315.oo7uxb6y75l6tv32@holly.lan> (raw)
In-Reply-To: <20200428141218.v3.7.I8fba5961bf452ab92350654aa61957f23ecf0100@changeid>

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.

  reply	other threads:[~2020-05-04 14:13 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-28 21:13 [PATCH v3 00/11] kgdb: Support late serial drivers; enable early debug w/ boot consoles Douglas Anderson
2020-04-28 21:13 ` [PATCH v3 01/11] kgdb: Disable WARN_CONSOLE_UNLOCKED for all kgdb Douglas Anderson
2020-04-28 21:13 ` [PATCH v3 02/11] Revert "kgdboc: disable the console lock when in kgdb" Douglas Anderson
2020-04-28 21:13 ` [PATCH v3 03/11] kgdboc: Use a platform device to handle tty drivers showing up late Douglas Anderson
2020-04-28 21:13 ` [PATCH v3 04/11] kgdb: Delay "kgdbwait" to dbg_late_init() by default Douglas Anderson
2020-04-30 15:49   ` Daniel Thompson
2020-04-30 16:35     ` Doug Anderson
2020-05-04 14:43       ` [Kgdb-bugreport] " Daniel Thompson
2020-04-28 21:13 ` [PATCH v3 05/11] arm64: Add call_break_hook() to early_brk64() for early kgdb Douglas Anderson
2020-05-11 14:59   ` Will Deacon
2020-05-11 22:45     ` Doug Anderson
2020-05-12  7:35       ` Will Deacon
2020-05-12 15:27         ` Doug Anderson
2020-05-13  6:17           ` Will Deacon
2020-05-13 23:08             ` Doug Anderson
2020-04-28 21:13 ` [PATCH v3 06/11] kgdb: Prevent infinite recursive entries to the debugger Douglas Anderson
2020-04-30 16:52   ` Daniel Thompson
2020-04-28 21:13 ` [PATCH v3 07/11] kgdboc: Add kgdboc_earlycon to support early kgdb using boot consoles Douglas Anderson
2020-05-04 14:13   ` Daniel Thompson [this message]
2020-04-28 21:13 ` [PATCH v3 08/11] Documentation: kgdboc: Document new kgdboc_earlycon parameter Douglas Anderson
2020-05-04 14:40   ` Daniel Thompson
2020-04-28 21:13 ` [PATCH v3 09/11] serial: qcom_geni_serial: Support kgdboc_earlycon Douglas Anderson
2020-04-28 21:13 ` [PATCH v3 10/11] serial: 8250_early: " Douglas Anderson
2020-04-28 21:13 ` [PATCH v3 11/11] serial: amba-pl011: " Douglas Anderson

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=20200504141315.oo7uxb6y75l6tv32@holly.lan \
    --to=daniel.thompson@linaro.org \
    --cc=agross@kernel.org \
    --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=mingo@redhat.com \
    --cc=sumit.garg@linaro.org \
    --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).