Linux-Serial Archive on
 help / color / Atom feed
From: Thomas Gleixner <>
To: LKML <>
Cc: Greg Kroah-Hartman <>,
	Jiri Slaby <>,
	Xinyu Chen <>,
	Dirk Behme <>,
	Shawn Guo <>,
	Tim Sander <>,
	Sascha Hauer <>,,
Subject: [PATCH] serial: imx: Fix recursive locking bug
Date: Thu, 14 Feb 2013 21:01:06 +0100 (CET)
Message-ID: <alpine.LFD.2.02.1302142006050.22263@ionos> (raw)

commit 9ec1882df2 (tty: serial: imx: console write routing is unsafe
on SMP) introduced a recursive locking bug in imx_console_write().

The callchain is:

          spin_lock_irqsave(&sport->port.lock,flags); <--- DEAD

The bad news is that the kernel debugging facilities can dectect the
problem, but the printks never surface on the serial console for
obvious reasons.

There is a similar issue with oops_in_progress. If the kernel crashes
we really don't want to be stuck on the lock and unable to tell what

In general most UP originated drivers miss these checks and nobody
ever notices because CONFIG_PROVE_LOCKING seems to be still ignored by
a large number of developers.

The solution is to avoid locking in the sysrq case and trylock in the
oops_in_progress case.

This scheme is used in other drivers as well and it would be nice if
we could move this to a common place, so the usual copy/paste/modify
bugs can be avoided.

Now there is another issue with this scheme:

CPU0 	    	     	 CPU1
			   sysrq_detection() -> sets port->sysrq
			 return from interrupt 
     if (port->sysrq)
     	avoid locking

port->sysrq is reset with the next receive character. So as long as
the port->sysrq is not reset and this can take an endless amount of
time if after the break no futher receive character follows, all
console writes happen unlocked.

While the current writer is protected against other console writers by
the console sem, it's unprotected against open/close or other
operations which fiddle with the port. That's what the above mentioned
commit tried to solve.

That's an issue in all drivers which use that scheme and unfortunately
there is no easy workaround. The only solution is to have a separate
indicator port->sysrq_cpu. uart_handle_sysrq_char() then sets it to
smp_processor_id() before calling into handle_sysrq() and resets it to
-1 after that. Then change the locking check to:

     if (port->sysrq_cpu == smp_processor_id())
     	 locked = 0;
     else if (oops_in_progress)
         locked = spin_trylock_irqsave(port->lock, flags);
  	 spin_lock_irqsave(port->lock, flags);

That would force all other cpus into the spin_lock path. Problem
solved, but that's way beyond the scope of this fix and really wants
to be implemented in a common function which calls the uart specific
write function to avoid another gazillion of hard to debug
copy/paste/modify bugs.

Reported-and-tested-by: Tim Sander <>
Signed-off-by: Thomas Gleixner <>

Index: linux-2.6/drivers/tty/serial/imx.c
--- linux-2.6.orig/drivers/tty/serial/imx.c
+++ linux-2.6/drivers/tty/serial/imx.c
@@ -1213,8 +1213,14 @@ imx_console_write(struct console *co, co
 	struct imx_port_ucrs old_ucr;
 	unsigned int ucr1;
 	unsigned long flags;
+	int locked = 1;
-	spin_lock_irqsave(&sport->port.lock, flags);
+	if (sport->port.sysrq)
+		locked = 0;
+	else if (oops_in_progress)
+		locked = spin_trylock_irqsave(&sport->port.lock, flags);
+	else
+		spin_lock_irqsave(&sport->port.lock, flags);
 	 *	First, save UCR1/2/3 and then disable interrupts
@@ -1241,7 +1247,8 @@ imx_console_write(struct console *co, co
 	imx_port_ucrs_restore(&sport->port, &old_ucr);
-	spin_unlock_irqrestore(&sport->port.lock, flags);
+	if (locked)
+		spin_unlock_irqrestore(&sport->port.lock, flags);

             reply index

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-14 20:01 Thomas Gleixner [this message]
2013-02-15 17:30 ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LFD.2.02.1302142006050.22263@ionos \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Serial Archive on

Archives are clonable:
	git clone --mirror linux-serial/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-serial linux-serial/ \
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone