Subject: [PATCH] Fix character loss in drivers/tc/zs.c

From: Thiemo Seufer <[email-masked]>
Date: Sun, 21 Sep 2003 23:31:04 +0200

Hello Maciej,

this patch reduces the zs driver's character lossage.

Thiemo

diff -abdpruNPX /bigdisk/src/dontdiff linux-orig/drivers/tc/zs.c linux/drivers/tc/zs.c
--- linux-orig/drivers/tc/zs.c	Tue Aug 12 04:11:58 2003
+++ linux/drivers/tc/zs.c	Sun Sep 21 22:28:40 2003
@@ -456,7 +456,7 @@ static _INLINE_ void receive_chars(struc
 
 		if (info->hook && info->hook->rx_char) {
 			(*info->hook->rx_char)(ch, flag);
-			return;
+			break;
   		}
 
 		if (tty->flip.count >= TTY_FLIPBUF_SIZE) {

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 22 Sep 2003 19:38:04 +0200 (MET DST)

> this patch reduces the zs driver's character lossage.

 Can you please elaborate?  tty is expected to be NULL if info->hook is
not, so the code after the change should not differ effectively -- only a
useless check is added at the end.  Am I missing anything?

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email-masked], PGP key available        +

---

From: Thiemo Seufer <[email-masked]>
Date: Tue, 23 Sep 2003 10:40:05 +0200

> useless check is added at the end.  Am I missing anything?

AFAICS the

        while ((read_zsreg(info->zs_channel, R0) & Rx_CH_AV) != 0) {

loops over the FIFO contents and 'return' discards the remaining
part. The patch made a visible difference for me with some noisy
debug printk()'s in the kernel.

Thiemo

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Tue, 23 Sep 2003 13:31:43 +0200 (MET DST)

> debug printk()'s in the kernel.

 Hmm, a coincidence?  Without your patch the execution goes as follows:

receive_chars()
{
	...
	return;
}

And after the change it is as follows:

receive_chars()
{
	...
	(*info->hook->rx_char)(ch, flag);
	if (tty)
		tty_flip_buffer_push(tty);
	return;
}

and tty is NULL.  And I fail to see how it can it make any difference for
printk() output -- the code in question is only ever executed for input
from an LK201-type keyboard.

  Maciej

---

From: Thiemo Seufer <[email-masked]>
Date: Tue, 23 Sep 2003 14:14:10 +0200

>  Hmm, a coincidence?  Without your patch the execution goes as follows:

Well, the code suggests so. But thinking about it again, it should
probably read (untested):

--- linux-orig/drivers/tc/zs.c	Tue Aug 12 04:11:58 2003
+++ linux/drivers/tc/zs.c	Tue Sep 23 14:09:34 2003
@@ -456,7 +456,7 @@ static _INLINE_ void receive_chars(struc
 
+			continue;
   		}

[snip]
> from an LK201-type keyboard.

I found this on Karsten's 5000/150. We can try it out again soon. :-)

Thiemo

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Tue, 23 Sep 2003 14:36:25 +0200 (MET DST)

>  		if (tty->flip.count >= TTY_FLIPBUF_SIZE) {

 That might make some sense performance-wise, but it still doesn't make a
big difference -- continuing with the loop will let following pending
codes from the keyboard to be fetched immediately, while returning defers
that to the next SCC interrupt.

 Actually, I'm not that keen on making revolutionary changes to
drivers/tc/zs, just fixing bugs (of course, this might be one).  The
driver should be rewritten to become a front-end to
drivers/net/wan/z85230.c (the path seems unfortunate).  Then more
interesting stuff, like DMA and synchronous operation, will become
available. 

  Maciej