[PATCH v2 4/9] n_tty: Remove echo_lock

[Peter Hurley <peter@hurleysoftware.com>]

Adding data to echo_buf (via add_echo_byte()) is guaranteed to be
single-threaded, since all callers are from the n_tty_receive_buf()
path. Processing the echo_buf can be called from either the
n_tty_receive_buf() path or the n_tty_write() path; however, these
callers are already serialized by output_lock.

Publish cumulative echo_head changes to echo_commit; process echo_buf
from echo_tail to echo_commit; remove echo_lock.

On echo_buf overrun, claim output_lock to serialize changes to
echo_tail.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 75 ++++++++++++++++++++---------------------------------
 1 file changed, 28 insertions(+), 47 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 68dc9fd..d22c14c 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -106,10 +106,10 @@ struct n_tty_data {
 	/* consumer-published */
 	size_t read_tail;
 
-	/* protected by echo_lock */
 	unsigned char *echo_buf;
 	size_t echo_head;
 	size_t echo_tail;
+	size_t echo_commit;
 
 	/* protected by output lock */
 	unsigned int column;
@@ -117,7 +117,6 @@ struct n_tty_data {
 
 	struct mutex atomic_read_lock;
 	struct mutex output_lock;
-	struct mutex echo_lock;
 };
 
 static inline size_t read_cnt(struct n_tty_data *ldata)
@@ -332,10 +331,7 @@ static void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
 static void reset_buffer_flags(struct n_tty_data *ldata)
 {
 	ldata->read_head = ldata->canon_head = ldata->read_tail = 0;
-
-	mutex_lock(&ldata->echo_lock);
-	ldata->echo_head = ldata->echo_tail  = 0;
-	mutex_unlock(&ldata->echo_lock);
+	ldata->echo_head = ldata->echo_tail = ldata->echo_commit = 0;
 
 	ldata->erasing = 0;
 	bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
@@ -641,8 +637,7 @@ break_out:
  *	are prioritized.  Also, when control characters are echoed with a
  *	prefixed "^", the pair is treated atomically and thus not separated.
  *
- *	Locking: output_lock to protect column state and space left,
- *		 echo_lock to protect the echo buffer
+ *	Locking: output_lock to protect column state and space left
  */
 
 static void process_echoes(struct tty_struct *tty)
@@ -652,16 +647,15 @@ static void process_echoes(struct tty_struct *tty)
 	size_t tail;
 	unsigned char c;
 
-	if (ldata->echo_head == ldata->echo_tail)
+	if (ldata->echo_commit == ldata->echo_tail)
 		return;
 
 	mutex_lock(&ldata->output_lock);
-	mutex_lock(&ldata->echo_lock);
 
 	space = tty_write_room(tty);
 
 	tail = ldata->echo_tail;
-	nr = ldata->echo_head - ldata->echo_tail;
+	nr = ldata->echo_commit - ldata->echo_tail;
 	while (nr > 0) {
 		c = echo_buf(ldata, tail);
 		if (c == ECHO_OP_START) {
@@ -778,13 +772,21 @@ static void process_echoes(struct tty_struct *tty)
 
 	ldata->echo_tail = tail;
 
-	mutex_unlock(&ldata->echo_lock);
 	mutex_unlock(&ldata->output_lock);
 
 	if (tty->ops->flush_chars)
 		tty->ops->flush_chars(tty);
 }
 
+static void commit_echoes(struct tty_struct *tty)
+{
+	struct n_tty_data *ldata = tty->disc_data;
+
+	smp_mb();
+	ldata->echo_commit = ldata->echo_head;
+	process_echoes(tty);
+}
+
 /**
  *	add_echo_byte	-	add a byte to the echo buffer
  *	@c: unicode byte to echo
@@ -792,13 +794,16 @@ static void process_echoes(struct tty_struct *tty)
  *
  *	Add a character or operation byte to the echo buffer.
  *
- *	Should be called under the echo lock to protect the echo buffer.
+ *	Locks: may claim output_lock to prevent concurrent modify of
+ *	       echo_tail by process_echoes().
  */
 
 static void add_echo_byte(unsigned char c, struct n_tty_data *ldata)
 {
 	if (ldata->echo_head - ldata->echo_tail == N_TTY_BUF_SIZE) {
 		size_t head = ldata->echo_head;
+
+		mutex_lock(&ldata->output_lock);
 		/*
 		 * Since the buffer start position needs to be advanced,
 		 * be sure to step by a whole operation byte group.
@@ -810,6 +815,7 @@ static void add_echo_byte(unsigned char c, struct n_tty_data *ldata)
 				ldata->echo_tail += 2;
 		} else
 			ldata->echo_tail++;
+		mutex_unlock(&ldata->output_lock);
 	}
 
 	*echo_buf_addr(ldata, ldata->echo_head++) = c;
@@ -820,16 +826,12 @@ static void add_echo_byte(unsigned char c, struct n_tty_data *ldata)
  *	@ldata: n_tty data
  *
  *	Add an operation to the echo buffer to move back one column.
- *
- *	Locking: echo_lock to protect the echo buffer
  */
 
 static void echo_move_back_col(struct n_tty_data *ldata)
 {
-	mutex_lock(&ldata->echo_lock);
 	add_echo_byte(ECHO_OP_START, ldata);
 	add_echo_byte(ECHO_OP_MOVE_BACK_COL, ldata);
-	mutex_unlock(&ldata->echo_lock);
 }
 
 /**
@@ -838,16 +840,12 @@ static void echo_move_back_col(struct n_tty_data *ldata)
  *
  *	Add an operation to the echo buffer to set the canon column
  *	to the current column.
- *
- *	Locking: echo_lock to protect the echo buffer
  */
 
 static void echo_set_canon_col(struct n_tty_data *ldata)
 {
-	mutex_lock(&ldata->echo_lock);
 	add_echo_byte(ECHO_OP_START, ldata);
 	add_echo_byte(ECHO_OP_SET_CANON_COL, ldata);
-	mutex_unlock(&ldata->echo_lock);
 }
 
 /**
@@ -863,15 +861,11 @@ static void echo_set_canon_col(struct n_tty_data *ldata)
  *	of input.  This information will be used later, along with
  *	canon column (if applicable), to go back the correct number
  *	of columns.
- *
- *	Locking: echo_lock to protect the echo buffer
  */
 
 static void echo_erase_tab(unsigned int num_chars, int after_tab,
 			   struct n_tty_data *ldata)
 {
-	mutex_lock(&ldata->echo_lock);
-
 	add_echo_byte(ECHO_OP_START, ldata);
 	add_echo_byte(ECHO_OP_ERASE_TAB, ldata);
 
@@ -883,8 +877,6 @@ static void echo_erase_tab(unsigned int num_chars, int after_tab,
 		num_chars |= 0x80;
 
 	add_echo_byte(num_chars, ldata);
-
-	mutex_unlock(&ldata->echo_lock);
 }
 
 /**
@@ -896,20 +888,16 @@ static void echo_erase_tab(unsigned int num_chars, int after_tab,
  *	L_ECHO(tty) is true. Called from the driver receive_buf path.
  *
  *	This variant does not treat control characters specially.
- *
- *	Locking: echo_lock to protect the echo buffer
  */
 
 static void echo_char_raw(unsigned char c, struct n_tty_data *ldata)
 {
-	mutex_lock(&ldata->echo_lock);
 	if (c == ECHO_OP_START) {
 		add_echo_byte(ECHO_OP_START, ldata);
 		add_echo_byte(ECHO_OP_START, ldata);
 	} else {
 		add_echo_byte(c, ldata);
 	}
-	mutex_unlock(&ldata->echo_lock);
 }
 
 /**
@@ -922,16 +910,12 @@ static void echo_char_raw(unsigned char c, struct n_tty_data *ldata)
  *
  *	This variant tags control characters to be echoed as "^X"
  *	(where X is the letter representing the control char).
- *
- *	Locking: echo_lock to protect the echo buffer
  */
 
 static void echo_char(unsigned char c, struct tty_struct *tty)
 {
 	struct n_tty_data *ldata = tty->disc_data;
 
-	mutex_lock(&ldata->echo_lock);
-
 	if (c == ECHO_OP_START) {
 		add_echo_byte(ECHO_OP_START, ldata);
 		add_echo_byte(ECHO_OP_START, ldata);
@@ -940,8 +924,6 @@ static void echo_char(unsigned char c, struct tty_struct *tty)
 			add_echo_byte(ECHO_OP_START, ldata);
 		add_echo_byte(c, ldata);
 	}
-
-	mutex_unlock(&ldata->echo_lock);
 }
 
 /**
@@ -1283,7 +1265,7 @@ static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
 			if (ldata->canon_head == ldata->read_head)
 				echo_set_canon_col(ldata);
 			echo_char(c, tty);
-			process_echoes(tty);
+			commit_echoes(tty);
 		}
 		if (parmrk)
 			put_tty_queue(c, ldata);
@@ -1294,7 +1276,7 @@ static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
 	if (I_IXON(tty)) {
 		if (c == START_CHAR(tty)) {
 			start_tty(tty);
-			process_echoes(tty);
+			commit_echoes(tty);
 			return;
 		}
 		if (c == STOP_CHAR(tty)) {
@@ -1325,7 +1307,7 @@ send_signal:
 				start_tty(tty);
 			if (L_ECHO(tty)) {
 				echo_char(c, tty);
-				process_echoes(tty);
+				commit_echoes(tty);
 			}
 			isig(signal, tty);
 			return;
@@ -1344,7 +1326,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);
+			commit_echoes(tty);
 			return;
 		}
 		if (c == LNEXT_CHAR(tty) && L_IEXTEN(tty)) {
@@ -1354,7 +1336,7 @@ send_signal:
 				if (L_ECHOCTL(tty)) {
 					echo_char_raw('^', ldata);
 					echo_char_raw('\b', ldata);
-					process_echoes(tty);
+					commit_echoes(tty);
 				}
 			}
 			return;
@@ -1370,7 +1352,7 @@ send_signal:
 				echo_char(read_buf(ldata, tail), tty);
 				tail++;
 			}
-			process_echoes(tty);
+			commit_echoes(tty);
 			return;
 		}
 		if (c == '\n') {
@@ -1381,7 +1363,7 @@ send_signal:
 			}
 			if (L_ECHO(tty) || L_ECHONL(tty)) {
 				echo_char_raw('\n', ldata);
-				process_echoes(tty);
+				commit_echoes(tty);
 			}
 			goto handle_newline;
 		}
@@ -1410,7 +1392,7 @@ send_signal:
 				if (ldata->canon_head == ldata->read_head)
 					echo_set_canon_col(ldata);
 				echo_char(c, tty);
-				process_echoes(tty);
+				commit_echoes(tty);
 			}
 			/*
 			 * XXX does PARMRK doubling happen for
@@ -1447,7 +1429,7 @@ handle_newline:
 				echo_set_canon_col(ldata);
 			echo_char(c, tty);
 		}
-		process_echoes(tty);
+		commit_echoes(tty);
 	}
 
 	if (parmrk)
@@ -1712,7 +1694,6 @@ static int n_tty_open(struct tty_struct *tty)
 	ldata->overrun_time = jiffies;
 	mutex_init(&ldata->atomic_read_lock);
 	mutex_init(&ldata->output_lock);
-	mutex_init(&ldata->echo_lock);
 
 	/* These are ugly. Currently a malloc failure here can panic */
 	ldata->read_buf = kzalloc(N_TTY_BUF_SIZE, GFP_KERNEL);
-- 
1.8.1.2