From: Remy Bohmer <[email protected]>
This patch splits up the interrupt handler of the serial port
into a interrupt top-half and a tasklet.
The goal is to get the interrupt top-half as short as possible to
minimize latencies on interrupts. But the old code also does some
calls in the interrupt handler that are not allowed on preempt-RT
in IRQF_NODELAY context. This handler is executed in this context
because of the interrupt sharing with the timer interrupt.
The timer interrupt on Preempt-RT runs in IRQF_NODELAY context.
The tasklet takes care of handling control status changes, pushing
incoming characters to the tty layer, handling break and other errors.
The Transmit path was IRQF_NODELAY safe by itself, and is not adapted.
The read path for DMA(PDC) is also not adapted, because that code
does not run in IRQF_NODELAY context due to irq-sharing. The DBGU
which is shared with the timer-irq does not work with DMA, so
therefor this is no problem.
Reading the complete receive queue is still done in the top-half
because we never want to miss any incoming character.
This patch demands the following patches to be installed first:
* atmel_serial_cleanup.patch
[[email protected]: misc cleanups and simplifications]
Signed-off-by: Remy Bohmer <[email protected]>
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
Changes since v1:
Whitespace cleanups
Allocate rx ring data separately
Move more of the rx processing into tasklet
Consolidate rx and status tasklets into one
Use circ_buf instead of atmel_uart_ring
Eliminate RX ring locking
In this version of the patch, we try to only do things that are
absolutely necessary in the interrupt handler, storing away the
status register along with the received character and letting the
tasklet handle break, sysrq, error flags, etc.
Since we don't need to store the tty flags and overrun bit, the size
of each entry in the RX ring is reduced from 16 bytes to 4 bytes.
I've also used the first SMP barrier pair in my life; hope it's
correct. I don't think we need full barriers because we don't deal
with I/O, but we need SMP barriers to get correct ordering across CPUs
and to prevent the compiler from reading things in the wrong order.
This patch should apply on top of the cleanup patch I sent earlier
today. Or at least I think so...I'll send the full series once
everyone are happy.
drivers/serial/atmel_serial.c | 208 ++++++++++++++++++++++++++++++++---------
1 files changed, 162 insertions(+), 46 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index bd41529..a733db5 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -111,6 +111,13 @@
static int (*atmel_open_hook) (struct uart_port *);
static void (*atmel_close_hook) (struct uart_port *);
+struct atmel_uart_char {
+ u16 status;
+ u16 ch;
+};
+
+#define ATMEL_SERIAL_RINGSIZE 1024
+
/*
* We wrap our port structure around the generic uart_port.
*/
@@ -119,6 +126,12 @@ struct atmel_uart_port {
struct clk *clk; /* uart clock */
unsigned short suspended; /* is port suspended? */
int break_active; /* break being received */
+
+ struct tasklet_struct tasklet;
+ unsigned int irq_pending;
+ unsigned int irq_status;
+
+ struct circ_buf rx_ring;
};
static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
@@ -248,22 +261,42 @@ static void atmel_break_ctl(struct uart_port *port, int break_state)
}
/*
+ * Stores the incoming character in the ring buffer
+ */
+static void
+atmel_buffer_rx_char(struct uart_port *port, unsigned int status,
+ unsigned int ch)
+{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct circ_buf *ring = &atmel_port->rx_ring;
+ struct atmel_uart_char *c;
+
+ if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE))
+ /* Buffer overflow, ignore char */
+ return;
+
+ c = &((struct atmel_uart_char *)ring->buf)[ring->head];
+ c->status = status;
+ c->ch = ch;
+
+ /* Make sure the character is stored before we update head. */
+ smp_wmb();
+
+ ring->head = (ring->head + 1) & (ATMEL_SERIAL_RINGSIZE - 1);
+}
+
+/*
* Characters received (called from interrupt handler)
*/
static void atmel_rx_chars(struct uart_port *port)
{
struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
- struct tty_struct *tty = port->info->tty;
- unsigned int status, ch, flg;
+ unsigned int status, ch;
status = UART_GET_CSR(port);
while (status & ATMEL_US_RXRDY) {
ch = UART_GET_CHAR(port);
- port->icount.rx++;
-
- flg = TTY_NORMAL;
-
/*
* note that the error handling code is
* out of the main execution path
@@ -271,17 +304,14 @@ static void atmel_rx_chars(struct uart_port *port)
if (unlikely(status & (ATMEL_US_PARE | ATMEL_US_FRAME
| ATMEL_US_OVRE | ATMEL_US_RXBRK)
|| atmel_port->break_active)) {
+
/* clear error */
UART_PUT_CR(port, ATMEL_US_RSTSTA);
+
if (status & ATMEL_US_RXBRK
&& !atmel_port->break_active) {
- /* ignore side-effect */
- status &= ~(ATMEL_US_PARE | ATMEL_US_FRAME);
- port->icount.brk++;
atmel_port->break_active = 1;
UART_PUT_IER(port, ATMEL_US_RXBRK);
- if (uart_handle_break(port))
- goto ignore_char;
} else {
/*
* This is either the end-of-break
@@ -294,33 +324,13 @@ static void atmel_rx_chars(struct uart_port *port)
status &= ~ATMEL_US_RXBRK;
atmel_port->break_active = 0;
}
- if (status & ATMEL_US_PARE)
- port->icount.parity++;
- if (status & ATMEL_US_FRAME)
- port->icount.frame++;
- if (status & ATMEL_US_OVRE)
- port->icount.overrun++;
-
- status &= port->read_status_mask;
-
- if (status & ATMEL_US_RXBRK)
- flg = TTY_BREAK;
- else if (status & ATMEL_US_PARE)
- flg = TTY_PARITY;
- else if (status & ATMEL_US_FRAME)
- flg = TTY_FRAME;
}
- if (uart_handle_sysrq_char(port, ch))
- goto ignore_char;
-
- uart_insert_char(port, status, ATMEL_US_OVRE, ch, flg);
-
-ignore_char:
+ atmel_buffer_rx_char(port, status, ch);
status = UART_GET_CSR(port);
}
- tty_flip_buffer_push(tty);
+ tasklet_schedule(&atmel_port->tasklet);
}
/*
@@ -379,7 +389,7 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)
}
/*
- * transmit interrupt handler.
+ * transmit interrupt handler. (Transmit is IRQF_NODELAY safe)
*/
static void
atmel_handle_transmit(struct uart_port *port, unsigned int pending)
@@ -396,18 +406,16 @@ static void
atmel_handle_status(struct uart_port *port, unsigned int pending,
unsigned int status)
{
- /* TODO: All reads to CSR will clear these interrupts! */
- if (pending & ATMEL_US_RIIC)
- port->icount.rng++;
- if (pending & ATMEL_US_DSRIC)
- port->icount.dsr++;
- if (pending & ATMEL_US_DCDIC)
- uart_handle_dcd_change(port, !(status & ATMEL_US_DCD));
- if (pending & ATMEL_US_CTSIC)
- uart_handle_cts_change(port, !(status & ATMEL_US_CTS));
- if (pending & (ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC
- | ATMEL_US_CTSIC))
- wake_up_interruptible(&port->info->delta_msr_wait);
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+
+ spin_lock(&port->lock);
+
+ atmel_port->irq_pending |= pending;
+ atmel_port->irq_status = status;
+
+ spin_unlock(&port->lock);
+
+ tasklet_schedule(&atmel_port->tasklet);
}
/*
@@ -435,6 +443,100 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
}
/*
+ * tasklet handling tty stuff outside the interrupt handler.
+ */
+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;
+ struct circ_buf *ring = &atmel_port->rx_ring;
+ unsigned long flags;
+ unsigned int status;
+ unsigned int flg;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ if (atmel_port->irq_pending) {
+ unsigned int pending;
+
+ pending = atmel_port->irq_pending;
+ status = atmel_port->irq_status;
+ atmel_port->irq_pending = 0;
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ /* TODO: All reads to CSR will clear these interrupts! */
+ if (pending & ATMEL_US_RIIC)
+ port->icount.rng++;
+ if (pending & ATMEL_US_DSRIC)
+ port->icount.dsr++;
+ if (pending & ATMEL_US_DCDIC)
+ uart_handle_dcd_change(port, !(status & ATMEL_US_DCD));
+ if (pending & ATMEL_US_CTSIC)
+ uart_handle_cts_change(port, !(status & ATMEL_US_CTS));
+ if (pending & (ATMEL_US_RIIC | ATMEL_US_DSRIC
+ | ATMEL_US_DCDIC | ATMEL_US_CTSIC))
+ wake_up_interruptible(&port->info->delta_msr_wait);
+ } else {
+ spin_unlock_irqrestore(&port->lock, flags);
+ }
+
+ while (ring->head != ring->tail) {
+ struct atmel_uart_char c;
+
+ /* Make sure c is loaded after head. */
+ smp_rmb();
+
+ c = ((struct atmel_uart_char *)ring->buf)[ring->tail];
+
+ ring->tail = (ring->tail + 1) & (ATMEL_SERIAL_RINGSIZE - 1);
+
+ port->icount.rx++;
+ status = c.status;
+ flg = TTY_NORMAL;
+
+ /*
+ * note that the error handling code is
+ * out of the main execution path
+ */
+ if (unlikely(status & (ATMEL_US_PARE | ATMEL_US_FRAME
+ | ATMEL_US_OVRE | ATMEL_US_RXBRK))) {
+ if (status & ATMEL_US_RXBRK) {
+ /* ignore side-effect */
+ status &= ~(ATMEL_US_PARE | ATMEL_US_FRAME);
+
+ port->icount.brk++;
+ if (uart_handle_break(port))
+ continue;
+ }
+ if (status & ATMEL_US_PARE)
+ port->icount.parity++;
+ if (status & ATMEL_US_FRAME)
+ port->icount.frame++;
+ if (status & ATMEL_US_OVRE)
+ port->icount.overrun++;
+
+ status &= port->read_status_mask;
+
+ if (status & ATMEL_US_RXBRK)
+ flg = TTY_BREAK;
+ else if (status & ATMEL_US_PARE)
+ flg = TTY_PARITY;
+ else if (status & ATMEL_US_FRAME)
+ flg = TTY_FRAME;
+ }
+
+
+ if (uart_handle_sysrq_char(port, c.ch))
+ continue;
+
+ uart_insert_char(port, status, ATMEL_US_OVRE, c.ch, flg);
+ }
+
+ tty_flip_buffer_push(port->info->tty);
+}
+
+/*
* Perform initialization and enable port for reception
*/
static int atmel_startup(struct uart_port *port)
@@ -765,6 +867,11 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
port->mapbase = pdev->resource[0].start;
port->irq = pdev->resource[1].start;
+ tasklet_init(&atmel_port->tasklet, atmel_tasklet_func,
+ (unsigned long)port);
+
+ memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
+
if (data->regs)
/* Already mapped by setup code */
port->membase = data->regs;
@@ -995,11 +1102,19 @@ static int atmel_serial_resume(struct platform_device *pdev)
static int __devinit atmel_serial_probe(struct platform_device *pdev)
{
struct atmel_uart_port *port;
+ void *data;
int ret;
+ BUILD_BUG_ON(!is_power_of_2(ATMEL_SERIAL_RINGSIZE));
+
port = &atmel_ports[pdev->id];
atmel_init_port(port, pdev);
+ data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+ port->rx_ring.buf = data;
+
ret = uart_add_one_port(&atmel_uart, &port->uart);
if (!ret) {
device_init_wakeup(&pdev->dev, 1);
@@ -1023,6 +1138,7 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
if (port) {
ret = uart_remove_one_port(&atmel_uart, port);
+ kfree(atmel_port->rx_ring.buf);
kfree(port);
}
--
1.5.3.4
Hello Haavard,
A few remarks:
> From: Remy Bohmer <[email protected]>
My name, at your address ;-)))
> This patch splits up the interrupt handler of the serial port
> into a interrupt top-half and a tasklet.
I see you moved the handling of the sysrq-key to the tasklet. This was
actually a very nice feature in the IRQ-top half on preempt-RT. This
helps debugging running away RT-processes.
> In this version of the patch, we try to only do things that are
> absolutely necessary in the interrupt handler, storing away the
> status register along with the received character and letting the
> tasklet handle break, sysrq, error flags, etc.
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.
On preempt-RT this spinlock must be a raw-spinlock. (If this type is
known in the mainline kernel, you can apply that patch it anyway)
BTW: Attached I have added a 2nd patch that I use for Preempt-RT. (For
cleaner startup, and to get rid of useless IRQ-threads.
> This patch should apply on top of the cleanup patch I sent earlier
For the cleanup patch:
Acked-by: Remy Bohmer <[email protected]>
> today. Or at least I think so...I'll send the full series once
> everyone are happy.
So, for this patch: I am almost happy ;-)
Kind Regards,
Remy
Crud...my mailer helpfully filtered this into the huge linux-kernel
bin instead of leaving it in my inbox...
On Tue, 18 Dec 2007 16:23:11 +0100
"Remy Bohmer" <[email protected]> wrote:
> Hello Haavard,
>
> A few remarks:
>
> > From: Remy Bohmer <[email protected]>
>
> My name, at your address ;-)))
Right. Wonder how that happened...I'll try to fix it manually before I
send the next patchbomb.
> > This patch splits up the interrupt handler of the serial port
> > into a interrupt top-half and a tasklet.
>
> I see you moved the handling of the sysrq-key to the tasklet. This was
> actually a very nice feature in the IRQ-top half on preempt-RT. This
> helps debugging running away RT-processes.
Ah. Good point. I guess we should move it back, then.
> > In this version of the patch, we try to only do things that are
> > absolutely necessary in the interrupt handler, storing away the
> > status register along with the received character and letting the
> > tasklet handle break, sysrq, error flags, etc.
>
> 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.
> On preempt-RT this spinlock must be a raw-spinlock. (If this type is
> known in the mainline kernel, you can apply that patch it anyway)
I'll see if that works.
> BTW: Attached I have added a 2nd patch that I use for Preempt-RT. (For
> cleaner startup, and to get rid of useless IRQ-threads.
Hrm. That assumption isn't valid on AVR32...on AP7000, for example,
IRQ1 is used by the LCD controller.
> > This patch should apply on top of the cleanup patch I sent earlier
>
> For the cleanup patch:
> Acked-by: Remy Bohmer <[email protected]>
>
> > today. Or at least I think so...I'll send the full series once
> > everyone are happy.
>
> So, for this patch: I am almost happy ;-)
Great :-)
Haavard
Hello Haavard,
> > BTW: Attached I have added a 2nd patch that I use for Preempt-RT. (For
> > cleaner startup, and to get rid of useless IRQ-threads.
>
> Hrm. That assumption isn't valid on AVR32...on AP7000, for example,
> IRQ1 is used by the LCD controller.
In that case, forget that (dirty) patch completely for now. It does
not break things on Preempt-RT, we only will have a IRQ-1 kernel
thread that is never scheduled. (Of which I think is actually a
irq-manage bug, if there are no handlers that are scheduled on that
thread, it should not even start the thread.)
I will verify/test the complete patchset tomorrow.
Kind Regards,
Remy
On Tue, 18 Dec 2007 16:23:11 +0100
"Remy Bohmer" <[email protected]> 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))
Hello Haavard,
> Hmm...perhaps we can eliminate the locking in the status handler
> too...? Does anyone see a problem with this patch?
I have not seen any problem so far, besides, I am very happy with a
lockless interrupt handling, because this helps reducing latencies.
Tested it on top of the other 5 patches, and everything still works,
also tested under stress conditions.
So:
Acked-by: Remy Bohmer <[email protected]>
Kind Regards,
Remy
On Wed, 19 Dec 2007 13:50:16 +0100
"Remy Bohmer" <[email protected]> wrote:
> Hello Haavard,
>
> > Hmm...perhaps we can eliminate the locking in the status handler
> > too...? Does anyone see a problem with this patch?
>
> I have not seen any problem so far, besides, I am very happy with a
> lockless interrupt handling, because this helps reducing latencies.
>
> Tested it on top of the other 5 patches, and everything still works,
> also tested under stress conditions.
>
> So:
> Acked-by: Remy Bohmer <[email protected]>
Thanks. I think we can actually do it even simpler -- just check if any
of the relevant bits in pending are set, and schedule the tasklet if
they are.
Now, I suspect the locking is currently broken -- we need to guard
against updates to read_status_mask and ignore_status_mask, but I think
we can get away with only adding some locking to the tasklet, not the
interrupt handler.
Hrm. We probably need to lock while updating icount. That's a problem
since we do that from the tx interrupt handler...and I don't suppose we
want to move most of the atmel_tx_chars() code into the tasklet too...?
Haavard
Hello Haavard,
> Hrm. We probably need to lock while updating icount. That's a problem
> since we do that from the tx interrupt handler...and I don't suppose we
> want to move most of the atmel_tx_chars() code into the tasklet too...?
I do not see a reason why not moving transmit to a tasklet. It is only
time critical to read in time. If the transmit is postponed for a
while, it will only slow down transmission, while not reading in time
results in lost data.
Kind Regards,
Remy
On Wed, 19 Dec 2007 14:18:18 +0100
"Remy Bohmer" <[email protected]> wrote:
> Hello Haavard,
>
> > Hrm. We probably need to lock while updating icount. That's a problem
> > since we do that from the tx interrupt handler...and I don't suppose we
> > want to move most of the atmel_tx_chars() code into the tasklet too...?
>
> I do not see a reason why not moving transmit to a tasklet. It is only
> time critical to read in time. If the transmit is postponed for a
> while, it will only slow down transmission, while not reading in time
> results in lost data.
Ok, let's try that. But I'm afraid this means that we can't move the
sysrq processing back into hardirq context since if we're going to do
break handling, we need to update icount.brk as well.
Haavard