[PATCH] tty: kgdb_nmi: Convert kgdb_nmi_tty_enabled into a module parameter

[PATCH] tty: kgdb_nmi: Convert kgdb_nmi_tty_enabled into a module parameter

[Daniel Thompson]

At present it is not possible to boot with the ttyNMI0 console treating
character input normally. To use the console requires that kdb be
entered and the nmi_console command be used to enable the console (or if
only kgdb is present then gdb must directly manipulate the value of
kgdb_nmi_tty_enabled).

Introducing a module parameter makes the console much more usable.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/kgdb_nmi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index cfadf29..9361d69 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -43,6 +43,11 @@ module_param_named(magic, kgdb_nmi_magic, charp, 0600);
 MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
 
 static bool kgdb_nmi_tty_enabled;
+module_param_named(tty, kgdb_nmi_tty_enabled, bool, 0600);
+MODULE_PARM_DESC(tty, "if set to false (default), characters received from "
+		      "the UART will be passed exclusively to the knock "
+		      "detector; when set to true characters will be passed "
+		      "both to the knock detector and to the TTY layer");
 
 static int kgdb_nmi_console_setup(struct console *co, char *options)
 {
-- 
1.9.3

Re: [PATCH] tty: kgdb_nmi: Convert kgdb_nmi_tty_enabled into a module parameter

[Greg Kroah-Hartman]

On Fri, Jul 11, 2014 at 01:30:11PM +0100, Daniel Thompson wrote:
> At present it is not possible to boot with the ttyNMI0 console treating
> character input normally. To use the console requires that kdb be
> entered and the nmi_console command be used to enable the console (or if
> only kgdb is present then gdb must directly manipulate the value of
> kgdb_nmi_tty_enabled).
> 
> Introducing a module parameter makes the console much more usable.
> 
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/kgdb_nmi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
> index cfadf29..9361d69 100644
> --- a/drivers/tty/serial/kgdb_nmi.c
> +++ b/drivers/tty/serial/kgdb_nmi.c
> @@ -43,6 +43,11 @@ module_param_named(magic, kgdb_nmi_magic, charp, 0600);
>  MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
>  
>  static bool kgdb_nmi_tty_enabled;
> +module_param_named(tty, kgdb_nmi_tty_enabled, bool, 0600);
> +MODULE_PARM_DESC(tty, "if set to false (default), characters received from "
> +		      "the UART will be passed exclusively to the knock "
> +		      "detector; when set to true characters will be passed "
> +		      "both to the knock detector and to the TTY layer");

Module options suck rocks through straws.

Isn't there _any_ other way to determine this "dynamically"?  How will
someone know to set this option?

Yeah, there are other options in this module, but please, why add more?

greg k-h

Re: [PATCH] tty: kgdb_nmi: Convert kgdb_nmi_tty_enabled into a module parameter

[Daniel Thompson]

On 12/07/14 01:47, Greg Kroah-Hartman wrote:
> On Fri, Jul 11, 2014 at 01:30:11PM +0100, Daniel Thompson wrote:
>> At present it is not possible to boot with the ttyNMI0 console treating
>> character input normally. To use the console requires that kdb be
>> entered and the nmi_console command be used to enable the console (or if
>> only kgdb is present then gdb must directly manipulate the value of
>> kgdb_nmi_tty_enabled).
>>
>> Introducing a module parameter makes the console much more usable.
>>
>> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Jiri Slaby <jslaby@suse.cz>
>> Cc: linux-serial@vger.kernel.org
>> ---
>>  drivers/tty/serial/kgdb_nmi.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
>> index cfadf29..9361d69 100644
>> --- a/drivers/tty/serial/kgdb_nmi.c
>> +++ b/drivers/tty/serial/kgdb_nmi.c
>> @@ -43,6 +43,11 @@ module_param_named(magic, kgdb_nmi_magic, charp, 0600);
>>  MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
>>  
>>  static bool kgdb_nmi_tty_enabled;
>> +module_param_named(tty, kgdb_nmi_tty_enabled, bool, 0600);
>> +MODULE_PARM_DESC(tty, "if set to false (default), characters received from "
>> +		      "the UART will be passed exclusively to the knock "
>> +		      "detector; when set to true characters will be passed "
>> +		      "both to the knock detector and to the TTY layer");
> 
> Module options suck rocks through straws.
> 
> Isn't there _any_ other way to determine this "dynamically"?  How will
> someone know to set this option?
> 
> Yeah, there are other options in this module, but please, why add more?

Thanks for the comment.

Yes. I think we can do this dynamically.

The two modes currently offered are:

1. characters bypass TTY layer and knock detector interactively prompts
   for the knock ('Press $3#33 to enter debugger>')

2. characters are routed to TTY layer and to a (silenced) knock
   detector

I think we might be able to select modes based on whether there is a
userspace process reading from the tty (i.e. if there is a getty then
act like a proper tty, if there is no getty allow prompting).

[PATCH v2] tty: kgdb_nmi: Automatically manage tty enable

[Daniel Thompson]

At present it is not possible to boot with the ttyNMI0 console treating
character input normally, instead character input triggers a prompt
telling the user how to trigger the knock detector and enter the
debugger. To use the console normally requires that kdb be entered and
the nmi_console command be used to enable the console (or if only kgdb
is present then gdb must directly manipulate the value of
kgdb_nmi_tty_enabled).

This patch automates the management of kgdb_nmi_tty_enabled by keeping
track of the number of file handles that are open for reading and using
that to determine how to tty should operate.

The approach means that:

1. Behaviour before init starts is unchanged.

2. If the userspace runs a getty or some other interactive process on
   /dev/console (or explicitly on /dev/ttyNMI0) the tty will treat
   character input like any other tty.

3. If the userspace doesn't use /dev/console or if it uses /dev/console
   only to log messages (O_WRONLY) then the user prompt is retained.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: linux-serial@vger.kernel.org
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: kgdb-bugreport@lists.sourceforge.net
---
 drivers/tty/serial/kgdb_nmi.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/kgdb_nmi.c b/drivers/tty/serial/kgdb_nmi.c
index cfadf29..6ec7501 100644
--- a/drivers/tty/serial/kgdb_nmi.c
+++ b/drivers/tty/serial/kgdb_nmi.c
@@ -42,7 +42,7 @@ static char *kgdb_nmi_magic = "$3#33";
 module_param_named(magic, kgdb_nmi_magic, charp, 0600);
 MODULE_PARM_DESC(magic, "magic sequence to enter NMI debugger (default $3#33)");
 
-static bool kgdb_nmi_tty_enabled;
+static atomic_t kgdb_nmi_num_readers = ATOMIC_INIT(0);
 
 static int kgdb_nmi_console_setup(struct console *co, char *options)
 {
@@ -136,7 +136,7 @@ static int kgdb_nmi_poll_one_knock(void)
 		n = 0;
 	}
 
-	if (kgdb_nmi_tty_enabled) {
+	if (atomic_read(&kgdb_nmi_num_readers)) {
 		kgdb_tty_recv(c);
 		return 0;
 	}
@@ -197,7 +197,8 @@ static void kgdb_nmi_tty_receiver(unsigned long data)
 	priv->timer.expires = jiffies + (HZ/100);
 	add_timer(&priv->timer);
 
-	if (likely(!kgdb_nmi_tty_enabled || !kfifo_len(&priv->fifo)))
+	if (likely(!atomic_read(&kgdb_nmi_num_readers) ||
+		   !kfifo_len(&priv->fifo)))
 		return;
 
 	while (kfifo_out(&priv->fifo, &ch, 1))
@@ -270,13 +271,23 @@ static void kgdb_nmi_tty_cleanup(struct tty_struct *tty)
 static int kgdb_nmi_tty_open(struct tty_struct *tty, struct file *file)
 {
 	struct kgdb_nmi_tty_priv *priv = tty->driver_data;
+	unsigned int mode = file->f_flags & O_ACCMODE;
+	int ret;
+
+	ret = tty_port_open(&priv->port, tty, file);
+	if (!ret && (mode == O_RDONLY || mode == O_RDWR))
+		atomic_inc(&kgdb_nmi_num_readers);
 
-	return tty_port_open(&priv->port, tty, file);
+	return ret;
 }
 
 static void kgdb_nmi_tty_close(struct tty_struct *tty, struct file *file)
 {
 	struct kgdb_nmi_tty_priv *priv = tty->driver_data;
+	unsigned int mode = file->f_flags & O_ACCMODE;
+
+	if (mode == O_RDONLY || mode == O_RDWR)
+		atomic_dec(&kgdb_nmi_num_readers);
 
 	tty_port_close(&priv->port, tty, file);
 }
@@ -313,12 +324,6 @@ static const struct tty_operations kgdb_nmi_tty_ops = {
 	.write		= kgdb_nmi_tty_write,
 };
 
-static int kgdb_nmi_enable_console(int argc, const char *argv[])
-{
-	kgdb_nmi_tty_enabled = !(argc == 1 && !strcmp(argv[1], "off"));
-	return 0;
-}
-
 int kgdb_register_nmi_console(void)
 {
 	int ret;
@@ -348,19 +353,10 @@ int kgdb_register_nmi_console(void)
 		goto err_drv_reg;
 	}
 
-	ret = kdb_register("nmi_console", kgdb_nmi_enable_console, "[off]",
-			   "switch to Linux NMI console", 0);
-	if (ret) {
-		pr_err("%s: can't register kdb command: %d\n", __func__, ret);
-		goto err_kdb_reg;
-	}
-
 	register_console(&kgdb_nmi_console);
 	arch_kgdb_ops.enable_nmi(1);
 
 	return 0;
-err_kdb_reg:
-	tty_unregister_driver(kgdb_nmi_tty_driver);
 err_drv_reg:
 	put_tty_driver(kgdb_nmi_tty_driver);
 	return ret;
@@ -375,8 +371,6 @@ int kgdb_unregister_nmi_console(void)
 		return 0;
 	arch_kgdb_ops.enable_nmi(0);
 
-	kdb_unregister("nmi_console");
-
 	ret = unregister_console(&kgdb_nmi_console);
 	if (ret)
 		return ret;
-- 
1.9.3