Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops
@ 2019-11-24 15:43 Leo Yan
  2019-11-24 15:43 ` [PATCH 2/2] tty: serial: msm_serial: Fix deadlock caused by recursive output Leo Yan
  2019-11-24 23:00 ` [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops Jeffrey Hugo
  0 siblings, 2 replies; 4+ messages in thread
From: Leo Yan @ 2019-11-24 15:43 UTC (permalink / raw)
  To: Andy Gross, Greg Kroah-Hartman, Jiri Slaby, Bjorn Andersson,
	Stephen Boyd, Nicolas Dechesne, linux-arm-msm, linux-serial,
	linux-kernel
  Cc: Leo Yan

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.

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);
+
 	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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] tty: serial: msm_serial: Fix deadlock caused by recursive output
  2019-11-24 15:43 [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops Leo Yan
@ 2019-11-24 15:43 ` Leo Yan
  2019-11-24 23:00 ` [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops Jeffrey Hugo
  1 sibling, 0 replies; 4+ messages in thread
From: Leo Yan @ 2019-11-24 15:43 UTC (permalink / raw)
  To: Andy Gross, Greg Kroah-Hartman, Jiri Slaby, Bjorn Andersson,
	Stephen Boyd, Nicolas Dechesne, linux-arm-msm, linux-serial,
	linux-kernel
  Cc: Leo Yan

The uart driver might run into deadlock caused by recursive output; the
basic flow is after uart driver has acquired the port spinlock, then it
invokes some functions, e.g. dma engine operations and allocate dma
descriptor with kzalloc(), if the system has very less free memory, the
kernel will give out warning by printing logs, thus recursive output
will happen and at the second time the attempting to acquire lock will
cause deadlock.  The detailed flow is shown as below:

  msm_uart_irq()
    spin_lock_irqsave(&port->lock, flags)  => First time to acquire lock
    msm_handle_tx(port)
      msm_handle_tx_dma()
        dmaengine_prep_slave_single()
          bam_prep_slave_sg()
            kzalloc()
               __kmalloc()
                 ___slab_alloc()
                   alloc_pages_current()
                     __alloc_pages_nodemask()
                       warn_alloc()
                         printk()
                           msm_console_write()
                             __msm_console_write()
                               spin_lock(&port->lock) => Cause deadlock

This patch fixes the deadlock issue for recursive output; it adds a
variable 'curr_user' to indicate the uart port is used by which CPU, if
the CPU has acquired spinlock and wants to execute recursive output,
it will directly bail out.  Here we don't choose to avoid locking and
print out log, the reason is in this case we don't want to reset the
uart port with function msm_reset_dm_count(); otherwise it can introduce
confliction with other flows and results in uart port malfunction and
later cannot output anymore.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/tty/serial/msm_serial.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 889538182e83..06076cd2948f 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -182,6 +182,7 @@ struct msm_port {
 	bool			break_detected;
 	struct msm_dma		tx_dma;
 	struct msm_dma		rx_dma;
+	struct cpumask		curr_user;
 };
 
 #define UART_TO_MSM(uart_port)	container_of(uart_port, struct msm_port, uart)
@@ -436,6 +437,7 @@ static void msm_complete_tx_dma(void *args)
 	u32 val;
 
 	spin_lock_irqsave(&port->lock, flags);
+	cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
 
 	/* Already stopped */
 	if (!dma->count)
@@ -470,6 +472,7 @@ static void msm_complete_tx_dma(void *args)
 
 	msm_handle_tx(port);
 done:
+	cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 
@@ -544,6 +547,7 @@ static void msm_complete_rx_dma(void *args)
 	u32 val;
 
 	spin_lock_irqsave(&port->lock, flags);
+	cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
 
 	/* Already stopped */
 	if (!dma->count)
@@ -590,6 +594,7 @@ static void msm_complete_rx_dma(void *args)
 
 	msm_start_rx_dma(msm_port);
 done:
+	cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	if (count)
@@ -931,6 +936,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id)
 	u32 val;
 
 	spin_lock_irqsave(&port->lock, flags);
+	cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
 	misr = msm_read(port, UART_MISR);
 	msm_write(port, 0, UART_IMR); /* disable interrupt */
 
@@ -962,6 +968,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id)
 		msm_handle_delta_cts(port);
 
 	msm_write(port, msm_port->imr, UART_IMR); /* restore interrupt */
+	cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	return IRQ_HANDLED;
@@ -1572,6 +1579,7 @@ static inline struct uart_port *msm_get_port_from_line(unsigned int line)
 static void __msm_console_write(struct uart_port *port, const char *s,
 				unsigned int count, bool is_uartdm)
 {
+	struct msm_port *msm_port = UART_TO_MSM(port);
 	int i;
 	int num_newlines = 0;
 	bool replaced = false;
@@ -1593,6 +1601,8 @@ static void __msm_console_write(struct uart_port *port, const char *s,
 		locked = 0;
 	else if (oops_in_progress)
 		locked = spin_trylock(&port->lock);
+	else if (cpumask_test_cpu(smp_processor_id(), &msm_port->curr_user))
+		return;
 	else
 		spin_lock(&port->lock);
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops
  2019-11-24 15:43 [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops Leo Yan
  2019-11-24 15:43 ` [PATCH 2/2] tty: serial: msm_serial: Fix deadlock caused by recursive output Leo Yan
@ 2019-11-24 23:00 ` Jeffrey Hugo
  2019-11-25  1:21   ` Leo Yan
  1 sibling, 1 reply; 4+ messages in thread
From: Jeffrey Hugo @ 2019-11-24 23:00 UTC (permalink / raw)
  To: Leo Yan
  Cc: Andy Gross, Greg Kroah-Hartman, Jiri Slaby, Bjorn Andersson,
	Stephen Boyd, Nicolas Dechesne, MSM, linux-serial, lkml

On Sun, Nov 24, 2019 at 8:44 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.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Shouldn't this patch have a Fixes tag?

Was there a cover letter?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops
  2019-11-24 23:00 ` [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops Jeffrey Hugo
@ 2019-11-25  1:21   ` Leo Yan
  0 siblings, 0 replies; 4+ messages in thread
From: Leo Yan @ 2019-11-25  1:21 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Andy Gross, Greg Kroah-Hartman, Jiri Slaby, Bjorn Andersson,
	Stephen Boyd, Nicolas Dechesne, MSM, linux-serial, lkml

Hi Jeffrey,

On Sun, Nov 24, 2019 at 04:00:21PM -0700, Jeffrey Hugo wrote:
> On Sun, Nov 24, 2019 at 8:44 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.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> 
> Shouldn't this patch have a Fixes tag?

Will add fixes tag in next spin.

> Was there a cover letter?

Okay, I will add cover letter for patch set v2.

Thanks for reviewing!
Leo

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-24 15:43 [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops Leo Yan
2019-11-24 15:43 ` [PATCH 2/2] tty: serial: msm_serial: Fix deadlock caused by recursive output Leo Yan
2019-11-24 23:00 ` [PATCH 1/2] tty: serial: msm_serial: Fix lockup for sysrq and oops Jeffrey Hugo
2019-11-25  1:21   ` Leo Yan

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git