serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250

I started out this series trying to make sysrq work over the serial console on qcom_geni_serial, then fell into a rat's nest.

To solve the deadlock I faced when enabling sysrq I tried to borrow code from '8250_port.c' which avoided grabbing the port lock in console_write(). ...but since these days I try to run with lockdep on all the time, I found it caused an annoying lockdep splat (which I also reproduced on my rk3399 board). ...so I instead changed my qcom_geni_serial solution to borrow code from 'msm_serial.c'

I wasn't super happy with the solution in 'msm_serial.c' though. I don't like releasing the spinlock there. Not only is it ugly but it means we are unlocking / re-locking _all the time_ even though sysrq characters are rare. ...so I came up with what I think is a better solution and then implemented it for qcom_geni_serial.

Since I had a good way to test 8250-based UARTs, I also fixed that driver to use my new method. When doing so, I ran into a missing include in serial_core.h. NOTE: I didn't have a way to test msm_serial.c at all, so I didn't switch that (or all other serial drivers for that matter) to the new method.

NOTE: from a serial point of view v2 is the same as v1 but I've removed the extra kgdb-related patches and made it obvious that this is really for all sysrq, not just kgdb. I've also generally tried to curate the CCs more properly.

Douglas Anderson (5):
serial: qcom_geni_serial: Finish supporting sysrq
serial: core: Allow processing sysrq at port unlock time
serial: qcom_geni_serial: Process sysrq at port unlock time
serial: core: Include console.h from serial_core.h
serial: 8250: Process sysrq at port unlock time

 drivers/tty/serial/8250/8250_aspeed_vuart.c |  6 +++-
 drivers/tty/serial/8250/8250_fsl.c          |  6 +++-
 drivers/tty/serial/8250/8250_omap.c         |  6 +++-
 drivers/tty/serial/8250/8250_port.c         |  8 ++---
 drivers/tty/serial/qcom_geni_serial.c       | 10 ++++--
 include/linux/serial_core.h                 | 38 ++++++++++++++++++++-
 6 files changed, 63 insertions(+), 11 deletions(-)

2.19.1.568.g152ad8e336-goog

The geni serial driver already had some sysrq code in it, but since SUPPORT_SYSRQ wasn't defined the code didn't do anything useful. Let's make it useful by adding that define using the same formula found in other serial drivers.

In order to prevent deadlock, we'll take a page from the 'msm_serial.c' where the spinlock is released around uart_handle_sysrq_char(). This seemed better than copying from '8250_port.c' where we skip locking in the console_write function since the '8250_port.c' method can cause lockdep warnings when dropping into kgdb.

---

 drivers/tty/serial/qcom_geni_serial.c | 7 +++++++
 1 file changed, 7 insertions(+)

index 1515074e18fb..b83e3554bced 100644
@@ -1,6 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
 
+#if defined(CONFIG_SERIAL_QCOM_GENI_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+# define SUPPORT_SYSRQ
+#endif
+
 #include <linux/clk.h>
 #include <linux/console.h>
 #include <linux/io.h>

@@ -495,7 +499,10 @@ static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
 					continue;
 			}
 
+			spin_unlock(&uport->lock);
 			sysrq = uart_handle_sysrq_char(uport, buf[c]);
+			spin_lock(&uport->lock);
+
 			if (!sysrq)
 				tty_insert_flip_char(tport, buf[c], TTY_NORMAL);
 		}

2.19.1.568.g152ad8e336-goog

Right now serial drivers process sysrq keys deep in their character receiving code. This means that they've already grabbed their port->lock spinlock. This can end up getting in the way if we've go to do serial stuff (especially kgdb) in response to the sysrq.

Serial drivers have various hacks in them to handle this. Looking at '8250_port.c' you can see that the console_write() skips locking if we're in the sysrq handler. Looking at 'msm_serial.c' you can see that the port lock is dropped around uart_handle_sysrq_char().

