From: Daniel Thompson <daniel.thompson@linaro.org>
To: Douglas Anderson <dianders@chromium.org>
Cc: jason.wessel@windriver.com, gregkh@linuxfoundation.org,
corbet@lwn.net, frowand.list@gmail.com,
bjorn.andersson@linaro.org, linux-serial@vger.kernel.org,
mingo@redhat.com, hpa@zytor.com, jslaby@suse.com,
kgdb-bugreport@lists.sourceforge.net, sumit.garg@linaro.org,
will@kernel.org, tglx@linutronix.de, agross@kernel.org,
catalin.marinas@arm.com, bp@alien8.de,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 03/12] kgdboc: Use a platform device to handle tty drivers showing up late
Date: Mon, 18 May 2020 17:46:15 +0100 [thread overview]
Message-ID: <20200518164615.xeqjvlg27evajzdx@holly.lan> (raw)
In-Reply-To: <20200507130644.v4.3.I4a493cfb0f9f740ce8fd2ab58e62dc92d18fed30@changeid>
On Thu, May 07, 2020 at 01:08:41PM -0700, Douglas Anderson wrote:
> If you build CONFIG_KGDB_SERIAL_CONSOLE into the kernel then you
> should be able to have KGDB init itself at bootup by specifying the
> "kgdboc=..." kernel command line parameter. This has worked OK for me
> for many years, but on a new device I switched to it stopped working.
>
> The problem is that on this new device the serial driver gets its
> probe deferred. Now when kgdb initializes it can't find the tty
> driver and when it gives up it never tries again.
>
> We could try to find ways to move up the initialization of the serial
> driver and such a thing might be worthwhile, but it's nice to be
> robust against serial drivers that load late. We could move kgdb to
> init itself later but that penalizes our ability to debug early boot
> code on systems where the driver inits early. We could roll our own
> system of detecting when new tty drivers get loaded and then use that
> to figure out when kgdb can init, but that's ugly.
>
> Instead, let's jump on the -EPROBE_DEFER bandwagon. We'll create a
> singleton instance of a "kgdboc" platform device. If we can't find
> our tty device when the singleton "kgdboc" probes we'll return
> -EPROBE_DEFER which means that the system will call us back later to
> try again when the tty device might be there.
>
> We won't fully transition all of the kgdboc to a platform device
> because early kgdb initialization (via the "ekgdboc" kernel command
> line parameter) still runs before the platform device has been
> created. The kgdb platform device is merely used as a convenient way
> to hook into the system's normal probe deferral mechanisms.
>
> As part of this, we'll ever-so-slightly change how the "kgdboc=..."
> kernel command line parameter works. Previously if you booted up and
> kgdb couldn't find the tty driver then later reading
> '/sys/module/kgdboc/parameters/kgdboc' would return a blank string.
> Now kgdb will keep track of the string that came as part of the
> command line and give it back to you. It's expected that this should
> be an OK change.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> drivers/tty/serial/kgdboc.c | 126 +++++++++++++++++++++++++++++-------
> 1 file changed, 101 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 8a1a4d1b6768..519d8cfbfbed 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -20,6 +20,7 @@
> #include <linux/vt_kern.h>
> #include <linux/input.h>
> #include <linux/module.h>
> +#include <linux/platform_device.h>
>
> #define MAX_CONFIG_LEN 40
>
> @@ -27,6 +28,7 @@ static struct kgdb_io kgdboc_io_ops;
>
> /* -1 = init not run yet, 0 = unconfigured, 1 = configured. */
> static int configured = -1;
> +DEFINE_MUTEX(config_mutex);
Sparse gently pointed out to me that this should be static. Don't
worry about it though... I will fixup.
Daniel.
>
> static char config[MAX_CONFIG_LEN];
> static struct kparam_string kps = {
> @@ -38,6 +40,8 @@ static int kgdboc_use_kms; /* 1 if we use kernel mode switching */
> static struct tty_driver *kgdb_tty_driver;
> static int kgdb_tty_line;
>
> +static struct platform_device *kgdboc_pdev;
> +
> #ifdef CONFIG_KDB_KEYBOARD
> static int kgdboc_reset_connect(struct input_handler *handler,
> struct input_dev *dev,
> @@ -133,11 +137,13 @@ static void kgdboc_unregister_kbd(void)
>
> static void cleanup_kgdboc(void)
> {
> + if (configured != 1)
> + return;
> +
> if (kgdb_unregister_nmi_console())
> return;
> kgdboc_unregister_kbd();
> - if (configured == 1)
> - kgdb_unregister_io_module(&kgdboc_io_ops);
> + kgdb_unregister_io_module(&kgdboc_io_ops);
> }
>
> static int configure_kgdboc(void)
> @@ -198,20 +204,79 @@ static int configure_kgdboc(void)
> kgdb_unregister_io_module(&kgdboc_io_ops);
> noconfig:
> kgdboc_unregister_kbd();
> - config[0] = 0;
> configured = 0;
> - cleanup_kgdboc();
>
> return err;
> }
>
> +static int kgdboc_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> +
> + mutex_lock(&config_mutex);
> + if (configured != 1) {
> + ret = configure_kgdboc();
> +
> + /* Convert "no device" to "defer" so we'll keep trying */
> + if (ret == -ENODEV)
> + ret = -EPROBE_DEFER;
> + }
> + mutex_unlock(&config_mutex);
> +
> + return ret;
> +}
> +
> +static struct platform_driver kgdboc_platform_driver = {
> + .probe = kgdboc_probe,
> + .driver = {
> + .name = "kgdboc",
> + .suppress_bind_attrs = true,
> + },
> +};
> +
> static int __init init_kgdboc(void)
> {
> - /* Already configured? */
> - if (configured == 1)
> + int ret;
> +
> + /*
> + * kgdboc is a little bit of an odd "platform_driver". It can be
> + * up and running long before the platform_driver object is
> + * created and thus doesn't actually store anything in it. There's
> + * only one instance of kgdb so anything is stored as global state.
> + * The platform_driver is only created so that we can leverage the
> + * kernel's mechanisms (like -EPROBE_DEFER) to call us when our
> + * underlying tty is ready. Here we init our platform driver and
> + * then create the single kgdboc instance.
> + */
> + ret = platform_driver_register(&kgdboc_platform_driver);
> + if (ret)
> + return ret;
> +
> + kgdboc_pdev = platform_device_alloc("kgdboc", PLATFORM_DEVID_NONE);
> + if (!kgdboc_pdev) {
> + ret = -ENOMEM;
> + goto err_did_register;
> + }
> +
> + ret = platform_device_add(kgdboc_pdev);
> + if (!ret)
> return 0;
>
> - return configure_kgdboc();
> + platform_device_put(kgdboc_pdev);
> +
> +err_did_register:
> + platform_driver_unregister(&kgdboc_platform_driver);
> + return ret;
> +}
> +
> +static void exit_kgdboc(void)
> +{
> + mutex_lock(&config_mutex);
> + cleanup_kgdboc();
> + mutex_unlock(&config_mutex);
> +
> + platform_device_unregister(kgdboc_pdev);
> + platform_driver_unregister(&kgdboc_platform_driver);
> }
>
> static int kgdboc_get_char(void)
> @@ -234,24 +299,20 @@ static int param_set_kgdboc_var(const char *kmessage,
> const struct kernel_param *kp)
> {
> size_t len = strlen(kmessage);
> + int ret = 0;
>
> if (len >= MAX_CONFIG_LEN) {
> pr_err("config string too long\n");
> return -ENOSPC;
> }
>
> - /* Only copy in the string if the init function has not run yet */
> - if (configured < 0) {
> - strcpy(config, kmessage);
> - return 0;
> - }
> -
> if (kgdb_connected) {
> pr_err("Cannot reconfigure while KGDB is connected.\n");
> -
> return -EBUSY;
> }
>
> + mutex_lock(&config_mutex);
> +
> strcpy(config, kmessage);
> /* Chop out \n char as a result of echo */
> if (len && config[len - 1] == '\n')
> @@ -260,8 +321,30 @@ static int param_set_kgdboc_var(const char *kmessage,
> if (configured == 1)
> cleanup_kgdboc();
>
> - /* Go and configure with the new params. */
> - return configure_kgdboc();
> + /*
> + * Configure with the new params as long as init already ran.
> + * Note that we can get called before init if someone loads us
> + * with "modprobe kgdboc kgdboc=..." or if they happen to use the
> + * the odd syntax of "kgdboc.kgdboc=..." on the kernel command.
> + */
> + if (configured >= 0)
> + ret = configure_kgdboc();
> +
> + /*
> + * If we couldn't configure then clear out the config. Note that
> + * specifying an invalid config on the kernel command line vs.
> + * through sysfs have slightly different behaviors. If we fail
> + * to configure what was specified on the kernel command line
> + * we'll leave it in the 'config' and return -EPROBE_DEFER from
> + * our probe. When specified through sysfs userspace is
> + * responsible for loading the tty driver before setting up.
> + */
> + if (ret)
> + config[0] = '\0';
> +
> + mutex_unlock(&config_mutex);
> +
> + return ret;
> }
>
> static int dbg_restore_graphics;
> @@ -320,15 +403,8 @@ __setup("kgdboc=", kgdboc_option_setup);
> /* This is only available if kgdboc is a built in for early debugging */
> static int __init kgdboc_early_init(char *opt)
> {
> - /* save the first character of the config string because the
> - * init routine can destroy it.
> - */
> - char save_ch;
> -
> kgdboc_option_setup(opt);
> - save_ch = config[0];
> - init_kgdboc();
> - config[0] = save_ch;
> + configure_kgdboc();
> return 0;
> }
>
> @@ -336,7 +412,7 @@ early_param("ekgdboc", kgdboc_early_init);
> #endif /* CONFIG_KGDB_SERIAL_CONSOLE */
>
> module_init(init_kgdboc);
> -module_exit(cleanup_kgdboc);
> +module_exit(exit_kgdboc);
> module_param_call(kgdboc, param_set_kgdboc_var, param_get_string, &kps, 0644);
> MODULE_PARM_DESC(kgdboc, "<serial_device>[,baud]");
> MODULE_DESCRIPTION("KGDB Console TTY Driver");
> --
> 2.26.2.645.ge9eca65c58-goog
>
next prev parent reply other threads:[~2020-05-18 16:46 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-07 20:08 [PATCH v4 00/12] kgdb: Support late serial drivers; enable early debug w/ boot consoles Douglas Anderson
2020-05-07 20:08 ` [PATCH v4 01/12] kgdb: Disable WARN_CONSOLE_UNLOCKED for all kgdb Douglas Anderson
2020-05-07 20:08 ` [PATCH v4 02/12] Revert "kgdboc: disable the console lock when in kgdb" Douglas Anderson
2020-05-07 20:08 ` [PATCH v4 03/12] kgdboc: Use a platform device to handle tty drivers showing up late Douglas Anderson
2020-05-18 16:46 ` Daniel Thompson [this message]
2020-05-07 20:08 ` [PATCH v4 04/12] kgdb: Delay "kgdbwait" to dbg_late_init() by default Douglas Anderson
2020-05-15 16:18 ` Daniel Thompson
2020-05-07 20:08 ` [PATCH v4 05/12] arm64: Add call_break_hook() to early_brk64() for early kgdb Douglas Anderson
2020-05-13 23:12 ` Doug Anderson
2020-05-07 20:08 ` [PATCH v4 06/12] kgdb: Prevent infinite recursive entries to the debugger Douglas Anderson
2020-05-07 20:08 ` [PATCH v4 07/12] kgdboc: Remove useless #ifdef CONFIG_KGDB_SERIAL_CONSOLE in kgdboc Douglas Anderson
2020-05-11 16:02 ` Daniel Thompson
2020-05-07 20:08 ` [PATCH v4 08/12] kgdboc: Add kgdboc_earlycon to support early kgdb using boot consoles Douglas Anderson
2020-05-07 20:08 ` [PATCH v4 09/12] Documentation: kgdboc: Document new kgdboc_earlycon parameter Douglas Anderson
2020-05-07 20:08 ` [PATCH v4 10/12] serial: qcom_geni_serial: Support kgdboc_earlycon Douglas Anderson
2020-05-07 20:08 ` [PATCH v4 11/12] serial: 8250_early: " Douglas Anderson
2020-05-07 20:08 ` [PATCH v4 12/12] serial: amba-pl011: " Douglas Anderson
2020-05-14 16:21 ` [PATCH v4 00/12] kgdb: Support late serial drivers; enable early debug w/ boot consoles Daniel Thompson
2020-05-14 16:34 ` Doug Anderson
2020-05-14 16:36 ` Greg Kroah-Hartman
2020-05-19 10:37 ` Daniel Thompson
2020-05-19 10:36 ` 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=20200518164615.xeqjvlg27evajzdx@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).