Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653185 > unrolled thread

[RFC, PATCH] imx: serial: Take tty->files_lock opportunistically

Back to article view | Back to linux.kernel

Contents

  [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-05-30 14:40 +0200
    Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically Rob Herring <robherring2@gmail.com> - 2017-05-30 15:00 +0200
      Re: [RFC, PATCH] imx: serial: Take tty->files_lock  opportunistically Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-05-30 15:50 +0200
        Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically Peter Senna Tschudin <peter.senna@collabora.com> - 2017-05-30 15:50 +0200
          Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-03 16:20 +0200

#1653185 — [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically

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

[toc] | [next] | [standalone]

#1653217

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

[toc] | [prev] | [next] | [standalone]

#1653243 — Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically

> > 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

[toc] | [prev] | [next] | [standalone]

#1653247

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

[toc] | [prev] | [next] | [standalone]

#1656835

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web