Re: TTY loosing bytes ?

[Felipe Balbi <felipe.balbi@nokia.com>]

On Tue, Oct 06, 2009 at 02:12:30PM +0200, ext Alan Cox wrote:
> > why doesn't receive_buf() return the amount of bytes actually received ?
> 
> You'd have to ask whoever wrote the code in 199something.

fair enough ;-)

> > I see flush_to_ldisc() believes it can flush everything before even
> > calling receive_buf() then it will never act on the possibility of
> > receive_buf() not being able to receive the entire data.
> 
> The ldisc is responsible for maintaining tty->receive_room correctly at
> all times.
> 
> > Am I right ? Should receive_buf() return the amount of bytes actually
> > received ? Also, why isn't receive_room enough to be sure there's
> > enough space to really receive that block of data ?
> 
> I've not seen this reported elsewhere so I assume you are somehow
> tripping a bug in the n_tty ldisc code. The other possibility is that you
> are in canonical mode and some of your input is intentionally discarded
> by the ldisc either as errors, overruns or through things like quoting or
> flow control.

hmm, not canonical, no. I'm falling on the if (tty->real_raw) in
n_tty_receive_buf() for sure. Have prints there.

The following patch helps a whole lot but sometimes it still gets stuck
and I'm now debugging that:

diff --git a/drivers/char/n_tty.c b/drivers/char/n_tty.c
index 2e50f4d..a00bd8d 100644
--- a/drivers/char/n_tty.c
+++ b/drivers/char/n_tty.c
@@ -1348,7 +1348,7 @@ static void n_tty_write_wakeup(struct tty_struct *tty)
  *	calls one at a time and in order (or using flush_to_ldisc)
  */
 
-static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
+static int n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 			      char *fp, int count)
 {
 	const unsigned char *p;
@@ -1356,9 +1356,10 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 	int	i;
 	char	buf[64];
 	unsigned long cpuflags;
+	int ret = 0;
 
 	if (!tty->read_buf)
-		return;
+		return 0;
 
 	if (tty->real_raw) {
 		spin_lock_irqsave(&tty->read_lock, cpuflags);
@@ -1370,6 +1371,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 		tty->read_cnt += i;
 		cp += i;
 		count -= i;
+		ret += i;
 
 		i = min(N_TTY_BUF_SIZE - tty->read_cnt,
 			N_TTY_BUF_SIZE - tty->read_head);
@@ -1377,8 +1379,11 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 		memcpy(tty->read_buf + tty->read_head, cp, i);
 		tty->read_head = (tty->read_head + i) & (N_TTY_BUF_SIZE-1);
 		tty->read_cnt += i;
+		ret += i;
 		spin_unlock_irqrestore(&tty->read_lock, cpuflags);
+
 	} else {
+		ret = count;
 		for (i = count, p = cp, f = fp; i; i--, p++) {
 			if (f)
 				flags = *f++;
@@ -1421,6 +1426,8 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 	 */
 	if (tty->receive_room < TTY_THRESHOLD_THROTTLE)
 		tty_throttle(tty);
+
+	return ret;
 }
 
 int is_ignored(int sig)
diff --git a/drivers/char/tty_buffer.c b/drivers/char/tty_buffer.c
index 3108991..e53adb7 100644
--- a/drivers/char/tty_buffer.c
+++ b/drivers/char/tty_buffer.c
@@ -58,7 +58,7 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_struct *tty, size_t size)
 {
 	struct tty_buffer *p;
 
-	if (tty->buf.memory_used + size > 65536)
+	if (tty->buf.memory_used + size > 96 * 1024)
 		return NULL;
 	p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
 	if (p == NULL)
@@ -417,6 +417,7 @@ static void flush_to_ldisc(struct work_struct *work)
 	if (head != NULL) {
 		tty->buf.head = NULL;
 		for (;;) {
+			int copied;
 			int count = head->commit - head->read;
 			if (!count) {
 				if (head->next == NULL)
@@ -439,11 +440,11 @@ static void flush_to_ldisc(struct work_struct *work)
 				count = tty->receive_room;
 			char_buf = head->char_buf_ptr + head->read;
 			flag_buf = head->flag_buf_ptr + head->read;
-			head->read += count;
 			spin_unlock_irqrestore(&tty->buf.lock, flags);
-			disc->ops->receive_buf(tty, char_buf,
+			copied = disc->ops->receive_buf(tty, char_buf,
 							flag_buf, count);
 			spin_lock_irqsave(&tty->buf.lock, flags);
+			head->read += copied;
 		}
 		/* Restore the queue head */
 		tty->buf.head = head;
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 0c4ee9b..e1c940f 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -133,7 +133,7 @@ struct tty_ldisc_ops {
 	/*
 	 * The following routines are called from below.
 	 */
-	void	(*receive_buf)(struct tty_struct *, const unsigned char *cp,
+	int	(*receive_buf)(struct tty_struct *, const unsigned char *cp,
 			       char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
 

to me it seems that receive_room is being mis-set as I can see from some
debugging messages I added:

[  517.793792] first: read_cnt 3586 read_head 2904, second: read_cnt 4096 read_head 3414, room 1021
[  517.800994] Fuck, lost bytes (510, 512)!
[  524.998687] first: read_cnt 3591 read_head 3408, second: read_cnt 4096 read_head 3913, room 1016
[  525.005889] Fuck, lost bytes (505, 512)!

and it goes on and on.

With the patch above I still get this messages but it still goes through
since not receive_buf is returning the amount of bytes actually
received. Then flush_to_ldisc() will retry those bytes on the next
iteration. Maybe this is not the desired patch though ?

Thanks a lot for the comments Alan.

-- 
balbi