Question about TTY_DO_WRITE_WAKEUP

Question about TTY_DO_WRITE_WAKEUP

[Thomas Pfaff]

I have written a program that reads and writes data to the serial uart using 
async io.
Unfortunately it does not work properly because a SIGIO is never gerated when 
output becomes possible, the reason is that TTY_DO_WRITE_WAKEUP is not set for 
most of the tty drivers. After i set the bit in serial_core.c the program works 
as expected.
Now i wonder why TTY_DO_WRITE_WAKEUP is almost always disabled ?

Below is a test case.

Thank you in advance,

Thomas

#include <stdlib.h>
#include <unistd.h>
#include <signal.h>
#include <string.h>
#include <stdlib.h>
#include <sys/fcntl.h>
#include <termios.h>
#include <errno.h>

static const char outbuf [] = "01234567890abcdef\n";

static void sigio_handler (int sig __attribute__((unused)))
{
}

static int sigact (int sig, void (*handler)(int))
{	
	struct sigaction sa;

	memset (&sa, 0, sizeof (sa));

	sa.sa_flags = SA_NOCLDWAIT | SA_NOCLDSTOP;
	sigfillset (&sa.sa_mask);
	sa.sa_handler = handler;

	return sigaction (sig, &sa, NULL);
}

static int uart_set_termio (int fd)
{
	struct termios term;

	if (tcgetattr (fd, &term) == -1)
		return -1;

	term.c_cflag = B19200 | CS8 | CREAD | CLOCAL;
	term.c_iflag = IGNPAR;
	term.c_oflag = 0;
	term.c_lflag = 0;

	term.c_cc [VTIME] = 0;
	term.c_cc [VMIN] = 0;

	return tcsetattr (fd, TCSAFLUSH, &term);
}

static int uart_fd_init (int fd)
{
	if (fcntl (fd, F_SETSIG, 0) == -1 ||
			fcntl (fd, F_SETOWN, getpid ()) == -1)
		return -1;

	return fcntl (fd, F_SETFL, fcntl (fd, F_GETFL) | O_ASYNC | O_NONBLOCK);
}

static int uart_sig_init (void)
{
	sigset_t sigset;

	if (sigact (SIGIO, sigio_handler) == -1)
		return -1;

	if (sigemptyset (&sigset) == -1 ||
			sigaddset (&sigset, SIGIO) == -1)
		return -1;

	return sigprocmask (SIG_BLOCK, &sigset, NULL);
}

int main (int argc, char *argv [])
{
	int uart_fd;
	sigset_t sigmask;
	const int outbuflen = strlen (outbuf);
	const char *dev = "/dev/ttyS0";

	if (argc > 1)
		dev = argv [1];

	if (uart_sig_init () == -1)
		return EXIT_FAILURE;

	uart_fd = open (dev, O_RDWR | O_NOCTTY | O_NONBLOCK);
	if (uart_fd == -1)
		return EXIT_FAILURE;

	if (uart_set_termio (uart_fd) == -1 ||
			uart_fd_init (uart_fd) == -1)
		return EXIT_FAILURE;

	if (sigprocmask (SIG_SETMASK, NULL, &sigmask) == -1 ||
			sigdelset (&sigmask, SIGIO) == -1)
		return EXIT_FAILURE;

	while (1)
	{
		int written;
		int towrite = outbuflen;
		const char *buf = outbuf;

		while (towrite)
		{
			written = write (uart_fd, buf, towrite);
			if (written == -1)
			{
				if (errno == EAGAIN)
				{
					sigsuspend (&sigmask);
					continue;
				}
				else
					return EXIT_FAILURE;
			}

			buf += written;
			towrite -= written;
		}
	}

	return EXIT_SUCCESS;
}

Re: Question about TTY_DO_WRITE_WAKEUP

[Alan Cox]

On Thu, 20 Nov 2008 11:06:19 +0100 (CET)
Thomas Pfaff <tpfaff@pcs.com> wrote:

> I have written a program that reads and writes data to the serial uart using 
> async io.
> Unfortunately it does not work properly because a SIGIO is never gerated when 
> output becomes possible, the reason is that TTY_DO_WRITE_WAKEUP is not set for 
> most of the tty drivers. After i set the bit in serial_core.c the program works 
> as expected.
> Now i wonder why TTY_DO_WRITE_WAKEUP is almost always disabled ?

Almost nobody uses SIGIO based I/O, especially on serial so I'm not
actually suprised by this one. If you've got patches that fix it then
please send them in.

Alan

Re: Question about TTY_DO_WRITE_WAKEUP

[Thomas Pfaff]

On Thu, 20 Nov 2008, Alan Cox wrote:

> On Thu, 20 Nov 2008 11:06:19 +0100 (CET)
> Thomas Pfaff <tpfaff@pcs.com> wrote:
> 
> > I have written a program that reads and writes data to the serial uart using 
> > async io.
> > Unfortunately it does not work properly because a SIGIO is never gerated when 
> > output becomes possible, the reason is that TTY_DO_WRITE_WAKEUP is not set for 
> > most of the tty drivers. After i set the bit in serial_core.c the program works 
> > as expected.
> > Now i wonder why TTY_DO_WRITE_WAKEUP is almost always disabled ?
> 
> Almost nobody uses SIGIO based I/O, especially on serial so I'm not
> actually suprised by this one. If you've got patches that fix it then
> please send them in.

Is the TTY_DO_WRITE_WAKEUP bit some kind of cruft that can be removed entirely 
or should i generate a patch for serial devices only ?

Removing it seems to be cleaner to me but i am unsure if it will break some 
other stuff.

