Messages in this thread | | | Date | Wed, 30 Jan 2008 10:41:13 +0100 | From | Haavard Skinnemoen <> | Subject | Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler |
| |
On Wed, 30 Jan 2008 00:12:23 +0100 michael <trimarchi@gandalf.sssup.it> wrote:
> I'm testing this patch on an at91sam9260 on 2.6.24-rt. I'm using this > patch with the tclib support for hrtimer and the clocksource pit_clk. > These are the results: > > - Voluntary Kernel Preemption the system (crash) > - Preemptible Kernel (crash)
Ouch. I'm assuming this is with DMA disabled?
> /* > * Drop the lock here since it might end up calling > * uart_start(), which takes the lock. > spin_unlock(&port->lock); > */ > tty_flip_buffer_push(port->info->tty); > /* > spin_lock(&port->lock); > */ > The same code with this comments out runs
Now, _that_ is strange. I can't see anything that needs protection across that call; in fact, I think we can lock a lot less than what we currently do.
> Complete Preemption (Real-Time) ok but the serials is just unusable due > to too many overruns (just using lrz)
Is it worse than before? IIRC Remy mentioned something about IRQF_NODELAY being the reason for moving all this code to softirq context in the first place; does the interrupt handler run in hardirq context?
> The system is stable and doesn't crash reverting this patch. > I don't test with the thread hardirqs active.
Ok.
> >> + ret = -ENOMEM; > >> + data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL); > >> + if (!data) > >> + goto err_alloc_ring; > >> + port->rx_ring.buf = data; > >> + > >> ret = uart_add_one_port(&atmel_uart, &port->uart); > >> > Is the kmalloc correct? > maybe: > data = kmalloc(ATMEL_SERIAL_RINGSIZE * sizeof(struct atmel_uart_char), > GFP_KERNEL);
I think you're right. Can you change it and see if it helps?
I guess I didn't test it thoroughly enough with DMA disabled...slub_debug ought to catch such things, but not until we receive enough data to actually overflow the buffer.
> >> @@ -1033,6 +1165,9 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev) > >> > >> ret = uart_remove_one_port(&atmel_uart, port); > >> > >> + tasklet_kill(&atmel_port->tasklet); > >> + kfree(atmel_port->rx_ring.buf); > >> + > >> > Why the tasklet_kill is not done in atmel_shutdown?
Why should it be? If it should, we must move the call to tasklet_init into atmel_startup too, and I don't really see the point.
Haavard
| |