It turns out that these hacks aren't exactly perfect. If you have lockdep turned on and use something like the 8250_port hack you'll get a splat that looks like:

WARNING: possible circular locking dependency detected [...] is trying to acquire lock:
... (console_owner){-.-.}, at: console_unlock+0x2e0/0x5e4

but task is already holding lock:
... (&port_lock_key){-.-.}, at: serial8250_handle_irq+0x30/0xe4

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&port_lock_key){-.-.}:

         _raw_spin_lock_irqsave+0x58/0x70
         serial8250_console_write+0xa8/0x250
         univ8250_console_write+0x40/0x4c
         console_unlock+0x528/0x5e4
         register_console+0x2c4/0x3b0
         uart_add_one_port+0x350/0x478
         serial8250_register_8250_port+0x350/0x3a8
         dw8250_probe+0x67c/0x754
         platform_drv_probe+0x58/0xa4
         really_probe+0x150/0x294
         driver_probe_device+0xac/0xe8
         __driver_attach+0x98/0xd0
         bus_for_each_dev+0x84/0xc8
         driver_attach+0x2c/0x34
         bus_add_driver+0xf0/0x1ec
         driver_register+0xb4/0x100
         __platform_driver_register+0x60/0x6c
         dw8250_platform_driver_init+0x20/0x28
	 ...

-> #0 (console_owner){-.-.}:

         lock_acquire+0x1e8/0x214
         console_unlock+0x35c/0x5e4
         vprintk_emit+0x230/0x274
         vprintk_default+0x7c/0x84
         vprintk_func+0x190/0x1bc
         printk+0x80/0xa0
         __handle_sysrq+0x104/0x21c
         handle_sysrq+0x30/0x3c
         serial8250_read_char+0x15c/0x18c
         serial8250_rx_chars+0x34/0x74
         serial8250_handle_irq+0x9c/0xe4
         dw8250_handle_irq+0x98/0xcc
         serial8250_interrupt+0x50/0xe8
         ...

other info that might help us debug this:

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ----
    lock(&port_lock_key);
                                 lock(console_owner);
                                 lock(&port_lock_key);
    lock(console_owner);

*** DEADLOCK ***

The hack used in 'msm_serial.c' doesn't cause the above splats but it seems a bit ugly to unlock / lock our spinlock deep in our irq handler.

It seems like we could defer processing the sysrq until the end of the interrupt handler right after we've unlocked the port. With this scheme if a whole batch of sysrq characters comes in one irq then we won't handle them all, but that seems like it should be a fine compromise.

---

 include/linux/serial_core.h | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

index 406edae44ca3..3460b15a2607 100644
@@ -173,6 +173,7 @@ struct uart_port {
 	struct console		*cons;			/* struct console, if any */
 #if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(SUPPORT_SYSRQ)
 	unsigned long		sysrq;			/* sysrq timeout */
+	unsigned int		sysrq_ch;		/* char for sysrq */
 #endif
 
 	/* flags must be updated while holding port mutex */

@@ -482,8 +483,42 @@ uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 	}
 	return 0;
 }
+static inline int
+uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	if (port->sysrq) {
+		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;
+
+	sysrq_ch = port->sysrq_ch;
+	port->sysrq_ch = 0;
+
+	spin_unlock_irqrestore(&port->lock, irqflags);
+
+	if (sysrq_ch)
+		handle_sysrq(sysrq_ch);
+}
 #else
