[v2,1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops

Message ID 20191127141544.4277-2-leo.yan@linaro.org
State New
Headers show
Series
  • [v2,1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops
Related show

Commit Message

Leo Yan Nov. 27, 2019, 2:15 p.m.
As the commit 677fe555cbfb ("serial: imx: Fix recursive locking bug")
has mentioned the uart driver might cause recursive locking between
normal printing and the kernel debugging facilities (e.g. sysrq and
oops).  In the commit it gave out suggestion for fixing recursive
locking issue: "The solution is to avoid locking in the sysrq case
and trylock in the oops_in_progress case."

This patch follows the suggestion (also used the exactly same code with
other serial drivers, e.g. amba-pl011.c) to fix the recursive locking
issue, this can avoid stuck caused by deadlock and print out log for
sysrq and oops.

Fixes: 04896a77a97b ("msm_serial: serial driver for MSM7K onboard serial peripheral.")
Signed-off-by: Leo Yan <leo.yan@linaro.org>

---
 drivers/tty/serial/msm_serial.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Doug Anderson Dec. 5, 2019, 12:40 a.m. | #1
Hi,

On Wed, Nov 27, 2019 at 10:16 PM Leo Yan <leo.yan@linaro.org> wrote:
>

> As the commit 677fe555cbfb ("serial: imx: Fix recursive locking bug")

> has mentioned the uart driver might cause recursive locking between

> normal printing and the kernel debugging facilities (e.g. sysrq and

> oops).  In the commit it gave out suggestion for fixing recursive

> locking issue: "The solution is to avoid locking in the sysrq case

> and trylock in the oops_in_progress case."

>

> This patch follows the suggestion (also used the exactly same code with

> other serial drivers, e.g. amba-pl011.c) to fix the recursive locking

> issue, this can avoid stuck caused by deadlock and print out log for

> sysrq and oops.

>

> Fixes: 04896a77a97b ("msm_serial: serial driver for MSM7K onboard serial peripheral.")

> Signed-off-by: Leo Yan <leo.yan@linaro.org>

> ---

>  drivers/tty/serial/msm_serial.c | 13 +++++++++++--

>  1 file changed, 11 insertions(+), 2 deletions(-)

>

> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c

> index 3657a24913fc..889538182e83 100644

> --- a/drivers/tty/serial/msm_serial.c

> +++ b/drivers/tty/serial/msm_serial.c

> @@ -1576,6 +1576,7 @@ static void __msm_console_write(struct uart_port *port, const char *s,

>         int num_newlines = 0;

>         bool replaced = false;

>         void __iomem *tf;

> +       int locked = 1;

>

>         if (is_uartdm)

>                 tf = port->membase + UARTDM_TF;

> @@ -1588,7 +1589,13 @@ static void __msm_console_write(struct uart_port *port, const char *s,

>                         num_newlines++;

>         count += num_newlines;

>

> -       spin_lock(&port->lock);

> +       if (port->sysrq)

> +               locked = 0;

> +       else if (oops_in_progress)

> +               locked = spin_trylock(&port->lock);

> +       else

> +               spin_lock(&port->lock);


I don't have tons of experience with the "msm" serial driver, but the
above snippet tickled a memory in my brain for when I was looking at
the "qcom_geni" serial driver, which is a close cousin.

I seemed to remember that the "if (port->sysrq)" was something you
didn't want.  ...but maybe that's only if you do something like commit
336447b3298c ("serial: qcom_geni_serial: Process sysrq at port unlock
time")?  Any way you can try making a similar change to the msm driver
and see if it allow you to remove the special case for "port->sysrq"?

-Doug

Patch

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 3657a24913fc..889538182e83 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -1576,6 +1576,7 @@  static void __msm_console_write(struct uart_port *port, const char *s,
 	int num_newlines = 0;
 	bool replaced = false;
 	void __iomem *tf;
+	int locked = 1;
 
 	if (is_uartdm)
 		tf = port->membase + UARTDM_TF;
@@ -1588,7 +1589,13 @@  static void __msm_console_write(struct uart_port *port, const char *s,
 			num_newlines++;
 	count += num_newlines;
 
-	spin_lock(&port->lock);
+	if (port->sysrq)
+		locked = 0;
+	else if (oops_in_progress)
+		locked = spin_trylock(&port->lock);
+	else
+		spin_lock(&port->lock);
+
 	if (is_uartdm)
 		msm_reset_dm_count(port, count);
 
@@ -1624,7 +1631,9 @@  static void __msm_console_write(struct uart_port *port, const char *s,
 		iowrite32_rep(tf, buf, 1);
 		i += num_chars;
 	}
-	spin_unlock(&port->lock);
+
+	if (locked)
+		spin_unlock(&port->lock);
 }
 
 static void msm_console_write(struct console *co, const char *s,