From d945cb9cce20ac7143c2de8d88b187f62db99bdc Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Tue, 7 Jul 2009 16:39:41 +0100 Subject: pty: Rework the pty layer to use the normal buffering logic This fixes the ppp problems and various other issues with call locking caused by one side of a pty called in one locking context trying to match another with differing rules on the other side. We also get a big slack space to work with that means we can bury the flow control deadlock case for any conceivable real world situation. Signed-off-by: Alan Cox Signed-off-by: Linus Torvalds --- drivers/char/pty.c | 154 ++++++++++++++++++++--------------------------------- 1 file changed, 59 insertions(+), 95 deletions(-) (limited to 'drivers/char/pty.c') diff --git a/drivers/char/pty.c b/drivers/char/pty.c index daebe1ba43d..9d1b4f548f6 100644 --- a/drivers/char/pty.c +++ b/drivers/char/pty.c @@ -75,114 +75,88 @@ static void pty_close(struct tty_struct *tty, struct file *filp) */ static void pty_unthrottle(struct tty_struct *tty) { - struct tty_struct *o_tty = tty->link; - - if (!o_tty) - return; - - tty_wakeup(o_tty); + tty_wakeup(tty->link); set_bit(TTY_THROTTLED, &tty->flags); } -/* - * WSH 05/24/97: modified to - * (1) use space in tty->flip instead of a shared temp buffer - * The flip buffers aren't being used for a pty, so there's lots - * of space available. The buffer is protected by a per-pty - * semaphore that should almost never come under contention. - * (2) avoid redundant copying for cases where count >> receive_room - * N.B. Calls from user space may now return an error code instead of - * a count. +/** + * pty_space - report space left for writing + * @to: tty we are writing into * - * FIXME: Our pty_write method is called with our ldisc lock held but - * not our partners. We can't just wait on the other one blindly without - * risking deadlocks. At some point when everything has settled down we need - * to look into making pty_write at least able to sleep over an ldisc change. + * The tty buffers allow 64K but we sneak a peak and clip at 8K this + * allows a lot of overspill room for echo and other fun messes to + * be handled properly + */ + +static int pty_space(struct tty_struct *to) +{ + int n = 8192 - to->buf.memory_used; + if (n < 0) + return 0; + return n; +} + +/** + * pty_write - write to a pty + * @tty: the tty we write from + * @buf: kernel buffer of data + * @count: bytes to write * - * The return on no ldisc is a bit counter intuitive but the logic works - * like this. During an ldisc change the other end will flush its buffers. We - * thus return the full length which is identical to the case where we had - * proper locking and happened to queue the bytes just before the flush during - * the ldisc change. + * Our "hardware" write method. Data is coming from the ldisc which + * may be in a non sleeping state. We simply throw this at the other + * end of the link as if we were an IRQ handler receiving stuff for + * the other side of the pty/tty pair. */ + static int pty_write(struct tty_struct *tty, const unsigned char *buf, int count) { struct tty_struct *to = tty->link; - struct tty_ldisc *ld; - int c = count; + int c; - if (!to || tty->stopped) + if (tty->stopped) return 0; - ld = tty_ldisc_ref(to); - - if (ld) { - c = to->receive_room; - if (c > count) - c = count; - ld->ops->receive_buf(to, buf, NULL, c); - tty_ldisc_deref(ld); + + /* This isn't locked but our 8K is quite sloppy so no + big deal */ + + c = pty_space(to); + if (c > count) + c = count; + if (c > 0) { + /* Stuff the data into the input queue of the other end */ + c = tty_insert_flip_string(to, buf, c); + /* And shovel */ + tty_flip_buffer_push(to); + tty_wakeup(tty); } return c; } +/** + * pty_write_room - write space + * @tty: tty we are writing from + * + * Report how many bytes the ldisc can send into the queue for + * the other device. + */ + static int pty_write_room(struct tty_struct *tty) { - struct tty_struct *to = tty->link; - - if (!to || tty->stopped) - return 0; - - return to->receive_room; + return pty_space(tty->link); } -/* - * WSH 05/24/97: Modified for asymmetric MASTER/SLAVE behavior - * The chars_in_buffer() value is used by the ldisc select() function - * to hold off writing when chars_in_buffer > WAKEUP_CHARS (== 256). - * The pty driver chars_in_buffer() Master/Slave must behave differently: - * - * The Master side needs to allow typed-ahead commands to accumulate - * while being canonicalized, so we report "our buffer" as empty until - * some threshold is reached, and then report the count. (Any count > - * WAKEUP_CHARS is regarded by select() as "full".) To avoid deadlock - * the count returned must be 0 if no canonical data is available to be - * read. (The N_TTY ldisc.chars_in_buffer now knows this.) +/** + * pty_chars_in_buffer - characters currently in our tx queue + * @tty: our tty * - * The Slave side passes all characters in raw mode to the Master side's - * buffer where they can be read immediately, so in this case we can - * return the true count in the buffer. + * Report how much we have in the transmit queue. As everything is + * instantly at the other end this is easy to implement. */ + static int pty_chars_in_buffer(struct tty_struct *tty) { - struct tty_struct *to = tty->link; - struct tty_ldisc *ld; - int count = 0; - - /* We should get the line discipline lock for "tty->link" */ - if (!to) - return 0; - /* We cannot take a sleeping reference here without deadlocking with - an ldisc change - but it doesn't really matter */ - ld = tty_ldisc_ref(to); - if (ld == NULL) - return 0; - - /* The ldisc must report 0 if no characters available to be read */ - if (ld->ops->chars_in_buffer) - count = ld->ops->chars_in_buffer(to); - - tty_ldisc_deref(ld); - - if (tty->driver->subtype == PTY_TYPE_SLAVE) - return count; - - /* Master side driver ... if the other side's read buffer is less than - * half full, return 0 to allow writers to proceed; otherwise return - * the count. This leaves a comfortable margin to avoid overflow, - * and still allows half a buffer's worth of typed-ahead commands. - */ - return (count < N_TTY_BUF_SIZE/2) ? 0 : count; + return 0; } /* Set the lock flag on a pty */ @@ -202,20 +176,10 @@ static void pty_flush_buffer(struct tty_struct *tty) { struct tty_struct *to = tty->link; unsigned long flags; - struct tty_ldisc *ld; if (!to) return; - ld = tty_ldisc_ref(to); - - /* The other end is changing discipline */ - if (!ld) - return; - - if (ld->ops->flush_buffer) - to->ldisc->ops->flush_buffer(to); - tty_ldisc_deref(ld); - + /* tty_buffer_flush(to); FIXME */ if (to->packet) { spin_lock_irqsave(&tty->ctrl_lock, flags); tty->ctrl_status |= TIOCPKT_FLUSHWRITE; -- cgit v1.2.3 From 405f55712dfe464b3240d7816cc4fe4174831be2 Mon Sep 17 00:00:00 2001 From: Alexey Dobriyan Date: Sat, 11 Jul 2009 22:08:37 +0400 Subject: headers: smp_lock.h redux * Remove smp_lock.h from files which don't need it (including some headers!) * Add smp_lock.h to files which do need it * Make smp_lock.h include conditional in hardirq.h It's needed only for one kernel_locked() usage which is under CONFIG_PREEMPT This will make hardirq.h inclusion cheaper for every PREEMPT=n config (which includes allmodconfig/allyesconfig, BTW) Signed-off-by: Alexey Dobriyan Signed-off-by: Linus Torvalds --- drivers/char/pty.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/char/pty.c') diff --git a/drivers/char/pty.c b/drivers/char/pty.c index 9d1b4f548f6..6e6942c45f5 100644 --- a/drivers/char/pty.c +++ b/drivers/char/pty.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include -- cgit v1.2.3 From 3a54297478e6578f96fd54bf4daa1751130aca86 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Mon, 27 Jul 2009 22:17:51 +0100 Subject: pty: quickfix for the pty ENXIO timing problems This also makes close stall in the normal case which is apparently needed to fix emacs Signed-off-by: Alan Cox Signed-off-by: Linus Torvalds --- drivers/char/pty.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/char/pty.c') diff --git a/drivers/char/pty.c b/drivers/char/pty.c index 6e6942c45f5..3850a68f265 100644 --- a/drivers/char/pty.c +++ b/drivers/char/pty.c @@ -52,6 +52,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp) return; tty->link->packet = 0; set_bit(TTY_OTHER_CLOSED, &tty->link->flags); + tty_flip_buffer_push(tty->link); wake_up_interruptible(&tty->link->read_wait); wake_up_interruptible(&tty->link->write_wait); if (tty->driver->subtype == PTY_TYPE_MASTER) { @@ -207,6 +208,7 @@ static int pty_open(struct tty_struct *tty, struct file *filp) clear_bit(TTY_OTHER_CLOSED, &tty->link->flags); set_bit(TTY_THROTTLED, &tty->flags); retval = 0; + tty->low_latency = 1; out: return retval; } -- cgit v1.2.3 From e043e42bdb66885b3ac10d27a01ccb9972e2b0a3 Mon Sep 17 00:00:00 2001 From: OGAWA Hirofumi Date: Wed, 29 Jul 2009 12:15:56 -0700 Subject: pty: avoid forcing 'low_latency' tty flag We really don't want to mark the pty as a low-latency device, because as Alan points out, the ->write method can be called from an IRQ (ppp?), and that means we can't use ->low_latency=1 as we take mutexes in the low_latency case. So rather than using low_latency to force the written data to be pushed to the ldisc handling at 'write()' time, just make the reader side (or the poll function) do the flush when it checks whether there is data to be had. This also fixes the problem with lost data in an emacs compile buffer (bugzilla 13815), and we can thus revert the low_latency pty hack (commit 3a54297478e6578f96fd54bf4daa1751130aca86: "pty: quickfix for the pty ENXIO timing problems"). Signed-off-by: OGAWA Hirofumi Tested-by: Aneesh Kumar K.V [ Modified to do the tty_flush_to_ldisc() inside input_available_p() so that it triggers for both read and poll() - Linus] Signed-off-by: Linus Torvalds --- drivers/char/pty.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'drivers/char/pty.c') diff --git a/drivers/char/pty.c b/drivers/char/pty.c index 3850a68f265..6e6942c45f5 100644 --- a/drivers/char/pty.c +++ b/drivers/char/pty.c @@ -52,7 +52,6 @@ static void pty_close(struct tty_struct *tty, struct file *filp) return; tty->link->packet = 0; set_bit(TTY_OTHER_CLOSED, &tty->link->flags); - tty_flip_buffer_push(tty->link); wake_up_interruptible(&tty->link->read_wait); wake_up_interruptible(&tty->link->write_wait); if (tty->driver->subtype == PTY_TYPE_MASTER) { @@ -208,7 +207,6 @@ static int pty_open(struct tty_struct *tty, struct file *filp) clear_bit(TTY_OTHER_CLOSED, &tty->link->flags); set_bit(TTY_THROTTLED, &tty->flags); retval = 0; - tty->low_latency = 1; out: return retval; } -- cgit v1.2.3 From 85dfd81dc57e8183a277ddd7a56aa65c96f3f487 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Mon, 10 Aug 2009 13:21:19 -0700 Subject: pty: fix data loss when stopped (^S/^Q) Commit d945cb9cc ("pty: Rework the pty layer to use the normal buffering logic") dropped the test for 'tty->stopped' in pty_write_room(), which then causes the n_tty line discipline thing to not throttle the data properly when the tty is stopped. So instead of pausing the write due to the tty being stopped, the ldisc layer would go ahead and push it down to the pty. The pty write() routine would then refuse to take the data (because it _did_ check 'stopped'), and the data wouldn't actually be written. This whole stopped test should eventually be moved into the tty ldisc layer rather than have low-level tty drivers care about these things, but right now the fix is to just re-instate the missing pty 'stopped' handling. Reported-and-tested-by: Artur Skawina Cc: Alan Cox Signed-off-by: Linus Torvalds --- drivers/char/pty.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/char/pty.c') diff --git a/drivers/char/pty.c b/drivers/char/pty.c index 6e6942c45f5..d083c73d784 100644 --- a/drivers/char/pty.c +++ b/drivers/char/pty.c @@ -144,6 +144,8 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf, static int pty_write_room(struct tty_struct *tty) { + if (tty->stopped) + return 0; return pty_space(tty->link); } -- cgit v1.2.3 From ac89a9174decf343de049a06fad75681f71890eb Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sat, 5 Sep 2009 13:27:10 -0700 Subject: pty: don't limit the writes to 'pty_space()' inside 'pty_write()' The whole write-room thing is something that is up to the _caller_ to worry about, not the pty layer itself. The total buffer space will still be limited by the buffering routines themselves, so there is no advantage or need in having pty_write() artificially limit the size somehow. And what happened was that the caller (the n_tty line discipline, in this case) may have verified that there is room for 2 bytes to be written (for NL -> CRNL expansion), and it used to then do those writes as two single-byte writes. And if the first byte written (CR) then caused a new tty buffer to be allocated, pty_space() may have returned zero when trying to write the second byte (LF), and then incorrectly failed the write - leading to a lost newline character. This should finally fix http://bugzilla.kernel.org/show_bug.cgi?id=14015 Reported-by: Mikael Pettersson Acked-by: Alan Cox Signed-off-by: Linus Torvalds --- drivers/char/pty.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) (limited to 'drivers/char/pty.c') diff --git a/drivers/char/pty.c b/drivers/char/pty.c index d083c73d784..b33d6688e91 100644 --- a/drivers/char/pty.c +++ b/drivers/char/pty.c @@ -109,21 +109,13 @@ static int pty_space(struct tty_struct *to) * the other side of the pty/tty pair. */ -static int pty_write(struct tty_struct *tty, const unsigned char *buf, - int count) +static int pty_write(struct tty_struct *tty, const unsigned char *buf, int c) { struct tty_struct *to = tty->link; - int c; if (tty->stopped) return 0; - /* This isn't locked but our 8K is quite sloppy so no - big deal */ - - c = pty_space(to); - if (c > count) - c = count; if (c > 0) { /* Stuff the data into the input queue of the other end */ c = tty_insert_flip_string(to, buf, c); -- cgit v1.2.3