Re: [PATCH] atmel_serial: Split the interrupt handler
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_serial: Split the interrupt handler
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_serial: Split the interrupt handler
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_serial: Split the interrupt handler
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_serial: Split the interrupt handler
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 intirq_pending;
unsigned intirq_status;
+ unsigned intirq_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))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_serial: Split the interrupt handler
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_serial: Split the interrupt handler
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_serial: Split the interrupt handler
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
This patch prevents starting up a useless IRQ-thread for IRQ-1,
because this IRQ is running in IRQF_NODELAY context due to the
timer interrupt.
Signed-off-by: Remy Bohmer <[EMAIL PROTECTED]>
---
drivers/serial/atmel_serial.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
Index: linux-2.6.23/drivers/serial/atmel_serial.c
===
--- linux-2.6.23.orig/drivers/serial/atmel_serial.c 2007-12-18 15:49:02.0 +0100
+++ linux-2.6.23/drivers/serial/atmel_serial.c 2007-12-18 15:56:37.0 +0100
@@ -549,6 +549,7 @@ static int atmel_startup(struct uart_por
{
struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
int retval;
+ unsigned long irqflags = IRQF_SHARED;
/*
* Ensure that no interrupts are enabled otherwise when
@@ -557,10 +558,17 @@ static int atmel_startup(struct uart_por
*/
UART_PUT_IDR(port, -1);
+#ifdef CONFIG_PREEMPT_RT
+ /* IRQ1 is the System IRQ, which is shared with the Timer-IRQ, and
+ thus it is running in IRQF_NODELAY context. By setting this flag
+ here also, we prevent starting up a useless IRQ-thread.*/
+ if (port->irq == 1)
+ irqflags |= IRQF_NODELAY;
+#endif
/*
* Allocate the IRQ
*/
- retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
+ retval = request_irq(port->irq, atmel_interrupt, irqflags,
"atmel_serial", port);
if (retval) {
printk("atmel_serial: atmel_startup - Can't get irq\n");
[PATCH] atmel_serial: Split the interrupt handler
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 intirq_pending;
+ unsigned intirq_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;
-
/*
