[PATCH 0/4] serial: core: fix up sysrq regressions

[PATCH 0/4] serial: core: fix up sysrq regressions

[Johan Hovold]

This series fixes a few regressions introduced by the recent sysrq
rework that went into 5.6.

The port unlock fix is tagged for stable, and the fix for the
unnecessary per-character overhead probably could be as well although
it is a bit more intrusive.

Johan


Johan Hovold (4):
  Revert "serial: core: Refactor uart_unlock_and_check_sysrq()"
  serial: core: fix broken sysrq port unlock
  serial: core: fix sysrq overhead regression
  serial: core: drop redundant sysrq checks

 drivers/tty/serial/serial_core.c |  96 +---------------------------
 include/linux/serial_core.h      | 105 +++++++++++++++++++++++++++++--
 2 files changed, 103 insertions(+), 98 deletions(-)

-- 
2.26.2

[PATCH 1/4] Revert "serial: core: Refactor uart_unlock_and_check_sysrq()"

[Johan Hovold]

This reverts commit da9a5aa3402db0ff3b57216d8dbf2478e1046cae.

In order to ease backporting a fix for broken port unlock, revert this
rewrite which was since added on top.

The other sysrq helpers bail out early when sysrq is not enabled; it's
better to keep that pattern here as well.

Note that the __releases() attribute won't be needed after the follow-on
fix either.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 23 +++++++++++++----------
 include/linux/serial_core.h      |  3 ++-
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 66a5e2faf57e..edfb7bc14bbf 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3239,19 +3239,22 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
 }
 EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
 
-void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags)
-__releases(&port->lock)
+void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
 {
-	if (port->has_sysrq) {
-		int sysrq_ch = port->sysrq_ch;
+	int sysrq_ch;
 
-		port->sysrq_ch = 0;
-		spin_unlock_irqrestore(&port->lock, flags);
-		if (sysrq_ch)
-			handle_sysrq(sysrq_ch);
-	} else {
-		spin_unlock_irqrestore(&port->lock, flags);
+	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);
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 92f5eba86052..1f4443db5474 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -462,7 +462,8 @@ extern void uart_insert_char(struct uart_port *port, unsigned int status,
 
 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 flags);
+extern void uart_unlock_and_check_sysrq(struct uart_port *port,
+					unsigned long irqflags);
 extern int uart_handle_break(struct uart_port *port);
 
 /*
-- 
2.26.2

[PATCH 2/4] serial: core: fix broken sysrq port unlock

[Johan Hovold]

Commit d6e1935819db ("serial: core: Allow processing sysrq at port
unlock time") worked around a circular locking dependency by adding
helpers used to defer sysrq processing to when the port lock was
released.

A later commit unfortunately converted these inline helpers to exported
functions despite the fact that the unlock helper was restoring irq
flags, something which needs to be done in the same function that saved
them (e.g. on SPARC).

Fixes: 8e20fc391711 ("serial_core: Move sysrq functions from header file")
Cc: stable <stable@vger.kernel.org>     # 5.6
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 19 -------------------
 include/linux/serial_core.h      | 21 +++++++++++++++++++--
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index edfb7bc14bbf..f6cf9cc4ce69 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3239,25 +3239,6 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
 }
 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...
  */
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 1f4443db5474..858c5dd926ad 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -462,8 +462,25 @@ extern void uart_insert_char(struct uart_port *port, unsigned int status,
 
 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);
+
+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);
+}
+
 extern int uart_handle_break(struct uart_port *port);
 
 /*
-- 
2.26.2

[PATCH 3/4] serial: core: fix sysrq overhead regression

[Johan Hovold]

Commit 8e20fc391711 ("serial_core: Move sysrq functions from header
file") converted the inline sysrq helpers to exported functions which
are now called for every received character and break signal also on
systems without CONFIG_MAGIC_SYSRQ_SERIAL instead of being optimised
away by the compiler.

Inlining these helpers again also avoids the function call overhead when
CONFIG_MAGIC_SYSRQ_SERIAL is enabled.

Fixes: 8e20fc391711 ("serial_core: Move sysrq functions from header file")
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 80 +-----------------------------
 include/linux/serial_core.h      | 85 ++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 81 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f6cf9cc4ce69..a714402dcf4e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -41,8 +41,6 @@ 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);
@@ -3163,7 +3161,7 @@ static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
  *	Returns false if @ch is out of enabling sequence and should be
  *	handled some other way, true if @ch was consumed.
  */
-static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
+bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
 {
 	int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
 
@@ -3186,83 +3184,9 @@ static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
 	port->sysrq = 0;
 	return true;
 }
-#else
-static inline bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
-{
-	return false;
-}
+EXPORT_SYMBOL_GPL(uart_try_toggle_sysrq);
 #endif
 
