Hello Haavard (and Michael), First I want to mention that I did not found the time yet to test your latest integration of these atmel_serial patches, and I only know that your set of the end of December works fine. I do not know the changes you made since posting that set and your latest patch-set.
But let me clear some things up: The original patchset I posted, existed of a set of patches suitable for the mainline kernel, plus an additional patch for Preempt-RT only. So, the splitup of the interrupt handler was also needed for Preempt-RT, but it was not the only patch that was needed on preempt-rt. Now looking at this problem: > > * 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 I expect the UART generating the problem is the DBGU port. The DBGU shares its interrupt line with the timer interrupt with the IRQF_TIMER flag set, and thus the DBGU interrupt handler is running in IRQF_NODELAY context. Within this context it is forbidden to lock a normal spinlock, because a normal spinlock is converted to a mutex on Preempt-RT; a mutex can sleep which is forbidden in interrupt context. So, to get around this problem, this lock spinlock has to be of the raw_spinlock_t type. The raw_spinlock_t is the normal mainline-kernel spinlock, and as such it is not converted to a mutex, and will therefor never sleep. Attached a patch that changes this spinlock type. I used it in my patchset, but your updates of December last year do not need this patch anymore, so apparantly you changed something that has a regression on Preempt-RT... > > Complete Preemption (Real-Time) ok but the serials is just unusable due > > to too many overruns (just using lrz) This problem is not there in the December-set either. It works like a charm... I believe I have to look at the latest set of patches, and try to find any regressions. Do you have a location somewhere where I can download the latest versions? Or do I need to dig through LKML to find the latest... ;-) Kind Regards, Remy 2008/1/30, Haavard Skinnemoen <[EMAIL PROTECTED]>: > On Wed, 30 Jan 2008 00:12:23 +0100 > michael <[EMAIL PROTECTED]> 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 >
On PREEMPT-RT we may not block on a normal spinlock in IRQ/IRQF_NODELAY-context. This patch fixes this by making the lock a raw_spinlock_t for the Atmel serial console. This patch demands the following patches to be installed first: * atmel_serial_cleanup.patch * atmel_serial_irq_splitup.patch Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]> --- include/linux/serial_core.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) Index: linux-2.6.23/include/linux/serial_core.h =================================================================== --- linux-2.6.23.orig/include/linux/serial_core.h 2007-12-13 13:31:27.000000000 +0100 +++ linux-2.6.23/include/linux/serial_core.h 2007-12-13 16:32:22.000000000 +0100 @@ -226,7 +226,12 @@ struct uart_icount { typedef unsigned int __bitwise__ upf_t; struct uart_port { - spinlock_t lock; /* port lock */ + +#ifdef CONFIG_SERIAL_ATMEL + raw_spinlock_t lock; /* port lock */ +#else + spinlock_t lock; /* port lock */ +#endif unsigned int iobase; /* in/out[bwl] */ unsigned char __iomem *membase; /* read/write[bwl] */ unsigned int irq; /* irq number */