[PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Wang YanQing]

I meet emacs hang in start if I do the operation below:
  1: echo 3 > /proc/sys/vm/drop_caches
  2: emacs BigFile
  3: Press CTRL-S follow 2 immediately

Then emacs hang on, CTRL-Q can't resume, the terminal
hang on, you can do nothing with this terminal except
close it.

The reason is before emacs takeover control the tty,
we use CTRL-S to XOFF it. Then when emacs takeover the
control, it may don't use the flow-control, so emacs hang.
But after search the emacs's startup codes, I find they use TCXONC
to workaround this situation:

  /* This code added to insure that, if flow-control is not to be used,
     we have an unlocked terminal at the start. */
  if (!tty_out->flow_control) ioctl (fileno (tty_out->input), TCXONC, 1);
  ioctl (fileno (tty_out->input), TCXONC, 1);

But this workaround never work due the kernel's current code.
This patch fix it.

Below is the ChangeLog introduce the tty->flow_stopped flag:

Thu Nov 21 10:05:22 1996  Theodre Ts'o  <tytso@localhost.mit.edu>

       * tty_ioctl.c (tty_wait_until_sent): Always check the driver
               wait_until_ready routine, even if there are no characters
               in the xmit buffer.  (There may be charactes in the device
               FIFO.)
               (n_tty_ioctl): Add new flag tty->flow_stopped which
               indicates whether the tty is stopped due to a request by
               the TCXONC ioctl (used by tcflow).  If so, don't let an
               incoming XOFF character restart the tty.  The tty can only
               be restarted by another TCXONC request.

So I think this patch make TCXONC can restart tty which stopped by
STOP_CHAR don't break the original meaning.

This patch will fix some strange tty relation hang problem,
I believe I meet it with vim in ssh, and also see below bug report:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=465823

Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 drivers/tty/tty_ioctl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index e4455e0..42e08e5 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -1129,11 +1129,12 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 		case TCOOFF:
 			if (!tty->flow_stopped) {
 				tty->flow_stopped = 1;
-				stop_tty(tty);
+				if (!tty->stopped)
+					stop_tty(tty);
 			}
 			break;
 		case TCOON:
-			if (tty->flow_stopped) {
+			if (tty->flow_stopped || tty->stopped) {
 				tty->flow_stopped = 0;
 				start_tty(tty);
 			}
-- 
1.7.12.4.dirty

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Greg KH]

On Wed, May 08, 2013 at 02:47:34AM +0800, Wang YanQing wrote:
> I meet emacs hang in start if I do the operation below:
>   1: echo 3 > /proc/sys/vm/drop_caches
>   2: emacs BigFile
>   3: Press CTRL-S follow 2 immediately
> 
> Then emacs hang on, CTRL-Q can't resume, the terminal
> hang on, you can do nothing with this terminal except
> close it.
> 
> The reason is before emacs takeover control the tty,
> we use CTRL-S to XOFF it. Then when emacs takeover the
> control, it may don't use the flow-control, so emacs hang.
> But after search the emacs's startup codes, I find they use TCXONC
> to workaround this situation:
> 
>   /* This code added to insure that, if flow-control is not to be used,
>      we have an unlocked terminal at the start. */
>   if (!tty_out->flow_control) ioctl (fileno (tty_out->input), TCXONC, 1);
>   ioctl (fileno (tty_out->input), TCXONC, 1);
> 
> But this workaround never work due the kernel's current code.
> This patch fix it.
> 
> Below is the ChangeLog introduce the tty->flow_stopped flag:
> 
> Thu Nov 21 10:05:22 1996  Theodre Ts'o  <tytso@localhost.mit.edu>
> 
>        * tty_ioctl.c (tty_wait_until_sent): Always check the driver
>                wait_until_ready routine, even if there are no characters
>                in the xmit buffer.  (There may be charactes in the device
>                FIFO.)
>                (n_tty_ioctl): Add new flag tty->flow_stopped which
>                indicates whether the tty is stopped due to a request by
>                the TCXONC ioctl (used by tcflow).  If so, don't let an
>                incoming XOFF character restart the tty.  The tty can only
>                be restarted by another TCXONC request.
> 
> So I think this patch make TCXONC can restart tty which stopped by
> STOP_CHAR don't break the original meaning.
> 
> This patch will fix some strange tty relation hang problem,
> I believe I meet it with vim in ssh, and also see below bug report:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=465823