-#define uart_handle_sysrq_char(port,ch) ({ (void)port; 0; })
+static inline int
+uart_handle_sysrq_char(struct uart_port *port, unsigned int ch) { return 0; }
+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
 
 /*

2.19.1.568.g152ad8e336-goog

Let's take advantage of the new ("serial: core: Allow processing sysrq at port unlock time") to handle sysrqs more cleanly.

---

 drivers/tty/serial/qcom_geni_serial.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

index b83e3554bced..29ddc1fc65f8 100644
@@ -499,9 +499,7 @@ static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
 					continue;
 			}
 
-			spin_unlock(&uport->lock);
-			sysrq = uart_handle_sysrq_char(uport, buf[c]);
-			spin_lock(&uport->lock);
+			sysrq = uart_prepare_sysrq_char(uport, buf[c]);
 
 			if (!sysrq)
 				tty_insert_flip_char(tport, buf[c], TTY_NORMAL);

@@ -811,7 +809,8 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 		qcom_geni_serial_handle_rx(uport, drop_rx);
 
 out_unlock:
-	spin_unlock_irqrestore(&uport->lock, flags);
+	uart_unlock_and_check_sysrq(uport, flags);
+
 	return IRQ_HANDLED;
 }
 

2.19.1.568.g152ad8e336-goog

In the static inline function uart_handle_break() in serial_core.h we dereference port->cons. That gives an error unless console.h is also included.

This error hasn't shown up till now because everyone who has defined SUPPORT_SYSRQ has also included console.h, but it's a bit ugly to make this requirement. Let's make the include explicit.

---

 include/linux/serial_core.h | 1 +
 1 file changed, 1 insertion(+)

index 3460b15a2607..ff9d0ee32f11 100644
@@ -22,6 +22,7 @@
 
 #include <linux/bitops.h>
 #include <linux/compiler.h>
+#include <linux/console.h>
 #include <linux/interrupt.h>
 #include <linux/circ_buf.h>
 #include <linux/spinlock.h>

2.19.1.568.g152ad8e336-goog

Let's take advantage of the new ("serial: core: Allow processing sysrq at port unlock time") to handle sysrqs more cleanly.

---
I don't have a great way to test all 8250 variants, but I've at least tested rk3288 / rk3399 and they seem to work. Hopefully I got the aspeed_vuart / fsl / omap variants right (I only compile tested those).

 drivers/tty/serial/8250/8250_aspeed_vuart.c | 6 +++++-
 drivers/tty/serial/8250/8250_fsl.c          | 6 +++++-
 drivers/tty/serial/8250/8250_omap.c         | 6 +++++-
 drivers/tty/serial/8250/8250_port.c         | 8 +++-----
 4 files changed, 18 insertions(+), 8 deletions(-)

index 435bec40dee6..0438d9a905ce 100644
@@ -5,6 +5,10 @@
  *    Copyright (C) 2016 Jeremy Kerr <[email protected]>, IBM Corp.
  *    Copyright (C) 2006 Arnd Bergmann <[email protected]>, IBM Corp.
  */
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/of_address.h>

@@ -293,7 +297,7 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
 	if (lsr & UART_LSR_THRE)
 		serial8250_tx_chars(up);
 
-	spin_unlock_irqrestore(&port->lock, flags);
+	uart_unlock_and_check_sysrq(port, flags);
 
 	return 1;
 }

index 6640a4c7ddd1..ff3dcaea5d93 100644
@@ -1,4 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
 #include <linux/serial_reg.h>
 #include <linux/serial_8250.h>
 

@@ -54,7 +58,7 @@ int fsl8250_handle_irq(struct uart_port *port)
 		serial8250_tx_chars(up);
 
 	up->lsr_saved_flags = orig_lsr;
-	spin_unlock_irqrestore(&up->port.lock, flags);
+	uart_unlock_and_check_sysrq(&up->port, flags);
 	return 1;
 }
 EXPORT_SYMBOL_GPL(fsl8250_handle_irq);

index a019286f8bb6..ad7ba7d0f28d 100644
@@ -8,6 +8,10 @@
  *
  */
 
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/module.h>

@@ -1085,7 +1089,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 		}
 	}
 
-	spin_unlock_irqrestore(&port->lock, flags);
+	uart_unlock_and_check_sysrq(port, flags);
 	serial8250_rpm_put(up);
 	return 1;
 }

