Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Joe Peterson <joe@skyrush.com>]

Andrew Morton wrote:
> On Mon, 08 Sep 2008 10:11:46 -0600
> Joe Peterson <joe@skyrush.com> wrote:
> 
>> +	spin_lock_irqsave(&tty->echo_lock, flags);
>>  	lock_kernel();
> 
> Taking a spinlock outside lock_kernel() isn't good, and is quite unusual.
> - It might be ab/ba deadlockable (I didn't check) (I trust you always
>   test with lockdep enabled?)

Indeed - and, as Alan said, lock_kernel() can sleep (a nuance I had not
realized until looking more into the kernel locking mechanisms just
now).  Although I have seen no issues during testing (and I do have
lockdep in the kernel), you are 100% right.

I had wanted to keep from disturbing the locking situation in n_tty, but
maybe it is time to get rid of the BKL there.

My echo buffer patches actually isolate the tty column state stuff to
the output processing functions now anyway, so the BLK may not really be
necessary at this point.  This inspires me to look into this.

> swapping the above two lines would presumably be an easy fix, but one
> wonders whether we still need lock_kernel() in there once you've added
> this lock.

I don't think this is a good idea either, since I don't want to spinlock
during the output processing, which calls the driver output func.  I
think a mutex is more appropriate anyway (and there are some already
defined and in use for tty write locking, etc.) - let me know if you
think otherwise.  I will play around with this and re-post a patch for
review soon.

						-Thanks, Joe