[PATCH] TTY: Fix loss of echoed characters

[PATCH] TTY: Fix loss of echoed characters

[Joe Peterson]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

Andrew, attached is a patch that fixes the loss of echoed characters in
two common cases: 1) tty output buffer is full due to lots of output and
2) tty is stopped.  It was discussed on the list here (and I had
supplied an earlier rev of this patch at the end of the thread last week
- no further comments were posted):

	http://marc.info/?l=linux-kernel&m=121788756631983&w=2

I have been testing on several machines for the past week or so, and
things look good.

					Thanks, Joe

[-- Attachment #2: tty-fix-loss-of-echoed-characters.patch --]
[-- Type: text/plain, Size: 29056 bytes --]

Fixes the loss of echoed (and other ldisc-generated characters) when
the tty is stopped or when the driver output buffer is full (happens
frequently for input during continuous program output, such as ^C).

Adds an "echo buffer" to the N_TTY line discipline that handles all
ldisc-generated output (including echoed characters).  Along with the
loss of characters, this also fixes the associated loss of sync between
tty output and the ldisc state when characters cannot be immediately
written to the tty driver.

The echo buffer stores (in addition to characters) state operations that need
to be done at the time of character output (like management of the column
position).  This allows echo to cooperate correctly with program output,
since the ldisc state remains consistent with actual characters written.

Highlights are:

* Handles echo (and other ldisc output) when tty driver buffer is full
  - continuous program output can block echo
* Saves echo when tty is in stopped state (e.g. ^S)
  - (e.g.: ^Q will correctly cause held characters to be released for output)
* Control character pairs (e.g. "^C") are treated atomically and not
  split up by interleaved program output
* Line discipline state is kept consistent with characters sent to
  the tty driver

Signed-off-by: Joe Peterson <joe@skyrush.com>
---

diff -Nurp linux.old/drivers/char/n_tty.c linux.new/drivers/char/n_tty.c
--- linux.old/drivers/char/n_tty.c	2008-08-19 20:34:04.835223908 -0600
+++ linux.new/drivers/char/n_tty.c	2008-08-19 20:39:54.115223028 -0600
@@ -62,6 +62,17 @@
 #define TTY_THRESHOLD_THROTTLE		128 /* now based on remaining room */
 #define TTY_THRESHOLD_UNTHROTTLE 	128
 
+/*
+ * Special byte codes used in the echo buffer to represent operations
+ * or special handling of characters.  Bytes in the echo buffer that
+ * are not part of such special blocks are treated as normal character
+ * codes.
+ */
+#define ECHO_OP_START 0xff
+#define ECHO_OP_MOVE_BACK_COL 0x80
+#define ECHO_OP_SET_CANON_COL 0x81
+#define ECHO_OP_TAB_ERASE 0x82
+
 static inline unsigned char *alloc_buf(void)
 {
 	gfp_t prio = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
@@ -159,6 +170,7 @@ static void check_unthrottle(struct tty_
  *	and make sure the driver is unthrottled. Called
  *	from n_tty_open() and n_tty_flush_buffer().
  */
+
 static void reset_buffer_flags(struct tty_struct *tty)
 {
 	unsigned long flags;
@@ -166,6 +178,9 @@ static void reset_buffer_flags(struct tt
 	spin_lock_irqsave(&tty->read_lock, flags);
 	tty->read_head = tty->read_tail = tty->read_cnt = 0;
 	spin_unlock_irqrestore(&tty->read_lock, flags);
+	spin_lock_irqsave(&tty->echo_lock, flags);
+	tty->echo_pos = tty->echo_cnt = tty->echo_overrun = 0;
+	spin_unlock_irqrestore(&tty->echo_lock, flags);
 	tty->canon_head = tty->canon_data = tty->erasing = 0;
 	memset(&tty->read_flags, 0, sizeof tty->read_flags);
 	n_tty_set_room(tty);
@@ -254,89 +269,118 @@ static inline int is_continuation(unsign
 }
 
 /**
- *	opost			-	output post processor
+ *	do_output_char			-	output one character
  *	@c: character (or partial unicode symbol)
  *	@tty: terminal device
+ *	@space: space available in write buffer
  *
- *	Perform OPOST processing.  Returns -1 when the output device is
- *	full and the character must be retried. Note that Linux currently
- *	ignores TABDLY, CRDLY, VTDLY, FFDLY and NLDLY. They simply aren't
- *	relevant in the world today. If you ever need them, add them here.
+ *	This is a helper function that handles one output character
+ *	(including special characters like TAB, CR, LF, etc.),
+ *	putting the results in the tty driver's write buffer.
+ *	It is assumed that the calling function does the required
+ *	locking and has already determined the space left in the tty
+ *	driver's write buffer.
  *
- *	Called from both the receive and transmit sides and can be called
- *	re-entrantly. Relies on lock_kernel() for tty->column state.
+ *	Note that Linux currently ignores TABDLY, CRDLY, VTDLY, FFDLY
+ *	and NLDLY.  They simply aren't relevant in the world today.
+ *	If you ever need them, add them here.
+ *
+ *	Returns the number of bytes of buffer space used or -1 if
+ *	no space left.
  */
 
-static int opost(unsigned char c, struct tty_struct *tty)
+static int do_output_char(unsigned char c, struct tty_struct *tty, int space)
 {
-	int	space, spaces;
+	int	spaces;
 
-	space = tty_write_room(tty);
 	if (!space)
 		return -1;
-
-	lock_kernel();
-	if (O_OPOST(tty)) {
-		switch (c) {
-		case '\n':
-			if (O_ONLRET(tty))
-				tty->column = 0;
-			if (O_ONLCR(tty)) {
-				if (space < 2) {
-					unlock_kernel();
-					return -1;
-				}
-				tty_put_char(tty, '\r');
-				tty->column = 0;
-			}
-			tty->canon_column = tty->column;
-			break;
-		case '\r':
-			if (O_ONOCR(tty) && tty->column == 0) {
-				unlock_kernel();
-				return 0;
-			}
-			if (O_OCRNL(tty)) {
-				c = '\n';
-				if (O_ONLRET(tty))
-					tty->canon_column = tty->column = 0;
-				break;
-			}
+	
+	switch (c) {
+	case '\n':
+		if (O_ONLRET(tty))
+			tty->column = 0;
+		if (O_ONLCR(tty)) {
+			if (space < 2)
+				return -1;
 			tty->canon_column = tty->column = 0;
+			tty_put_char(tty, '\r');
+			tty_put_char(tty, c);
+			return 2;
+		}
+		tty->canon_column = tty->column;
+		break;
+	case '\r':
+		if (O_ONOCR(tty) && tty->column == 0)
+			return 0;
+		if (O_OCRNL(tty)) {
+			c = '\n';
+			if (O_ONLRET(tty))
+				tty->canon_column = tty->column = 0;
 			break;
-		case '\t':
-			spaces = 8 - (tty->column & 7);
-			if (O_TABDLY(tty) == XTABS) {
-				if (space < spaces) {
-					unlock_kernel();
-					return -1;
-				}
-				tty->column += spaces;
-				tty->ops->write(tty, "        ", spaces);
-				unlock_kernel();
-				return 0;
-			}
+		}
+		tty->canon_column = tty->column = 0;
+		break;
+	case '\t':
+		spaces = 8 - (tty->column & 7);
+		if (O_TABDLY(tty) == XTABS) {
+			if (space < spaces)
+				return -1;
 			tty->column += spaces;
-			break;
-		case '\b':
-			if (tty->column > 0)
-				tty->column--;
-			break;
-		default:
-			if (O_OLCUC(tty))
-				c = toupper(c);
-			if (!iscntrl(c) && !is_continuation(c, tty))
-				tty->column++;
-			break;
+			tty->ops->write(tty, "        ", spaces);
+			return spaces;
 		}
+		tty->column += spaces;
+		break;
+	case '\b':
+		if (tty->column > 0)
+			tty->column--;
+		break;
+	default:
+		if (O_OLCUC(tty))
+			c = toupper(c);
+		if (!iscntrl(c) && !is_continuation(c, tty))
+			tty->column++;
+		break;
 	}
+
 	tty_put_char(tty, c);
+	return 1;
+}
+
+/**
+ *	process_output			-	output post processor
+ *	@c: character (or partial unicode symbol)
+ *	@tty: terminal device
+ *
+ *	Perform OPOST processing.  Returns -1 when the output device is
+ *	full and the character must be retried.
+ *
+ *	Called from both the receive and transmit sides and can be called
+ *	re-entrantly.  Relies on lock_kernel for tty->column state.
+ *	Since we rely on lock_kernel to prevent the tty_write_room from
+ *	changing after it is obtained, an alternate means to ensure this
+ *	would need to be implemented if the locking mechanism changed.
+ */
+
+static int process_output(unsigned char c, struct tty_struct *tty)
+{
+	int	space, retval;
+
+	lock_kernel();
+
+	space = tty_write_room(tty);
+	retval = do_output_char(c, tty, space);
+
 	unlock_kernel();
-	return 0;
+	if (retval < 0)
+		return -1;
+	else
+		return 0;
 }
 
 /**
- *	opost_block		-	block postprocess
+ *	process_output_block		-	block post processor
  *	@tty: terminal device
  *	@inbuf: user buffer
  *	@nr: number of bytes
@@ -346,24 +390,31 @@ static int opost(unsigned char c, struct
  *	the simple cases normally found and helps to generate blocks of
  *	symbols for the console driver and thus improve performance.
  *
- *	Called from write_chan under the tty layer write lock. Relies
- *	on lock_kernel for the tty->column state.
+ *	Called from write_chan under the tty layer write lock.
+ *	Relies on lock_kernel for the tty->column state.
+ *	Since we rely on lock_kernel to prevent the tty_write_room from
+ *	changing after it is obtained, an alternate means to ensure this
+ *	would need to be implemented if the locking mechanism changed.
  */
 
-static ssize_t opost_block(struct tty_struct *tty,
-		       const unsigned char *buf, unsigned int nr)
+static ssize_t process_output_block(struct tty_struct *tty,
+				    const unsigned char *buf, unsigned int nr)
 {
 	int	space;
 	int 	i;
 	const unsigned char *cp;
 
+	lock_kernel();
+
 	space = tty_write_room(tty);
 	if (!space)
+	{
+		unlock_kernel();
 		return 0;
+	}
 	if (nr > space)
 		nr = space;
 
-	lock_kernel();
 	for (i = 0, cp = buf; i < nr; i++, cp++) {
 		switch (*cp) {
 		case '\n':
@@ -395,38 +446,349 @@ static ssize_t opost_block(struct tty_st
 		}
 	}
 break_out:
-	if (tty->ops->flush_chars)
-		tty->ops->flush_chars(tty);
+	//if (tty->ops->flush_chars)
+	//	tty->ops->flush_chars(tty);
 	i = tty->ops->write(tty, buf, i);
+
 	unlock_kernel();
 	return i;
 }
 
+/**
+ *	process_echoes	-	write pending echoed characters
+ *	@tty: terminal device
+ *
+ *	Write echoed (and other ldisc-generated) characters to the
+ *	tty that have been stored previously in the echo buffer.
+ *
+ *	Characters generated by the ldisc (including echoes) need to
+ *	be buffered because the driver's write buffer can fill during
+ *	heavy program output.  Echoing straight to the driver will
+ *	often fail under these conditions, causing lost characters and
+ *	resulting mismatches of ldisc state information.
+ *
+ *	Since the ldisc state must represent the characters actually sent
+ *	to the driver at the time of the write, operations like certain
+ *	changes in column state are also saved in the buffer and executed
+ *	here.
+ *
+ *	A circular fifo buffer is used so that the most recent characters
+ *	are prioritized.  Also, when control characters are echoed with a
+ *	prefixed "^", the pair is treated atomically and thus not separated.
+ *
+ *	Like the process_output functions, this relies on lock_kernel.
+ *	If this lock is ever removed, we should make sure this function
+ *	is locked against other code that currently uses echo_lock,
+ *	and we should also lock against normal tty output done in
+ *	write_chan (so the control pairs do not get separated).  Also,
+ *	since we rely on lock_kernel to prevent the tty_write_room from
+ *	changing after it is obtained, an alternate means to ensure this
+ *	would need to be implemented if the locking mechanism changed.
+ */
+
+static void process_echoes(struct tty_struct *tty)
+{
+	int	space, num, nr;
+	unsigned char c;
+	unsigned char *cp, *buf_end;
+
+	if (!(num = tty->echo_cnt))
+		return;
+
+	lock_kernel();
+
+	space = tty_write_room(tty);
+
+	buf_end = tty->echo_buf + N_TTY_BUF_SIZE;
+	cp = tty->echo_buf + tty->echo_pos;
+	nr = num;
+	while (nr > 0) {
+		c = *cp;
+		if (c == ECHO_OP_START) {
+			unsigned char op;
+			unsigned char *opp;
+			int no_space_left = 0;
+
+			/*
+			 * If the buffer byte is the start of a multi-byte
+			 * operation, get the next byte, which is either the
+			 * op code or a control character value.
+			 */
+			opp = cp + 1;
+			if (opp == buf_end)
+				opp -= N_TTY_BUF_SIZE;
+			op = *opp;
+			
+			switch (op) {
+				unsigned char lbyte, hbyte;
+				unsigned short rdiff;
+				int num_bs;
+				unsigned int col;
+
+			case ECHO_OP_TAB_ERASE:
+				/* Extract rdiff value from next two bytes */
+				if (++opp == buf_end)
+					opp -= N_TTY_BUF_SIZE;
+				lbyte = *opp;
+				if (++opp == buf_end)
+					opp -= N_TTY_BUF_SIZE;
+				hbyte = *opp;
+				rdiff = (hbyte << 8) | lbyte;
+				
+				col = tty->canon_column + rdiff;
+
+				/* should never happen */
+				if (tty->column > 0x80000000)
+					tty->column = 0;
+
+				num_bs = tty->column - col;
+				if (num_bs < 0)
+					num_bs = 0;
+				if (num_bs > space) {
+					no_space_left = 1;
+					break;
+				}
+
+				/* Now backup to that column. */
+				while (tty->column > col) {
+					tty_put_char(tty, '\b');
+					if (tty->column > 0)
+						tty->column--;
+				}
+				space -= num_bs;
+				cp += 4;
+				nr -= 4;
+				break;
+
+			case ECHO_OP_SET_CANON_COL:
+				tty->canon_column = tty->column;
+				cp += 2;
+				nr -= 2;
+				break;
+
+			case ECHO_OP_MOVE_BACK_COL:
+				if (tty->column > 0)
+					tty->column--;
+				cp += 2;
+				nr -= 2;
+				break;
+
+			case ECHO_OP_START:
+				/* This is an escaped echo op start code */
+				if (!space) {
+					no_space_left = 1;
+					break;
+				}
+				tty_put_char(tty, ECHO_OP_START);
+				tty->column++;
+				space--;
+				cp += 2;
+				nr -= 2;
+				break;
+
+			default:
+				if (iscntrl(op)) {
+					if (L_ECHOCTL(tty)) {
+						/*
+						 * Ensure there is enough space
+						 * for the whole ctrl pair.
+						 */
+						if (space < 2) {
+							no_space_left = 1;
+							break;
+						}
+						tty_put_char(tty, '^');
+						tty_put_char(tty, op ^ 0100);
+						tty->column += 2;
+						space -= 2;
+					} else {
+						if (!space) {
+							no_space_left = 1;
+							break;
+						}
+						tty_put_char(tty, op);
+						space--;
+					}
+				}
+				/*
+				 * If above falls through, this was an
+				 * undefined op.
+				 */
+				cp += 2;
+				nr -= 2;
+			}
+
+			if (no_space_left)
+				break;
+		} else {
+			int retval;
+
+			if ((retval = do_output_char(c, tty, space)) < 0)
+				break;
+			space -= retval;
+			cp += 1;
+			nr -= 1;
+		}
+
+		/* When end of circular buffer reached, wrap around */
+		if (cp >= buf_end)
+			cp -= N_TTY_BUF_SIZE;
+	}
+
+	tty->echo_cnt = nr;
+	if (tty->echo_cnt == 0) {
+		tty->echo_pos = 0;
+		tty->echo_overrun = 0;
+	} else {
+		int num_processed = (num - nr);
+		tty->echo_pos += num_processed;
+		tty->echo_pos &= (N_TTY_BUF_SIZE - 1);
+		if (num_processed > 0)
+			tty->echo_overrun = 0;
+	}
+
+	unlock_kernel();
+
+	if (tty->ops->flush_chars)
+		tty->ops->flush_chars(tty);
+}
+
+/**
+ *	add_echo_byte	-	add a byte to the echo buffer
+ *	@c: unicode byte to echo
+ *	@tty: terminal device
+ *
+ *	Add a character or operation byte to the echo buffer.
+ */
+
+static void add_echo_byte(unsigned char c, struct tty_struct *tty)
+{
+	int 	add_char_pos;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->echo_lock, flags);
+
+	add_char_pos = tty->echo_pos + tty->echo_cnt;
+	if (add_char_pos >= N_TTY_BUF_SIZE)
+		add_char_pos -= N_TTY_BUF_SIZE;
+
+	/* Detect overrun */
+	if (tty->echo_cnt == N_TTY_BUF_SIZE) {
+		/*
+		 * If the start position pointer needs to be advanced
+		 * due to running out of buffer space, be sure it is
+		 * done in such a way as to remove whole
+		 * operation byte groups.
+		 */
+		if (*(tty->echo_buf +
+		      (tty->echo_pos & (N_TTY_BUF_SIZE - 1))) == ECHO_OP_START)
+		{
+			if (*(tty->echo_buf +
+			      ((tty->echo_pos + 1) & (N_TTY_BUF_SIZE - 1))) == ECHO_OP_TAB_ERASE) {
+				tty->echo_pos += 4;
+				tty->echo_cnt -= 3;
+			} else {
+				tty->echo_pos += 2;
+				tty->echo_cnt -= 1;
+			}
+		} else
+			tty->echo_pos++;
+		tty->echo_pos &= (N_TTY_BUF_SIZE - 1);
+		tty->echo_overrun = 1;
+	} else
+		tty->echo_cnt++;
+
+	tty->echo_buf[add_char_pos] = c;
+
+	spin_unlock_irqrestore(&tty->echo_lock, flags);
+}
+
+/**
+ *	echo_move_back_col	-	add operation to move back a column
+ *	@tty: terminal device
+ *
+ *	Add an operation to the echo buffer to move back one column.
+ */
+
+static void echo_move_back_col(struct tty_struct *tty)
+{
+	add_echo_byte(ECHO_OP_START, tty);
+	add_echo_byte(ECHO_OP_MOVE_BACK_COL, tty);
+}
+
+/**
+ *	echo_set_canon_col	-	add operation to set the canon column
+ *	@tty: terminal device
+ *
+ *	Add an operation to the echo buffer to set the canon column
+ *	to the current column.
+ */
+
+static void echo_set_canon_col(struct tty_struct *tty)
+{
+	add_echo_byte(ECHO_OP_START, tty);
+	add_echo_byte(ECHO_OP_SET_CANON_COL, tty);
+}
+
+/**
+ *	echo_tab_erase	-	add operation to erase tabs
+ *	@tty: terminal device
+ *
+ *	Add an operation to the echo buffer to set the canon column
+ *	to the current column.
+ */
+
+static void echo_tab_erase(unsigned short rdiff, struct tty_struct *tty)
+{
+	add_echo_byte(ECHO_OP_START, tty);
+	add_echo_byte(ECHO_OP_TAB_ERASE, tty);
+	add_echo_byte(rdiff & 0xff, tty);
+	add_echo_byte(rdiff >> 8, tty);
+}
 
 /**
- *	echo_char	-	echo characters
+ *	echo_char_raw	-	echo a character raw
  *	@c: unicode byte to echo
  *	@tty: terminal device
  *
  *	Echo user input back onto the screen. This must be called only when
  *	L_ECHO(tty) is true. Called from the driver receive_buf path.
+ *
+ *	This variant does not treat control characters specially.
  */
 
-static void echo_char(unsigned char c, struct tty_struct *tty)
+static void echo_char_raw(unsigned char c, struct tty_struct *tty)
 {
-	if (L_ECHOCTL(tty) && iscntrl(c) && c != '\t') {
-		tty_put_char(tty, '^');
-		tty_put_char(tty, c ^ 0100);
-		tty->column += 2;
+	if (c == ECHO_OP_START) {
+		add_echo_byte(ECHO_OP_START, tty);
+		add_echo_byte(ECHO_OP_START, tty);
 	} else
-		opost(c, tty);
+		add_echo_byte(c, tty);
+}
+
+/**
+ *	echo_char	-	echo a character
+ *	@c: unicode byte to echo
+ *	@tty: terminal device
+ *
+ *	Echo user input back onto the screen. This must be called only when
+ *	L_ECHO(tty) is true. Called from the driver receive_buf path.
+ *
+ *	This variant tags control characters to be possibly echoed as
+ *	as "^X" (where X is the letter representing the control char).
+ */
+
+static void echo_char(unsigned char c, struct tty_struct *tty)
+{
+	if (iscntrl(c) && c != '\t')
+		add_echo_byte(ECHO_OP_START, tty);
+	echo_char_raw(c, tty);
 }
 
 static inline void finish_erasing(struct tty_struct *tty)
 {
 	if (tty->erasing) {
-		tty_put_char(tty, '/');
-		tty->column++;
+		echo_char_raw('/', tty);
 		tty->erasing = 0;
 	}
 }
@@ -448,7 +810,7 @@ static void eraser(unsigned char c, stru
 	unsigned long flags;
 
 	if (tty->read_head == tty->canon_head) {
-		/* opost('\a', tty); */		/* what do you think? */
+		/* echo_char_raw('\a', tty); */ /* what do you think? */
 		return;
 	}
 	if (c == ERASE_CHAR(tty))
@@ -474,7 +836,7 @@ static void eraser(unsigned char c, stru
 			echo_char(KILL_CHAR(tty), tty);
 			/* Add a newline if ECHOK is on and ECHOKE is off. */
 			if (L_ECHOK(tty))
-				opost('\n', tty);
+				echo_char_raw('\n', tty);
 			return;
 		}
 		kill_type = KILL;
@@ -509,67 +871,58 @@ static void eraser(unsigned char c, stru
 		if (L_ECHO(tty)) {
 			if (L_ECHOPRT(tty)) {
 				if (!tty->erasing) {
-					tty_put_char(tty, '\\');
-					tty->column++;
+					echo_char_raw('\\', tty);
 					tty->erasing = 1;
 				}
 				/* if cnt > 1, output a multi-byte character */
 				echo_char(c, tty);
 				while (--cnt > 0) {
 					head = (head+1) & (N_TTY_BUF_SIZE-1);
-					tty_put_char(tty, tty->read_buf[head]);
+					echo_char_raw(tty->read_buf[head], tty);
+					echo_move_back_col(tty);
 				}
 			} else if (kill_type == ERASE && !L_ECHOE(tty)) {
 				echo_char(ERASE_CHAR(tty), tty);
 			} else if (c == '\t') {
-				unsigned int col = tty->canon_column;
+				unsigned short rdiff = 0;
 				unsigned long tail = tty->canon_head;
 
-				/* Find the column of the last char. */
+				/*
+				 * Find number of columns from canon_head
+				 * to read_head.  This will later be
+				 * added to the canon_column to determine
+				 * how far to erase up to the cur column.
+				 */
 				while (tail != tty->read_head) {
 					c = tty->read_buf[tail];
 					if (c == '\t')
-						col = (col | 7) + 1;
+						rdiff = (rdiff | 7) + 1;
 					else if (iscntrl(c)) {
 						if (L_ECHOCTL(tty))
-							col += 2;
+							rdiff += 2;
 					} else if (!is_continuation(c, tty))
-						col++;
+						rdiff++;
 					tail = (tail+1) & (N_TTY_BUF_SIZE-1);
 				}
 
-				/* should never happen */
-				if (tty->column > 0x80000000)
-					tty->column = 0;
-
-				/* Now backup to that column. */
-				while (tty->column > col) {
-					/* Can't use opost here. */
-					tty_put_char(tty, '\b');
-					if (tty->column > 0)
-						tty->column--;
-				}
+				echo_tab_erase(rdiff, tty);
 			} else {
 				if (iscntrl(c) && L_ECHOCTL(tty)) {
-					tty_put_char(tty, '\b');
-					tty_put_char(tty, ' ');
-					tty_put_char(tty, '\b');
-					if (tty->column > 0)
-						tty->column--;
+					echo_char_raw('\b', tty);
+					echo_char_raw(' ', tty);
+					echo_char_raw('\b', tty);
 				}
 				if (!iscntrl(c) || L_ECHOCTL(tty)) {
-					tty_put_char(tty, '\b');
-					tty_put_char(tty, ' ');
-					tty_put_char(tty, '\b');
-					if (tty->column > 0)
-						tty->column--;
+					echo_char_raw('\b', tty);
+					echo_char_raw(' ', tty);
+					echo_char_raw('\b', tty);
 				}
 			}
 		}
 		if (kill_type == ERASE)
 			break;
 	}
-	if (tty->read_head == tty->canon_head)
+	if (tty->read_head == tty->canon_head && L_ECHO(tty))
 		finish_erasing(tty);
 }
 
@@ -698,14 +1051,18 @@ static inline void n_tty_receive_char(st
 		c=tolower(c);
 
 	if (tty->stopped && !tty->flow_stopped && I_IXON(tty) &&
-	    ((I_IXANY(tty) && c != START_CHAR(tty) && c != STOP_CHAR(tty)) ||
-	     c == INTR_CHAR(tty) || c == QUIT_CHAR(tty) || c == SUSP_CHAR(tty)))
+	    I_IXANY(tty) && c != START_CHAR(tty) && c != STOP_CHAR(tty) &&
+	    c != INTR_CHAR(tty) && c != QUIT_CHAR(tty) && c != SUSP_CHAR(tty)) {
 		start_tty(tty);
+		process_echoes(tty);
+	}
 
 	if (tty->closing) {
 		if (I_IXON(tty)) {
-			if (c == START_CHAR(tty))
+			if (c == START_CHAR(tty)) {
 				start_tty(tty);
+				process_echoes(tty);
+			}
 			else if (c == STOP_CHAR(tty))
 				stop_tty(tty);
 		}
@@ -719,17 +1076,20 @@ static inline void n_tty_receive_char(st
 	 * up.
 	 */
 	if (!test_bit(c, tty->process_char_map) || tty->lnext) {
-		finish_erasing(tty);
 		tty->lnext = 0;
 		if (L_ECHO(tty)) {
+			finish_erasing(tty);
 			if (tty->read_cnt >= N_TTY_BUF_SIZE-1) {
-				tty_put_char(tty, '\a'); /* beep if no space */
+				/* beep if no space */
+				echo_char_raw('\a', tty);
+				process_echoes(tty);
 				return;
 			}
 			/* Record the column of first canon char. */
 			if (tty->canon_head == tty->read_head)
-				tty->canon_column = tty->column;
+				echo_set_canon_col(tty);
 			echo_char(c, tty);
+			process_echoes(tty);
 		}
 		if (I_PARMRK(tty) && c == (unsigned char) '\377')
 			put_tty_queue(c, tty);
@@ -740,6 +1100,7 @@ static inline void n_tty_receive_char(st
 	if (I_IXON(tty)) {
 		if (c == START_CHAR(tty)) {
 			start_tty(tty);
+			process_echoes(tty);
 			return;
 		}
 		if (c == STOP_CHAR(tty)) {
@@ -760,7 +1121,6 @@ static inline void n_tty_receive_char(st
 		if (c == SUSP_CHAR(tty)) {
 send_signal:
 			/*
-			 * Echo character, and then send the signal.
 			 * Note that we do not use isig() here because we want
 			 * the order to be:
 			 * 1) flush, 2) echo, 3) signal
@@ -769,8 +1129,12 @@ send_signal:
 				n_tty_flush_buffer(tty);
 				tty_driver_flush_buffer(tty);
 			}
-			if (L_ECHO(tty))
+			if (I_IXON(tty))
+				start_tty(tty);
+			if (L_ECHO(tty)) {
 				echo_char(c, tty);
+				process_echoes(tty);
+			}
 			if (tty->pgrp)
 				kill_pgrp(tty->pgrp, signal, 1);
 			return;
@@ -789,6 +1153,7 @@ send_signal:
 		if (c == ERASE_CHAR(tty) || c == KILL_CHAR(tty) ||
 		    (c == WERASE_CHAR(tty) && L_IEXTEN(tty))) {
 			eraser(c, tty);
+			process_echoes(tty);
 			return;
 		}
 		if (c == LNEXT_CHAR(tty) && L_IEXTEN(tty)) {
@@ -796,8 +1161,9 @@ send_signal:
 			if (L_ECHO(tty)) {
 				finish_erasing(tty);
 				if (L_ECHOCTL(tty)) {
-					tty_put_char(tty, '^');
-					tty_put_char(tty, '\b');
+					echo_char_raw('^', tty);
+					echo_char_raw('\b', tty);
+					process_echoes(tty);
 				}
 			}
 			return;
@@ -808,18 +1174,20 @@ send_signal:
 
 			finish_erasing(tty);
 			echo_char(c, tty);
-			opost('\n', tty);
+			echo_char_raw('\n', tty);
 			while (tail != tty->read_head) {
 				echo_char(tty->read_buf[tail], tty);
 				tail = (tail+1) & (N_TTY_BUF_SIZE-1);
 			}
+			process_echoes(tty);
 			return;
 		}
 		if (c == '\n') {
 			if (L_ECHO(tty) || L_ECHONL(tty)) {
 				if (tty->read_cnt >= N_TTY_BUF_SIZE-1)
-					tty_put_char(tty, '\a');
-				opost('\n', tty);
+					echo_char_raw('\a', tty);
+				echo_char_raw('\n', tty);
+				process_echoes(tty);
 			}
 			goto handle_newline;
 		}
@@ -836,11 +1204,12 @@ send_signal:
 			 */
 			if (L_ECHO(tty)) {
 				if (tty->read_cnt >= N_TTY_BUF_SIZE-1)
-					tty_put_char(tty, '\a');
+					echo_char_raw('\a', tty);
 				/* Record the column of first canon char. */
 				if (tty->canon_head == tty->read_head)
-					tty->canon_column = tty->column;
+					echo_set_canon_col(tty);
 				echo_char(c, tty);
+				process_echoes(tty);
 			}
 			/*
 			 * XXX does PARMRK doubling happen for
@@ -863,20 +1232,23 @@ handle_newline:
 		}
 	}
 
-	finish_erasing(tty);
 	if (L_ECHO(tty)) {
+		finish_erasing(tty);
 		if (tty->read_cnt >= N_TTY_BUF_SIZE-1) {
-			tty_put_char(tty, '\a'); /* beep if no space */
+			/* beep if no space */
+			echo_char_raw('\a', tty);
+			process_echoes(tty);
 			return;
 		}
 		if (c == '\n')
-			opost('\n', tty);
+			echo_char_raw('\n', tty);
 		else {
 			/* Record the column of first canon char. */
 			if (tty->canon_head == tty->read_head)
-				tty->canon_column = tty->column;
+				echo_set_canon_col(tty);
 			echo_char(c, tty);
 		}
+		process_echoes(tty);
 	}
 
 	if (I_PARMRK(tty) && c == (unsigned char) '\377')
@@ -897,6 +1269,9 @@ handle_newline:
 
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
+	/* Write out any echoed characters that are still pending */
+	process_echoes(tty);
+	
 	if (tty->fasync) {
 		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
@@ -1094,6 +1469,10 @@ static void n_tty_close(struct tty_struc
 		free_buf(tty->read_buf);
 		tty->read_buf = NULL;
 	}
+	if (tty->echo_buf) {
+		free_buf(tty->echo_buf);
+		tty->echo_buf = NULL;
+	}
 }
 
 /**
@@ -1111,13 +1490,19 @@ static int n_tty_open(struct tty_struct 
 	if (!tty)
 		return -EINVAL;
 
-	/* This one is ugly. Currently a malloc failure here can panic */
+	/* These are ugly. Currently a malloc failure here can panic */
 	if (!tty->read_buf) {
 		tty->read_buf = alloc_buf();
 		if (!tty->read_buf)
 			return -ENOMEM;
 	}
+	if (!tty->echo_buf) {
+		tty->echo_buf = alloc_buf();
+		if (!tty->echo_buf)
+			return -ENOMEM;
+	}
 	memset(tty->read_buf, 0, N_TTY_BUF_SIZE);
+	memset(tty->echo_buf, 0, N_TTY_BUF_SIZE);
 	reset_buffer_flags(tty);
 	tty->column = 0;
 	n_tty_set_termios(tty, NULL);
@@ -1473,6 +1858,9 @@ static ssize_t write_chan(struct tty_str
 			return retval;
 	}
 
+	/* Write out any echoed characters that are still pending */
+	process_echoes(tty);
+	
 	add_wait_queue(&tty->write_wait, &wait);
 	while (1) {
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -1486,7 +1874,7 @@ static ssize_t write_chan(struct tty_str
 		}
 		if (O_OPOST(tty) && !(test_bit(TTY_HW_COOK_OUT, &tty->flags))) {
 			while (nr > 0) {
-				ssize_t num = opost_block(tty, b, nr);
+				ssize_t num = process_output_block(tty, b, nr);
 				if (num < 0) {
 					if (num == -EAGAIN)
 						break;
@@ -1498,7 +1886,7 @@ static ssize_t write_chan(struct tty_str
 				if (nr == 0)
 					break;
 				c = *b;
-				if (opost(c, tty) < 0)
+				if (process_output(c, tty) < 0)
 					break;
 				b++; nr--;
 			}
diff -Nurp linux.old/drivers/char/tty_io.c linux.new/drivers/char/tty_io.c
--- linux.old/drivers/char/tty_io.c	2008-08-19 20:34:11.655223072 -0600
+++ linux.new/drivers/char/tty_io.c	2008-08-19 20:47:35.235222707 -0600
@@ -3344,6 +3344,7 @@ static void initialize_tty_struct(struct
 	mutex_init(&tty->atomic_write_lock);
 	spin_lock_init(&tty->read_lock);
 	spin_lock_init(&tty->ctrl_lock);
+	spin_lock_init(&tty->echo_lock);
 	INIT_LIST_HEAD(&tty->tty_files);
 	INIT_WORK(&tty->SAK_work, do_SAK_work);
 }
diff -Nurp linux.old/drivers/char/vt.c linux.new/drivers/char/vt.c
--- linux.old/drivers/char/vt.c	2008-08-14 09:38:48.728008823 -0600
+++ linux.new/drivers/char/vt.c	2008-08-20 08:36:09.454627331 -0600
@@ -2656,7 +2656,7 @@ static int con_write_room(struct tty_str
 {
 	if (tty->stopped)
 		return 0;
-	return 4096;		/* No limit, really; we're not buffering */
+	return 32768;		/* No limit, really; we're not buffering */
 }
 
 static int con_chars_in_buffer(struct tty_struct *tty)
diff -Nurp linux.old/include/linux/tty.h linux.new/include/linux/tty.h
--- linux.old/include/linux/tty.h	2008-08-19 20:32:56.045223069 -0600
+++ linux.new/include/linux/tty.h	2008-08-19 20:39:54.195223961 -0600
@@ -249,6 +249,7 @@ struct tty_struct {
 	unsigned int column;
 	unsigned char lnext:1, erasing:1, raw:1, real_raw:1, icanon:1;
 	unsigned char closing:1;
+	unsigned char echo_overrun:1;
 	unsigned short minimum_to_wake;
 	unsigned long overrun_time;
 	int num_overrun;
@@ -258,6 +259,9 @@ struct tty_struct {
 	int read_tail;
 	int read_cnt;
 	unsigned long read_flags[N_TTY_BUF_SIZE/(8*sizeof(unsigned long))];
+	unsigned char *echo_buf;
+	unsigned int echo_pos;
+	unsigned int echo_cnt;
 	int canon_data;
 	unsigned long canon_head;
 	unsigned int canon_column;
@@ -266,6 +270,7 @@ struct tty_struct {
 	unsigned char *write_buf;
 	int write_cnt;
 	spinlock_t read_lock;
+	spinlock_t echo_lock;
 	/* If the tty has a pending do_SAK, queue it here - akpm */
 	struct work_struct SAK_work;
 	struct tty_port *port;

Re: [PATCH] TTY: Fix loss of echoed characters

[Joe Peterson]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

Joe Peterson wrote:
> Andrew, attached is a patch that fixes the loss of echoed characters in
> two common cases: 1) tty output buffer is full due to lots of output and
> 2) tty is stopped.

Attached is a follow-on patch.  It just does some code style cleanup and
minor code efficiency tweaks.  Please apply it after the main patch
(probably should be combined into one patch).

						Thanks, Joe

[-- Attachment #2: tty-fix-loss-of-echoed-characters-cleanup.patch --]
[-- Type: text/plain, Size: 4012 bytes --]

Minor code efficiency and style cleanup.  No behavior changes.
Apply after tty-fix-loss-of-echoed-characters.patch.

Signed-off-by: Joe Peterson <joe@skyrush.com>
---

--- linux/drivers/char/n_tty.c.orig	2008-08-20 18:19:06.000000000 -0600
+++ linux/drivers/char/n_tty.c	2008-08-25 13:41:30.000000000 -0600
@@ -446,8 +446,6 @@ static ssize_t process_output_block(stru
 		}
 	}
 break_out:
-	//if (tty->ops->flush_chars)
-	//	tty->ops->flush_chars(tty);
 	i = tty->ops->write(tty, buf, i);
 
 	unlock_kernel();
@@ -455,11 +453,11 @@ break_out:
 }
 
 /**
- *	process_echoes	-	write pending echoed characters
+ *	process_echoes	-	write pending echo characters
  *	@tty: terminal device
  *
- *	Write echoed (and other ldisc-generated) characters to the
- *	tty that have been stored previously in the echo buffer.
+ *	Write previously buffered echo (and other ldisc-generated)
+ *	characters to the tty.
  *
  *	Characters generated by the ldisc (including echoes) need to
  *	be buffered because the driver's write buffer can fill during
@@ -488,11 +486,11 @@ break_out:
 
 static void process_echoes(struct tty_struct *tty)
 {
-	int	space, num, nr;
+	int	space, nr;
 	unsigned char c;
 	unsigned char *cp, *buf_end;
 
-	if (!(num = tty->echo_cnt))
+	if (!tty->echo_cnt)
 		return;
 
 	lock_kernel();
@@ -501,7 +499,7 @@ static void process_echoes(struct tty_st
 
 	buf_end = tty->echo_buf + N_TTY_BUF_SIZE;
 	cp = tty->echo_buf + tty->echo_pos;
-	nr = num;
+	nr = tty->echo_cnt;
 	while (nr > 0) {
 		c = *cp;
 		if (c == ECHO_OP_START) {
@@ -635,14 +633,15 @@ static void process_echoes(struct tty_st
 			cp -= N_TTY_BUF_SIZE;
 	}
 
-	tty->echo_cnt = nr;
-	if (tty->echo_cnt == 0) {
+	if (nr == 0) {
 		tty->echo_pos = 0;
+		tty->echo_cnt = 0;
 		tty->echo_overrun = 0;
 	} else {
-		int num_processed = (num - nr);
+		int num_processed = tty->echo_cnt - nr;
 		tty->echo_pos += num_processed;
-		tty->echo_pos &= (N_TTY_BUF_SIZE - 1);
+		tty->echo_pos &= N_TTY_BUF_SIZE - 1;
+		tty->echo_cnt = nr;
 		if (num_processed > 0)
 			tty->echo_overrun = 0;
 	}
@@ -663,42 +662,43 @@ static void process_echoes(struct tty_st
 
 static void add_echo_byte(unsigned char c, struct tty_struct *tty)
 {
-	int 	add_char_pos;
+	int	new_byte_pos;
 	unsigned long flags;
 
 	spin_lock_irqsave(&tty->echo_lock, flags);
 
-	add_char_pos = tty->echo_pos + tty->echo_cnt;
-	if (add_char_pos >= N_TTY_BUF_SIZE)
-		add_char_pos -= N_TTY_BUF_SIZE;
-
-	/* Detect overrun */
 	if (tty->echo_cnt == N_TTY_BUF_SIZE) {
+		/* Circular buffer is already at capacity */
+		new_byte_pos = tty->echo_pos;
+
 		/*
-		 * If the start position pointer needs to be advanced
-		 * due to running out of buffer space, be sure it is
-		 * done in such a way as to remove whole
-		 * operation byte groups.
+		 * Since the buffer start position needs to be advanced,
+		 * be sure to step by a whole operation byte group.
 		 */
-		if (*(tty->echo_buf +
-		      (tty->echo_pos & (N_TTY_BUF_SIZE - 1))) == ECHO_OP_START)
+		if (tty->echo_buf[tty->echo_pos] == ECHO_OP_START)
 		{
-			if (*(tty->echo_buf +
-			      ((tty->echo_pos + 1) & (N_TTY_BUF_SIZE - 1))) == ECHO_OP_TAB_ERASE) {
+			if (tty->echo_buf[(tty->echo_pos + 1) &
+					  (N_TTY_BUF_SIZE - 1)] ==
+						ECHO_OP_TAB_ERASE) {
 				tty->echo_pos += 4;
 				tty->echo_cnt -= 3;
 			} else {
 				tty->echo_pos += 2;
 				tty->echo_cnt -= 1;
 			}
-		} else
+		} else {
 			tty->echo_pos++;
-		tty->echo_pos &= (N_TTY_BUF_SIZE - 1);
+		}
+		tty->echo_pos &= N_TTY_BUF_SIZE - 1;
+
 		tty->echo_overrun = 1;
-	} else
+	} else {
+		new_byte_pos = tty->echo_pos + tty->echo_cnt;
+		new_byte_pos &= N_TTY_BUF_SIZE - 1;
 		tty->echo_cnt++;
+	}
 
-	tty->echo_buf[add_char_pos] = c;
+	tty->echo_buf[new_byte_pos] = c;
 
 	spin_unlock_irqrestore(&tty->echo_lock, flags);
 }
@@ -762,8 +762,9 @@ static void echo_char_raw(unsigned char 
 	if (c == ECHO_OP_START) {
 		add_echo_byte(ECHO_OP_START, tty);
 		add_echo_byte(ECHO_OP_START, tty);
-	} else
+	} else {
 		add_echo_byte(c, tty);
+	}
 }
 
 /**

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Joe Peterson]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

Here is another follow-on patch.  It gets applied after and probably
should be consolidated (for final submission) with the following two:

	tty-fix-loss-of-echoed-characters.patch
	tty-minor-code-efficiency-and-style-cleanup.patch

This one fixes handling of some tab erasure cases and also improves
locking for the echo buffer.

						Thanks, Joe

[-- Attachment #2: tty-fix-echo-tab-erase-and-locking.patch --]
[-- Type: text/plain, Size: 9543 bytes --]

1) Fix tab erasure handling
2) Improve locking when working with the echo buffer

Tab erasure handling is now more robust and able to handle non-zero
canon column cases more correctly.  This is done by making correct use
of what is known in the eraser function (read buffer contents) and what
is known at the time of processing the tab erasure (column state).

Also, better locking of the echo buffer will now prevent any attempts to
process partial multi-byte echo operations.

Signed-off-by: Joe Peterson <joe@skyrush.com>
---

--- linux/drivers/char/n_tty.c.orig	2008-08-25 16:43:40.000000000 -0600
+++ linux/drivers/char/n_tty.c	2008-09-08 08:34:10.000000000 -0600
@@ -71,7 +71,7 @@
 #define ECHO_OP_START 0xff
 #define ECHO_OP_MOVE_BACK_COL 0x80
 #define ECHO_OP_SET_CANON_COL 0x81
-#define ECHO_OP_TAB_ERASE 0x82
+#define ECHO_OP_ERASE_TAB 0x82
 
 static inline unsigned char *alloc_buf(void)
 {
@@ -474,14 +474,11 @@ break_out:
  *	are prioritized.  Also, when control characters are echoed with a
  *	prefixed "^", the pair is treated atomically and thus not separated.
  *
- *	Like the process_output functions, this relies on lock_kernel.
- *	If this lock is ever removed, we should make sure this function
- *	is locked against other code that currently uses echo_lock,
- *	and we should also lock against normal tty output done in
- *	write_chan (so the control pairs do not get separated).  Also,
- *	since we rely on lock_kernel to prevent the tty_write_room from
- *	changing after it is obtained, an alternate means to ensure this
- *	would need to be implemented if the locking mechanism changed.
+ *	This locks against other functions that alter the echo buffer.
+ *	And like the process_output functions, it also relies on lock_kernel.
+ *	If lock_kernel is ever removed, we need to provide an alternate means
+ *	to protect the tty->column state as well as to prevent the splitting
+ *	of control character pairs by other output.
  */
 
 static void process_echoes(struct tty_struct *tty)
@@ -489,10 +486,12 @@ static void process_echoes(struct tty_st
 	int	space, nr;
 	unsigned char c;
 	unsigned char *cp, *buf_end;
+	unsigned long flags;
 
 	if (!tty->echo_cnt)
 		return;
 
+	spin_lock_irqsave(&tty->echo_lock, flags);
 	lock_kernel();
 
 	space = tty_write_room(tty);
@@ -518,44 +517,39 @@ static void process_echoes(struct tty_st
 			op = *opp;
 			
 			switch (op) {
-				unsigned char lbyte, hbyte;
-				unsigned short rdiff;
-				int num_bs;
-				unsigned int col;
+				unsigned int num_chars, num_bs;
 
-			case ECHO_OP_TAB_ERASE:
-				/* Extract rdiff value from next two bytes */
+			case ECHO_OP_ERASE_TAB:
 				if (++opp == buf_end)
 					opp -= N_TTY_BUF_SIZE;
-				lbyte = *opp;
-				if (++opp == buf_end)
-					opp -= N_TTY_BUF_SIZE;
-				hbyte = *opp;
-				rdiff = (hbyte << 8) | lbyte;
-				
-				col = tty->canon_column + rdiff;
-
-				/* should never happen */
-				if (tty->column > 0x80000000)
-					tty->column = 0;
-
-				num_bs = tty->column - col;
-				if (num_bs < 0)
-					num_bs = 0;
+				num_chars = *opp;
+
+				/*
+				 * Determine how many columns to go back
+				 * in order to erase the tab.
+				 * This depends on the number of columns
+				 * used by other characters within the tab
+				 * area.  If this (modulo 8) count is from
+				 * the start of input rather than from a
+				 * previous tab, we offset by canon column.
+				 * Otherwise, tab spacing is normal.
+				 */
+				if (!(num_chars & 0x80))
+					num_chars += tty->canon_column;
+				num_bs = 8 - (num_chars & 7);
+
 				if (num_bs > space) {
 					no_space_left = 1;
 					break;
 				}
-
-				/* Now backup to that column. */
-				while (tty->column > col) {
+				space -= num_bs;
+				while (num_bs--) {
 					tty_put_char(tty, '\b');
 					if (tty->column > 0)
 						tty->column--;
 				}
-				space -= num_bs;
-				cp += 4;
-				nr -= 4;
+				cp += 3;
+				nr -= 3;
 				break;
 
 			case ECHO_OP_SET_CANON_COL:
@@ -647,6 +641,7 @@ static void process_echoes(struct tty_st
 	}
 
 	unlock_kernel();
+	spin_unlock_irqrestore(&tty->echo_lock, flags);
 
 	if (tty->ops->flush_chars)
 		tty->ops->flush_chars(tty);
@@ -663,9 +658,6 @@ static void process_echoes(struct tty_st
 static void add_echo_byte(unsigned char c, struct tty_struct *tty)
 {
 	int	new_byte_pos;
-	unsigned long flags;
-
-	spin_lock_irqsave(&tty->echo_lock, flags);
 
 	if (tty->echo_cnt == N_TTY_BUF_SIZE) {
 		/* Circular buffer is already at capacity */
@@ -679,9 +671,9 @@ static void add_echo_byte(unsigned char 
 		{
 			if (tty->echo_buf[(tty->echo_pos + 1) &
 					  (N_TTY_BUF_SIZE - 1)] ==
-						ECHO_OP_TAB_ERASE) {
-				tty->echo_pos += 4;
-				tty->echo_cnt -= 3;
+						ECHO_OP_ERASE_TAB) {
+				tty->echo_pos += 3;
+				tty->echo_cnt -= 2;
 			} else {
 				tty->echo_pos += 2;
 				tty->echo_cnt -= 1;
@@ -699,8 +691,6 @@ static void add_echo_byte(unsigned char 
 	}
 
 	tty->echo_buf[new_byte_pos] = c;
-
-	spin_unlock_irqrestore(&tty->echo_lock, flags);
 }
 
 /**
@@ -712,8 +702,14 @@ static void add_echo_byte(unsigned char 
 
 static void echo_move_back_col(struct tty_struct *tty)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->echo_lock, flags);
+
 	add_echo_byte(ECHO_OP_START, tty);
 	add_echo_byte(ECHO_OP_MOVE_BACK_COL, tty);
+
+	spin_unlock_irqrestore(&tty->echo_lock, flags);
 }
 
 /**
@@ -726,24 +722,51 @@ static void echo_move_back_col(struct tt
 
 static void echo_set_canon_col(struct tty_struct *tty)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->echo_lock, flags);
+
 	add_echo_byte(ECHO_OP_START, tty);
 	add_echo_byte(ECHO_OP_SET_CANON_COL, tty);
+
+	spin_unlock_irqrestore(&tty->echo_lock, flags);
 }
 
 /**
- *	echo_tab_erase	-	add operation to erase tabs
+ *	echo_erase_tab	-	add operation to erase a tab
+ *	@num_chars: number of character columns already used
+ *	@after_tab: true if num_chars starts after a previous tab
  *	@tty: terminal device
  *
- *	Add an operation to the echo buffer to set the canon column
- *	to the current column.
+ *	Add an operation to the echo buffer to erase a tab.
+ *
+ *	Called by the eraser function, which knows how many character
+ *	columns have been used since either a previous tab or the start
+ *	of input.  This information will be used later, along with
+ *	canon column (if applicable), to go back the correct number
+ *	of columns.
  */
 
-static void echo_tab_erase(unsigned short rdiff, struct tty_struct *tty)
+static void echo_erase_tab(unsigned int num_chars, int after_tab,
+			   struct tty_struct *tty)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->echo_lock, flags);
+
 	add_echo_byte(ECHO_OP_START, tty);
-	add_echo_byte(ECHO_OP_TAB_ERASE, tty);
-	add_echo_byte(rdiff & 0xff, tty);
-	add_echo_byte(rdiff >> 8, tty);
+	add_echo_byte(ECHO_OP_ERASE_TAB, tty);
+
+	/* We only need to know this modulo 8 (tab spacing) */
+	num_chars &= 7;
+
+	/* Set the high bit as a flag if num_chars is after a previous tab */
+	if (after_tab)
+		num_chars |= 0x80;
+	
+	add_echo_byte(num_chars, tty);
+
+	spin_unlock_irqrestore(&tty->echo_lock, flags);
 }
 
 /**
@@ -759,12 +782,18 @@ static void echo_tab_erase(unsigned shor
 
 static void echo_char_raw(unsigned char c, struct tty_struct *tty)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->echo_lock, flags);
+
 	if (c == ECHO_OP_START) {
 		add_echo_byte(ECHO_OP_START, tty);
 		add_echo_byte(ECHO_OP_START, tty);
 	} else {
 		add_echo_byte(c, tty);
 	}
+
+	spin_unlock_irqrestore(&tty->echo_lock, flags);
 }
 
 /**
@@ -781,9 +810,20 @@ static void echo_char_raw(unsigned char 
 
 static void echo_char(unsigned char c, struct tty_struct *tty)
 {
-	if (iscntrl(c) && c != '\t')
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->echo_lock, flags);
+
+	if (c == ECHO_OP_START) {
 		add_echo_byte(ECHO_OP_START, tty);
-	echo_char_raw(c, tty);
+		add_echo_byte(ECHO_OP_START, tty);
+	} else {
+		if (iscntrl(c) && c != '\t')
+			add_echo_byte(ECHO_OP_START, tty);
+		add_echo_byte(c, tty);
+	}
+
+	spin_unlock_irqrestore(&tty->echo_lock, flags);
 }
 
 static inline void finish_erasing(struct tty_struct *tty)
@@ -885,28 +925,32 @@ static void eraser(unsigned char c, stru
 			} else if (kill_type == ERASE && !L_ECHOE(tty)) {
 				echo_char(ERASE_CHAR(tty), tty);
 			} else if (c == '\t') {
-				unsigned short rdiff = 0;
-				unsigned long tail = tty->canon_head;
+				unsigned int num_chars = 0;
+				int after_tab = 0;
+				unsigned long tail = tty->read_head;
 
 				/*
-				 * Find number of columns from canon_head
-				 * to read_head.  This will later be
-				 * added to the canon_column to determine
-				 * how far to erase up to the cur column.
+				 * Count the columns used for characters
+				 * since the start of input or after a
+				 * previous tab.
+				 * This info is used to go back the correct
+				 * number of columns.
 				 */
-				while (tail != tty->read_head) {
+				while (tail != tty->canon_head) {
+					tail = (tail-1) & (N_TTY_BUF_SIZE-1);
 					c = tty->read_buf[tail];
-					if (c == '\t')
-						rdiff = (rdiff | 7) + 1;
+					if (c == '\t') {
+						after_tab = 1;
+						break;
+					}
 					else if (iscntrl(c)) {
 						if (L_ECHOCTL(tty))
-							rdiff += 2;
-					} else if (!is_continuation(c, tty))
-						rdiff++;
-					tail = (tail+1) & (N_TTY_BUF_SIZE-1);
+							num_chars += 2;
+					} else if (!is_continuation(c, tty)) {
+						num_chars++;
+					}
 				}
-
-				echo_tab_erase(rdiff, tty);
+				echo_erase_tab(num_chars, after_tab, tty);
 			} else {
 				if (iscntrl(c) && L_ECHOCTL(tty)) {
 					echo_char_raw('\b', tty);

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Andrew Morton]

On Mon, 08 Sep 2008 10:11:46 -0600
Joe Peterson <joe@skyrush.com> wrote:

> +	spin_lock_irqsave(&tty->echo_lock, flags);
>  	lock_kernel();

Taking a spinlock outside lock_kernel() isn't good, and is quite unusual.

- It might be ab/ba deadlockable (I didn't check) (I trust you always
  test with lockdep enabled?)

- will make Ingo unhappy when he applies his "make lock_kernel use
  mutex_lock" patch (if it's still around).

- will probably give the -rt guys conniptions.


swapping the above two lines would presumably be an easy fix, but one
wonders whether we still need lock_kernel() in there once you've added
this lock.

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Alan Cox]

On Mon, 8 Sep 2008 17:32:50 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 08 Sep 2008 10:11:46 -0600
> Joe Peterson <joe@skyrush.com> wrote:
> 
> > +	spin_lock_irqsave(&tty->echo_lock, flags);
> >  	lock_kernel();
> 
> Taking a spinlock outside lock_kernel() isn't good, and is quite unusual.
> 
> - It might be ab/ba deadlockable (I didn't check) (I trust you always
>   test with lockdep enabled?)

lock_kernel can sleep. Its not allowed...

Alan

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Andrew Morton]

On Tue, 9 Sep 2008 11:55:36 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Mon, 8 Sep 2008 17:32:50 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Mon, 08 Sep 2008 10:11:46 -0600
> > Joe Peterson <joe@skyrush.com> wrote:
> > 
> > > +	spin_lock_irqsave(&tty->echo_lock, flags);
> > >  	lock_kernel();
> > 
> > Taking a spinlock outside lock_kernel() isn't good, and is quite unusual.
> > 
> > - It might be ab/ba deadlockable (I didn't check) (I trust you always
> >   test with lockdep enabled?)
> 
> lock_kernel can sleep.

That used to be the case, but the mutex_lock() version of the bkl got
removed.  It may of course come back.

> Its not allowed...
> 

yup.

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Joe Peterson]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

Andrew Morton wrote:
> On Tue, 9 Sep 2008 11:55:36 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> lock_kernel can sleep.
> 
> That used to be the case, but the mutex_lock() version of the bkl got
> removed.  It may of course come back.
> 
>> Its not allowed...
>>
> 
> yup.

OK, attached is a new version of this patch:

	tty-fix-echo-tab-erase-and-locking.patch

I have removed lock_kernel() from n_tty.c completely (which is a nice
benefit).  As we discussed, this is now easier due to my echo patch
isolating the column state code.  In its place, I created two new mutex
locks: "output_lock" (protects column state and and driver buffer space
left) and "echo_lock" (protects the echo buffer).  I could have used
just one to protect all of this, but there was no need to make the lock
that wide-ranging (e.g., it's OK to add stuff to the echo buffer while
regular output is happening).

All echo buffer operations take the echo lock, and all process_out*
functions take output lock.  The one function that takes both locks is
process_echoes (this is the one that erroneously took both the echo spin
lock and the BKL before).  Note that the process_output functions are at
least sometimes (and maybe all of the time) locked by tty_write_lock (as
Alan mentioned), but this does not protect echo from interfering with
regular output.

Let me know what you think of this patch.  I am using it now, and it
appears to work well so far.

						Thanks again, Joe



[-- Attachment #2: tty-fix-echo-tab-erase-and-locking.patch --]
[-- Type: text/plain, Size: 16757 bytes --]

1) Fix tab erasure handling
2) Improve locking when working with the echo buffer
3) Remove the big kernel lock (BKL) from n_tty

Tab erasure handling is now more robust and able to handle non-zero
canon column cases more correctly.  This is done by making correct use
of what is known in the eraser function (read buffer contents) and what
is known at the time of processing the tab erasure (column state).

Also, better locking of the echo buffer will now prevent any attempts to
process partial multi-byte echo operations.  And since the echo buffer
code now isolates the tty column state code to the process_out* and
process_echoes functions, we can remove the big kernel lock (BKL)
and replace it with more modern mutex locks.

Signed-off-by: Joe Peterson <joe@skyrush.com>
---

diff -Nurp a/drivers/char/n_tty.c b/drivers/char/n_tty.c
--- a/drivers/char/n_tty.c	2008-09-09 14:03:06.492758079 -0600
+++ b/drivers/char/n_tty.c	2008-09-09 14:07:05.582759317 -0600
@@ -71,7 +71,7 @@
 #define ECHO_OP_START 0xff
 #define ECHO_OP_MOVE_BACK_COL 0x80
 #define ECHO_OP_SET_CANON_COL 0x81
-#define ECHO_OP_TAB_ERASE 0x82
+#define ECHO_OP_ERASE_TAB 0x82
 
 static inline unsigned char *alloc_buf(void)
 {
@@ -178,9 +178,11 @@ static void reset_buffer_flags(struct tt
 	spin_lock_irqsave(&tty->read_lock, flags);
 	tty->read_head = tty->read_tail = tty->read_cnt = 0;
 	spin_unlock_irqrestore(&tty->read_lock, flags);
-	spin_lock_irqsave(&tty->echo_lock, flags);
+
+	mutex_lock(&tty->echo_lock);
 	tty->echo_pos = tty->echo_cnt = tty->echo_overrun = 0;
-	spin_unlock_irqrestore(&tty->echo_lock, flags);
+	mutex_unlock(&tty->echo_lock);
+
 	tty->canon_head = tty->canon_data = tty->erasing = 0;
 	memset(&tty->read_flags, 0, sizeof tty->read_flags);
 	n_tty_set_room(tty);
@@ -272,14 +274,11 @@ static inline int is_continuation(unsign
  *	do_output_char			-	output one character
  *	@c: character (or partial unicode symbol)
  *	@tty: terminal device
- *	@space: space available in write buffer
+ *	@space: space available in tty driver write buffer
  *
  *	This is a helper function that handles one output character
  *	(including special characters like TAB, CR, LF, etc.),
  *	putting the results in the tty driver's write buffer.
- *	It is assumed that the calling function does the required
- *	locking and has already determined the space left in the tty
- *	driver's write buffer.
  *
  *	Note that Linux currently ignores TABDLY, CRDLY, VTDLY, FFDLY
  *	and NLDLY.  They simply aren't relevant in the world today.
@@ -287,6 +286,9 @@ static inline int is_continuation(unsign
  *
  *	Returns the number of bytes of buffer space used or -1 if
  *	no space left.
+ *
+ *	Should be called under the output lock to protect column state and
+ *	space left in the tty driver buffer.
  */
 
 static int do_output_char(unsigned char c, struct tty_struct *tty, int space)
@@ -356,23 +358,21 @@ static int do_output_char(unsigned char 
  *	Perform OPOST processing.  Returns -1 when the output device is
  *	full and the character must be retried.
  *
- *	Called from both the receive and transmit sides and can be called
- *	re-entrantly.  Relies on lock_kernel for tty->column state.
- *	Since we rely on lock_kernel to prevent the tty_write_room from
- *	changing after it is obtained, an alternate means to ensure this
- *	would need to be implemented if the locking mechanism changed.
+ *	Called from write_chan under the tty layer write lock.
+ *	Also takes the output lock to protect column state and
+ *	space left in the tty driver buffer.
  */
 
 static int process_output(unsigned char c, struct tty_struct *tty)
 {
 	int	space, retval;
 
-	lock_kernel();
+	mutex_lock(&tty->output_lock);
 
 	space = tty_write_room(tty);
 	retval = do_output_char(c, tty, space);
 
-	unlock_kernel();
+	mutex_unlock(&tty->output_lock);
 	if (retval < 0)
 		return -1;
 	else
@@ -391,10 +391,8 @@ static int process_output(unsigned char 
  *	symbols for the console driver and thus improve performance.
  *
  *	Called from write_chan under the tty layer write lock.
- *	Relies on lock_kernel for the tty->column state.
- *	Since we rely on lock_kernel to prevent the tty_write_room from
- *	changing after it is obtained, an alternate means to ensure this
- *	would need to be implemented if the locking mechanism changed.
+ *	Also takes the output lock to protect column state and
+ *	space left in the tty driver buffer.
  */
 
 static ssize_t process_output_block(struct tty_struct *tty,
@@ -404,12 +402,12 @@ static ssize_t process_output_block(stru
 	int 	i;
 	const unsigned char *cp;
 
-	lock_kernel();
+	mutex_lock(&tty->output_lock);
 
 	space = tty_write_room(tty);
 	if (!space)
 	{
-		unlock_kernel();
+		mutex_unlock(&tty->output_lock);
 		return 0;
 	}
 	if (nr > space)
@@ -448,7 +446,7 @@ static ssize_t process_output_block(stru
 break_out:
 	i = tty->ops->write(tty, buf, i);
 
-	unlock_kernel();
+	mutex_unlock(&tty->output_lock);
 	return i;
 }
 
@@ -474,14 +472,9 @@ break_out:
  *	are prioritized.  Also, when control characters are echoed with a
  *	prefixed "^", the pair is treated atomically and thus not separated.
  *
- *	Like the process_output functions, this relies on lock_kernel.
- *	If this lock is ever removed, we should make sure this function
- *	is locked against other code that currently uses echo_lock,
- *	and we should also lock against normal tty output done in
- *	write_chan (so the control pairs do not get separated).  Also,
- *	since we rely on lock_kernel to prevent the tty_write_room from
- *	changing after it is obtained, an alternate means to ensure this
- *	would need to be implemented if the locking mechanism changed.
+ *	Takes the output lock to protect column state and space left
+ *	in the tty driver buffer. 
+ *	Also takes the echo lock to protect the echo buffer.
  */
 
 static void process_echoes(struct tty_struct *tty)
@@ -493,7 +486,8 @@ static void process_echoes(struct tty_st
 	if (!tty->echo_cnt)
 		return;
 
-	lock_kernel();
+	mutex_lock(&tty->output_lock);
+	mutex_lock(&tty->echo_lock);
 
 	space = tty_write_room(tty);
 
@@ -518,44 +512,39 @@ static void process_echoes(struct tty_st
 			op = *opp;
 			
 			switch (op) {
-				unsigned char lbyte, hbyte;
-				unsigned short rdiff;
-				int num_bs;
-				unsigned int col;
+				unsigned int num_chars, num_bs;
 
-			case ECHO_OP_TAB_ERASE:
-				/* Extract rdiff value from next two bytes */
-				if (++opp == buf_end)
-					opp -= N_TTY_BUF_SIZE;
-				lbyte = *opp;
+			case ECHO_OP_ERASE_TAB:
 				if (++opp == buf_end)
 					opp -= N_TTY_BUF_SIZE;
-				hbyte = *opp;
-				rdiff = (hbyte << 8) | lbyte;
-				
-				col = tty->canon_column + rdiff;
-
-				/* should never happen */
-				if (tty->column > 0x80000000)
-					tty->column = 0;
-
-				num_bs = tty->column - col;
-				if (num_bs < 0)
-					num_bs = 0;
+				num_chars = *opp;
+
+				/*
+				 * Determine how many columns to go back
+				 * in order to erase the tab.
+				 * This depends on the number of columns
+				 * used by other characters within the tab
+				 * area.  If this (modulo 8) count is from
+				 * the start of input rather than from a
+				 * previous tab, we offset by canon column.
+				 * Otherwise, tab spacing is normal.
+				 */
+				if (!(num_chars & 0x80))
+					num_chars += tty->canon_column;
+				num_bs = 8 - (num_chars & 7);
+
 				if (num_bs > space) {
 					no_space_left = 1;
 					break;
 				}
-
-				/* Now backup to that column. */
-				while (tty->column > col) {
+				space -= num_bs;
+				while (num_bs--) {
 					tty_put_char(tty, '\b');
 					if (tty->column > 0)
 						tty->column--;
 				}
-				space -= num_bs;
-				cp += 4;
-				nr -= 4;
+				cp += 3;
+				nr -= 3;
 				break;
 
 			case ECHO_OP_SET_CANON_COL:
@@ -646,7 +635,8 @@ static void process_echoes(struct tty_st
 			tty->echo_overrun = 0;
 	}
 
-	unlock_kernel();
+	mutex_unlock(&tty->echo_lock);
+	mutex_unlock(&tty->output_lock);
 
 	if (tty->ops->flush_chars)
 		tty->ops->flush_chars(tty);
@@ -658,14 +648,13 @@ static void process_echoes(struct tty_st
  *	@tty: terminal device
  *
  *	Add a character or operation byte to the echo buffer.
+ *
+ *	Should be called under the echo lock to protect the echo buffer.
  */
 
 static void add_echo_byte(unsigned char c, struct tty_struct *tty)
 {
 	int	new_byte_pos;
-	unsigned long flags;
-
-	spin_lock_irqsave(&tty->echo_lock, flags);
 
 	if (tty->echo_cnt == N_TTY_BUF_SIZE) {
 		/* Circular buffer is already at capacity */
@@ -679,9 +668,9 @@ static void add_echo_byte(unsigned char 
 		{
 			if (tty->echo_buf[(tty->echo_pos + 1) &
 					  (N_TTY_BUF_SIZE - 1)] ==
-						ECHO_OP_TAB_ERASE) {
-				tty->echo_pos += 4;
-				tty->echo_cnt -= 3;
+						ECHO_OP_ERASE_TAB) {
+				tty->echo_pos += 3;
+				tty->echo_cnt -= 2;
 			} else {
 				tty->echo_pos += 2;
 				tty->echo_cnt -= 1;
@@ -699,8 +688,6 @@ static void add_echo_byte(unsigned char 
 	}
 
 	tty->echo_buf[new_byte_pos] = c;
-
-	spin_unlock_irqrestore(&tty->echo_lock, flags);
 }
 
 /**
@@ -708,12 +695,18 @@ static void add_echo_byte(unsigned char 
  *	@tty: terminal device
  *
  *	Add an operation to the echo buffer to move back one column.
+ *
+ *	Takes the echo lock to protect the echo buffer.
  */
 
 static void echo_move_back_col(struct tty_struct *tty)
 {
+	mutex_lock(&tty->echo_lock);
+
 	add_echo_byte(ECHO_OP_START, tty);
 	add_echo_byte(ECHO_OP_MOVE_BACK_COL, tty);
+
+	mutex_unlock(&tty->echo_lock);
 }
 
 /**
@@ -722,28 +715,55 @@ static void echo_move_back_col(struct tt
  *
  *	Add an operation to the echo buffer to set the canon column
  *	to the current column.
+ *
+ *	Takes the echo lock to protect the echo buffer.
  */
 
 static void echo_set_canon_col(struct tty_struct *tty)
 {
+	mutex_lock(&tty->echo_lock);
+
 	add_echo_byte(ECHO_OP_START, tty);
 	add_echo_byte(ECHO_OP_SET_CANON_COL, tty);
+
+	mutex_unlock(&tty->echo_lock);
 }
 
 /**
- *	echo_tab_erase	-	add operation to erase tabs
+ *	echo_erase_tab	-	add operation to erase a tab
+ *	@num_chars: number of character columns already used
+ *	@after_tab: true if num_chars starts after a previous tab
  *	@tty: terminal device
  *
- *	Add an operation to the echo buffer to set the canon column
- *	to the current column.
+ *	Add an operation to the echo buffer to erase a tab.
+ *
+ *	Called by the eraser function, which knows how many character
+ *	columns have been used since either a previous tab or the start
+ *	of input.  This information will be used later, along with
+ *	canon column (if applicable), to go back the correct number
+ *	of columns.
+ *
+ *	Takes the echo lock to protect the echo buffer.
  */
 
-static void echo_tab_erase(unsigned short rdiff, struct tty_struct *tty)
+static void echo_erase_tab(unsigned int num_chars, int after_tab,
+			   struct tty_struct *tty)
 {
+	mutex_lock(&tty->echo_lock);
+
 	add_echo_byte(ECHO_OP_START, tty);
-	add_echo_byte(ECHO_OP_TAB_ERASE, tty);
-	add_echo_byte(rdiff & 0xff, tty);
-	add_echo_byte(rdiff >> 8, tty);
+	add_echo_byte(ECHO_OP_ERASE_TAB, tty);
+
+	/* We only need to know this modulo 8 (tab spacing) */
+	num_chars &= 7;
+
+	/* Set the high bit as a flag if num_chars is after a previous tab */
+	if (after_tab)
+		num_chars |= 0x80;
+	
+	add_echo_byte(num_chars, tty);
+
+	mutex_unlock(&tty->echo_lock);
 }
 
 /**
@@ -755,16 +775,22 @@ static void echo_tab_erase(unsigned shor
  *	L_ECHO(tty) is true. Called from the driver receive_buf path.
  *
  *	This variant does not treat control characters specially.
+ *
+ *	Takes the echo lock to protect the echo buffer.
  */
 
 static void echo_char_raw(unsigned char c, struct tty_struct *tty)
 {
+	mutex_lock(&tty->echo_lock);
+
 	if (c == ECHO_OP_START) {
 		add_echo_byte(ECHO_OP_START, tty);
 		add_echo_byte(ECHO_OP_START, tty);
 	} else {
 		add_echo_byte(c, tty);
 	}
+
+	mutex_unlock(&tty->echo_lock);
 }
 
 /**
@@ -777,13 +803,24 @@ static void echo_char_raw(unsigned char 
  *
  *	This variant tags control characters to be possibly echoed as
  *	as "^X" (where X is the letter representing the control char).
+ *
+ *	Takes the echo lock to protect the echo buffer.
  */
 
 static void echo_char(unsigned char c, struct tty_struct *tty)
 {
-	if (iscntrl(c) && c != '\t')
+	mutex_lock(&tty->echo_lock);
+
+	if (c == ECHO_OP_START) {
+		add_echo_byte(ECHO_OP_START, tty);
 		add_echo_byte(ECHO_OP_START, tty);
-	echo_char_raw(c, tty);
+	} else {
+		if (iscntrl(c) && c != '\t')
+			add_echo_byte(ECHO_OP_START, tty);
+		add_echo_byte(c, tty);
+	}
+
+	mutex_unlock(&tty->echo_lock);
 }
 
 static inline void finish_erasing(struct tty_struct *tty)
@@ -885,28 +922,32 @@ static void eraser(unsigned char c, stru
 			} else if (kill_type == ERASE && !L_ECHOE(tty)) {
 				echo_char(ERASE_CHAR(tty), tty);
 			} else if (c == '\t') {
-				unsigned short rdiff = 0;
-				unsigned long tail = tty->canon_head;
+				unsigned int num_chars = 0;
+				int after_tab = 0;
+				unsigned long tail = tty->read_head;
 
 				/*
-				 * Find number of columns from canon_head
-				 * to read_head.  This will later be
-				 * added to the canon_column to determine
-				 * how far to erase up to the cur column.
+				 * Count the columns used for characters
+				 * since the start of input or after a
+				 * previous tab.
+				 * This info is used to go back the correct
+				 * number of columns.
 				 */
-				while (tail != tty->read_head) {
+				while (tail != tty->canon_head) {
+					tail = (tail-1) & (N_TTY_BUF_SIZE-1);
 					c = tty->read_buf[tail];
-					if (c == '\t')
-						rdiff = (rdiff | 7) + 1;
+					if (c == '\t') {
+						after_tab = 1;
+						break;
+					}
 					else if (iscntrl(c)) {
 						if (L_ECHOCTL(tty))
-							rdiff += 2;
-					} else if (!is_continuation(c, tty))
-						rdiff++;
-					tail = (tail+1) & (N_TTY_BUF_SIZE-1);
+							num_chars += 2;
+					} else if (!is_continuation(c, tty)) {
+						num_chars++;
+					}
 				}
-
-				echo_tab_erase(rdiff, tty);
+				echo_erase_tab(num_chars, after_tab, tty);
 			} else {
 				if (iscntrl(c) && L_ECHOCTL(tty)) {
 					echo_char_raw('\b', tty);
@@ -1836,10 +1877,13 @@ do_it_again:
  *	@buf: userspace buffer pointer
  *	@nr: size of I/O
  *
- *	Write function of the terminal device. This is serialized with
+ *	Write function of the terminal device.  This is serialized with
  *	respect to other write callers but not to termios changes, reads
- *	and other such events. We must be careful with N_TTY as the receive
- *	code will echo characters, thus calling driver write methods.
+ *	and other such events.  Since the receive code will echo characters,
+ *	thus calling driver write methods, the output processing functions
+ *	called here, as well as the echo processing function, also take the
+ *	output lock to protect the column state and space left in the
+ *	tty driver buffer.
  *
  *	This code must be sure never to sleep through a hangup.
  */
@@ -1977,4 +2021,3 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.receive_buf     = n_tty_receive_buf,
 	.write_wakeup    = n_tty_write_wakeup
 };
-
diff -Nurp a/drivers/char/tty_io.c b/drivers/char/tty_io.c
--- a/drivers/char/tty_io.c	2008-09-09 14:02:47.002757274 -0600
+++ b/drivers/char/tty_io.c	2008-09-09 14:07:05.592758393 -0600
@@ -1470,9 +1470,7 @@ out:
  *		Locks the line discipline as required
  *		Writes to the tty driver are serialized by the atomic_write_lock
  *	and are then processed in chunks to the device. The line discipline
- *	write method will not be involked in parallel for each device
- *		The line discipline write method is called under the big
- *	kernel lock for historical reasons. New code should not rely on this.
+ *	write method will not be involked in parallel for each device.
  */
 
 static ssize_t tty_write(struct file *file, const char __user *buf,
@@ -3343,9 +3341,10 @@ static void initialize_tty_struct(struct
 	INIT_WORK(&tty->hangup_work, do_tty_hangup);
 	mutex_init(&tty->atomic_read_lock);
 	mutex_init(&tty->atomic_write_lock);
+	mutex_init(&tty->output_lock);
+	mutex_init(&tty->echo_lock);
 	spin_lock_init(&tty->read_lock);
 	spin_lock_init(&tty->ctrl_lock);
-	spin_lock_init(&tty->echo_lock);
 	INIT_LIST_HEAD(&tty->tty_files);
 	INIT_WORK(&tty->SAK_work, do_SAK_work);
 }
diff -Nurp a/include/linux/tty.h b/include/linux/tty.h
--- a/include/linux/tty.h	2008-09-09 14:02:47.082756223 -0600
+++ b/include/linux/tty.h	2008-09-09 14:07:05.612756690 -0600
@@ -267,10 +267,11 @@ struct tty_struct {
 	unsigned int canon_column;
 	struct mutex atomic_read_lock;
 	struct mutex atomic_write_lock;
+	struct mutex output_lock;
+	struct mutex echo_lock;
 	unsigned char *write_buf;
 	int write_cnt;
 	spinlock_t read_lock;
-	spinlock_t echo_lock;
 	/* If the tty has a pending do_SAK, queue it here - akpm */
 	struct work_struct SAK_work;
 	struct tty_port *port;

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Andrew Morton]

On Tue, 09 Sep 2008 14:42:12 -0600
Joe Peterson <joe@skyrush.com> wrote:

> 1) Fix tab erasure handling
> 2) Improve locking when working with the echo buffer
> 3) Remove the big kernel lock (BKL) from n_tty
> 
> Tab erasure handling is now more robust and able to handle non-zero
> canon column cases more correctly.  This is done by making correct use
> of what is known in the eraser function (read buffer contents) and what
> is known at the time of processing the tab erasure (column state).
> 
> Also, better locking of the echo buffer will now prevent any attempts to
> process partial multi-byte echo operations.  And since the echo buffer
> code now isolates the tty column state code to the process_out* and
> process_echoes functions, we can remove the big kernel lock (BKL)
> and replace it with more modern mutex locks.

Boy.  Has this been carefully tested with lockdep enabled?

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Joe Peterson]

Andrew Morton wrote:
> On Tue, 09 Sep 2008 14:42:12 -0600
> Joe Peterson <joe@skyrush.com> wrote:
> 
>> 1) Fix tab erasure handling
>> 2) Improve locking when working with the echo buffer
>> 3) Remove the big kernel lock (BKL) from n_tty
>>
>> Tab erasure handling is now more robust and able to handle non-zero
>> canon column cases more correctly.  This is done by making correct use
>> of what is known in the eraser function (read buffer contents) and what
>> is known at the time of processing the tab erasure (column state).
>>
>> Also, better locking of the echo buffer will now prevent any attempts to
>> process partial multi-byte echo operations.  And since the echo buffer
>> code now isolates the tty column state code to the process_out* and
>> process_echoes functions, we can remove the big kernel lock (BKL)
>> and replace it with more modern mutex locks.
> 
> Boy.  Has this been carefully tested with lockdep enabled?

Yes, I have the following in my kernel config:

CONFIG_KALLSYMS_ALL=y
CONFIG_RT_MUTEX_TESTER=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_PROVE_LOCKING=y
CONFIG_LOCKDEP=y
CONFIG_LOCK_STAT=y
CONFIG_DEBUG_LOCKDEP=y
CONFIG_TRACE_IRQFLAGS=y
CONFIG_STACKTRACE=y

I have run the various tests that I used to find/investigate the echo
loss problem to begin with (i.e. that make output compete with echo).
Also some manual tests that fill the echo buffer during a stopped tty,
test the eraser interaction, etc.  Things look fine so far.

						-Joe

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Joe Peterson]

Andrew Morton wrote:
> On Mon, 08 Sep 2008 10:11:46 -0600
> Joe Peterson <joe@skyrush.com> wrote:
> 
>> +	spin_lock_irqsave(&tty->echo_lock, flags);
>>  	lock_kernel();
> 
> Taking a spinlock outside lock_kernel() isn't good, and is quite unusual.
> - It might be ab/ba deadlockable (I didn't check) (I trust you always
>   test with lockdep enabled?)

Indeed - and, as Alan said, lock_kernel() can sleep (a nuance I had not
realized until looking more into the kernel locking mechanisms just
now).  Although I have seen no issues during testing (and I do have
lockdep in the kernel), you are 100% right.

I had wanted to keep from disturbing the locking situation in n_tty, but
maybe it is time to get rid of the BKL there.

My echo buffer patches actually isolate the tty column state stuff to
the output processing functions now anyway, so the BLK may not really be
necessary at this point.  This inspires me to look into this.

> swapping the above two lines would presumably be an easy fix, but one
> wonders whether we still need lock_kernel() in there once you've added
> this lock.

I don't think this is a good idea either, since I don't want to spinlock
during the output processing, which calls the driver output func.  I
think a mutex is more appropriate anyway (and there are some already
defined and in use for tty write locking, etc.) - let me know if you
think otherwise.  I will play around with this and re-post a patch for
review soon.

						-Thanks, Joe

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Alan Cox]

> I don't think this is a good idea either, since I don't want to spinlock
> during the output processing, which calls the driver output func.  I
> think a mutex is more appropriate anyway (and there are some already
> defined and in use for tty write locking, etc.) - let me know if you
> think otherwise.  I will play around with this and re-post a patch for
> review soon.

The driver output side can sleep, and it has to be able to sleep because
the drivers like USB need to be able to sleep.

If you have the column handling isolated and locked that is a big step
towards exterminating the BKL in the n_tty code. It also illustrates why
locking people always say "lock data not code".

Alan

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Joe Peterson]

Alan Cox wrote:
> If you have the column handling isolated and locked that is a big step
> towards exterminating the BKL in the n_tty code. It also illustrates why
> locking people always say "lock data not code".

Well, it's isolated, but still locked with the BKL, which would be great
to get rid of.  A few questions for you, since you've worked with this
code (and kernel locking stuff) a lot longer than I:

1) Now that column state is confined to the process_out/echo funcs in
n_tty, would using tty_write_lock() (the defined atomic write lock
mutex) be a good replacement for lock_kernel(), even though interruptible?

2) To protect echo buffer operations, I would lean toward using a
separate echo lock mutex so it does not lock against non-echo-buffer
output.  Would nesting this with #1 be advisable?  Should it be
interruptable?

3) tty_write() mentions refers to ldisc use of the BKL.  If we change
this, are there any considerations for the tty_io or driver code?

					Thanks, Joe

Re: [PATCH] TTY: Fix loss of echoed characters (2nd follow-on PATCH attached)

[Alan Cox]

> 1) Now that column state is confined to the process_out/echo funcs in
> n_tty, would using tty_write_lock() (the defined atomic write lock
> mutex) be a good replacement for lock_kernel(), even though interruptible?

That lock orders writes to the ldisc so you would often be called with it
held anyway.

> 2) To protect echo buffer operations, I would lean toward using a
> separate echo lock mutex so it does not lock against non-echo-buffer
> output.  Would nesting this with #1 be advisable?  Should it be
> interruptable?

I would start with a lock for each object you need to get the locking
right for. If you ever take more than one at once it must be in the same
order (a lock heirarchy) to avoid deadlocks.

After that works then worry about whether you have too many locks.

> 3) tty_write() mentions refers to ldisc use of the BKL.  If we change
> this, are there any considerations for the tty_io or driver code?

Shouldn't be. n_tty is the last real user of the BKL in the ldisc layer.
I've been over the driver write methods already and they *should* now be
sorted and safe.

Alan