Messages in this thread Patch in this message | | | Date | Wed, 19 Dec 2007 13:34:11 +0100 | From | Haavard Skinnemoen <> | Subject | Re: [PATCH] atmel_serial: Split the interrupt handler |
| |
On Tue, 18 Dec 2007 16:23:11 +0100 "Remy Bohmer" <linux@bohmer.net> wrote:
> Preempt-RT now absolutely requires my (4th) IRQ_NODELAY patch, because > the spinlock now is always inside the code, and not only in > theexception path, and thus without my NO_DELAY patch we have a panic > during boot.
Hmm...perhaps we can eliminate the locking in the status handler too...? Does anyone see a problem with this patch?
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c index bce5d2a..fac37dc 100644 --- a/drivers/serial/atmel_serial.c +++ b/drivers/serial/atmel_serial.c @@ -152,6 +152,7 @@ struct atmel_uart_port { struct tasklet_struct tasklet; unsigned int irq_pending; unsigned int irq_status; + unsigned int irq_status_prev; struct circ_buf rx_ring; }; @@ -585,12 +586,19 @@ atmel_handle_status(struct uart_port *port, unsigned int pending, { struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port; - spin_lock(&port->lock); - - atmel_port->irq_pending |= pending; + /* + * Try to avoid locking here since it may cause problems on + * -rt. This may cause bits to re-appear in irq_pending due to + * a race with atmel_tasklet_func(), but since the previous + * status is tracked explicitly, this will only mean that the + * tasklet will do a bit more work than strictly necessary. + * + * We must make sure that status is written before pending, + * hence the barrier. + */ atmel_port->irq_status = status; - - spin_unlock(&port->lock); + smp_mb(); + atmel_port->irq_pending |= pending; tasklet_schedule(&atmel_port->tasklet); } @@ -750,33 +758,32 @@ static void atmel_tasklet_func(unsigned long data) { struct uart_port *port = (struct uart_port *)data; struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port; - unsigned long flags; unsigned int status; + unsigned int status_change; unsigned int pending; - spin_lock_irqsave(&port->lock, flags); + pending = xchg(&atmel_port->irq_pending, 0); - if (atmel_port->irq_pending) { - pending = atmel_port->irq_pending; + if (pending) { + /* must read/update pending before reading status */ + smp_mb(); status = atmel_port->irq_status; - atmel_port->irq_pending = 0; - - spin_unlock_irqrestore(&port->lock, flags); + status_change = status ^ atmel_port->irq_status_prev; /* TODO: All reads to CSR will clear these interrupts! */ - if (pending & ATMEL_US_RIIC) + if (status_change & ATMEL_US_RI) port->icount.rng++; - if (pending & ATMEL_US_DSRIC) + if (status_change & ATMEL_US_DSR) port->icount.dsr++; - if (pending & ATMEL_US_DCDIC) + if (status_change & ATMEL_US_DCD) uart_handle_dcd_change(port, !(status & ATMEL_US_DCD)); - if (pending & ATMEL_US_CTSIC) + if (status_change & ATMEL_US_CTS) uart_handle_cts_change(port, !(status & ATMEL_US_CTS)); - if (pending & (ATMEL_US_RIIC | ATMEL_US_DSRIC - | ATMEL_US_DCDIC | ATMEL_US_CTSIC)) + if (status_change & (ATMEL_US_RI | ATMEL_US_DSR + | ATMEL_US_DCD | ATMEL_US_CTS)) wake_up_interruptible(&port->info->delta_msr_wait); - } else { - spin_unlock_irqrestore(&port->lock, flags); + + atmel_port->irq_status_prev = status; } if (atmel_use_dma_rx(port))
| |