From: Sam Nobs <samuel.nobs@taitradio.com>
To: Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>,
Fabio Estevam <festevam@gmail.com>,
NXP Linux Team <linux-imx@nxp.com>,
linux-serial@vger.kernel.org, jun qian <hangdianqj@163.com>,
Barry Song <21cnbao@gmail.com>,
Sam Nobs <samuel.nobs@taitradio.com>
Subject: [PATCH v2] tty: serial: imx: fix potential deadlock
Date: Tue, 10 Nov 2020 09:50:06 +1300 [thread overview]
Message-ID: <1604955006-9363-1-git-send-email-samuel.nobs@taitradio.com> (raw)
Enabling the lock dependency validator has revealed
that the way spinlocks are used in the IMX serial
port could result in a deadlock.
Specifically, imx_uart_int() acquires a spinlock
without disabling the interrupts, meaning that another
interrupt could come along and try to acquire the same
spinlock, potentially causing the two to wait for each
other indefinitely.
Use spin_lock_irqsave() instead to disable interrupts
upon acquisition of the spinlock.
Signed-off-by: Sam Nobs <samuel.nobs@taitradio.com>
---
patch changelog:
v2 - added comment explaining why we need the irqsave variant of spin_lock(),
as suggested by Uwe
Here's the lockdep splat for reference:
================================
WARNING: inconsistent lock state
5.4.46 #1 Not tainted
--------------------------------
inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
irq/26-30860000/992 [HC0[0]:SC0[2]:HE1:SE0] takes:
ffff00006671d098 (&port_lock_key){?...}, at: imx_uart_int+0x28/0x338
{IN-HARDIRQ-W} state was registered at:
lock_acquire+0xd0/0x288
_raw_spin_lock_irqsave+0x58/0x80
imx_uart_console_write+0x1e4/0x220
console_unlock+0x2ac/0x610
vprintk_emit+0x178/0x330
vprintk_default+0x48/0x58
vprintk_func+0xe4/0x248
printk+0x70/0x90
crng_fast_load+0x1c4/0x1c8
add_interrupt_randomness+0x348/0x3e8
handle_irq_event_percpu+0x50/0x98
handle_irq_event+0x4c/0xd0
handle_fasteoi_irq+0xe0/0x188
generic_handle_irq+0x34/0x50
__handle_domain_irq+0x98/0x108
gic_handle_irq+0xd4/0x178
el1_irq+0xbc/0x180
arch_cpu_idle+0x34/0x220
default_idle_call+0x40/0x4c
do_idle+0x254/0x268
cpu_startup_entry+0x28/0x48
rest_init+0x1b4/0x284
arch_call_rest_init+0x14/0x1c
start_kernel+0x48c/0x4bc
irq event stamp: 30
hardirqs last enabled at (29): [<ffff800010b22a60>] _raw_spin_unlock_irq+0x38/0x70
hardirqs last disabled at (28): [<ffff800010b1b060>] __schedule+0xb0/0x770
softirqs last enabled at (0): [<ffff8000100b28c0>] copy_process+0x8d8/0x19b0
softirqs last disabled at (30): [<ffff8000101343f8>] irq_forced_thread_fn+0x0/0xc0
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&port_lock_key);
<Interrupt>
lock(&port_lock_key);
*** DEADLOCK ***
no locks held by irq/26-30860000/992.
stack backtrace:
CPU: 0 PID: 992 Comm: irq/26-30860000 Not tainted 5.4.46 #1
Hardware name: Tait i.MX8MM smarc-rcb (DT)
Call trace:
dump_backtrace+0x0/0x178
show_stack+0x24/0x30
dump_stack+0xdc/0x144
print_usage_bug+0x1c8/0x1e0
mark_lock+0x57c/0x740
__lock_acquire+0x684/0x16d0
lock_acquire+0xd0/0x288
_raw_spin_lock+0x44/0x58
imx_uart_int+0x28/0x338
irq_forced_thread_fn+0x40/0xc0
irq_thread+0x1ac/0x280
kthread+0x138/0x140
ret_from_fork+0x10/0x18
drivers/tty/serial/imx.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1731d97..2185184 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -942,8 +942,14 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id)
struct imx_port *sport = dev_id;
unsigned int usr1, usr2, ucr1, ucr2, ucr3, ucr4;
irqreturn_t ret = IRQ_NONE;
-
- spin_lock(&sport->port.lock);
+ unsigned long flags = 0;
+
+ /*
+ * IRQs might not be disabled upon entering this interrupt handler,
+ * e.g. when interrupt handlers are forced to be threaded. To support
+ * this scenario as well, disable IRQs when acquiring the spinlock.
+ */
+ spin_lock_irqsave(&sport->port.lock, flags);
usr1 = imx_uart_readl(sport, USR1);
usr2 = imx_uart_readl(sport, USR2);
@@ -1013,7 +1019,7 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id)
ret = IRQ_HANDLED;
}
- spin_unlock(&sport->port.lock);
+ spin_unlock_irqrestore(&sport->port.lock, flags);
return ret;
}
--
2.7.4
next reply other threads:[~2020-11-09 21:01 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-09 20:50 Sam Nobs [this message]
2020-11-09 21:26 ` [PATCH v2] tty: serial: imx: fix potential deadlock Uwe Kleine-König
2020-11-12 8:32 ` Greg KH
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1604955006-9363-1-git-send-email-samuel.nobs@taitradio.com \
--to=samuel.nobs@taitradio.com \
--cc=21cnbao@gmail.com \
--cc=festevam@gmail.com \
--cc=hangdianqj@163.com \
--cc=kernel@pengutronix.de \
--cc=linux-imx@nxp.com \
--cc=linux-serial@vger.kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.