| Message ID | 20191127141544.4277-2-leo.yan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tty: serial: msm_serial: Fix lockup issues | expand |
On Wed, Nov 27, 2019 at 7:16 AM 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> Looks sane to me Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > --- > 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); > + > 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, > -- > 2.17.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
Hi Doug, On Thu, Dec 05, 2019 at 08:40:20AM +0800, Doug Anderson wrote: > 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. Good point and thanks for sharing info. > 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")? The patch you mentioned can allow to read sysrq chars without acquiring port lock. > 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"? I was deliberately to add the case for "port->sysrq" in the function __msm_console_write() when I prepared this patch. Let's see the issue obersved: when the UART drive is deadlock by any reason, when sent break + h to test system is alive or not, sysrq tries to ouput log by calling __msm_console_write(), it tries to acquire console's spinlock but also run into deadlock; finally it doesn't have chance to output log for sysrq. P.s. Sorry my long late and didn't follow good practice to reply quickly due worked on other works. Thanks, Leo Yan
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,
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(-)