So this has been a problem since 1996?  Or is it newly introduced due to
some changes in this area in a newer kernel?

thanks,

greg k-h

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Wang YanQing]

On Tue, May 07, 2013 at 11:58:00AM -0700, Greg KH wrote:
> On Wed, May 08, 2013 at 02:47:34AM +0800, Wang YanQing wrote:
> > The reason is before emacs takeover control the tty,
> > we use CTRL-S to XOFF it. Then when emacs takeover the
> > control, it may don't use the flow-control, so emacs hang.
> > But after search the emacs's startup codes, I find they use TCXONC
> > to workaround this situation:
> > 
> >   /* This code added to insure that, if flow-control is not to be used,
> >      we have an unlocked terminal at the start. */
> >   if (!tty_out->flow_control) ioctl (fileno (tty_out->input), TCXONC, 1);
> >   ioctl (fileno (tty_out->input), TCXONC, 1);

The second line of ioctl:
"ioctl (fileno (tty_out->input), TCXONC, 1);"

It is not there in emacs' code, I add it for debuging, and forget to
delete it.
Greg KH, could you delete it from ChangeLog if you accept this patch?


> > But this workaround never work due the kernel's current code.
> > This patch fix it.
> > 
> > Below is the ChangeLog introduce the tty->flow_stopped flag:
> > 
> > Thu Nov 21 10:05:22 1996  Theodre Ts'o  <tytso@localhost.mit.edu>
> > 
> >        * tty_ioctl.c (tty_wait_until_sent): Always check the driver
> >                wait_until_ready routine, even if there are no characters
> >                in the xmit buffer.  (There may be charactes in the device
> >                FIFO.)
> >                (n_tty_ioctl): Add new flag tty->flow_stopped which
> >                indicates whether the tty is stopped due to a request by
> >                the TCXONC ioctl (used by tcflow).  If so, don't let an
> >                incoming XOFF character restart the tty.  The tty can only
> >                be restarted by another TCXONC request.
> > 
> > So I think this patch make TCXONC can restart tty which stopped by
> > STOP_CHAR don't break the original meaning.
> > 
> > This patch will fix some strange tty relation hang problem,
> > I believe I meet it with vim in ssh, and also see below bug report:
> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=465823
> 
> So this has been a problem since 1996?  Or is it newly introduced due to
> some changes in this area in a newer kernel?

I have tested the v2.6.32.45, and have the same issue, my current environment
can't compile and test v2.6.24/v2.6.25, due make version maybe.

But after explore history codes, I want to say,
yes, this issue since 1996 that we can't use TCXONC 
to restart tty which be stopped by STOP_CHAR.

Kernel process the STOP_CHAR/START_CHAR to implement 
flow control, but userspace should have methold to 
START/STOP it by ioctl or others' similar positive method
which don't depend on another side, that's TCXONC.

According http://en.wikibooks.org/wiki/Serial_Programming/termio

"
TCXONC
    Start or stop the output. 
"

Thanks.

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Greg KH]

On Wed, May 08, 2013 at 09:55:45AM +0800, Wang YanQing wrote:
> On Tue, May 07, 2013 at 11:58:00AM -0700, Greg KH wrote:
> > On Wed, May 08, 2013 at 02:47:34AM +0800, Wang YanQing wrote:
> > > The reason is before emacs takeover control the tty,
> > > we use CTRL-S to XOFF it. Then when emacs takeover the
> > > control, it may don't use the flow-control, so emacs hang.
> > > But after search the emacs's startup codes, I find they use TCXONC
> > > to workaround this situation:
> > > 
> > >   /* This code added to insure that, if flow-control is not to be used,
> > >      we have an unlocked terminal at the start. */
> > >   if (!tty_out->flow_control) ioctl (fileno (tty_out->input), TCXONC, 1);
> > >   ioctl (fileno (tty_out->input), TCXONC, 1);
> 
> The second line of ioctl:
> "ioctl (fileno (tty_out->input), TCXONC, 1);"
> 
> It is not there in emacs' code, I add it for debuging, and forget to
> delete it.
> Greg KH, could you delete it from ChangeLog if you accept this patch?

