| Message ID | 1604858095-12477-1-git-send-email-samuel.nobs@taitradio.com |
|---|---|
| State | New |
| Headers | show |
| Series | tty: serial: imx: fix potential deadlock | expand |
On Mon, Nov 09, 2020 at 06:54:55AM +1300, Sam Nobs wrote: > Enabling the lock dependency validator has revealed > that the way spinlocks are used in the IMX serial > port could result in a deadlock. Note that the configuration > I'm using has the IRQs forced to run in threads via > the kernel parameter threadirqs. > > 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. That's because the console functions might be called with irqs off and so in these functions the _irqsave variants are used for locking. (In general not using spin_lock_irqsave but plain spin_lock in an irq handler is fine iff there are no other functions taking the lock that might run with irqs off.) > Use spin_lock_irqsave() instead to disable interrupts > upon acquisition of the spinlock. > > Signed-off-by: Sam Nobs <samuel.nobs@taitradio.com> > --- > 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 | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index 1731d97..29ceaea 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -942,8 +942,9 @@ 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; > + unsigned long flags = 0; > > - spin_lock(&sport->port.lock); > + spin_lock_irqsave(&sport->port.lock, flags); There was an earlier commit (c974991d2620419fe21508fc4529014369d16df7) that changed spin_lock_irqsave to spin_lock under the assumption that in the irq handler irqs are disabled. I added the author and the reviewer of this patch to Cc. To prevent the next person to think this can be converted to spin_lock I'd like to have a comment here that tells about why we need the irqsave variant. Best regards Uwe
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 1731d97..29ceaea 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -942,8 +942,9 @@ 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; + unsigned long flags = 0; - spin_lock(&sport->port.lock); + spin_lock_irqsave(&sport->port.lock, flags); usr1 = imx_uart_readl(sport, USR1); usr2 = imx_uart_readl(sport, USR2); @@ -1013,7 +1014,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; }
Enabling the lock dependency validator has revealed that the way spinlocks are used in the IMX serial port could result in a deadlock. Note that the configuration I'm using has the IRQs forced to run in threads via the kernel parameter threadirqs. 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> --- 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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)