* [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
@ 2017-05-30 12:37 Andrey Smirnov
2017-05-30 12:59 ` Rob Herring
0 siblings, 1 reply; 9+ messages in thread
From: Andrey Smirnov @ 2017-05-30 12:37 UTC (permalink / raw)
To: linux-serial
Cc: Andrey Smirnov, cphealy, Peter Senna Tschudin,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel
Trying to load a serdev driver agains a tty port on i.MX6Q results in
the following lockdep warning:
kworker/u8:1/100 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
(&(&tty->files_lock)->rlock){+.+...}, at: [<c04d3d4c>] imx_startup+0x2c0/0x50c
and this task is already holding:
(&port_lock_key){-.....}, at: [<c04d3b98>] imx_startup+0x10c/0x50c
which would create a new lock dependency:
(&port_lock_key){-.....} -> (&(&tty->files_lock)->rlock){+.+...}
but this new dependency connects a HARDIRQ-irq-safe lock:
(&port_lock_key){-.....}
... which became HARDIRQ-irq-safe at:
lock_acquire+0x74/0x94
_raw_spin_lock_irqsave+0x40/0x54
imx_txint+0x18/0x1d8
imx_int+0xc0/0x21c
__handle_irq_event_percpu+0x8c/0x124
handle_irq_event_percpu+0x24/0x60
handle_irq_event+0x40/0x64
handle_fasteoi_irq+0xd4/0x1ac
generic_handle_irq+0x28/0x3c
__handle_domain_irq+0x6c/0xe8
gic_handle_irq+0x58/0xb8
__irq_svc+0x70/0x98
cpuidle_enter_state+0x18c/0x2b8
cpuidle_enter+0x1c/0x20
call_cpuidle+0x28/0x44
do_idle+0x1b0/0x224
cpu_startup_entry+0x20/0x24
rest_init+0x128/0x168
start_kernel+0x328/0x39c
0x1000807c
to a HARDIRQ-irq-unsafe lock:
(&(&tty->files_lock)->rlock){+.+...}
... which became HARDIRQ-irq-unsafe at:
...
lock_acquire+0x74/0x94
_raw_spin_lock+0x30/0x40
tty_add_file+0x28/0x50
tty_open+0x9c/0x490
chrdev_open+0xa4/0x180
do_dentry_open+0x1f0/0x318
vfs_open+0x54/0x84
path_openat+0x32c/0xfbc
do_filp_open+0x68/0xcc
do_sys_open+0x108/0x1d0
SyS_open+0x20/0x24
kernel_init_freeable+0x15c/0x200
kernel_init+0x10/0x11c
ret_from_fork+0x14/0x24
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&(&tty->files_lock)->rlock);
local_irq_disable();
lock(&port_lock_key);
lock(&(&tty->files_lock)->rlock);
<Interrupt>
lock(&port_lock_key);
*** DEADLOCK ***
In order to avoid this problem, change the code to opportunisticaly
take 'tty->files_lock' by using spin_trylock() in the offending
codepath instead.
Fixes: 18a4208826dd0a13eb06de724c86bba2c225f943 ("imx-serial: Reduce
RX DMA startup latency when opening for reading")
Cc: cphealy@gmail.com
Cc: Peter Senna Tschudin <peter.senna@collabora.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
Not sure if this is the best way to solve the problem (hence the RFC
tag). If anyone has a better idea, or if there's a better fix for this
already, please let me know.
Thanks,
Andrey Smirnov
drivers/tty/serial/imx.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 33509b4..24fe7fc 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1350,14 +1350,14 @@ static int imx_startup(struct uart_port *port)
struct tty_file_private *file_priv;
int readcnt = 0;
- spin_lock(&tty->files_lock);
+ if (spin_trylock(&tty->files_lock)) {
+ if (!list_empty(&tty->tty_files))
+ list_for_each_entry(file_priv, &tty->tty_files, list)
+ if (!(file_priv->file->f_flags & O_WRONLY))
+ readcnt++;
- if (!list_empty(&tty->tty_files))
- list_for_each_entry(file_priv, &tty->tty_files, list)
- if (!(file_priv->file->f_flags & O_WRONLY))
- readcnt++;
-
- spin_unlock(&tty->files_lock);
+ spin_unlock(&tty->files_lock);
+ }
if (readcnt > 0) {
imx_disable_rx_int(sport);
--
2.9.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
2017-05-30 12:37 [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically Andrey Smirnov
@ 2017-05-30 12:59 ` Rob Herring
2017-05-30 13:42 ` Alan Cox
0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2017-05-30 12:59 UTC (permalink / raw)
To: Andrey Smirnov
Cc: linux-serial, Chris Healy, Peter Senna Tschudin,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel
On Tue, May 30, 2017 at 7:37 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Trying to load a serdev driver agains a tty port on i.MX6Q results in
> the following lockdep warning:
>
> kworker/u8:1/100 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> (&(&tty->files_lock)->rlock){+.+...}, at: [<c04d3d4c>] imx_startup+0x2c0/0x50c
>
> and this task is already holding:
> (&port_lock_key){-.....}, at: [<c04d3b98>] imx_startup+0x10c/0x50c
> which would create a new lock dependency:
> (&port_lock_key){-.....} -> (&(&tty->files_lock)->rlock){+.+...}
>
> but this new dependency connects a HARDIRQ-irq-safe lock:
> (&port_lock_key){-.....}
>
> ... which became HARDIRQ-irq-safe at:
> lock_acquire+0x74/0x94
> _raw_spin_lock_irqsave+0x40/0x54
> imx_txint+0x18/0x1d8
> imx_int+0xc0/0x21c
> __handle_irq_event_percpu+0x8c/0x124
> handle_irq_event_percpu+0x24/0x60
> handle_irq_event+0x40/0x64
> handle_fasteoi_irq+0xd4/0x1ac
> generic_handle_irq+0x28/0x3c
> __handle_domain_irq+0x6c/0xe8
> gic_handle_irq+0x58/0xb8
> __irq_svc+0x70/0x98
> cpuidle_enter_state+0x18c/0x2b8
> cpuidle_enter+0x1c/0x20
> call_cpuidle+0x28/0x44
> do_idle+0x1b0/0x224
> cpu_startup_entry+0x20/0x24
> rest_init+0x128/0x168
> start_kernel+0x328/0x39c
> 0x1000807c
>
> to a HARDIRQ-irq-unsafe lock:
> (&(&tty->files_lock)->rlock){+.+...}
>
> ... which became HARDIRQ-irq-unsafe at:
> ...
> lock_acquire+0x74/0x94
> _raw_spin_lock+0x30/0x40
> tty_add_file+0x28/0x50
> tty_open+0x9c/0x490
> chrdev_open+0xa4/0x180
> do_dentry_open+0x1f0/0x318
> vfs_open+0x54/0x84
> path_openat+0x32c/0xfbc
> do_filp_open+0x68/0xcc
> do_sys_open+0x108/0x1d0
> SyS_open+0x20/0x24
> kernel_init_freeable+0x15c/0x200
> kernel_init+0x10/0x11c
> ret_from_fork+0x14/0x24
>
> other info that might help us debug this:
>
> Possible interrupt unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&(&tty->files_lock)->rlock);
> local_irq_disable();
> lock(&port_lock_key);
> lock(&(&tty->files_lock)->rlock);
> <Interrupt>
> lock(&port_lock_key);
>
> *** DEADLOCK ***
>
> In order to avoid this problem, change the code to opportunisticaly
> take 'tty->files_lock' by using spin_trylock() in the offending
> codepath instead.
>
> Fixes: 18a4208826dd0a13eb06de724c86bba2c225f943 ("imx-serial: Reduce
> RX DMA startup latency when opening for reading")
>
> Cc: cphealy@gmail.com
> Cc: Peter Senna Tschudin <peter.senna@collabora.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>
> Not sure if this is the best way to solve the problem (hence the RFC
> tag). If anyone has a better idea, or if there's a better fix for this
> already, please let me know.
IMO, the low level serial drivers shouldn't be accessing
tty->tty_files in the first place. Is being opened for write-only that
common and is skipping the DMA setup really necessary?
Rob
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
2017-05-30 12:59 ` Rob Herring
@ 2017-05-30 13:42 ` Alan Cox
2017-05-30 13:44 ` Peter Senna Tschudin
0 siblings, 1 reply; 9+ messages in thread
From: Alan Cox @ 2017-05-30 13:42 UTC (permalink / raw)
To: Rob Herring
Cc: Andrey Smirnov, linux-serial, Chris Healy, Peter Senna Tschudin,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel
> > Fixes: 18a4208826dd0a13eb06de724c86bba2c225f943 ("imx-serial: Reduce
> > RX DMA startup latency when opening for reading")
> >
> > Cc: cphealy@gmail.com
> > Cc: Peter Senna Tschudin <peter.senna@collabora.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >
> > Not sure if this is the best way to solve the problem (hence the RFC
> > tag). If anyone has a better idea, or if there's a better fix for this
> > already, please let me know.
>
> IMO, the low level serial drivers shouldn't be accessing
> tty->tty_files in the first place. Is being opened for write-only that
> common and is skipping the DMA setup really necessary?
Seconded - the Reduce RX DMA startup latency patch should just be
reverted (and shouldn't ever IMHO have gotten in).
Not all readers and writers to a tty have a file handle any more anyway,
so it's not only icky and layer violating it's fundamentally broken
beyond the locking.
Alan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
2017-05-30 13:42 ` Alan Cox
@ 2017-05-30 13:44 ` Peter Senna Tschudin
2017-06-03 9:30 ` Greg Kroah-Hartman
0 siblings, 1 reply; 9+ messages in thread
From: Peter Senna Tschudin @ 2017-05-30 13:44 UTC (permalink / raw)
To: Alan Cox
Cc: Rob Herring, Andrey Smirnov, linux-serial, Chris Healy,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel
On Tue, May 30, 2017 at 02:42:13PM +0100, Alan Cox wrote:
I sent a second patch recently:
https://patchwork.kernel.org/patch/9725625/
> > > Fixes: 18a4208826dd0a13eb06de724c86bba2c225f943 ("imx-serial: Reduce
> > > RX DMA startup latency when opening for reading")
> > >
> > > Cc: cphealy@gmail.com
> > > Cc: Peter Senna Tschudin <peter.senna@collabora.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Jiri Slaby <jslaby@suse.com>
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > ---
> > >
> > > Not sure if this is the best way to solve the problem (hence the RFC
> > > tag). If anyone has a better idea, or if there's a better fix for this
> > > already, please let me know.
> >
> > IMO, the low level serial drivers shouldn't be accessing
> > tty->tty_files in the first place. Is being opened for write-only that
> > common and is skipping the DMA setup really necessary?
>
> Seconded - the Reduce RX DMA startup latency patch should just be
> reverted (and shouldn't ever IMHO have gotten in).
>
> Not all readers and writers to a tty have a file handle any more anyway,
> so it's not only icky and layer violating it's fundamentally broken
> beyond the locking.
>
> Alan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
2017-05-30 13:44 ` Peter Senna Tschudin
@ 2017-06-03 9:30 ` Greg Kroah-Hartman
2017-06-12 0:33 ` Fabio Estevam
0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2017-06-03 9:30 UTC (permalink / raw)
To: Peter Senna Tschudin
Cc: Alan Cox, Rob Herring, Andrey Smirnov, linux-serial, Chris Healy,
Jiri Slaby, linux-kernel
On Tue, May 30, 2017 at 03:44:27PM +0200, Peter Senna Tschudin wrote:
> On Tue, May 30, 2017 at 02:42:13PM +0100, Alan Cox wrote:
>
> I sent a second patch recently:
>
> https://patchwork.kernel.org/patch/9725625/
And it's already in linux-next so all should be good here.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
2017-06-03 9:30 ` Greg Kroah-Hartman
@ 2017-06-12 0:33 ` Fabio Estevam
2017-06-12 6:46 ` Greg Kroah-Hartman
0 siblings, 1 reply; 9+ messages in thread
From: Fabio Estevam @ 2017-06-12 0:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Peter Senna Tschudin, Alan Cox, Rob Herring, Andrey Smirnov,
linux-serial, Chris Healy, Jiri Slaby, linux-kernel
Hi Greg,
On Sat, Jun 3, 2017 at 6:30 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, May 30, 2017 at 03:44:27PM +0200, Peter Senna Tschudin wrote:
>> On Tue, May 30, 2017 at 02:42:13PM +0100, Alan Cox wrote:
>>
>> I sent a second patch recently:
>>
>> https://patchwork.kernel.org/patch/9725625/
>
> And it's already in linux-next so all should be good here.
linux-next is good, but mainline is not.
Actually we need 4dec2f119e86f9c9 ("imx-serial: RX DMA startup
latency") to be applied to 4.12-rc as well.
Thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
2017-06-12 0:33 ` Fabio Estevam
@ 2017-06-12 6:46 ` Greg Kroah-Hartman
2017-06-12 11:55 ` Fabio Estevam
0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2017-06-12 6:46 UTC (permalink / raw)
To: Fabio Estevam
Cc: Peter Senna Tschudin, Alan Cox, Rob Herring, Andrey Smirnov,
linux-serial, Chris Healy, Jiri Slaby, linux-kernel
On Sun, Jun 11, 2017 at 09:33:13PM -0300, Fabio Estevam wrote:
> Hi Greg,
>
> On Sat, Jun 3, 2017 at 6:30 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, May 30, 2017 at 03:44:27PM +0200, Peter Senna Tschudin wrote:
> >> On Tue, May 30, 2017 at 02:42:13PM +0100, Alan Cox wrote:
> >>
> >> I sent a second patch recently:
> >>
> >> https://patchwork.kernel.org/patch/9725625/
> >
> > And it's already in linux-next so all should be good here.
>
> linux-next is good, but mainline is not.
>
> Actually we need 4dec2f119e86f9c9 ("imx-serial: RX DMA startup
> latency") to be applied to 4.12-rc as well.
Why? Can't it wait until 4.13-rc1?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
2017-06-12 6:46 ` Greg Kroah-Hartman
@ 2017-06-12 11:55 ` Fabio Estevam
2017-06-12 12:04 ` Greg Kroah-Hartman
0 siblings, 1 reply; 9+ messages in thread
From: Fabio Estevam @ 2017-06-12 11:55 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Peter Senna Tschudin, Alan Cox, Rob Herring, Andrey Smirnov,
linux-serial, Chris Healy, Jiri Slaby, linux-kernel
Hi Greg,
On Mon, Jun 12, 2017 at 3:46 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>> linux-next is good, but mainline is not.
>>
>> Actually we need 4dec2f119e86f9c9 ("imx-serial: RX DMA startup
>> latency") to be applied to 4.12-rc as well.
>
> Why? Can't it wait until 4.13-rc1?
Because without 4dec2f119e86f9c9 the imx serial driver behaves badly
in 4.12-rc.
Just tested 4.12-rc5 and there is scary verbose warning:
[ 11.714904] =====================================================
[ 11.721006] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
[ 11.727628] 4.12.0-rc5 #413 Not tainted
[ 11.731470] -----------------------------------------------------
[ 11.737572] rawtest/219 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
[ 11.744018] (&(&tty->files_lock)->rlock){+.+...}, at: [<c04d83a0>]
imx_startup+0x2c4/0x510
[ 11.752403]
[ 11.752403] and this task is already holding:
[ 11.758241] (&port_lock_key){-.....}, at: [<c04d81e8>]
imx_startup+0x10c/0x510
[ 11.765571] which would create a new lock dependency:
[ 11.770626] (&port_lock_key){-.....} -> (&(&tty->files_lock)->rlock){+.+...}
[ 11.777791]
[ 11.777791] but this new dependency connects a HARDIRQ-irq-safe lock:
[ 11.785712] (&port_lock_key){-.....}
[ 11.785726]
....
Complete stack at: https://pastebin.com/a2gUTwZK
This problem is caused by commit 18a4208826dd0 ("imx-serial: Reduce RX
DMA startup latency when opening for reading").
linux-next has a fix for it: 4dec2f119e86f9c9 ("imx-serial: RX DMA
startup latency")
Applying 4dec2f119e86f9c9 ("imx-serial: RX DMA startup latency") into
4.12-rc5 makes the issue go away.
Thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
2017-06-12 11:55 ` Fabio Estevam
@ 2017-06-12 12:04 ` Greg Kroah-Hartman
0 siblings, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2017-06-12 12:04 UTC (permalink / raw)
To: Fabio Estevam
Cc: Peter Senna Tschudin, Alan Cox, Rob Herring, Andrey Smirnov,
linux-serial, Chris Healy, Jiri Slaby, linux-kernel
On Mon, Jun 12, 2017 at 08:55:44AM -0300, Fabio Estevam wrote:
> Hi Greg,
>
> On Mon, Jun 12, 2017 at 3:46 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>
> >> linux-next is good, but mainline is not.
> >>
> >> Actually we need 4dec2f119e86f9c9 ("imx-serial: RX DMA startup
> >> latency") to be applied to 4.12-rc as well.
> >
> > Why? Can't it wait until 4.13-rc1?
>
> Because without 4dec2f119e86f9c9 the imx serial driver behaves badly
> in 4.12-rc.
>
> Just tested 4.12-rc5 and there is scary verbose warning:
>
> [ 11.714904] =====================================================
> [ 11.721006] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
> [ 11.727628] 4.12.0-rc5 #413 Not tainted
> [ 11.731470] -----------------------------------------------------
> [ 11.737572] rawtest/219 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> [ 11.744018] (&(&tty->files_lock)->rlock){+.+...}, at: [<c04d83a0>]
> imx_startup+0x2c4/0x510
> [ 11.752403]
> [ 11.752403] and this task is already holding:
> [ 11.758241] (&port_lock_key){-.....}, at: [<c04d81e8>]
> imx_startup+0x10c/0x510
> [ 11.765571] which would create a new lock dependency:
> [ 11.770626] (&port_lock_key){-.....} -> (&(&tty->files_lock)->rlock){+.+...}
> [ 11.777791]
> [ 11.777791] but this new dependency connects a HARDIRQ-irq-safe lock:
> [ 11.785712] (&port_lock_key){-.....}
> [ 11.785726]
> ....
>
> Complete stack at: https://pastebin.com/a2gUTwZK
>
> This problem is caused by commit 18a4208826dd0 ("imx-serial: Reduce RX
> DMA startup latency when opening for reading").
>
> linux-next has a fix for it: 4dec2f119e86f9c9 ("imx-serial: RX DMA
> startup latency")
>
> Applying 4dec2f119e86f9c9 ("imx-serial: RX DMA startup latency") into
> 4.12-rc5 makes the issue go away.
Ok, if I end up with some more tty/serial fixes for 4.12-final, I'll
consider queueing this one up as well, thanks.
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-12 12:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-30 12:37 [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically Andrey Smirnov
2017-05-30 12:59 ` Rob Herring
2017-05-30 13:42 ` Alan Cox
2017-05-30 13:44 ` Peter Senna Tschudin
2017-06-03 9:30 ` Greg Kroah-Hartman
2017-06-12 0:33 ` Fabio Estevam
2017-06-12 6:46 ` Greg Kroah-Hartman
2017-06-12 11:55 ` Fabio Estevam
2017-06-12 12:04 ` Greg Kroah-Hartman
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.