[PATCHv2-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[PATCHv2-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

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].

Changes since v1 [2]:
- Fix typo in pr_info() message (noticed by Randy Dunlap, thanks)
- Add SYSRQ_TIMEOUT define for timeout after BREAK and separate removing
  @unused member of uart_port into cleanup patch (by Greg's review, thanks)
- Add const qualifier, make uart_try_toggle_sysrq() bool function
  (Joe Perches, thanks)
- Fix !CONFIG_SYSRQ and CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE=""
  build failures (kudos to kbuild test robot)

[1]: https://lkml.kernel.org/r/20191213000657.931618-1-dima@arista.com
[2]: https://lkml.kernel.org/r/20200109215444.95995-1-dima@arista.com

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Iurii Zaikin <yzaikin@google.com>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Joe Perches <joe@perches.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Vasiliy Khoruzhick <vasilykh@arista.com>
Cc: linux-serial@vger.kernel.org

Thanks,
             Dmitry

Dmitry Safonov (3):
  sysctl/sysrq: Remove __sysrq_enabled copy
  serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE
  serial_core: Remove unused member in uart_port

 drivers/tty/serial/serial_core.c | 75 +++++++++++++++++++++++++++++---
 drivers/tty/sysrq.c              |  7 +++
 include/linux/serial_core.h      |  2 +-
 include/linux/sysrq.h            |  7 +++
 kernel/sysctl.c                  | 41 +++++++++--------
 lib/Kconfig.debug                |  8 ++++
 6 files changed, 113 insertions(+), 27 deletions(-)

-- 
2.24.1

[PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Dmitry Safonov]

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 <yzaikin@google.com>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/sysrq.c   |  7 +++++++
 include/linux/sysrq.h |  7 +++++++
 kernel/sysctl.c       | 41 ++++++++++++++++++++++-------------------
 3 files changed, 36 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..ad09a7eefda2 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
 
@@ -71,6 +72,12 @@ static inline int unregister_sysrq_key(int key, struct sysrq_key_op *op)
 	return -EINVAL;
 }
 
+static inline int sysrq_get_mask(void)
+{
+	/* Magic SysRq disabled mask */
+	return 0;
+}
+
 #endif
 
 #endif /* _LINUX_SYSRQ_H */
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

[PATCHv2-next 2/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

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 <vasilykh@arista.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/serial/serial_core.c | 75 +++++++++++++++++++++++++++++---
 include/linux/serial_core.h      |  1 +
 lib/Kconfig.debug                |  8 ++++
 3 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 76e506ee335c..3152df025569 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -20,6 +20,7 @@
 #include <linux/device.h>
 #include <linux/serial.h> /* for serial_state and serial_icounter_struct */
 #include <linux/serial_core.h>
+#include <linux/sysrq.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
 #include <linux/security.h>
@@ -40,6 +41,8 @@ static struct lock_class_key port_lock_key;
 
 #define HIGH_BITS_OFFSET	((sizeof(long)-sizeof(int))*8)
 
+#define SYSRQ_TIMEOUT	(HZ * 5)
+
 static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 					struct ktermios *old_termios);
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout);
@@ -3082,6 +3085,56 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
 }
 EXPORT_SYMBOL_GPL(uart_insert_char);
 