Yes, I can.

> > > But this workaround never work due the kernel's current code.
> > > This patch fix it.
> > > 
> > > Below is the ChangeLog introduce the tty->flow_stopped flag:
> > > 
> > > Thu Nov 21 10:05:22 1996  Theodre Ts'o  <tytso@localhost.mit.edu>
> > > 
> > >        * tty_ioctl.c (tty_wait_until_sent): Always check the driver
> > >                wait_until_ready routine, even if there are no characters
> > >                in the xmit buffer.  (There may be charactes in the device
> > >                FIFO.)
> > >                (n_tty_ioctl): Add new flag tty->flow_stopped which
> > >                indicates whether the tty is stopped due to a request by
> > >                the TCXONC ioctl (used by tcflow).  If so, don't let an
> > >                incoming XOFF character restart the tty.  The tty can only
> > >                be restarted by another TCXONC request.
> > > 
> > > So I think this patch make TCXONC can restart tty which stopped by
> > > STOP_CHAR don't break the original meaning.
> > > 
> > > This patch will fix some strange tty relation hang problem,
> > > I believe I meet it with vim in ssh, and also see below bug report:
> > > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=465823
> > 
> > So this has been a problem since 1996?  Or is it newly introduced due to
> > some changes in this area in a newer kernel?
> 
> I have tested the v2.6.32.45, and have the same issue, my current environment
> can't compile and test v2.6.24/v2.6.25, due make version maybe.
> 
> But after explore history codes, I want to say,
> yes, this issue since 1996 that we can't use TCXONC 
> to restart tty which be stopped by STOP_CHAR.
> 
> Kernel process the STOP_CHAR/START_CHAR to implement 
> flow control, but userspace should have methold to 
> START/STOP it by ioctl or others' similar positive method
> which don't depend on another side, that's TCXONC.
> 
> According http://en.wikibooks.org/wiki/Serial_Programming/termio
> 
> "
> TCXONC
>     Start or stop the output. 
> "

What about Peter's comments on this patch?

thanks,

greg k-h

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Wang YanQing]

On Tue, May 07, 2013 at 07:02:00PM -0700, Greg KH wrote:
> What about Peter's comments on this patch?
> 
Peter's comments will import policy,

I means we should let userspace to decide whether
and when to restart tty with the mechanism of TCXONC
instead of restart tty accidental when make n_tty_set_termios
call.

Thanks.

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Peter Hurley]

On 05/08/2013 09:16 AM, Wang YanQing wrote:
> On Tue, May 07, 2013 at 07:02:00PM -0700, Greg KH wrote:
>> What about Peter's comments on this patch?
>>
> Peter's comments will import policy,
>
> I means we should let userspace to decide whether
> and when to restart tty with the mechanism of TCXONC
> instead of restart tty accidental when make n_tty_set_termios
> call.

There would be no accidental restart. Userspace is specifically
disabling user-controlled output flow control by clearing
IXON in termios. Userspace is _expecting_ a 'started' tty.

If you insist that this must be controllable from userspace,
then that is already possible:

	tcflow(fd, TCOOFF);
	tcflow(fd, TCOON);


Regards,
Peter Hurley

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Wang YanQing]