-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)) {
-		if (sysrq_mask()) {
-			handle_sysrq(ch);
-			port->sysrq = 0;
-			return 1;
-		}
-		if (uart_try_toggle_sysrq(port, ch))
-			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)) {
-		if (sysrq_mask()) {
-			port->sysrq_ch = ch;
-			port->sysrq = 0;
-			return 1;
-		}
-		if (uart_try_toggle_sysrq(port, ch))
-			return 1;
-	}
-	port->sysrq = 0;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
-
-/*
- * 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 && uart_console(port)) {
-		if (!port->sysrq) {
-			port->sysrq = jiffies + SYSRQ_TIMEOUT;
-			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 858c5dd926ad..a8a213b71553 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -460,8 +460,49 @@ 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);
 
-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);
+#ifdef CONFIG_MAGIC_SYSRQ_SERIAL
+
+#define SYSRQ_TIMEOUT	(HZ * 5)
+
+bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch);
+
+static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	if (!port->has_sysrq || !port->sysrq)
+		return 0;
+
+	if (ch && time_before(jiffies, port->sysrq)) {
+		if (sysrq_mask()) {
+			handle_sysrq(ch);
+			port->sysrq = 0;
+			return 1;
+		}
+		if (uart_try_toggle_sysrq(port, ch))
+			return 1;
+	}
+	port->sysrq = 0;
+
+	return 0;
+}
+
+static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	if (!port->has_sysrq || !port->sysrq)
+		return 0;
+
+	if (ch && time_before(jiffies, port->sysrq)) {
+		if (sysrq_mask()) {
+			port->sysrq_ch = ch;
+			port->sysrq = 0;
+			return 1;
+		}
+		if (uart_try_toggle_sysrq(port, ch))
+			return 1;
+	}
+	port->sysrq = 0;
+
+	return 0;
+}
 
 static inline void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
 {
@@ -480,8 +521,46 @@ static inline void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned
 	if (sysrq_ch)
 		handle_sysrq(sysrq_ch);
 }
+#else	/* CONFIG_MAGIC_SYSRQ_SERIAL */
+static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	return 0;
+}
 
