Re: [PATCH] cpm_uart: Fix spinlock initialization

["David S. Miller" <davem@davemloft.net>]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Fri, 12 Aug 2005 23:03:05 +0100

> http://zeniv.linux.org.uk/~alan/serial.diff

I like this a lot, and the count return based error handling
is nice as well.  Should a driver sink any bytes that the
TTY flip interface couldn't eat or should it just wait for
the overrun event?

With respect to that, in fact, there seems to be some questions
of consistency regarding tty_insert_flip_char() and the
tty_prepare_*() interfaces.  If tty_insert_flip_char() fails
to stow away the character (due to memory allocation failure),
this just happens transparently.  However, when using the
tty_prepare_*() stuff the driver just doesn't sink the bytes
at that time.

Perhaps there should be some kind of counterpart for
tty_insert_flip_char() (something like tty_prepare_to_insert_*() or
whatever) so that the "out-of-space" handling can be made more
consistent.

Some other things caught my eye while reading this:

--- linux.vanilla-2.6.13-rc6/drivers/char/hvc_console.c	2005-08-10 13:57:08.000000000 +0100
+++ linux-2.6.13-rc6/drivers/char/hvc_console.c	2005-07-18 19:06:42.000000000 +0100
 ...
+		count = tty_buffer_request_room(tty. N_INBUF);

that "." should be a "," obviously.  Also:

--- linux.vanilla-2.6.13-rc6/drivers/char/pcmcia/synclink_cs.c	2005-08-10 13:57:08.000000000 +0100
+++ linux-2.6.13-rc6/drivers/char/pcmcia/synclink_cs.c	2005-07-25 15:49:51.000000000 +0100
 ...
-		printk("%s(%d):rx_ready_async count=%d\n",
-			__FILE__,__LINE__,tty->flip.count);
+		printk("%s(%d):rx_ready",
+			__FILE__,__LINE__);

The name of the function really is "rx_ready_async", no real need
to condense it to "rx_ready" and in fact this might confuse someone
actually reading this debug message.

Next, I have a question about the logic of tty_buffer_find().

I interpret it's intended semantics as "Give me TTY buffer of size
at least 'size'." But it seems to be checking something else
while traversing the buffer list:

+	while((*tbh) != NULL) {
+		struct tty_buffer *t = *tbh;
+		if(t->size <= size) {
 ...
+			return t;

This returns the first tty buffer found in the list which is
"smaller than or equal to" size, so I think this test is reversed
and should instead be:

+		if(t->size >= size) {

Keep up the good work :-)

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/