index 3f779d25ec0c..d2f3310abe54 100644
@@ -1736,7 +1736,7 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 		else if (lsr & UART_LSR_FE)
 			flag = TTY_FRAME;
 	}
-	if (uart_handle_sysrq_char(port, ch))
+	if (uart_prepare_sysrq_char(port, ch))
 		return;
 
 	uart_insert_char(port, lsr, UART_LSR_OE, ch, flag);

@@ -1878,7 +1878,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 	if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE))
 		serial8250_tx_chars(up);
 
-	spin_unlock_irqrestore(&port->lock, flags);
+	uart_unlock_and_check_sysrq(port, flags);
 	return 1;
 }
 EXPORT_SYMBOL_GPL(serial8250_handle_irq);

@@ -3239,9 +3239,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 
 	serial8250_rpm_get(up);
 
-	if (port->sysrq)
-		locked = 0;
-	else if (oops_in_progress)
+	if (oops_in_progress)
 		locked = spin_trylock_irqsave(&port->lock, flags);
 	else
 		spin_lock_irqsave(&port->lock, flags);

2.19.1.568.g152ad8e336-goog

It seems your forgot console people to Cc.

With Best Regards,
Andy Shevchenko

Hi,

Can you be more specific, please? Which section of the "MAINTAINERS" file should I be looking at for the "console" you are thinking of? Certainly there are lots of hits for "console" in MAINTAINERS but I don't think I see any that are relevant that I missed. Grepping:

$ grep  -i console MAINTAINERS
HYPERVISOR VIRTUAL CONSOLE DRIVER
F:      drivers/video/console/sti*
PCDP - PRIMARY CONSOLE AND DEBUG PORT
SGI SN-IA64 (Altix) SERIAL CONSOLE DRIVER
STAGING - SPEAKUP CONSOLE SPEECH DRIVER
VIRTIO CONSOLE DRIVER
F:      drivers/char/virtio_console.c
F:      include/linux/virtio_console.h
F:      include/uapi/linux/virtio_console.h

...none of those seem relevant upon first glance but I'm happy to stand corrected.

Ah! Based on who you added to the CC list I guess you meant to CC "printk" folks?

PRINTK

M:      Petr Mladek <[email protected]>
M:      Sergey Senozhatsky <[email protected]>
R:      Steven Rostedt <[email protected]>
S:      Maintained
F:      kernel/printk/
F:      include/linux/printk.h

I'd be happy to CC those folks on future spins (if there are any). I'm not convinced that these patches are directly relevant to the printk subsystem, but I'm always happy for more people to have a chance to review patches.

Hopefully anyone who needs this patch can find it on one of the relevant mailing lists. I screwed up and missed LKML this time around, but there are plenty of other mailing lists here that it could be found on. If requested I'm also happy to re-post the same series adding those 3 people if that's what everyone wants.

-Doug

I have added them to Cc list: Petr, Sergey, and Steven.

Correct.

If you look retrospectively in the mailing lists, you can find that they are doing most of the console core work, which I believe includes SysRq behaviour. So, don't be confused their names are listed under PRINTK.

With Best Regards,
Andy Shevchenko

I, as a printk maintainer, am not completely familiar with all the console driver problems. But we, printk maintainers, come to similar deadlocks from the printk side, so we are definitely interested into this kind of patches.

BTW: There was an attempt to avoid the console_unlock() related deadlocks a more generic way, see
https://lore.kernel.org/lkml/20181016050428.17966-1-sergey.senozhatsky at gmail.com Unfortunately, there is some push back against introducing a new printk-related-locking API.

I have glanced over the patches via
https://www.spinics.net/lists/linux-arm-msm/msg44083.html
I still have to think about it. I will be traveling next week so it might take some time.

Anyway, please CC printk people into v2 if any.

Best Reagards,
Petr

Looks good, thanks for cleaning this up. All now queued up.

greg k-h