On Wed, May 08, 2013 at 11:18:07AM -0400, Peter Hurley wrote:
> On 05/08/2013 09:16 AM, Wang YanQing wrote:
> > On Tue, May 07, 2013 at 07:02:00PM -0700, Greg KH wrote:
> >> What about Peter's comments on this patch?
> >>
> > Peter's comments will import policy,
> >
> > I means we should let userspace to decide whether
> > and when to restart tty with the mechanism of TCXONC
> > instead of restart tty accidental when make n_tty_set_termios
> > call.
> 
> There would be no accidental restart. Userspace is specifically
> disabling user-controlled output flow control by clearing
> IXON in termios. Userspace is _expecting_ a 'started' tty.
> 
> If you insist that this must be controllable from userspace,
> then that is already possible:
> 
> 	tcflow(fd, TCOOFF);
> 	tcflow(fd, TCOON);

Indeed you can't do what you said with TCOOFF and TCOON
if you read the codes, that's what this patch fix.

Thanks.

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Peter Hurley]

On 05/08/2013 01:12 PM, Wang YanQing wrote:
> On Wed, May 08, 2013 at 11:18:07AM -0400, Peter Hurley wrote:
>> On 05/08/2013 09:16 AM, Wang YanQing wrote:
>>> On Tue, May 07, 2013 at 07:02:00PM -0700, Greg KH wrote:
>>>> What about Peter's comments on this patch?
>>>>
>>> Peter's comments will import policy,
>>>
>>> I means we should let userspace to decide whether
>>> and when to restart tty with the mechanism of TCXONC
>>> instead of restart tty accidental when make n_tty_set_termios
>>> call.
>>
>> There would be no accidental restart. Userspace is specifically
>> disabling user-controlled output flow control by clearing
>> IXON in termios. Userspace is _expecting_ a 'started' tty.
>>
>> If you insist that this must be controllable from userspace,
>> then that is already possible:
>>
>> 	tcflow(fd, TCOOFF);
>> 	tcflow(fd, TCOON);
>
> Indeed you can't do what you said with TCOOFF and TCOON
> if you read the codes, that's what this patch fix.

Perhaps you misunderstood. The snippet above does indeed restart
a tty which has been stopped via STOP_CHAR(tty) and the termios
IXON flag cleared.

Below is a testcase which demonstrates the problem and
the userspace workaround from above.

--- >% ---
/**
  *	tty_unstop.c
  *
  *	Description: testcase for unsticking stopped tty from userspace
  *
  *	To build testcase:
  *	  gcc tty_unstop.c -o tty_unstop
  *
  *	To build exemplar userspace workaround:
  *	  gcc -D FIX_STOPPED_TTY tty_unstop.c -o tty_unstop
  */

#include <stdio.h>
#include <termios.h>
#include <unistd.h>
#include <errno.h>
#include <stdlib.h>
#include <string.h>
#include <limits.h>
#include <stdarg.h>

static void error_exit(char *f, ...)
{
	va_list va;

	va_start(va, f);
	vprintf(f, va);
	printf(": %s\n", strerror(errno));
	va_end(va);

	exit(EXIT_FAILURE);
}

int main(int argc, char *argv[]) {
	struct termios termios, save;
	char buffer[MAX_CANON];
	int err;
	ssize_t n;

	printf("Pause terminal now with Ctrl+S\n");
	sleep(5);

	err = tcgetattr(STDIN_FILENO, &termios);
	if (err < 0)
		error_exit("tcgetattr");

	save = termios;
	termios.c_iflag &= ~IXON;

	err = tcsetattr(STDIN_FILENO, TCSANOW, &termios);
	if (err < 0)
		error_exit("tcsetattr");

#ifdef FIX_STOPPED_TTY
	tcflow(STDIN_FILENO, TCOOFF);
	tcflow(STDIN_FILENO, TCOON);
#endif

	printf("Enter some text: ");
	fflush(stdout);
	n = read(STDIN_FILENO, buffer, sizeof(buffer));
	if (n < 0)
		error_exit("read");

	printf("%.*s", (int)n, buffer);

	err = tcsetattr(STDIN_FILENO, TCSAFLUSH, &save);
	if (err < 0)
		error_exit("tcsetattr");
	return 0;
}

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Wang YanQing]

On Wed, May 08, 2013 at 02:32:23PM -0400, Peter Hurley wrote:
> Perhaps you misunderstood. The snippet above does indeed restart
> a tty which has been stopped via STOP_CHAR(tty) and the termios
> IXON flag cleared.