Re: Question about TTY_DO_WRITE_WAKEUP

[Alan Cox]

> Is the TTY_DO_WRITE_WAKEUP bit some kind of cruft that can be removed entirely 
> or should i generate a patch for serial devices only ?

It shouldn't happen at the serial level. The line discipline sets the
flag as and when it has data for the process above.

The expected flow in normal use is

	app writes to tty
		data goes to n_tty ldisc
			goes to tty
		n_tty ldisc seems tty blocked
		n_tty ldisc sets DO_WRITE_WAKEUP

	tty gets ucongested
		calls n_tty write_wakeup
			SIGIO sent
			Flag set again

The important part of the logic is in n_tty.c and tty_io.c

Alan

Re: Question about TTY_DO_WRITE_WAKEUP

[Thomas Pfaff]

On Thu, 20 Nov 2008, Alan Cox wrote:

> > Is the TTY_DO_WRITE_WAKEUP bit some kind of cruft that can be removed entirely 
> > or should i generate a patch for serial devices only ?
> 
> It shouldn't happen at the serial level. The line discipline sets the
> flag as and when it has data for the process above.
> 
> The expected flow in normal use is
> 
> 	app writes to tty
> 		data goes to n_tty ldisc
> 			goes to tty
> 		n_tty ldisc seems tty blocked
> 		n_tty ldisc sets DO_WRITE_WAKEUP
> 
> 	tty gets ucongested
> 		calls n_tty write_wakeup
> 			SIGIO sent
> 			Flag set again
> 
> The important part of the logic is in n_tty.c and tty_io.c

Following the source the TTY_DO_WRITE_WAKEUP flag is set in n_tty_write_wakeup. 

But n_tty_write_wakeup is only called from tty_wakeup when this bit is already 
set, therefore it makes no sense to set this bit in n_tty_write_wakeup again.

The flow looks to me as

If the tty driver sets TTY_DO_WRITE_WAKEUP a SIGIO is generated on every 
tty_wakeup.

If it is not set then n_tty_write_wakeup is never called and a SIGIO is not 
generated.

Thomas

Re: Question about TTY_DO_WRITE_WAKEUP

[Alan Cox]

> But n_tty_write_wakeup is only called from tty_wakeup when this bit is already 
> set, therefore it makes no sense to set this bit in n_tty_write_wakeup again.

The base code should probably really use test_and_clear_bit() when
calling that method.
> 
> The flow looks to me as
> 
> If the tty driver sets TTY_DO_WRITE_WAKEUP a SIGIO is generated on every 
> tty_wakeup.
> 
> If it is not set then n_tty_write_wakeup is never called and a SIGIO is not 
> generated.

Which isn't perfect (excess SIGIO cases) but doesn't seem incorrect. If
you've not blocked the tty output buffer then write() has not returned a
short write and no SIGIO is due.

Alan

Re: Question about TTY_DO_WRITE_WAKEUP

[Thomas Pfaff]

On Thu, 20 Nov 2008, Alan Cox wrote:

> > But n_tty_write_wakeup is only called from tty_wakeup when this bit is already 
> > set, therefore it makes no sense to set this bit in n_tty_write_wakeup again.
> 
> The base code should probably really use test_and_clear_bit() when
> calling that method.

Why should you test it, clear it and set it again in n_tty_write_wakeup ?

> > 
> > The flow looks to me as
> > 
> > If the tty driver sets TTY_DO_WRITE_WAKEUP a SIGIO is generated on every 
> > tty_wakeup.
> > 
> > If it is not set then n_tty_write_wakeup is never called and a SIGIO is not 
> > generated.
> 
> Which isn't perfect (excess SIGIO cases) but doesn't seem incorrect. If
> you've not blocked the tty output buffer then write() has not returned a
> short write and no SIGIO is due.
> 

Of course this is not incorrect, but this does not solve my problem with the 
TTY_DO_WRITE_WAKEUP flag.

IMHO a SIGIO on write possible should always be generated if the user wants it, 
currently it is generated when the user wants it and the tty driver enables the 
TTY_DO_WRITE_WAKEUP flag. Unfortunately most drivers don't set it.

Regarding excess SIGIO cases:

Once a write fails with EAGAIN a flag can be set and only in that case a SIGIO is 
generated, afterwards the bit is cleared. Maybe that is what TTY_DO_WRITE_WAKEUP 
was intended for. 

Thomas

Re: Question about TTY_DO_WRITE_WAKEUP

[Alan Cox]

> > The base code should probably really use test_and_clear_bit() when
> > calling that method.
> 
> Why should you test it, clear it and set it again in n_tty_write_wakeup ?

Because it should only be set again if a wakeup is needed. If the fasync
list for the tty is now empty it should stay clear.

> IMHO a SIGIO on write possible should always be generated if the user wants it, 
> currently it is generated when the user wants it and the tty driver enables the 
> TTY_DO_WRITE_WAKEUP flag. Unfortunately most drivers don't set it.

It is nothing to do with the driver. The line discipline sets it - or
rather should set it. If you have a case where you get an EAGAIN or short
write and the line discipline is not setting it then that is what needs
fixing not the drivers.

> Once a write fails with EAGAIN a flag can be set and only in that case a SIGIO is 
> generated, afterwards the bit is cleared. Maybe that is what TTY_DO_WRITE_WAKEUP 
> was intended for. 

Correct.

Re: Question about TTY_DO_WRITE_WAKEUP

[Alan Cox]

A quick further look suggests you need to add

	if (tty->fasync && (b - buf) != nr)
		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);

at the bottom of n_tty_write just before the return statement.

Alan