[PATCH v2 0/5] serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250

[PATCH v2 0/5] serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250

[Douglas Anderson]

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

[PATCH v2 1/5] serial: qcom_geni_serial: Finish supporting sysrq

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

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

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 1515074e18fb..b83e3554bced 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -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

[PATCH v2 2/5] serial: core: Allow processing sysrq at port unlock time

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

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

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 406edae44ca3..3460b15a2607 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -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

[PATCH v2 3/5] serial: qcom_geni_serial: Process sysrq at port unlock time

[Douglas Anderson]

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

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

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

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index b83e3554bced..29ddc1fc65f8 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -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

[PATCH v2 4/5] serial: core: Include console.h from serial_core.h

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

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

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 3460b15a2607..ff9d0ee32f11 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -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

[PATCH v2 5/5] serial: 8250: Process sysrq at port unlock time

[Douglas Anderson]

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

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
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(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 435bec40dee6..0438d9a905ce 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -5,6 +5,10 @@
  *    Copyright (C) 2016 Jeremy Kerr <jk@ozlabs.org>, IBM Corp.
  *    Copyright (C) 2006 Arnd Bergmann <arnd@arndb.de>, 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;
 }
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 6640a4c7ddd1..ff3dcaea5d93 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -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);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index a019286f8bb6..ad7ba7d0f28d 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -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;
 }
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3f779d25ec0c..d2f3310abe54 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -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

[PATCH v2 0/5] serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250

[Andy Shevchenko]

On Tue, Oct 30, 2018 at 03:11:02PM -0700, Douglas Anderson wrote:
> 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.

It seems your forgot console people to Cc.

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

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 0/5] serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250

[Doug Anderson]

Hi,

On Wed, Nov 7, 2018 at 10:23 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Oct 30, 2018 at 03:11:02PM -0700, Douglas Anderson wrote:
> > 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.
>
> It seems your forgot console people to Cc.

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 <pmladek@suse.com>
M:      Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
R:      Steven Rostedt <rostedt@goodmis.org>
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

[PATCH v2 0/5] serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250

[Andy Shevchenko]

On Wed, Nov 07, 2018 at 11:26:56AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Wed, Nov 7, 2018 at 10:23 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Oct 30, 2018 at 03:11:02PM -0700, Douglas Anderson wrote:
> > > 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.
> >
> > It seems your forgot console people to Cc.
> 
> Can you be more specific, please?  Which section of the "MAINTAINERS"
> file should I be looking at for the "console" you are thinking of?

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

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

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

Correct.

> PRINTK
> M:      Petr Mladek <pmladek@suse.com>
> M:      Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> R:      Steven Rostedt <rostedt@goodmis.org>
> 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.

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.

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

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 0/5] serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250

[Petr Mladek]

On Wed 2018-11-07 11:26:56, Doug Anderson wrote:
> On Wed, Nov 7, 2018 at 10:23 AM Andy Shevchenko
> Ah!  Based on who you added to the CC list I guess you meant to CC
> "printk" folks?
> 
> PRINTK
> M:      Petr Mladek <pmladek@suse.com>
> M:      Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> R:      Steven Rostedt <rostedt@goodmis.org>
> 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.

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.


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

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

[PATCH v2 0/5] serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250

[Greg Kroah-Hartman]

On Tue, Oct 30, 2018 at 03:11:02PM -0700, Douglas Anderson wrote:
> 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.

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

greg k-h