[PATCH] tty: Fix tcdrain hang due to tty chars_in_buffer errors

[PATCH] tty: Fix tcdrain hang due to tty chars_in_buffer errors

[Jeff Harris]

If a tty driver's chars_in_buffer callback reports an error, the wait loop in
tty_wait_until_sent may never exit until its timeout.  A tty device
disconnection will hangup the tty causing the tty_wait_until_sent loop to
wake-up, but if the subsequent call to chars_in_buffer reports an error
instead of zero, the loop will go back to sleep instead of exiting.

A process calling tcdrain will block indefinitely if it calls tcdrain when the
device is disconnected.

Signed-off-by: Jeff Harris <jeff_harris@kentrox.com>
---
 drivers/char/tty_ioctl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tty_ioctl.c b/drivers/char/tty_ioctl.c
index ad6ba4e..306d52e 100644
--- a/drivers/char/tty_ioctl.c
+++ b/drivers/char/tty_ioctl.c
@@ -156,7 +156,7 @@ void tty_wait_until_sent(struct tty_struct *tty, long timeout)
 	if (!timeout)
 		timeout = MAX_SCHEDULE_TIMEOUT;
 	if (wait_event_interruptible_timeout(tty->write_wait,
-			!tty_chars_in_buffer(tty), timeout) >= 0) {
+			tty_chars_in_buffer(tty) <= 0, timeout) >= 0) {
 		if (tty->ops->wait_until_sent)
 			tty->ops->wait_until_sent(tty, timeout);
 	}
@@ -332,7 +332,7 @@ void tty_termios_encode_baud_rate(struct ktermios *termios,
 	int iclose = ibaud/50, oclose = obaud/50;
 	int ibinput = 0;
 
-	if (obaud == 0)			/* CD dropped 		  */
+	if (obaud == 0)			/* CD dropped		  */
 		ibaud = 0;		/* Clear ibaud to be sure */
 
 	termios->c_ispeed = ibaud;
-- 
1.5.5.1

Re: [PATCH] tty: Fix tcdrain hang due to tty chars_in_buffer errors

[Alan Cox]

On Fri, 17 Jul 2009 11:07:54 -0400
Jeff Harris <jeff_harris@kentrox.com> wrote:

> If a tty driver's chars_in_buffer callback reports an error, the wait loop in

chars_in_buffer cannot report an error. It reports how many characters
are in the buffer.

> A process calling tcdrain will block indefinitely if it calls tcdrain when the
> device is disconnected.

Fix your chars_in_buffer. On a disconnect its quite reasonable to return
0, as they are not buffered any more.

Alan

RE: [PATCH] tty: Fix tcdrain hang due to tty chars_in_buffer errors

[Harris, Jeff]

That was my other thought, but there are other kernel drivers which do
report errors from chars_in_buffer.  A grep of the code shows
ipw_chars_in_buffer in drivers/char/pcmcia/ipwireless/tty.c,
hvc_chars_in_buffer in hvc_console.c, ntty_chars_in_buffer in nozomi.c,
if_chars_in_buffer in isdn/gigaset/interface.c, etc.  Do they all need
to be updated?  

Should the return type be unsigned int to make the behavior more
explicit?  The signed type may be interpreted along the lines of a read
or write call which returns a count or a negative value to indicate a
failure.

Jeff

-----Original Message-----
From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk] 
Sent: Friday, July 17, 2009 12:01 PM
To: Harris, Jeff
Cc: alan@linux.intel.com; vapier@gentoo.org; adobriyan@gmail.com;
linux-kernel@vger.kernel.org; Harris, Jeff
Subject: Re: [PATCH] tty: Fix tcdrain hang due to tty chars_in_buffer
errors

On Fri, 17 Jul 2009 11:07:54 -0400
Jeff Harris <jeff_harris@kentrox.com> wrote:

> If a tty driver's chars_in_buffer callback reports an error, the wait
loop in

chars_in_buffer cannot report an error. It reports how many characters
are in the buffer.

> A process calling tcdrain will block indefinitely if it calls tcdrain
when the
> device is disconnected.

Fix your chars_in_buffer. On a disconnect its quite reasonable to return
0, as they are not buffered any more.

Alan

Re: [PATCH] tty: Fix tcdrain hang due to tty chars_in_buffer errors

[Alan Cox]

On Fri, 17 Jul 2009 12:36:22 -0400
"Harris, Jeff" <Jeff_Harris@kentrox.com> wrote:

> That was my other thought, but there are other kernel drivers which do
> report errors from chars_in_buffer.  A grep of the code shows
> ipw_chars_in_buffer in drivers/char/pcmcia/ipwireless/tty.c,
> hvc_chars_in_buffer in hvc_console.c, ntty_chars_in_buffer in nozomi.c,
> if_chars_in_buffer in isdn/gigaset/interface.c, etc.  Do they all need
> to be updated?  

Yes I've squashed a pile and there may be more left.

> Should the return type be unsigned int to make the behavior more
> explicit?  The signed type may be interpreted along the lines of a read
> or write call which returns a count or a negative value to indicate a
> failure.

That would simply have people returning very large numbers. I don't think
the compiler can cure that. Putting a WARN_ON(x < 0); might liven it up
though.

Alan