Re: [PATCH V2 00/25] tty: serial: drop uart_port->lock before calling

[Peter Hurley <peter@hurleysoftware.com>]

On 08/19/2013 08:18 PM, Viresh Kumar wrote:
> Hi Peter,
>
> On 19 August 2013 21:01, Peter Hurley <peter@hurleysoftware.com> wrote:
>
>> Please review commit 677fe555cbfb188af58cce105f4dae9505e58c31
>> 'serial: imx: Fix recursive locking bug' to see if that is actually the
>> problem
>> you're having with the samsung serial driver.
>
> No, this doesn't seem to be the same issue... The issue here is:
>
> s3c24xx_serial_rx_chars()
> -> spin_lock_irqsave(&port->lock, flags);
> -> tty_flip_buffer_push()
>    -> flush_to_ldisc()
>      -> n_tty_receive_buf2()
>        -> __receive_buf()
>           -> uart_start()
>              -> spin_lock_irqsave(&port->lock, flags);

Nope.

First, you have two stack traces from your previous message.
The BUG report is the second stack trace:

worker_thread
   process_one_work
     flush_to_ldisc
       n_tty_receive_buf2
         __receive_buf
           uart_start
             raw_spin_lock_irqsave(&port->lock)

IOW, no lock recursion because it's running on a separate
kworker thread, and did not arrive from s3c24xx_serial_rx_chars()
(other than indirectly via schedule_work()).

It's BUGing because the uart_port spinlock cannot be acquired
_even though apparently there is no owning CPU_.

The key to why the flush_to_disc() worker thread cannot acquire the
uart_port spinlock is in the first stack trace:

s3c24xx_serial_rx_chars
   raw_spin_unlock_irqrestore
     do_raw_spin_unlock
       dump_stack

So the real question here is why do_raw_spin_unlock() is doing
a dump_stack()?  Is one of the SPIN_BUG_ON() asserts in debug_spin_unlock()
triggering?

> This seems to be a generic enough problem as many drivers are already
> doing the right thing. They release lock before calling
> tty_flip_buffer_push()..

No. Those drivers are carrying vestige code from the original 2.6
tree import.

As documented in tty_flip_buffer_push():

  *	Queue a push of the terminal flip buffers to the line discipline. This
  *	function must not be called from IRQ context if port->low_latency is
  *	set.

IOW, s3c24xx_serial_rx_chars() executing in IRQ context cannot call
flush_to_ldisc() -- the work must be scheduled instead. Since flush_to_ldisc()
will be scheduled on a separate kworker thread, the uart_port spin lock will
not be recursively claimed.

So, recursive locking of the uart_port lock is not the problem
(at least, not wrt the flush_to_ldisc() call chain).

Regards,
Peter Hurley