Magic sysrq has proven for Arista usecases to be useful for debugging
issues in field, over serial line when the switch is in such bad state
that it can't accept network connections anymore.
Unfortunately, having sysrq always enabled doesn't work for some
embedded boards that tend to generate garbage on serial line (including
BREAKs). Since commit 732dbf3a6104 ("serial: do not accept sysrq
characters via serial port"), it's possible to keep sysrq enabled, but
over serial line.
Add a way to enable sysrq on a uart, where currently it can be
constantly either on or off (CONFIG_MAGIC_SYSRQ_SERIAL).
While doing so, cleanup __sysrq_enabled and serial_core header file.
Sending against -next tree as it's based on removing SUPPORT_SYSRQ
ifdeffery [1].
[1]: https://lkml.kernel.org/r/[email protected]
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: Vasiliy Khoruzhick <[email protected]>
Cc: [email protected]
Thanks,
Dmitry
Dmitry Safonov (3):
serial_core: Move sysrq functions from header file
sysctl/sysrq: Remove __sysrq_enabled copy
serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE
drivers/tty/serial/serial_core.c | 123 +++++++++++++++++++++++++++++++
drivers/tty/sysrq.c | 7 ++
include/linux/serial_core.h | 86 ++-------------------
include/linux/sysrq.h | 1 +
kernel/sysctl.c | 41 ++++++-----
lib/Kconfig.debug | 8 ++
6 files changed, 167 insertions(+), 99 deletions(-)
--
2.24.1
It's not worth to have them in every serial driver and I'm about to add
another helper function.
Signed-off-by: Dmitry Safonov <[email protected]>
---
drivers/tty/serial/serial_core.c | 83 +++++++++++++++++++++++++++++++
include/linux/serial_core.h | 84 ++------------------------------
2 files changed, 88 insertions(+), 79 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7c2782785736..6ac9dfed3423 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3081,6 +3081,89 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
}
EXPORT_SYMBOL_GPL(uart_insert_char);
+int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+ if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
+ return 0;
+
+ if (!port->has_sysrq || !port->sysrq)
+ return 0;
+
+ if (ch && time_before(jiffies, port->sysrq)) {
+ handle_sysrq(ch);
+ port->sysrq = 0;
+ return 1;
+ }
+ port->sysrq = 0;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(uart_handle_sysrq_char);
+
+int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+ if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
+ return 0;
+
+ if (!port->has_sysrq || !port->sysrq)
+ return 0;
+
+ if (ch && time_before(jiffies, port->sysrq)) {
+ port->sysrq_ch = ch;
+ port->sysrq = 0;
+ return 1;
+ }
+ port->sysrq = 0;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
+
+void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+{
+ int sysrq_ch;
+
+ if (!port->has_sysrq) {
+ spin_unlock_irqrestore(&port->lock, irqflags);
+ return;
+ }
+
+ sysrq_ch = port->sysrq_ch;
+ port->sysrq_ch = 0;
+
+ spin_unlock_irqrestore(&port->lock, irqflags);
+
+ if (sysrq_ch)
+ handle_sysrq(sysrq_ch);
+}
+EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
+
+/*
+ * We do the SysRQ and SAK checking like this...
+ */
+int uart_handle_break(struct uart_port *port)
+{
+ struct uart_state *state = port->state;
+
+ if (port->handle_break)
+ port->handle_break(port);
+
+ if (port->has_sysrq) {
+ if (port->cons && port->cons->index == port->line) {
+ if (!port->sysrq) {
+ port->sysrq = jiffies + HZ*5;
+ return 1;
+ }
+ port->sysrq = 0;
+ }
+ }
+
+ if (port->flags & UPF_SAK)
+ do_SAK(state->port.tty);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(uart_handle_break);
+
EXPORT_SYMBOL(uart_write_wakeup);
EXPORT_SYMBOL(uart_register_driver);
EXPORT_SYMBOL(uart_unregister_driver);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 9cf1682dc580..255e86a474e9 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -460,85 +460,11 @@ extern void uart_handle_cts_change(struct uart_port *uport,
extern void uart_insert_char(struct uart_port *port, unsigned int status,
unsigned int overrun, unsigned int ch, unsigned int flag);
-static inline int
-uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
-{
- if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
- return 0;
-
- if (!port->has_sysrq || !port->sysrq)
- return 0;
-
- if (ch && time_before(jiffies, port->sysrq)) {
- handle_sysrq(ch);
- port->sysrq = 0;
- return 1;
- }
- port->sysrq = 0;
-
- return 0;
-}
-static inline int
-uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
-{
- if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
- return 0;
-
- if (!port->has_sysrq || !port->sysrq)
- return 0;
-
- if (ch && time_before(jiffies, port->sysrq)) {
- port->sysrq_ch = ch;
- port->sysrq = 0;
- return 1;
- }
- port->sysrq = 0;
-
- return 0;
-}
-static inline void
-uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
-{
- int sysrq_ch;
-
- if (!port->has_sysrq) {
- spin_unlock_irqrestore(&port->lock, irqflags);
- return;
- }
-
- sysrq_ch = port->sysrq_ch;
- port->sysrq_ch = 0;
-
- spin_unlock_irqrestore(&port->lock, irqflags);
-
- if (sysrq_ch)
- handle_sysrq(sysrq_ch);
-}
-
-/*
- * We do the SysRQ and SAK checking like this...
- */
-static inline int uart_handle_break(struct uart_port *port)
-{
- struct uart_state *state = port->state;
-
- if (port->handle_break)
- port->handle_break(port);
-
- if (port->has_sysrq) {
- if (port->cons && port->cons->index == port->line) {
- if (!port->sysrq) {
- port->sysrq = jiffies + HZ*5;
- return 1;
- }
- port->sysrq = 0;
- }
- }
-
- if (port->flags & UPF_SAK)
- do_SAK(state->port.tty);
- return 0;
-}
+extern int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch);
+extern int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch);
+extern void uart_unlock_and_check_sysrq(struct uart_port *port,
+ unsigned long irqflags);
+extern int uart_handle_break(struct uart_port *port);
/*
* UART_ENABLE_MS - determine if port should enable modem status irqs
--
2.24.1
Many embedded boards have a disconnected TTL level serial which can
generate some garbage that can lead to spurious false sysrq detects.
Currently, sysrq can be either completely disabled for serial console
or always disabled (with CONFIG_MAGIC_SYSRQ_SERIAL), since
commit 732dbf3a6104 ("serial: do not accept sysrq characters via serial port")
At Arista, we have such boards that can generate BREAK and random
garbage. While disabling sysrq for serial console would solve
the problem with spurious false sysrq triggers, it's also desirable
to have a way to enable sysrq back.
Having the way to enable sysrq was beneficial to debug lockups with
a manual investigation in field and on the other side preventing false
sysrq detections.
As a preparation to add sysrq_toggle_support() call into uart,
remove a private copy of sysrq_enabled from sysctl - it should reflect
the actual status of sysrq.
Furthermore, the private copy isn't correct already in case
sysrq_always_enabled is true. So, remove __sysrq_enabled and use a
getter-helper for sysrq enabled status.
Cc: Iurii Zaikin <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry Safonov <[email protected]>
---
drivers/tty/sysrq.c | 7 +++++++
include/linux/sysrq.h | 1 +
kernel/sysctl.c | 41 ++++++++++++++++++++++-------------------
3 files changed, 30 insertions(+), 19 deletions(-)
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index f724962a5906..ef3e78967146 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -73,6 +73,13 @@ static bool sysrq_on_mask(int mask)
(sysrq_enabled & mask);
}
+int sysrq_get_mask(void)
+{
+ if (sysrq_always_enabled)
+ return 1;
+ return sysrq_enabled;
+}
+
static int __init sysrq_always_enabled_setup(char *str)
{
sysrq_always_enabled = true;
diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
index 8c71874e8485..4a0b351fa2d3 100644
--- a/include/linux/sysrq.h
+++ b/include/linux/sysrq.h
@@ -50,6 +50,7 @@ int unregister_sysrq_key(int key, struct sysrq_key_op *op);
struct sysrq_key_op *__sysrq_get_key_op(int key);
int sysrq_toggle_support(int enable_mask);
+int sysrq_get_mask(void);
#else
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index d396aaaf19a3..6ddb4d7df0e1 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -229,25 +229,8 @@ static int proc_dopipe_max_size(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos);
#ifdef CONFIG_MAGIC_SYSRQ
-/* Note: sysrq code uses its own private copy */
-static int __sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE;
-
static int sysrq_sysctl_handler(struct ctl_table *table, int write,
- void __user *buffer, size_t *lenp,
- loff_t *ppos)
-{
- int error;
-
- error = proc_dointvec(table, write, buffer, lenp, ppos);
- if (error)
- return error;
-
- if (write)
- sysrq_toggle_support(__sysrq_enabled);
-
- return 0;
-}
-
+ void __user *buffer, size_t *lenp, loff_t *ppos);
#endif
static struct ctl_table kern_table[];
@@ -747,7 +730,7 @@ static struct ctl_table kern_table[] = {
#ifdef CONFIG_MAGIC_SYSRQ
{
.procname = "sysrq",
- .data = &__sysrq_enabled,
+ .data = NULL,
.maxlen = sizeof (int),
.mode = 0644,
.proc_handler = sysrq_sysctl_handler,
@@ -2844,6 +2827,26 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
}
#endif
+#ifdef CONFIG_MAGIC_SYSRQ
+static int sysrq_sysctl_handler(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ int tmp, ret;
+
+ tmp = sysrq_get_mask();
+
+ ret = __do_proc_dointvec(&tmp, table, write, buffer,
+ lenp, ppos, NULL, NULL);
+ if (ret || !write)
+ return ret;
+
+ if (write)
+ sysrq_toggle_support(tmp);
+
+ return 0;
+}
+#endif
+
static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,
void __user *buffer,
size_t *lenp, loff_t *ppos,
--
2.24.1
Many embedded boards have a disconnected TTL level serial which can
generate some garbage that can lead to spurious false sysrq detects.
Currently, sysrq can be either completely disabled for serial console
or always disabled (with CONFIG_MAGIC_SYSRQ_SERIAL), since
commit 732dbf3a6104 ("serial: do not accept sysrq characters via serial port")
At Arista, we have such boards that can generate BREAK and random
garbage. While disabling sysrq for serial console would solve
the problem with spurious false sysrq triggers, it's also desirable
to have a way to enable sysrq back.
As a measure of balance between on and off options, add
MAGIC_SYSRQ_SERIAL_SEQUENCE which is a string sequence that can enable
sysrq if it follows BREAK on a serial line. The longer the string - the
less likely it may be in the garbage.
Having the way to enable sysrq was beneficial to debug lockups with
a manual investigation in field and on the other side preventing false
sysrq detections.
Based-on-patch-by: Vasiliy Khoruzhick <[email protected]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
drivers/tty/serial/serial_core.c | 52 ++++++++++++++++++++++++++++----
include/linux/serial_core.h | 2 +-
lib/Kconfig.debug | 8 +++++
3 files changed, 55 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6ac9dfed3423..f70eba032d0b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
}
EXPORT_SYMBOL_GPL(uart_insert_char);
+const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
+
+static void uart_sysrq_on(struct work_struct *w)
+{
+ sysrq_toggle_support(1);
+ pr_info("SysRq is enabled by magic sequience on serial\n");
+}
+static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
+
+static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
+{
+ if (sysrq_toggle_seq[0] == '\0')
+ return 0;
+
+ BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
+ if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
+ port->sysrq_seq = 0;
+ return 0;
+ }
+
+ /* Without the last \0 */
+ if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
+ port->sysrq = jiffies + HZ*5;
+ return 1;
+ }
+
+ schedule_work(&sysrq_enable_work);
+
+ port->sysrq = 0;
+ return 1;
+}
+
int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
{
if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
@@ -3090,9 +3122,13 @@ int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
return 0;
if (ch && time_before(jiffies, port->sysrq)) {
- handle_sysrq(ch);
- port->sysrq = 0;
- return 1;
+ if (sysrq_get_mask()) {
+ handle_sysrq(ch);
+ port->sysrq = 0;
+ return 1;
+ }
+ if (uart_try_toggle_sysrq(port, ch))
+ return 1;
}
port->sysrq = 0;
@@ -3109,9 +3145,13 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
return 0;
if (ch && time_before(jiffies, port->sysrq)) {
- port->sysrq_ch = ch;
- port->sysrq = 0;
- return 1;
+ if (sysrq_get_mask()) {
+ port->sysrq_ch = ch;
+ port->sysrq = 0;
+ return 1;
+ }
+ if (uart_try_toggle_sysrq(port, ch))
+ return 1;
}
port->sysrq = 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 255e86a474e9..1f4443db5474 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -243,10 +243,10 @@ struct uart_port {
unsigned long sysrq; /* sysrq timeout */
unsigned int sysrq_ch; /* char for sysrq */
unsigned char has_sysrq;
+ unsigned char sysrq_seq; /* index in sysrq_toggle_seq */
unsigned char hub6; /* this should be in the 8250 driver */
unsigned char suspended;
- unsigned char unused;
const char *name; /* port name */
struct attribute_group *attr_group; /* port specific attributes */
const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 330909c600b1..f6f3e82fee25 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -431,6 +431,14 @@ config MAGIC_SYSRQ_SERIAL
This option allows you to decide whether you want to enable the
magic SysRq key.
+config MAGIC_SYSRQ_SERIAL_SEQUENCE
+ string "Char sequence that enables magic SysRq over serial"
+ depends on MAGIC_SYSRQ_SERIAL
+ default ""
+ help
+ Specifies a sequence of characters that can follow BREAK to enable
+ SysRq on a serial console.
+
config DEBUG_FS
bool "Debug Filesystem"
help
--
2.24.1
Hi,
On 1/9/20 1:54 PM, Dmitry Safonov wrote:
>
> Based-on-patch-by: Vasiliy Khoruzhick <[email protected]>
> Signed-off-by: Dmitry Safonov <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 52 ++++++++++++++++++++++++++++----
> include/linux/serial_core.h | 2 +-
> lib/Kconfig.debug | 8 +++++
> 3 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 6ac9dfed3423..f70eba032d0b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
> }
> EXPORT_SYMBOL_GPL(uart_insert_char);
>
> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
> +
> +static void uart_sysrq_on(struct work_struct *w)
> +{
> + sysrq_toggle_support(1);
> + pr_info("SysRq is enabled by magic sequience on serial\n");
typo: sequence
> +}
> +static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
--
~Randy
> Furthermore, the private copy isn't correct already in case
> sysrq_always_enabled is true. So, remove __sysrq_enabled and use a
> getter-helper for sysrq enabled status.
Since the old way was known to be unreliable, the new way looks like a
great candidate for a KUnit test.
Off topic: I wonder if Magic Sysrq could be extended with reasonable
effort to be triggered by a sequence of keystrokes which would be less
likely to be generated by a noisy serial.
On Thu, Jan 09, 2020 at 09:54:43PM +0000, Dmitry Safonov wrote:
> Many embedded boards have a disconnected TTL level serial which can
> generate some garbage that can lead to spurious false sysrq detects.
>
> Currently, sysrq can be either completely disabled for serial console
> or always disabled (with CONFIG_MAGIC_SYSRQ_SERIAL), since
> commit 732dbf3a6104 ("serial: do not accept sysrq characters via serial port")
>
> At Arista, we have such boards that can generate BREAK and random
> garbage. While disabling sysrq for serial console would solve
> the problem with spurious false sysrq triggers, it's also desirable
> to have a way to enable sysrq back.
>
> Having the way to enable sysrq was beneficial to debug lockups with
> a manual investigation in field and on the other side preventing false
> sysrq detections.
>
> As a preparation to add sysrq_toggle_support() call into uart,
> remove a private copy of sysrq_enabled from sysctl - it should reflect
> the actual status of sysrq.
>
> Furthermore, the private copy isn't correct already in case
> sysrq_always_enabled is true. So, remove __sysrq_enabled and use a
> getter-helper for sysrq enabled status.
>
> Cc: Iurii Zaikin <[email protected]>
> Cc: Jiri Slaby <[email protected]>
> Cc: Luis Chamberlain <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: [email protected]
> Signed-off-by: Dmitry Safonov <[email protected]>
> ---
> drivers/tty/sysrq.c | 7 +++++++
> include/linux/sysrq.h | 1 +
> kernel/sysctl.c | 41 ++++++++++++++++++++++-------------------
> 3 files changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index f724962a5906..ef3e78967146 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -73,6 +73,13 @@ static bool sysrq_on_mask(int mask)
> (sysrq_enabled & mask);
> }
>
> +int sysrq_get_mask(void)
> +{
> + if (sysrq_always_enabled)
> + return 1;
> + return sysrq_enabled;
> +}
> +
> static int __init sysrq_always_enabled_setup(char *str)
> {
> sysrq_always_enabled = true;
> diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
> index 8c71874e8485..4a0b351fa2d3 100644
> --- a/include/linux/sysrq.h
> +++ b/include/linux/sysrq.h
> @@ -50,6 +50,7 @@ int unregister_sysrq_key(int key, struct sysrq_key_op *op);
> struct sysrq_key_op *__sysrq_get_key_op(int key);
>
> int sysrq_toggle_support(int enable_mask);
> +int sysrq_get_mask(void);
>
> #else
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index d396aaaf19a3..6ddb4d7df0e1 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -229,25 +229,8 @@ static int proc_dopipe_max_size(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos);
>
> #ifdef CONFIG_MAGIC_SYSRQ
> -/* Note: sysrq code uses its own private copy */
> -static int __sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE;
> -
> static int sysrq_sysctl_handler(struct ctl_table *table, int write,
> - void __user *buffer, size_t *lenp,
> - loff_t *ppos)
> -{
> - int error;
> -
> - error = proc_dointvec(table, write, buffer, lenp, ppos);
> - if (error)
> - return error;
> -
> - if (write)
> - sysrq_toggle_support(__sysrq_enabled);
> -
> - return 0;
> -}
> -
> + void __user *buffer, size_t *lenp, loff_t *ppos);
> #endif
>
> static struct ctl_table kern_table[];
> @@ -747,7 +730,7 @@ static struct ctl_table kern_table[] = {
> #ifdef CONFIG_MAGIC_SYSRQ
> {
> .procname = "sysrq",
> - .data = &__sysrq_enabled,
> + .data = NULL,
> .maxlen = sizeof (int),
> .mode = 0644,
> .proc_handler = sysrq_sysctl_handler,
> @@ -2844,6 +2827,26 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
> }
> #endif
>
> +#ifdef CONFIG_MAGIC_SYSRQ
> +static int sysrq_sysctl_handler(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> + int tmp, ret;
> +
> + tmp = sysrq_get_mask();
> +
> + ret = __do_proc_dointvec(&tmp, table, write, buffer,
> + lenp, ppos, NULL, NULL);
> + if (ret || !write)
> + return ret;
> +
> + if (write)
> + sysrq_toggle_support(tmp);
> +
> + return 0;
> +}
> +#endif
Why did you move this function down here? Can't it stay where it is and
you can just fix the logic there? Now you have two different #ifdef
blocks intead of just one :(
thanks,
greg k-h
On Thu, Jan 09, 2020 at 09:54:44PM +0000, Dmitry Safonov wrote:
> Many embedded boards have a disconnected TTL level serial which can
> generate some garbage that can lead to spurious false sysrq detects.
>
> Currently, sysrq can be either completely disabled for serial console
> or always disabled (with CONFIG_MAGIC_SYSRQ_SERIAL), since
> commit 732dbf3a6104 ("serial: do not accept sysrq characters via serial port")
>
> At Arista, we have such boards that can generate BREAK and random
> garbage. While disabling sysrq for serial console would solve
> the problem with spurious false sysrq triggers, it's also desirable
> to have a way to enable sysrq back.
>
> As a measure of balance between on and off options, add
> MAGIC_SYSRQ_SERIAL_SEQUENCE which is a string sequence that can enable
> sysrq if it follows BREAK on a serial line. The longer the string - the
> less likely it may be in the garbage.
>
> Having the way to enable sysrq was beneficial to debug lockups with
> a manual investigation in field and on the other side preventing false
> sysrq detections.
>
> Based-on-patch-by: Vasiliy Khoruzhick <[email protected]>
> Signed-off-by: Dmitry Safonov <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 52 ++++++++++++++++++++++++++++----
> include/linux/serial_core.h | 2 +-
> lib/Kconfig.debug | 8 +++++
> 3 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 6ac9dfed3423..f70eba032d0b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
> }
> EXPORT_SYMBOL_GPL(uart_insert_char);
>
> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
> +
> +static void uart_sysrq_on(struct work_struct *w)
> +{
> + sysrq_toggle_support(1);
> + pr_info("SysRq is enabled by magic sequience on serial\n");
Do we want to say what serial port it is enabled on?
And why is this done in a workqueue?
> +}
> +static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
> +
> +static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> +{
> + if (sysrq_toggle_seq[0] == '\0')
> + return 0;
Is constantly checking the data stream like this going to slow things
down overall? Ah, we are just checking this after BREAK, right? So
that hopefully will not be that bad...
> +
> + BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
> + if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
> + port->sysrq_seq = 0;
> + return 0;
> + }
> +
> + /* Without the last \0 */
> + if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
> + port->sysrq = jiffies + HZ*5;
5 second delay? You should document what this value is for somewhere
here...
> + return 1;
> + }
> +
> + schedule_work(&sysrq_enable_work);
> +
> + port->sysrq = 0;
> + return 1;
> +}
> +
> int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
> {
> if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
> @@ -3090,9 +3122,13 @@ int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
> return 0;
>
> if (ch && time_before(jiffies, port->sysrq)) {
> - handle_sysrq(ch);
> - port->sysrq = 0;
> - return 1;
> + if (sysrq_get_mask()) {
> + handle_sysrq(ch);
> + port->sysrq = 0;
> + return 1;
> + }
Isn't this change to test for sysrq_get_mask() a different change than
checking for the "magic" data stream?
> + if (uart_try_toggle_sysrq(port, ch))
> + return 1;
> }
> port->sysrq = 0;
>
> @@ -3109,9 +3145,13 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
> return 0;
>
> if (ch && time_before(jiffies, port->sysrq)) {
> - port->sysrq_ch = ch;
> - port->sysrq = 0;
> - return 1;
> + if (sysrq_get_mask()) {
> + port->sysrq_ch = ch;
> + port->sysrq = 0;
> + return 1;
> + }
> + if (uart_try_toggle_sysrq(port, ch))
> + return 1;
> }
> port->sysrq = 0;
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 255e86a474e9..1f4443db5474 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -243,10 +243,10 @@ struct uart_port {
> unsigned long sysrq; /* sysrq timeout */
> unsigned int sysrq_ch; /* char for sysrq */
> unsigned char has_sysrq;
> + unsigned char sysrq_seq; /* index in sysrq_toggle_seq */
>
> unsigned char hub6; /* this should be in the 8250 driver */
> unsigned char suspended;
> - unsigned char unused;
This is an unrelated change, let's leave it for a different patch that
cleans up the layout of this structure, ok?
thanks,
greg k-h
On Thu, Jan 09, 2020 at 09:54:42PM +0000, Dmitry Safonov wrote:
> It's not worth to have them in every serial driver and I'm about to add
> another helper function.
>
> Signed-off-by: Dmitry Safonov <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 83 +++++++++++++++++++++++++++++++
> include/linux/serial_core.h | 84 ++------------------------------
> 2 files changed, 88 insertions(+), 79 deletions(-)
This is a nice cleanup, so I took this patch now, but the last two
patches still need a bit of rework. Please do that and rebase, no need
to send this patch again.
thanks,
greg k-h
On Thu, 2020-01-09 at 21:54 +0000, Dmitry Safonov wrote:
> Many embedded boards have a disconnected TTL level serial which can
> generate some garbage that can lead to spurious false sysrq detects.
trivia:
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
[]
> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
[]
> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
static const?
> +static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
This function return might read better as bool not int
> +{
> + if (sysrq_toggle_seq[0] == '\0')
> + return 0;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
> + if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
> + port->sysrq_seq = 0;
> + return 0;
> + }
> +
> + /* Without the last \0 */
> + if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
> + port->sysrq = jiffies + HZ*5;
> + return 1;
> + }
> +
> + schedule_work(&sysrq_enable_work);
> +
> + port->sysrq = 0;
> + return 1;
> +}
[]
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 255e86a474e9..1f4443db5474 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -243,10 +243,10 @@ struct uart_port {
> unsigned long sysrq; /* sysrq timeout */
> unsigned int sysrq_ch; /* char for sysrq */
> unsigned char has_sysrq;
> + unsigned char sysrq_seq; /* index in sysrq_toggle_seq */
unsigned int?
Or maybe set a maximum length of MAGIC_SYSRQ_SERIAL_SEQUENCE.
>
> unsigned char hub6; /* this should be in the 8250 driver */
> unsigned char suspended;
> - unsigned char unused;
> const char *name; /* port name */
> struct attribute_group *attr_group; /* port specific attributes */
> const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 330909c600b1..f6f3e82fee25 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -431,6 +431,14 @@ config MAGIC_SYSRQ_SERIAL
> This option allows you to decide whether you want to enable the
> magic SysRq key.
>
> +config MAGIC_SYSRQ_SERIAL_SEQUENCE
> + string "Char sequence that enables magic SysRq over serial"
> + depends on MAGIC_SYSRQ_SERIAL
> + default ""
> + help
> + Specifies a sequence of characters that can follow BREAK to enable
> + SysRq on a serial console.
> +
> config DEBUG_FS
> bool "Debug Filesystem"
> help
Hi Dmitry,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on next-20200109]
[cannot apply to linux/master usb/usb-testing linus/master v5.5-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Dmitry-Safonov/serial-sysrq-Add-MAGIC_SYSRQ_SERIAL_SEQUENCE/20200110-191606
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: microblaze-nommu_defconfig (attached as .config)
compiler: microblaze-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=microblaze
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/tty/serial/serial_core.c:3084:33: error: 'CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE' undeclared here (not in a function)
const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/serial_core.c: In function 'uart_sysrq_on':
>> drivers/tty/serial/serial_core.c:3088:2: error: implicit declaration of function 'sysrq_toggle_support' [-Werror=implicit-function-declaration]
sysrq_toggle_support(1);
^~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/serial_core.c: In function 'uart_handle_sysrq_char':
>> drivers/tty/serial/serial_core.c:3125:7: error: implicit declaration of function 'sysrq_get_mask'; did you mean 'xas_get_mark'? [-Werror=implicit-function-declaration]
if (sysrq_get_mask()) {
^~~~~~~~~~~~~~
xas_get_mark
cc1: some warnings being treated as errors
vim +/CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE +3084 drivers/tty/serial/serial_core.c
3083
> 3084 const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
3085
3086 static void uart_sysrq_on(struct work_struct *w)
3087 {
> 3088 sysrq_toggle_support(1);
3089 pr_info("SysRq is enabled by magic sequience on serial\n");
3090 }
3091 static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
3092
3093 static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
3094 {
3095 if (sysrq_toggle_seq[0] == '\0')
3096 return 0;
3097
3098 BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
3099 if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
3100 port->sysrq_seq = 0;
3101 return 0;
3102 }
3103
3104 /* Without the last \0 */
3105 if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
3106 port->sysrq = jiffies + HZ*5;
3107 return 1;
3108 }
3109
3110 schedule_work(&sysrq_enable_work);
3111
3112 port->sysrq = 0;
3113 return 1;
3114 }
3115
3116 int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
3117 {
3118 if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
3119 return 0;
3120
3121 if (!port->has_sysrq || !port->sysrq)
3122 return 0;
3123
3124 if (ch && time_before(jiffies, port->sysrq)) {
> 3125 if (sysrq_get_mask()) {
3126 handle_sysrq(ch);
3127 port->sysrq = 0;
3128 return 1;
3129 }
3130 if (uart_try_toggle_sysrq(port, ch))
3131 return 1;
3132 }
3133 port->sysrq = 0;
3134
3135 return 0;
3136 }
3137 EXPORT_SYMBOL_GPL(uart_handle_sysrq_char);
3138
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Hi Dmitry,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on next-20200110]
[cannot apply to linux/master usb/usb-testing linus/master v5.5-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Dmitry-Safonov/serial-sysrq-Add-MAGIC_SYSRQ_SERIAL_SEQUENCE/20200110-191606
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: mips-randconfig-a001-20200109 (attached as .config)
compiler: mips-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/tty/serial/serial_core.c:3084:33: error: 'CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE' undeclared here (not in a function); did you mean 'CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE'?
const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE
vim +3084 drivers/tty/serial/serial_core.c
3083
> 3084 const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
3085
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Hi Greg,
On 1/10/20 4:40 PM, Greg Kroah-Hartman wrote:
> On Thu, Jan 09, 2020 at 09:54:43PM +0000, Dmitry Safonov wrote:
[..]
>> @@ -2844,6 +2827,26 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
>> }
>> #endif
>>
>> +#ifdef CONFIG_MAGIC_SYSRQ
>> +static int sysrq_sysctl_handler(struct ctl_table *table, int write,
>> + void __user *buffer, size_t *lenp, loff_t *ppos)
>> +{
>> + int tmp, ret;
>> +
>> + tmp = sysrq_get_mask();
>> +
>> + ret = __do_proc_dointvec(&tmp, table, write, buffer,
>> + lenp, ppos, NULL, NULL);
>> + if (ret || !write)
>> + return ret;
>> +
>> + if (write)
>> + sysrq_toggle_support(tmp);
>> +
>> + return 0;
>> +}
>> +#endif
>
> Why did you move this function down here? Can't it stay where it is and
> you can just fix the logic there? Now you have two different #ifdef
> blocks intead of just one :(
Yeah, well __do_proc_dointvec() made me do it.
sysrq_sysctl_handler() declaration should be before ctl_table array of
sysctls, so I couldn't remove the forward-declaration.
So, I could forward-declare __do_proc_dointvec() instead, but looking at
the neighborhood, I decided to follow the file-style (there is a couple
of forward-declarations before the sysctl array, some under ifdefs).
I admit that the result is imperfect and can put __do_proc_dointvec()
definition before instead, no hard feelings.
Thanks,
Dmitry
Hi Iurii,
On 1/10/20 3:19 AM, Iurii Zaikin wrote:
> Off topic: I wonder if Magic Sysrq could be extended with reasonable
> effort to be triggered by a sequence of keystrokes which would be less
> likely to be generated by a noisy serial.
That's what I do in 3/3 patch from the series:
https://lkml.kernel.org/r/[email protected]
Or do you mean something else?
Thanks,
Dmitry
On Fri, Jan 10, 2020 at 09:45:30PM +0000, Dmitry Safonov wrote:
> Hi Greg,
>
> On 1/10/20 4:40 PM, Greg Kroah-Hartman wrote:
> > On Thu, Jan 09, 2020 at 09:54:43PM +0000, Dmitry Safonov wrote:
> [..]
> >> @@ -2844,6 +2827,26 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
> >> }
> >> #endif
> >>
> >> +#ifdef CONFIG_MAGIC_SYSRQ
> >> +static int sysrq_sysctl_handler(struct ctl_table *table, int write,
> >> + void __user *buffer, size_t *lenp, loff_t *ppos)
> >> +{
> >> + int tmp, ret;
> >> +
> >> + tmp = sysrq_get_mask();
> >> +
> >> + ret = __do_proc_dointvec(&tmp, table, write, buffer,
> >> + lenp, ppos, NULL, NULL);
> >> + if (ret || !write)
> >> + return ret;
> >> +
> >> + if (write)
> >> + sysrq_toggle_support(tmp);
> >> +
> >> + return 0;
> >> +}
> >> +#endif
> >
> > Why did you move this function down here? Can't it stay where it is and
> > you can just fix the logic there? Now you have two different #ifdef
> > blocks intead of just one :(
>
> Yeah, well __do_proc_dointvec() made me do it.
>
> sysrq_sysctl_handler() declaration should be before ctl_table array of
> sysctls, so I couldn't remove the forward-declaration.
>
> So, I could forward-declare __do_proc_dointvec() instead, but looking at
> the neighborhood, I decided to follow the file-style (there is a couple
> of forward-declarations before the sysctl array, some under ifdefs).
>
> I admit that the result is imperfect and can put __do_proc_dointvec()
> definition before instead, no hard feelings.
Ah, no, nevermind, I missed that reason, sorry about that. Moving it is
fine.
greg k-h
Hi Randy,
On 1/9/20 11:53 PM, Randy Dunlap wrote:
> Hi,
>
> On 1/9/20 1:54 PM, Dmitry Safonov wrote:
>>
>> Based-on-patch-by: Vasiliy Khoruzhick <[email protected]>
>> Signed-off-by: Dmitry Safonov <[email protected]>
>> ---
>> drivers/tty/serial/serial_core.c | 52 ++++++++++++++++++++++++++++----
>> include/linux/serial_core.h | 2 +-
>> lib/Kconfig.debug | 8 +++++
>> 3 files changed, 55 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 6ac9dfed3423..f70eba032d0b 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>> }
>> EXPORT_SYMBOL_GPL(uart_insert_char);
>>
>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
>> +
>> +static void uart_sysrq_on(struct work_struct *w)
>> +{
>> + sysrq_toggle_support(1);
>> + pr_info("SysRq is enabled by magic sequience on serial\n");
>
> typo: sequence
Thanks on catching this,
Dmitry
Hi Joe,
On 1/10/20 4:50 PM, Joe Perches wrote:
> On Thu, 2020-01-09 at 21:54 +0000, Dmitry Safonov wrote:
>> Many embedded boards have a disconnected TTL level serial which can
>> generate some garbage that can lead to spurious false sysrq detects.
>
> trivia:
>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> []
>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
> []
>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
>
> static const?
Will do, thanks!
>
>> +static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
>
> This function return might read better as bool not int
Yeah, no hard feelings, will convert.
[..]
>> @@ -243,10 +243,10 @@ struct uart_port {
>> unsigned long sysrq; /* sysrq timeout */
>> unsigned int sysrq_ch; /* char for sysrq */
>> unsigned char has_sysrq;
>> + unsigned char sysrq_seq; /* index in sysrq_toggle_seq */
>
> unsigned int?
>
> Or maybe set a maximum length of MAGIC_SYSRQ_SERIAL_SEQUENCE.
I think, 256 chars should be enough to send on serial (c)
I'm not aware of a way to put the max string length in Kconfig, so I did
in the patch:
BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >=
sizeof(port->sysrq_seq)*U8_MAX);
Do you have something more elegant in your mind?
Thanks,
Dmitry
On 1/10/20 4:46 PM, Greg Kroah-Hartman wrote:
[..]
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 6ac9dfed3423..f70eba032d0b 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>> }
>> EXPORT_SYMBOL_GPL(uart_insert_char);
>>
>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
>> +
>> +static void uart_sysrq_on(struct work_struct *w)
>> +{
>> + sysrq_toggle_support(1);
>> + pr_info("SysRq is enabled by magic sequience on serial\n");
>
> Do we want to say what serial port it is enabled on?
Makes sense, will add.
> And why is this done in a workqueue?
uart_try_toggle_sysrq() sometimes is called under
spin_lock_irqsave(&port->lock, flags);
And sysrq_toggle_support() calls input_register_handler() internally
which can sleep.
>> +}
>> +static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
>> +
>> +static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
>> +{
>> + if (sysrq_toggle_seq[0] == '\0')
>> + return 0;
>
> Is constantly checking the data stream like this going to slow things
> down overall? Ah, we are just checking this after BREAK, right? So
> that hopefully will not be that bad...
Yes, it's after BREAK. In my POV it's fine as originally it would cause
sysrq handler being called (if sysrq is enabled).
>
>> +
>> + BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
>> + if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
>> + port->sysrq_seq = 0;
>> + return 0;
>> + }
>> +
>> + /* Without the last \0 */
>> + if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
>> + port->sysrq = jiffies + HZ*5;
>
> 5 second delay? You should document what this value is for somewhere
> here...
Fair enough, I'll add
#define SYSRQ_TIMEOUT (HZ*5)
And use it in uart_handle_break() too.
>> @@ -3090,9 +3122,13 @@ int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
>> return 0;
>>
>> if (ch && time_before(jiffies, port->sysrq)) {
>> - handle_sysrq(ch);
>> - port->sysrq = 0;
>> - return 1;
>> + if (sysrq_get_mask()) {
>> + handle_sysrq(ch);
>> + port->sysrq = 0;
>> + return 1;
>> + }
>
> Isn't this change to test for sysrq_get_mask() a different change than
> checking for the "magic" data stream?
It's for the case when sysrq is already enabled.
Than sysrq_get_mask() will return something and it makes uart call
handle_sysrq() instead of checking the toggle sequence.
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index 255e86a474e9..1f4443db5474 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -243,10 +243,10 @@ struct uart_port {
>> unsigned long sysrq; /* sysrq timeout */
>> unsigned int sysrq_ch; /* char for sysrq */
>> unsigned char has_sysrq;
>> + unsigned char sysrq_seq; /* index in sysrq_toggle_seq */
>>
>> unsigned char hub6; /* this should be in the 8250 driver */
>> unsigned char suspended;
>> - unsigned char unused;
>
> This is an unrelated change, let's leave it for a different patch that
> cleans up the layout of this structure, ok?
Yes, sure.
Thanks,
Dmitry
Hi Dmitry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on next-20200110]
[cannot apply to linux/master usb/usb-testing linus/master v5.5-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Dmitry-Safonov/serial-sysrq-Add-MAGIC_SYSRQ_SERIAL_SEQUENCE/20200110-191606
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
New smatch warnings:
drivers/tty/serial/serial_core.c:3105 uart_try_toggle_sysrq() warn: unsigned '++port->sysrq_seq' is never less than zero.
Old smatch warnings:
drivers/tty/serial/serial_core.c:295 uart_shutdown() error: we previously assumed 'uport' could be null (see line 291)
drivers/tty/serial/serial_core.c:2729 uart_get_attr_iomem_base() warn: argument 4 to %lX specifier is cast from pointer
vim +3105 drivers/tty/serial/serial_core.c
3092
3093 static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
3094 {
3095 if (sysrq_toggle_seq[0] == '\0')
3096 return 0;
3097
3098 BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
3099 if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
3100 port->sysrq_seq = 0;
3101 return 0;
3102 }
3103
3104 /* Without the last \0 */
> 3105 if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
3106 port->sysrq = jiffies + HZ*5;
3107 return 1;
3108 }
3109
3110 schedule_work(&sysrq_enable_work);
3111
3112 port->sysrq = 0;
3113 return 1;
3114 }
3115
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
On Fri, 2020-01-10 at 22:10 +0000, Dmitry Safonov wrote:
> Hi Joe,
Hi Dmitry.
> I'm not aware of a way to put the max string length in Kconfig,
Nor am I.
> so I did
> in the patch:
> BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >=
> sizeof(port->sysrq_seq)*U8_MAX);
>
> Do you have something more elegant in your mind?
No, I didn't notice this and think this is fine
so thanks for that.
It's be nice to have something like BUILD_BUG_ON_MSG.
On 1/10/20 10:32 PM, Dmitry Safonov wrote:
> On 1/10/20 4:46 PM, Greg Kroah-Hartman wrote:
> [..]
>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>> index 6ac9dfed3423..f70eba032d0b 100644
>>> --- a/drivers/tty/serial/serial_core.c
>>> +++ b/drivers/tty/serial/serial_core.c
>>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>>> }
>>> EXPORT_SYMBOL_GPL(uart_insert_char);
>>>
>>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
>>> +
>>> +static void uart_sysrq_on(struct work_struct *w)
>>> +{
>>> + sysrq_toggle_support(1);
>>> + pr_info("SysRq is enabled by magic sequience on serial\n");
>>
>> Do we want to say what serial port it is enabled on?
>
> Makes sense, will add.
Ah, I've managed to forget to mention that I didn't add the port name
into the message in v2. I experimented a bit - it's getting a bit
complicated how-to protect (char *name) for just this message.
Like, SysRq can be theoretically enabled on two serials at the same
moment - so some locking is needed to make the printed name sane.
As sysrq_toggle_support() is a global-enable knob for sysrq (also can be
switched in /proc/sys/kernel/sysrq) I'm not sure if it's worth to
complicate code to print through which serial console SysRq has been
enabled.
I can still do it in v3 if you insist.
And sorry about forgetting to mention this - thought I'll write reply
after I send v2 and somehow it slipped my mind.
Thanks,
Dmitry
On Tue, Jan 14, 2020 at 07:10:47PM +0000, Dmitry Safonov wrote:
> On 1/10/20 10:32 PM, Dmitry Safonov wrote:
> > On 1/10/20 4:46 PM, Greg Kroah-Hartman wrote:
> > [..]
> >>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> >>> index 6ac9dfed3423..f70eba032d0b 100644
> >>> --- a/drivers/tty/serial/serial_core.c
> >>> +++ b/drivers/tty/serial/serial_core.c
> >>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
> >>> }
> >>> EXPORT_SYMBOL_GPL(uart_insert_char);
> >>>
> >>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
> >>> +
> >>> +static void uart_sysrq_on(struct work_struct *w)
> >>> +{
> >>> + sysrq_toggle_support(1);
> >>> + pr_info("SysRq is enabled by magic sequience on serial\n");
> >>
> >> Do we want to say what serial port it is enabled on?
> >
> > Makes sense, will add.
>
> Ah, I've managed to forget to mention that I didn't add the port name
> into the message in v2. I experimented a bit - it's getting a bit
> complicated how-to protect (char *name) for just this message.
> Like, SysRq can be theoretically enabled on two serials at the same
> moment - so some locking is needed to make the printed name sane.
>
> As sysrq_toggle_support() is a global-enable knob for sysrq (also can be
> switched in /proc/sys/kernel/sysrq) I'm not sure if it's worth to
> complicate code to print through which serial console SysRq has been
> enabled.
>
> I can still do it in v3 if you insist.
No worries, if it's too hard to get to, that's fine. If it was
available, it would have been nice to have.
thanks,
greg k-h