+#ifdef CONFIG_MAGIC_SYSRQ_SERIAL
+static 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 sequence on serial\n");
+}
+static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
+
+/**
+ *	uart_try_toggle_sysrq - Enables SysRq from serial line
+ *	@port: uart_port structure where char(s) after BREAK met
+ *	@ch: new character in the sequence after received BREAK
+ *
+ *	Enables magic SysRq when the required sequence is met on port
+ *	(see CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE).
+ *
+ *	Returns 0 if @ch is out of enabling sequence and should be
+ *	handled some other way, 1 if @ch was consumed.
+ */
+static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
+{
+	if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
+		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 + SYSRQ_TIMEOUT;
+		return 1;
+	}
+
+	schedule_work(&sysrq_enable_work);
+
+	port->sysrq = 0;
+	return 1;
+}
+#else
+static inline bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
+{
+	return 0;
+}
+#endif
+
 int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 {
 	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
@@ -3091,9 +3144,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;
 
@@ -3110,9 +3167,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;
 
@@ -3152,7 +3213,7 @@ int uart_handle_break(struct uart_port *port)
 	if (port->has_sysrq) {
 		if (port->cons && port->cons->index == port->line) {
 			if (!port->sysrq) {
-				port->sysrq = jiffies + HZ*5;
+				port->sysrq = jiffies + SYSRQ_TIMEOUT;
 				return 1;
 			}
 			port->sysrq = 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 255e86a474e9..9e0b66278430 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -243,6 +243,7 @@ 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;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e4676b992eae..f21adfed2855 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

[PATCHv2-next 3/3] serial_core: Remove unused member in uart_port

[Dmitry Safonov]

It should remove the align-padding before @name.

Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 include/linux/serial_core.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 9e0b66278430..1f4443db5474 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -247,7 +247,6 @@ struct uart_port {
 
 	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) */
-- 
2.24.1

Re: [PATCHv2-next 2/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Joe Perches]

On Tue, 2020-01-14 at 17:19 +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.

Hi again Dmitry.  trivia:

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
[]
> +/**
> + *	uart_try_toggle_sysrq - Enables SysRq from serial line
> + *	@port: uart_port structure where char(s) after BREAK met
> + *	@ch: new character in the sequence after received BREAK
> + *
> + *	Enables magic SysRq when the required sequence is met on port
> + *	(see CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE).
> + *
> + *	Returns 0 if @ch is out of enabling sequence and should be
> + *	handled some other way, 1 if @ch was consumed.
> + */

Normally bool functions return values are true/false not 1/0.

> +static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> +{
> +	if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
> +		return 0;

		return false;

etc...

Re: [PATCHv2-next 3/3] serial_core: Remove unused member in uart_port

[Joe Perches]

On Tue, 2020-01-14 at 17:19 +0000, Dmitry Safonov wrote:
> It should remove the align-padding before @name.
[]
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
[]
> @@ -247,7 +247,6 @@ struct uart_port {
>  
>  	unsigned char		hub6;			/* this should be in the 8250 driver */
>  	unsigned char		suspended;
> -	unsigned char		unused;

I suggest this not be applied as this is just to let
readers know that there is an unused 1 byte alignment
hole here that could be used for something else.

Re: [PATCHv2-next 2/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

Hi Joe,

On 1/14/20 5:30 PM, Joe Perches wrote:
> On Tue, 2020-01-14 at 17:19 +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.
> 
> Hi again Dmitry.  trivia:
> 
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> []
>> +/**
>> + *	uart_try_toggle_sysrq - Enables SysRq from serial line
>> + *	@port: uart_port structure where char(s) after BREAK met
>> + *	@ch: new character in the sequence after received BREAK
>> + *
>> + *	Enables magic SysRq when the required sequence is met on port
>> + *	(see CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE).
>> + *
>> + *	Returns 0 if @ch is out of enabling sequence and should be
>> + *	handled some other way, 1 if @ch was consumed.
>> + */
> 
> Normally bool functions return values are true/false not 1/0.

I agree. Though, somehow it seems to me that `return 1` is a bit closer
to "one character consumed" than "return true".
Again, no hard feelings, just a preference.
I can change those returns to bools if you insist :)

> 
>> +static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
>> +{
>> +	if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
>> +		return 0;
> 
> 		return false;
> 
> etc...
> 
> 

Thanks,
          Dmitry

Re: [PATCHv2-next 3/3] serial_core: Remove unused member in uart_port

[Dmitry Safonov]

On 1/14/20 5:36 PM, Joe Perches wrote:
> On Tue, 2020-01-14 at 17:19 +0000, Dmitry Safonov wrote:
>> It should remove the align-padding before @name.
> []
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> []
>> @@ -247,7 +247,6 @@ struct uart_port {
>>  
>>  	unsigned char		hub6;			/* this should be in the 8250 driver */
>>  	unsigned char		suspended;
>> -	unsigned char		unused;
> 
> I suggest this not be applied as this is just to let
> readers know that there is an unused 1 byte alignment
> hole here that could be used for something else.

Heh, 2/3 adds another `unsigned char`, so the neighbours look like:

: 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 */

So the hole became 4 bytes on 64-bit.

I can make it unused[4], but..

Separated the patch per Greg's review and I think it makes sense to have
it separately from 2/3 because last time I've touched it, it actually
was in use by drivers (regardless the name).

Thanks,
          Dmitry

Re: [PATCHv2-next 2/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Randy Dunlap]

On 1/14/20 9:19 AM, Dmitry Safonov wrote:
> +static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> +{
> +	if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
> +		return 0;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);

That sizeof(...) is confusing (to me).  I would just write:

+	BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= U8_MAX);

> +	if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
> +		port->sysrq_seq = 0;
> +		return 0;
> +	}

cheers.
-- 
~Randy

Re: [PATCHv2-next 3/3] serial_core: Remove unused member in uart_port

[Greg Kroah-Hartman]

On Tue, Jan 14, 2020 at 05:47:33PM +0000, Dmitry Safonov wrote:
> On 1/14/20 5:36 PM, Joe Perches wrote:
> > On Tue, 2020-01-14 at 17:19 +0000, Dmitry Safonov wrote:
> >> It should remove the align-padding before @name.
> > []
> >> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > []
> >> @@ -247,7 +247,6 @@ struct uart_port {
> >>  
> >>  	unsigned char		hub6;			/* this should be in the 8250 driver */
> >>  	unsigned char		suspended;
> >> -	unsigned char		unused;
> > 
> > I suggest this not be applied as this is just to let
> > readers know that there is an unused 1 byte alignment
> > hole here that could be used for something else.
> 
> Heh, 2/3 adds another `unsigned char`, so the neighbours look like:
> 
> : 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 */
> 
> So the hole became 4 bytes on 64-bit.
> 
> I can make it unused[4], but..
> 
> Separated the patch per Greg's review and I think it makes sense to have
> it separately from 2/3 because last time I've touched it, it actually
> was in use by drivers (regardless the name).

Yes, it makes sense to remove it now.  And then we can properly
reorginize the structure if people really care about the padding issues
(hint, I really doubt it).  Someone can run pahole on the structure if
they care, but getting rid of "unused" is good as it has been abused in
the past.

thanks,

greg k-h

Re: [PATCHv2-next 3/3] serial_core: Remove unused member in uart_port

[Joe Perches]

On Tue, 2020-01-14 at 18:56 +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 14, 2020 at 05:47:33PM +0000, Dmitry Safonov wrote:
> > On 1/14/20 5:36 PM, Joe Perches wrote:
> > > On Tue, 2020-01-14 at 17:19 +0000, Dmitry Safonov wrote:
> > > > It should remove the align-padding before @name.
> > > []
> > > > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > > []
> > > > @@ -247,7 +247,6 @@ struct uart_port {
> > > >  
> > > >  	unsigned char		hub6;			/* this should be in the 8250 driver */
> > > >  	unsigned char		suspended;
> > > > -	unsigned char		unused;
> > > 
> > > I suggest this not be applied as this is just to let
> > > readers know that there is an unused 1 byte alignment
> > > hole here that could be used for something else.
> > 
> > Heh, 2/3 adds another `unsigned char`, so the neighbours look like:
> > 
> > : 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 */
> > 
> > So the hole became 4 bytes on 64-bit.

Ah, perhaps the commit message could state that
there is no longer an alignment hole instead.

cheers, Joe

Re: [PATCHv2-next 3/3] serial_core: Remove unused member in uart_port

[Dmitry Safonov]

On 1/14/20 6:32 PM, Joe Perches wrote:
> On Tue, 2020-01-14 at 18:56 +0100, Greg Kroah-Hartman wrote:
>> On Tue, Jan 14, 2020 at 05:47:33PM +0000, Dmitry Safonov wrote:
>>> On 1/14/20 5:36 PM, Joe Perches wrote:
>>>> On Tue, 2020-01-14 at 17:19 +0000, Dmitry Safonov wrote:
>>>>> It should remove the align-padding before @name.
>>>> []
>>>>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>>>> []
>>>>> @@ -247,7 +247,6 @@ struct uart_port {
>>>>>  
>>>>>  	unsigned char		hub6;			/* this should be in the 8250 driver */
>>>>>  	unsigned char		suspended;
>>>>> -	unsigned char		unused;
>>>>
>>>> I suggest this not be applied as this is just to let
>>>> readers know that there is an unused 1 byte alignment
>>>> hole here that could be used for something else.
>>>
>>> Heh, 2/3 adds another `unsigned char`, so the neighbours look like:
>>>
>>> : 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 */
>>>
>>> So the hole became 4 bytes on 64-bit.
> 
> Ah, perhaps the commit message could state that
> there is no longer an alignment hole instead.

Fair enough, slowly improving in my commit message skills :-)

Thanks,
          Dmitry

Re: [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Greg Kroah-Hartman]

On Tue, Jan 14, 2020 at 05:19:10PM +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 <yzaikin@google.com>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  drivers/tty/sysrq.c   |  7 +++++++
>  include/linux/sysrq.h |  7 +++++++
>  kernel/sysctl.c       | 41 ++++++++++++++++++++++-------------------
>  3 files changed, 36 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;
> +}

Naming is hard.  And this name is really hard to understand.

Traditionally get/put are used for incrementing reference counts.  You
don't have a sysrq_put_mask() call, right?  :)

I think what you want this function to do is, "is sysrq enabled right
now" (hint, it's a global function, add kernel-doc to it so we know what
it does...).  If so, it should maybe be something like:

	bool sysrq_is_enabled(void);

which to me makes more sense.

thoughts?

thanks,

greg k-h

Re: [PATCHv2-next 3/3] serial_core: Remove unused member in uart_port

[Greg Kroah-Hartman]

On Tue, Jan 14, 2020 at 05:19:12PM +0000, Dmitry Safonov wrote:
> It should remove the align-padding before @name.
> 
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  include/linux/serial_core.h | 1 -
>  1 file changed, 1 deletion(-)

I've applied this patch to my tree now.  See my comments on the first
patch and then feel free to rebase and redo those and resend.

thanks,

greg k-h

Re: [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Dmitry Safonov]

On 1/15/20 12:36 PM, Greg Kroah-Hartman wrote:
> On Tue, Jan 14, 2020 at 05:19:10PM +0000, Dmitry Safonov wrote:
[..]
>> +int sysrq_get_mask(void)
>> +{
>> +	if (sysrq_always_enabled)
>> +		return 1;
>> +	return sysrq_enabled;
>> +}
> 
> Naming is hard.  And this name is really hard to understand.

Agree.


> Traditionally get/put are used for incrementing reference counts.  You
> don't have a sysrq_put_mask() call, right?  :)

Yes, fair point


> I think what you want this function to do is, "is sysrq enabled right
> now" (hint, it's a global function, add kernel-doc to it so we know what
> it does...).  If so, it should maybe be something like:
> 
> 	bool sysrq_is_enabled(void);
> 
> which to me makes more sense.

Err, not exactly: there is a function for that which is sysrq_on().
But for sysctl the value of the mask (or 1 for always_enabled) is
actually needed to show a proper value back to the userspace reader.

Thanks,
          Dmitry

Re: [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Dmitry Safonov]

On 2/23/20 12:08 PM, Dmitry Safonov wrote:
> 
> On 1/15/20 12:36 PM, Greg Kroah-Hartman wrote:
>> On Tue, Jan 14, 2020 at 05:19:10PM +0000, Dmitry Safonov wrote:
> [..]
>>> +int sysrq_get_mask(void)
>>> +{
>>> +	if (sysrq_always_enabled)
>>> +		return 1;
>>> +	return sysrq_enabled;
>>> +}
>>
>> Naming is hard.  And this name is really hard to understand.
> 
> Agree.
> 
> 
>> Traditionally get/put are used for incrementing reference counts.  You
>> don't have a sysrq_put_mask() call, right?  :)
> 
> Yes, fair point
> 
> 
>> I think what you want this function to do is, "is sysrq enabled right
>> now" (hint, it's a global function, add kernel-doc to it so we know what
>> it does...).  If so, it should maybe be something like:
>>
>> 	bool sysrq_is_enabled(void);
>>
>> which to me makes more sense.
> 
> Err, not exactly: there is a function for that which is sysrq_on().
> But for sysctl the value of the mask (or 1 for always_enabled) is
> actually needed to show a proper value back to the userspace reader.

I'll call it sysrq_mask(), add the kernel-doc to it in v3.

Thanks again,
          Dmitry