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@xxxxxxxxxxxx>
> Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> ---
>
> 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
>
