[PATCH] cpm_uart: Fix spinlock initialization

[PATCH] cpm_uart: Fix spinlock initialization

[Kumar Gala]

The lack of spin_lock_init causes badness in the preempt configuration.

Signed-off-by: Vitaly Bordug <vbordug@ru.mvista.com>
Signed-off-by: Kumar Gala <kumar.gala@freescale.com>

---
commit d6dee08c314c1952921adc99e8f5ff6c332341ef
tree 96ca76ab7d75cf808287af50c88ed7d1aed49923
parent 51a75dcea8fe0407f06467de36e258616cda68f9
author Kumar K. Gala <kumar.gala@freescale.com> Fri, 12 Aug 2005 11:26:44 -0500
committer Kumar K. Gala <kumar.gala@freescale.com> Fri, 12 Aug 2005 11:26:44 -0500

 drivers/serial/cpm_uart/cpm_uart_core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -1087,6 +1087,7 @@ static int __init cpm_uart_console_setup
 	port =
 	    (struct uart_port *)&cpm_uart_ports[cpm_uart_port_map[co->index]];
 	pinfo = (struct uart_cpm_port *)port;
+	spin_lock_init(&port->lock);
 
 	pinfo->flags |= FLAG_CONSOLE;
 

Re: [PATCH] cpm_uart: Fix spinlock initialization

[Russell King]

On Fri, Aug 12, 2005 at 11:32:57AM -0500, Kumar Gala wrote:
> The lack of spin_lock_init causes badness in the preempt configuration.

Please don't - I have a patch in the wings which fixes this properly.
Unfortunately, its behind a ton of other patches, and I don't want
to try to find all these fixes in each driver.

This is wrong because we will re-initialise it from an indeterminant
state (it might be locked) later during the initialisation.  Let's
fix it properly.

(Since Linus pulled the shutters down when I still had a large chunk
of stuff in my serial tree, my serial patch merging is currently at
a complete standstill.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] cpm_uart: Fix spinlock initialization

[Kumar Gala]

On Aug 12, 2005, at 2:46 PM, Russell King wrote:


> On Fri, Aug 12, 2005 at 11:32:57AM -0500, Kumar Gala wrote:
>
>
>> The lack of spin_lock_init causes badness in the preempt
>>
>>
> configuration.
>
> Please don't - I have a patch in the wings which fixes this properly.
> Unfortunately, its behind a ton of other patches, and I don't want
> to try to find all these fixes in each driver.
>
> This is wrong because we will re-initialise it from an indeterminant
> state (it might be locked) later during the initialisation.  Let's
> fix it properly.
>
> (Since Linus pulled the shutters down when I still had a large chunk
> of stuff in my serial tree, my serial patch merging is currently at
> a complete standstill.)
>

Understood.  I'm fine with holding off for your proper fix.

Andrew, let's drop this patch.

- kumar

Re: [PATCH] cpm_uart: Fix spinlock initialization

[David S. Miller]

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 :-)

Re: [PATCH] cpm_uart: Fix spinlock initialization

[Alan Cox]

On Gwe, 2005-08-12 at 20:46 +0100, Russell King wrote:
> (Since Linus pulled the shutters down when I still had a large chunk
> of stuff in my serial tree, my serial patch merging is currently at
> a complete standstill.)

Well in the mean time for some more amusement see what you think of

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

which is the current kmalloc based serial buffering patch.

Alan

Re: [PATCH] cpm_uart: Fix spinlock initialization

[Alan Cox]

On Gwe, 2005-08-12 at 14:58 -0700, David S. Miller wrote:
> 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?

Its really driver dependant. A lot of the hardware doesn't seem to have
much choice. My assumption would be that it is better to stop reading
from the uart if possible and hope you avoid the later overrun on the
chip. Then agai with kmalloc backing the memory allocation it shouldn't
ever happen except perhaps to people with high speed DMA interfaces.

> 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

It returns 1 or 0. You can also use tty_buffer_request_room(tty, 1) if
you really want to know if there is a byte free. Its probably cheaper to
just remember the overflow in a static in the irq handler and push it
first next IRQ however.


> 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.

Not 100% sure I follow this ?

> 
> 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.

Thanks

> "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 :-)

Doh I'd been looking for that bug

Re: [PATCH] cpm_uart: Fix spinlock initialization

[Russell King]

On Fri, Aug 12, 2005 at 08:46:17PM +0100, Russell King wrote:
> On Fri, Aug 12, 2005 at 11:32:57AM -0500, Kumar Gala wrote:
> > The lack of spin_lock_init causes badness in the preempt configuration.
> 
> Please don't - I have a patch in the wings which fixes this properly.
> Unfortunately, its behind a ton of other patches, and I don't want
> to try to find all these fixes in each driver.

Actually, sorry, it's already merged -

http://www.kernel.org/git/gitweb.cgi?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=976ecd12b8144d066a23fe97c6fbfc1ac8470af7

I don't personally run preempt so I don't know what badness you're
seeing.  Can you check whether your kernel has this patch applied,
and if so provide a bug report so the problem can be analysed please?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core