Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751356AbZG0N7G (ORCPT ); Mon, 27 Jul 2009 09:59:06 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751242AbZG0N7F (ORCPT ); Mon, 27 Jul 2009 09:59:05 -0400 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:52521 "EHLO e23smtp08.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750959AbZG0N7D (ORCPT ); Mon, 27 Jul 2009 09:59:03 -0400 Date: Mon, 27 Jul 2009 19:28:53 +0530 From: "Aneesh Kumar K.V" To: Alan Cox Cc: OGAWA Hirofumi , Linus Torvalds , "Rafael J. Wysocki" , Ray Lee , LKML , Andrew Morton Subject: Re: [PATCH] kdesu broken Message-ID: <20090727135853.GA4223@skywalker> References: <20090724164058.21a054e6@lxorguk.ukuu.org.uk> <87ws5xjo2x.fsf@devron.myhome.or.jp> <20090725150510.35e8854d@lxorguk.ukuu.org.uk> <87ab2sx15g.fsf@devron.myhome.or.jp> <20090725163251.50e6f546@lxorguk.ukuu.org.uk> <87bpn7mzli.fsf@devron.myhome.or.jp> <20090727115723.1e8de60e@lxorguk.ukuu.org.uk> <873a8iqqgv.fsf@devron.myhome.or.jp> <20090727142303.41096bf5@lxorguk.ukuu.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090727142303.41096bf5@lxorguk.ukuu.org.uk> User-Agent: Mutt/1.5.19 (2009-01-05) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6898 Lines: 201 On Mon, Jul 27, 2009 at 02:23:03PM +0100, Alan Cox wrote: > > I worried "pair->packet = 0" when I'm thinking this. I guess it would be > > changed more early than before. Is it ok? > > I think so, and we can get stuck otherwise. > > > Tested patch: > > commit 70325fd1d4341896c17b6f1f1965370b5258d0b1 > Author: Alan Cox > Date: Mon Jul 27 14:18:52 2009 +0100 > > pty: ensure writes hit the reader before close > > Implement TTY_EOF/EOFPENDING flags so that we can propogate the close of the > pty through the buffering correctly. The new flag state is locked but the > tty buffer lock as it relates to buffers, and also because the buffer > lock is already held in the hot path. > > Signed-off-by: Alan Cox > > diff --git a/drivers/char/n_tty.c b/drivers/char/n_tty.c > index ff47907..acae995 100644 > --- a/drivers/char/n_tty.c > +++ b/drivers/char/n_tty.c > @@ -1777,7 +1777,8 @@ do_it_again: > tty->minimum_to_wake = (minimum - (b - buf)); > > if (!input_available_p(tty, 0)) { > - if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) { > + if (test_bit(TTY_EOF, &tty->flags)) { > + /* PTY pair closed and all data consumed */ > retval = -EIO; > break; > } > diff --git a/drivers/char/pty.c b/drivers/char/pty.c > index 6e6942c..de10cc0 100644 > --- a/drivers/char/pty.c > +++ b/drivers/char/pty.c > @@ -38,6 +38,9 @@ static struct tty_driver *pts_driver; > > static void pty_close(struct tty_struct *tty, struct file *filp) > { > + struct tty_struct *pair; > + unsigned long flags; > + > BUG_ON(!tty); > if (tty->driver->subtype == PTY_TYPE_MASTER) > WARN_ON(tty->count > 1); > @@ -47,13 +50,22 @@ static void pty_close(struct tty_struct *tty, struct file *filp) > } > wake_up_interruptible(&tty->read_wait); > wake_up_interruptible(&tty->write_wait); > + > tty->packet = 0; > - if (!tty->link) > + pair = tty->link; > + if (!pair) > return; > - tty->link->packet = 0; > - set_bit(TTY_OTHER_CLOSED, &tty->link->flags); > - wake_up_interruptible(&tty->link->read_wait); > - wake_up_interruptible(&tty->link->write_wait); > + > + spin_lock_irqsave(&pair->buf.lock, flags); > + pair->packet = 0; > + /* Indicate that the other end is now closed, set the > + ENDPENDING marker so that the true end can be processed by > + the line discipline */ > + set_bit(TTY_EOFPENDING, &pair->flags); > + set_bit(TTY_OTHER_CLOSED, &pair->flags); > + spin_unlock_irqrestore(&pair->buf.lock, flags); > + wake_up_interruptible(&pair->read_wait); > + wake_up_interruptible(&pair->write_wait); > if (tty->driver->subtype == PTY_TYPE_MASTER) { > set_bit(TTY_OTHER_CLOSED, &tty->flags); > #ifdef CONFIG_UNIX98_PTYS > @@ -180,7 +192,6 @@ static void pty_flush_buffer(struct tty_struct *tty) > > if (!to) > return; > - /* tty_buffer_flush(to); FIXME */ > if (to->packet) { > spin_lock_irqsave(&tty->ctrl_lock, flags); > tty->ctrl_status |= TIOCPKT_FLUSHWRITE; > @@ -191,23 +202,30 @@ static void pty_flush_buffer(struct tty_struct *tty) > > static int pty_open(struct tty_struct *tty, struct file *filp) > { > - int retval = -ENODEV; > + int retval = -EIO; > + unsigned long flags; > + struct tty_struct *pair; > > - if (!tty || !tty->link) > - goto out; > + if (tty == NULL || (pair = tty->link) == NULL) > + return -ENODEV; > > - retval = -EIO; > if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) > + return -EIO; > + spin_lock_irqsave(&pair->buf.lock, flags); > + if (test_bit(TTY_PTY_LOCK, &pair->flags)) > goto out; > - if (test_bit(TTY_PTY_LOCK, &tty->link->flags)) > - goto out; > - if (tty->link->count != 1) > + if (pair->count != 1) > goto out; > > - clear_bit(TTY_OTHER_CLOSED, &tty->link->flags); > + clear_bit(TTY_OTHER_CLOSED, &pair->flags); > + /* The buf.lock stops this racing a flush_to_ldisc from > + the other end */ > + clear_bit(TTY_EOFPENDING, &pair->flags); > + clear_bit(TTY_EOF, &pair->flags); > set_bit(TTY_THROTTLED, &tty->flags); > retval = 0; > out: > + spin_unlock_irqrestore(&pair->buf.lock, flags); > return retval; > } > > diff --git a/drivers/char/tty_buffer.c b/drivers/char/tty_buffer.c > index 810ee25..19a7ced 100644 > --- a/drivers/char/tty_buffer.c > +++ b/drivers/char/tty_buffer.c > @@ -119,6 +119,12 @@ static void __tty_buffer_flush(struct tty_struct *tty) > tty_buffer_free(tty, thead); > } > tty->buf.tail = NULL; > + /* We can EOF a pty/tty pair with a flush as well as by consuming > + all the data left over */ > + if (test_bit(TTY_EOFPENDING, &tty->flags)) { > + set_bit(TTY_EOF, &tty->flags); > + wake_up(&tty->read_wait); > + } > } > > /** > @@ -405,6 +411,7 @@ static void flush_to_ldisc(struct work_struct *work) > struct tty_buffer *tbuf, *head; > char *char_buf; > unsigned char *flag_buf; > + int done = 1; > > disc = tty_ldisc_ref(tty); > if (disc == NULL) /* !TTY_LDISC */ > @@ -433,10 +440,13 @@ static void flush_to_ldisc(struct work_struct *work) > break; > if (!tty->receive_room) { > schedule_delayed_work(&tty->buf.work, 1); > + done = 0; > break; > } > - if (count > tty->receive_room) > + if (count > tty->receive_room) { > count = tty->receive_room; > + done = 0; > + } > char_buf = head->char_buf_ptr + head->read; > flag_buf = head->flag_buf_ptr + head->read; > head->read += count; > @@ -454,6 +464,10 @@ static void flush_to_ldisc(struct work_struct *work) > __tty_buffer_flush(tty); > clear_bit(TTY_FLUSHPENDING, &tty->flags); > wake_up(&tty->read_wait); > + } else if (done && test_bit(TTY_EOFPENDING, &tty->flags)) { > + /* The last bits hit the ldisc so set EOF */ > + wake_up(&tty->read_wait); > + set_bit(TTY_EOF, &tty->flags); > } > clear_bit(TTY_FLUSHING, &tty->flags); > spin_unlock_irqrestore(&tty->buf.lock, flags); > diff --git a/include/linux/tty.h b/include/linux/tty.h > index 85aa525..427d107 100644 > --- a/include/linux/tty.h > +++ b/include/linux/tty.h > @@ -321,6 +321,8 @@ struct tty_struct { > #define TTY_LDISC 9 /* Line discipline attached */ > #define TTY_LDISC_CHANGING 10 /* Line discipline changing */ > #define TTY_LDISC_OPEN 11 /* Line discipline is open */ > +#define TTY_EOF 12 /* TTY/PTY pair at EOF */ > +#define TTY_EOFPENDING 13 /* TTY/PTY pair EOF when data emptied */ > #define TTY_HW_COOK_OUT 14 /* Hardware can do output cooking */ > #define TTY_HW_COOK_IN 15 /* Hardware can do input cooking */ > #define TTY_PTY_LOCK 16 /* pty private */ > > With this patch i still have "the compile in emacs" problem. But it is less frequent. 1 in 5 tries fail now -aneesh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/