-extern int uart_handle_break(struct uart_port *port);
+static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	return 0;
+}
+
+static inline void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+{
+	spin_unlock_irqrestore(&port->lock, irqflags);
+}
+#endif	/* CONFIG_MAGIC_SYSRQ_SERIAL */
+
+/*
+ * 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);
+
+#ifdef CONFIG_MAGIC_SYSRQ_SERIAL
+	if (port->has_sysrq && uart_console(port)) {
+		if (!port->sysrq) {
+			port->sysrq = jiffies + SYSRQ_TIMEOUT;
+			return 1;
+		}
+		port->sysrq = 0;
+	}
+#endif
+	if (port->flags & UPF_SAK)
+		do_SAK(state->port.tty);
+	return 0;
+}
 
 /*
  *	UART_ENABLE_MS - determine if port should enable modem status irqs
-- 
2.26.2

[PATCH 4/4] serial: core: drop redundant sysrq checks

[Johan Hovold]

The sysrq timestamp will never be set unless port->has_sysrq so drop the
redundant checks that where added by commit 1997e9dfdc84 ("serial_core:
Un-ifdef sysrq SUPPORT_SYSRQ").

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/serial_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index a8a213b71553..2f6c3cfe2ae7 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -468,7 +468,7 @@ bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch);
 
 static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 {
-	if (!port->has_sysrq || !port->sysrq)
+	if (!port->sysrq)
 		return 0;
 
 	if (ch && time_before(jiffies, port->sysrq)) {
@@ -487,7 +487,7 @@ static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch
 
 static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
 {
-	if (!port->has_sysrq || !port->sysrq)
+	if (!port->sysrq)
 		return 0;
 
 	if (ch && time_before(jiffies, port->sysrq)) {
-- 
2.26.2

Re: [PATCH 2/4] serial: core: fix broken sysrq port unlock

[Andy Shevchenko]

On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold <johan@kernel.org> wrote:
>
> Commit d6e1935819db ("serial: core: Allow processing sysrq at port
> unlock time") worked around a circular locking dependency by adding
> helpers used to defer sysrq processing to when the port lock was
> released.
>
> A later commit unfortunately converted these inline helpers to exported
> functions despite the fact that the unlock helper was restoring irq
> flags, something which needs to be done in the same function that saved
> them (e.g. on SPARC).

I'm not familiar with sparc, can you elaborate a bit what is ABI /
architecture lock implementation background?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] serial: core: fix broken sysrq port unlock

[Dmitry Safonov]

On 6/2/20 3:48 PM, Andy Shevchenko wrote:
> On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold <johan@kernel.org> wrote:
>>
>> Commit d6e1935819db ("serial: core: Allow processing sysrq at port
>> unlock time") worked around a circular locking dependency by adding
>> helpers used to defer sysrq processing to when the port lock was
>> released.
>>
>> A later commit unfortunately converted these inline helpers to exported
>> functions despite the fact that the unlock helper was restoring irq
>> flags, something which needs to be done in the same function that saved
>> them (e.g. on SPARC).
> 
> I'm not familiar with sparc, can you elaborate a bit what is ABI /
> architecture lock implementation background?

I remember that was a limitation a while ago to save/restore flags from
the same function. Though, I vaguely remember the reason.
I don't see this limitation in Documentation/*

Google suggests that it's related to storage location:
https://stackoverflow.com/a/34279032

Which is definitely non-issue with tty drivers: they call
spin_lock_irqsave() with local flags and pass them to
uart_unlock_and_check_sysrq().

Looking into arch/sparc I also can't catch if it's still a limitation.

Also, looking around, xa_unlock_irqrestore() is called not from the same
function. Maybe this issue is in history?

Johan, is it a theoretical problem or something you observe?
Also, some comments would be nice near functions in the header.

Thanks,
          Dmitry

Re: [PATCH 2/4] serial: core: fix broken sysrq port unlock

[Johan Hovold]

On Tue, Jun 02, 2020 at 04:34:16PM +0100, Dmitry Safonov wrote:
> On 6/2/20 3:48 PM, Andy Shevchenko wrote:
> > On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold <johan@kernel.org> wrote:
> >>
> >> Commit d6e1935819db ("serial: core: Allow processing sysrq at port
> >> unlock time") worked around a circular locking dependency by adding
> >> helpers used to defer sysrq processing to when the port lock was
> >> released.
> >>
> >> A later commit unfortunately converted these inline helpers to exported
> >> functions despite the fact that the unlock helper was restoring irq
> >> flags, something which needs to be done in the same function that saved
> >> them (e.g. on SPARC).
> > 
> > I'm not familiar with sparc, can you elaborate a bit what is ABI /
> > architecture lock implementation background?
> 
> I remember that was a limitation a while ago to save/restore flags from
> the same function. Though, I vaguely remember the reason.
> I don't see this limitation in Documentation/*

It's described in both LDD3 and LKD, which is possibly where I first
picked it up too (admittedly a long time ago).

> Google suggests that it's related to storage location:
> https://stackoverflow.com/a/34279032

No, that was never the issue.

SPARC includes the current register window in those flags, which at
least had to be restored in the same stack frame.

> Looking into arch/sparc I also can't catch if it's still a limitation.

Yeah, looking closer at the current implementation it seems this is no
longer an issue on SPARC.

> Also, looking around, xa_unlock_irqrestore() is called not from the same
> function. Maybe this issue is in history?

xa_unlock_irqrestore() is just a macro for spin_unlock_irqsave() it
seems, so not a counter example.

> Also, some comments would be nice near functions in the header.

Agreed. Let me respin this and either merge this with the next patch or
at least amend the commit message.

Johan

Re: [PATCH 2/4] serial: core: fix broken sysrq port unlock

[Johan Hovold]

On Wed, Jun 03, 2020 at 10:40:51AM +0200, Johan Hovold wrote:
> On Tue, Jun 02, 2020 at 04:34:16PM +0100, Dmitry Safonov wrote:
> > On 6/2/20 3:48 PM, Andy Shevchenko wrote:
> > > On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold <johan@kernel.org> wrote:
> > >>
> > >> Commit d6e1935819db ("serial: core: Allow processing sysrq at port
> > >> unlock time") worked around a circular locking dependency by adding
> > >> helpers used to defer sysrq processing to when the port lock was
> > >> released.
> > >>
> > >> A later commit unfortunately converted these inline helpers to exported
> > >> functions despite the fact that the unlock helper was restoring irq
> > >> flags, something which needs to be done in the same function that saved
> > >> them (e.g. on SPARC).
> > > 
> > > I'm not familiar with sparc, can you elaborate a bit what is ABI /
> > > architecture lock implementation background?
> > 
> > I remember that was a limitation a while ago to save/restore flags from
> > the same function. Though, I vaguely remember the reason.
> > I don't see this limitation in Documentation/*
> 
> It's described in both LDD3 and LKD, which is possibly where I first
> picked it up too (admittedly a long time ago).
> 
> > Google suggests that it's related to storage location:
> > https://stackoverflow.com/a/34279032
> 
> No, that was never the issue.
> 
> SPARC includes the current register window in those flags, which at
> least had to be restored in the same stack frame.
> 
> > Looking into arch/sparc I also can't catch if it's still a limitation.
> 
> Yeah, looking closer at the current implementation it seems this is no
> longer an issue on SPARC.
> 
> > Also, looking around, xa_unlock_irqrestore() is called not from the same
> > function. Maybe this issue is in history?
> 
> xa_unlock_irqrestore() is just a macro for spin_unlock_irqsave() it
> seems, so not a counter example.
>
> > Also, some comments would be nice near functions in the header.
> 
> Agreed. Let me respin this and either merge this with the next patch or
> at least amend the commit message.

I stand corrected; this appears to longer be an issue (on any arch)
as we these days have other common code passing the flags argument
around like this.

I'll respin.

Johan