Thanks Peter, I get your meaning, and I think your suggestion
is right, not only because your reason, but also I find
this patch change the user interface behavior silently,
it will cause data lost or others' undesired behavior if
TCXONC restart tty but another side is not ready, it is bad.

I will send the V2.

Thanks.

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Peter Hurley]

On 05/07/2013 02:47 PM, Wang YanQing wrote:
> I meet emacs hang in start if I do the operation below:
>    1: echo 3 > /proc/sys/vm/drop_caches
>    2: emacs BigFile
>    3: Press CTRL-S follow 2 immediately
>
> Then emacs hang on, CTRL-Q can't resume, the terminal
> hang on, you can do nothing with this terminal except
> close it.
>
> The reason is before emacs takeover control the tty,
> we use CTRL-S to XOFF it. Then when emacs takeover the
> control, it may don't use the flow-control, so emacs hang.
> But after search the emacs's startup codes, I find they use TCXONC
> to workaround this situation:
>
>    /* This code added to insure that, if flow-control is not to be used,
>       we have an unlocked terminal at the start. */
>    if (!tty_out->flow_control) ioctl (fileno (tty_out->input), TCXONC, 1);
>    ioctl (fileno (tty_out->input), TCXONC, 1);
>
> But this workaround never work due the kernel's current code.
> This patch fix it.
>
> Below is the ChangeLog introduce the tty->flow_stopped flag:
>
> Thu Nov 21 10:05:22 1996  Theodre Ts'o  <tytso@localhost.mit.edu>
>
>         * tty_ioctl.c (tty_wait_until_sent): Always check the driver
>                 wait_until_ready routine, even if there are no characters
>                 in the xmit buffer.  (There may be charactes in the device
>                 FIFO.)
>                 (n_tty_ioctl): Add new flag tty->flow_stopped which
>                 indicates whether the tty is stopped due to a request by
>                 the TCXONC ioctl (used by tcflow).  If so, don't let an
>                 incoming XOFF character restart the tty.  The tty can only
>                 be restarted by another TCXONC request.
>
> So I think this patch make TCXONC can restart tty which stopped by
> STOP_CHAR don't break the original meaning.
>
> This patch will fix some strange tty relation hang problem,
> I believe I meet it with vim in ssh, and also see below bug report:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=465823
>
> Signed-off-by: Wang YanQing <udknight@gmail.com>
> ---
>   drivers/tty/tty_ioctl.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index e4455e0..42e08e5 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -1129,11 +1129,12 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
>   		case TCOOFF:
>   			if (!tty->flow_stopped) {
>   				tty->flow_stopped = 1;
> -				stop_tty(tty);
> +				if (!tty->stopped)
> +					stop_tty(tty);
>   			}
>   			break;
>   		case TCOON:
> -			if (tty->flow_stopped) {
> +			if (tty->flow_stopped || tty->stopped) {
>   				tty->flow_stopped = 0;
>   				start_tty(tty);
>   			}

This should be fixed in n_tty_set_termios() instead of fixing userspace
workarounds.

The problem occurs when the tty has been stopped with STOP_CHAR(tty) and then
termios is changed so that START_CHAR(tty) is subsequently ignored
(in the reported case, I_IXON(tty) is cleared).

Regards,
Peter Hurley

Re: [PATCH]TTY: Fix tty can't be restarted by TCXONC ioctl request

[Wang YanQing]

On Tue, May 07, 2013 at 04:50:05PM -0400, Peter Hurley wrote:
> This should be fixed in n_tty_set_termios() instead of fixing userspace
> workarounds.

Sorry, I misuse the word "workaround", emacs just do the right thing I think.
But your suggestion maybe import policy.

> The problem occurs when the tty has been stopped with STOP_CHAR(tty) and then
> termios is changed so that START_CHAR(tty) is subsequently ignored
> (in the reported case, I_IXON(tty) is cleared).
> 
That's right.

Thanks.