Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 12/23] tty: Kick waiters _after_ the ldisc is locked | From | Peter Hurley <> | Date | Thu, 07 Feb 2013 22:16:58 -0500 |
| |
On Thu, 2013-02-07 at 16:47 +0100, Jiri Slaby wrote: > On 02/05/2013 09:20 PM, Peter Hurley wrote: > > The question is obvious: why? > > The waiters usually don't care about ldisc.
This patch really needed a decent commit message, but this isn't a fix, per se.
While I was auditing the ldisc hangup, I realized that when a reader is woken from sleeping in n_tty_read() and loops back around because there is more room in the reader's buffer (nr > 0), input_available_p() will schedule buffer work and wait for it to complete before testing if the tty is hung up.
This change ensures that flush_to_ldisc() sees a halted ldisc and promptly returns. A blocking reader whose read_buf is empty is the most likely state when a hangup occurs.
I didn't include it tty_ldisc_halt(), because tty_set_ldisc() can't kick readers out of their i/o loops and tty_ldisc_release() has no waiters.
Even if the change is nixed, the comments should be changed; the FIXME is from before waiting for readers to exit their i/o loops.
---- separate but related discussion ----
Another thing to note is this really does nothing to address the problem of a slave pty being unable to read all the data written by the master, if the master is closed with a lot of data in the tty buffer. In this case, when the slave is hungup as a result of the master closing, it's ldisc will be halted, preventing the tty_buffer from pushing more data.
In my opinion, this is an important fixme. Almost as important as replacing the TTY_LDISC flag + reference count with a rwsem to get lockdep support...
> > Signed-off-by: Peter Hurley <peter@hurleysoftware.com> > > --- > > drivers/tty/tty_ldisc.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c > > index c903778..8a86a72 100644 > > --- a/drivers/tty/tty_ldisc.c > > +++ b/drivers/tty/tty_ldisc.c > > @@ -604,6 +604,12 @@ static bool tty_ldisc_hangup_halt(struct tty_struct *tty) > > > > clear_bit(TTY_LDISC, &tty->flags); > > > > + /* Wakeup waiters so they can discover the tty is hupping, abort > > + * their i/o loops, and release their ldisc references > > + */ > > + wake_up_interruptible_poll(&tty->write_wait, POLLOUT); > > + wake_up_interruptible_poll(&tty->read_wait, POLLIN); > > + > > if (tty->ldisc) { /* Not yet closed */ > > tty_unlock(tty); > > > > @@ -875,12 +881,6 @@ void tty_ldisc_hangup(struct tty_struct *tty) > > tty_ldisc_deref(ld); > > } > > /* > > - * FIXME: Once we trust the LDISC code better we can wait here for > > - * ldisc completion and fix the driver call race > > - */ > > - wake_up_interruptible_poll(&tty->write_wait, POLLOUT); > > - wake_up_interruptible_poll(&tty->read_wait, POLLIN); > > - /* > > * Shutdown the current line discipline, and reset it to > > * N_TTY if need be. > > * > > > >
| |