Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, Remy Bohmer wrote: Hello All, All works now for me with preempt-rt. The problem is using hrtimer. I think that hrtimer are executed with interrupts disabled so, if this happen when I must receive a char, i have an overrun. No, they share the same interrupt line... I think that the hrtimer use and other interrupt line. The AT91SAM9260_ID_TC2. So, while the timer interrupt handler is running, the serial handler has to wait until the timer interrupt handler has finished. Notice that the HRT interrupt handler is quite heavy and takes a long time to complete. The problem is the heavy of HRT interrupt handler of course. And, as I already mentioned, related to the 1 byte FIFO and a interrupt latency of about 85us (without HRT), it is logical that you can get an overrun at the higher serial speeds... (>=115200bps) I don't have the same problem without the hrtimer, I suppose the the timer latency is not so heavy. The only solution was the dma support to serial device. Or, use flow control? Yes :) Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello All, > All works now for me with preempt-rt. The problem is using hrtimer. > I think that hrtimer are executed with interrupts disabled so, if > this happen when I must receive a char, i have an overrun. No, they share the same interrupt line... So, while the timer interrupt handler is running, the serial handler has to wait until the timer interrupt handler has finished. Notice that the HRT interrupt handler is quite heavy and takes a long time to complete. And, as I already mentioned, related to the 1 byte FIFO and a interrupt latency of about 85us (without HRT), it is logical that you can get an overrun at the higher serial speeds... (>=115200bps) > The only solution was the dma support to serial device. Or, use flow control? 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, All works now for me with preempt-rt. The problem is using hrtimer. I think that hrtimer are executed with interrupts disabled so, if this happen when I must receive a char, i have an overrun. The only solution was the dma support to serial device. Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, All works now for me with preempt-rt. The problem is using hrtimer. I think that hrtimer are executed with interrupts disabled so, if this happen when I must receive a char, i have an overrun. The only solution was the dma support to serial device. Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello All, All works now for me with preempt-rt. The problem is using hrtimer. I think that hrtimer are executed with interrupts disabled so, if this happen when I must receive a char, i have an overrun. No, they share the same interrupt line... So, while the timer interrupt handler is running, the serial handler has to wait until the timer interrupt handler has finished. Notice that the HRT interrupt handler is quite heavy and takes a long time to complete. And, as I already mentioned, related to the 1 byte FIFO and a interrupt latency of about 85us (without HRT), it is logical that you can get an overrun at the higher serial speeds... (=115200bps) The only solution was the dma support to serial device. Or, use flow control? 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, Remy Bohmer wrote: Hello All, All works now for me with preempt-rt. The problem is using hrtimer. I think that hrtimer are executed with interrupts disabled so, if this happen when I must receive a char, i have an overrun. No, they share the same interrupt line... I think that the hrtimer use and other interrupt line. The AT91SAM9260_ID_TC2. So, while the timer interrupt handler is running, the serial handler has to wait until the timer interrupt handler has finished. Notice that the HRT interrupt handler is quite heavy and takes a long time to complete. The problem is the heavy of HRT interrupt handler of course. And, as I already mentioned, related to the 1 byte FIFO and a interrupt latency of about 85us (without HRT), it is logical that you can get an overrun at the higher serial speeds... (=115200bps) I don't have the same problem without the hrtimer, I suppose the the timer latency is not so heavy. The only solution was the dma support to serial device. Or, use flow control? Yes :) Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello Michael, > the serial driver works fine. The problem seems to be related to the > tclib, when I use it as a clocksource. tclib as a clocksource should be no problem on Preempt-RT, do not use it as clockevent device, it will not work properly on preempt-rt on at91* yet, especially with the NO_HZ config. > The numbers of overruns depends on the type of > files too. It is possible? I am still very curious to your (RT-) thread-prio layout. The problems you mention can also be caused by a weird configuration of these threads on preempt-rt. Remy 2008/2/6, michael <[EMAIL PROTECTED]>: > Hi, > > the serial driver works fine. The problem seems to be related to the > tclib, when I use > it as a clocksource. The numbers of overruns depends on the type of > files too. > It is possible? > > Regards > Michael > > > -- > 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/ > -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 06 Feb 2008 14:41:09 +0100 michael <[EMAIL PROTECTED]> wrote: > I refer to this part of documentation: > > "The USART behavior when hardware handshaking is enabled is the same as > the behavior in > standard synchronous or asynchronous mode, except that the receiver > drives the RTS pin as > described below and the level on the CTS pin modifies the behavior of > the transmitter as > described below. Using this mode requires using the PDC channel for > reception. The transmitter > can handle hardware handshaking in any case." Oh. I guess the answer is no, then. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, Haavard Skinnemoen wrote: On Tue, 05 Feb 2008 13:29:35 +0100 michael <[EMAIL PROTECTED]> wrote: Just one question: Receiving with hardware handshake works without PDC? I don't know...I haven't tried. These patches shouldn't change anything though. 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/ I refer to this part of documentation: "The USART behavior when hardware handshaking is enabled is the same as the behavior in standard synchronous or asynchronous mode, except that the receiver drives the RTS pin as described below and the level on the CTS pin modifies the behavior of the transmitter as described below. Using this mode requires using the PDC channel for reception. The transmitter can handle hardware handshaking in any case." Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Tue, 05 Feb 2008 13:29:35 +0100 michael <[EMAIL PROTECTED]> wrote: > Just one question: > Receiving with hardware handshake works without PDC? I don't know...I haven't tried. These patches shouldn't change anything though. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, the serial driver works fine. The problem seems to be related to the tclib, when I use it as a clocksource. The numbers of overruns depends on the type of files too. It is possible? Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, the serial driver works fine. The problem seems to be related to the tclib, when I use it as a clocksource. The numbers of overruns depends on the type of files too. It is possible? Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Tue, 05 Feb 2008 13:29:35 +0100 michael [EMAIL PROTECTED] wrote: Just one question: Receiving with hardware handshake works without PDC? I don't know...I haven't tried. These patches shouldn't change anything though. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, Haavard Skinnemoen wrote: On Tue, 05 Feb 2008 13:29:35 +0100 michael [EMAIL PROTECTED] wrote: Just one question: Receiving with hardware handshake works without PDC? I don't know...I haven't tried. These patches shouldn't change anything though. 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/ I refer to this part of documentation: The USART behavior when hardware handshaking is enabled is the same as the behavior in standard synchronous or asynchronous mode, except that the receiver drives the RTS pin as described below and the level on the CTS pin modifies the behavior of the transmitter as described below. Using this mode requires using the PDC channel for reception. The transmitter can handle hardware handshaking in any case. Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 06 Feb 2008 14:41:09 +0100 michael [EMAIL PROTECTED] wrote: I refer to this part of documentation: The USART behavior when hardware handshaking is enabled is the same as the behavior in standard synchronous or asynchronous mode, except that the receiver drives the RTS pin as described below and the level on the CTS pin modifies the behavior of the transmitter as described below. Using this mode requires using the PDC channel for reception. The transmitter can handle hardware handshaking in any case. Oh. I guess the answer is no, then. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello Michael, the serial driver works fine. The problem seems to be related to the tclib, when I use it as a clocksource. tclib as a clocksource should be no problem on Preempt-RT, do not use it as clockevent device, it will not work properly on preempt-rt on at91* yet, especially with the NO_HZ config. The numbers of overruns depends on the type of files too. It is possible? I am still very curious to your (RT-) thread-prio layout. The problems you mention can also be caused by a weird configuration of these threads on preempt-rt. Remy 2008/2/6, michael [EMAIL PROTECTED]: Hi, the serial driver works fine. The problem seems to be related to the tclib, when I use it as a clocksource. The numbers of overruns depends on the type of files too. It is possible? Regards Michael -- 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/ -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
hi, diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c index cb70cc5..f310a80 100644 --- a/drivers/serial/atmel_serial.c +++ b/drivers/serial/atmel_serial.c @@ -552,7 +552,7 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id) atmel_handle_transmit(port, pending); } while (pass_counter++ < ATMEL_ISR_PASS_LIMIT); - return IRQ_HANDLED; + return pass_counter ? IRQ_HANDLED : IRQ_NONE; } /* Just one question: Receiving with hardware handshake works without PDC? Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
hi, diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c index cb70cc5..f310a80 100644 --- a/drivers/serial/atmel_serial.c +++ b/drivers/serial/atmel_serial.c @@ -552,7 +552,7 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id) atmel_handle_transmit(port, pending); } while (pass_counter++ ATMEL_ISR_PASS_LIMIT); - return IRQ_HANDLED; + return pass_counter ? IRQ_HANDLED : IRQ_NONE; } /* Just one question: Receiving with hardware handshake works without PDC? Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Mon, 04 Feb 2008 20:01:26 +0100 michael <[EMAIL PROTECTED]> wrote: > I think the the atmel_interrupt handler > must check the > pass_counter before return IRQ_HANDLED. I'm not sure if it helps in this particular case but yeah, since the interrupt may be shared, it's definitely wrong to always return IRQ_HANDLED. Nice catch. Could you try the patch below? Haavard diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c index cb70cc5..f310a80 100644 --- a/drivers/serial/atmel_serial.c +++ b/drivers/serial/atmel_serial.c @@ -552,7 +552,7 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id) atmel_handle_transmit(port, pending); } while (pass_counter++ < ATMEL_ISR_PASS_LIMIT); - return IRQ_HANDLED; + return pass_counter ? IRQ_HANDLED : IRQ_NONE; } /* -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello Haavard, > That's what I was thinking too. If this is indeed the cause, the > dev_err() added by the debug patch I posted should trigger and we may > consider boosting the priority of the tasklet (using > tasklet_hi_schedule.) Notice that we are talking about Preempt-RT here. Everything is running in thread context, even tasklets, softirqs etc. They are _all_ preemptible, and if Michael has some RT-thread in the system that has a higher priority than this tasklet or softirq, than the buffer will eventually overflow. I wonder also if Michael has set the RT-priorities correctly, on RT _every_ softirq/irq thread starts by default on priority 50, SCHED_FIFO. If they are still at 50, any other softirq/tasklet, or irq can make this behavior worse. Notice that the default 50 thingy rarely gives a decent behaving system. (But any other default will also give problems anyway, so it _has_ to be customized/tuned by the end user) 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi
Haavard
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 477950f..c61fcc3 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -337,9 +337,12 @@ atmel_buffer_rx_char(struct uart_port *port, unsigned int
status,
struct circ_buf *ring = _port->rx_ring;
struct atmel_uart_char *c;
- if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE))
+ if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE)) {
+ dev_err(port->dev, "RX ring buffer full, dropping data\n");
+
/* Buffer overflow, ignore char */
return;
+ }
c = &((struct atmel_uart_char *)ring->buf)[ring->head];
c->status= status;
I have already tried that but I have never seen the buffer full. So
tomorrow I can do other
tests with the serial device. I think the the atmel_interrupt handler
must check the
pass_counter before return IRQ_HANDLED.
Regards Michael
--
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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Thu, 31 Jan 2008 20:36:25 +0100 "Remy Bohmer" <[EMAIL PROTECTED]> wrote: > A long shot, but can it be that the ringbuffer overflows, and that > therefor characters are lost? That's what I was thinking too. If this is indeed the cause, the dev_err() added by the debug patch I posted should trigger and we may consider boosting the priority of the tasklet (using tasklet_hi_schedule.) Other solutions may involve trying to figure out what exactly is blocking the tasklet from running. I have a patch series for the macb driver that optimizes the RX processing a bit, but it obviously won't help at91rm9200 since it uses a different driver... 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi
Haavard
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 477950f..c61fcc3 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -337,9 +337,12 @@ atmel_buffer_rx_char(struct uart_port *port, unsigned int
status,
struct circ_buf *ring = atmel_port-rx_ring;
struct atmel_uart_char *c;
- if (!CIRC_SPACE(ring-head, ring-tail, ATMEL_SERIAL_RINGSIZE))
+ if (!CIRC_SPACE(ring-head, ring-tail, ATMEL_SERIAL_RINGSIZE)) {
+ dev_err(port-dev, RX ring buffer full, dropping data\n);
+
/* Buffer overflow, ignore char */
return;
+ }
c = ((struct atmel_uart_char *)ring-buf)[ring-head];
c-status= status;
I have already tried that but I have never seen the buffer full. So
tomorrow I can do other
tests with the serial device. I think the the atmel_interrupt handler
must check the
pass_counter before return IRQ_HANDLED.
Regards Michael
--
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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Thu, 31 Jan 2008 20:36:25 +0100 Remy Bohmer [EMAIL PROTECTED] wrote: A long shot, but can it be that the ringbuffer overflows, and that therefor characters are lost? That's what I was thinking too. If this is indeed the cause, the dev_err() added by the debug patch I posted should trigger and we may consider boosting the priority of the tasklet (using tasklet_hi_schedule.) Other solutions may involve trying to figure out what exactly is blocking the tasklet from running. I have a patch series for the macb driver that optimizes the RX processing a bit, but it obviously won't help at91rm9200 since it uses a different driver... 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello Haavard, That's what I was thinking too. If this is indeed the cause, the dev_err() added by the debug patch I posted should trigger and we may consider boosting the priority of the tasklet (using tasklet_hi_schedule.) Notice that we are talking about Preempt-RT here. Everything is running in thread context, even tasklets, softirqs etc. They are _all_ preemptible, and if Michael has some RT-thread in the system that has a higher priority than this tasklet or softirq, than the buffer will eventually overflow. I wonder also if Michael has set the RT-priorities correctly, on RT _every_ softirq/irq thread starts by default on priority 50, SCHED_FIFO. If they are still at 50, any other softirq/tasklet, or irq can make this behavior worse. Notice that the default 50 thingy rarely gives a decent behaving system. (But any other default will also give problems anyway, so it _has_ to be customized/tuned by the end user) 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Mon, 04 Feb 2008 20:01:26 +0100 michael [EMAIL PROTECTED] wrote: I think the the atmel_interrupt handler must check the pass_counter before return IRQ_HANDLED. I'm not sure if it helps in this particular case but yeah, since the interrupt may be shared, it's definitely wrong to always return IRQ_HANDLED. Nice catch. Could you try the patch below? Haavard diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c index cb70cc5..f310a80 100644 --- a/drivers/serial/atmel_serial.c +++ b/drivers/serial/atmel_serial.c @@ -552,7 +552,7 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id) atmel_handle_transmit(port, pending); } while (pass_counter++ ATMEL_ISR_PASS_LIMIT); - return IRQ_HANDLED; + return pass_counter ? IRQ_HANDLED : IRQ_NONE; } /* -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello Haavard, > It seems to be very sensitive to network traffic though...could it have > something to do with softirq scheduling? Could you try the patch below > and see if you can trigger the error message? Funny that you mention this. The largest latencies I currently have on RT (and rm9200) occur when using a telnet session or NFS filesystems, thus while using network. The impact on hardware Interrupt latencies are limited (<85us), so the interrupt handler should still be able to keep up the receive buffer, but context switches between threads can stall for a longer time under some conditions. A long shot, but can it be that the ringbuffer overflows, and that therefor characters are lost? 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Thu, 31 Jan 2008 02:53:50 +0100
michael <[EMAIL PROTECTED]> wrote:
> The overrun still remain. An lrz receive session is impossible using
> full preemption. I will try the dma patch too and test in iso mode for
> smart card.
Hmm. Seems to work reasonably well on a non-rt kernel -- I get quite a
few overruns, but nothing more than the Zmodem protocol can handle.
It seems to be very sensitive to network traffic though...could it have
something to do with softirq scheduling? Could you try the patch below
and see if you can trigger the error message?
Haavard
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 477950f..c61fcc3 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -337,9 +337,12 @@ atmel_buffer_rx_char(struct uart_port *port, unsigned int
status,
struct circ_buf *ring = _port->rx_ring;
struct atmel_uart_char *c;
- if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE))
+ if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE)) {
+ dev_err(port->dev, "RX ring buffer full, dropping data\n");
+
/* Buffer overflow, ignore char */
return;
+ }
c = &((struct atmel_uart_char *)ring->buf)[ring->head];
c->status = status;
--
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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Thu, 31 Jan 2008 02:53:50 +0100
michael [EMAIL PROTECTED] wrote:
The overrun still remain. An lrz receive session is impossible using
full preemption. I will try the dma patch too and test in iso mode for
smart card.
Hmm. Seems to work reasonably well on a non-rt kernel -- I get quite a
few overruns, but nothing more than the Zmodem protocol can handle.
It seems to be very sensitive to network traffic though...could it have
something to do with softirq scheduling? Could you try the patch below
and see if you can trigger the error message?
Haavard
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 477950f..c61fcc3 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -337,9 +337,12 @@ atmel_buffer_rx_char(struct uart_port *port, unsigned int
status,
struct circ_buf *ring = atmel_port-rx_ring;
struct atmel_uart_char *c;
- if (!CIRC_SPACE(ring-head, ring-tail, ATMEL_SERIAL_RINGSIZE))
+ if (!CIRC_SPACE(ring-head, ring-tail, ATMEL_SERIAL_RINGSIZE)) {
+ dev_err(port-dev, RX ring buffer full, dropping data\n);
+
/* Buffer overflow, ignore char */
return;
+ }
c = ((struct atmel_uart_char *)ring-buf)[ring-head];
c-status = status;
--
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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello Haavard, It seems to be very sensitive to network traffic though...could it have something to do with softirq scheduling? Could you try the patch below and see if you can trigger the error message? Funny that you mention this. The largest latencies I currently have on RT (and rm9200) occur when using a telnet session or NFS filesystems, thus while using network. The impact on hardware Interrupt latencies are limited (85us), so the interrupt handler should still be able to keep up the receive buffer, but context switches between threads can stall for a longer time under some conditions. A long shot, but can it be that the ringbuffer overflows, and that therefor characters are lost? 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, Haavard Skinnemoen wrote: On Wed, 30 Jan 2008 16:26:27 +0100 michael <[EMAIL PROTECTED]> wrote: I have no idea. Could you post some more specifics about what you modified, for example a diff? ... /* The interrupt handler does not take the lock */ spin_lock_irqsave(>lock, flags); atmel_tx_chars(port); spin_unlock_irqrestore(>lock, flags); Sorry, this isn't going to work. Please post a diff with the changes you did to the driver, and whatever output you got when it crashed. It's really difficult to help you when I don't know (a) what code you're actually running, or (b) anything about the crash. Ok, but the problem is that I have some added code for using the uart with smart card in iso mode, (is never called) and the patch is not so clean. Now I return to the original patch without the spin_lock_irqsave and with the fix of buffer allocation,and I don't see the crash anymore. In full preemptive all works with threading hardirqs and sofirqs. I will do other testing before posting again. The atmel_tx_chars using the serial device registers like the interrupt routine and so I think that it is possible to have interference during send operation. No, it's only called from the tasklet, and the interrupt handler doesn't touch the TX data register. There shouldn't be any need to disable interrupts around the call to atmel_tx_chars(). In fact, this may very well be the cause of the overruns you're seeing. Haavard The overrun still remain. An lrz receive session is impossible using full preemption. I will try the dma patch too and test in iso mode for smart card. Thanks -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 30 Jan 2008 16:26:27 +0100 michael <[EMAIL PROTECTED]> wrote: > > I have no idea. Could you post some more specifics about what you > > modified, for example a diff? > > > > > > ... > /* The interrupt handler does not take the lock */ > spin_lock_irqsave(>lock, flags); > atmel_tx_chars(port); > spin_unlock_irqrestore(>lock, flags); Sorry, this isn't going to work. Please post a diff with the changes you did to the driver, and whatever output you got when it crashed. It's really difficult to help you when I don't know (a) what code you're actually running, or (b) anything about the crash. > The atmel_tx_chars using the serial device registers like the interrupt > routine > and so I think that it is possible to have interference during send > operation. No, it's only called from the tasklet, and the interrupt handler doesn't touch the TX data register. There shouldn't be any need to disable interrupts around the call to atmel_tx_chars(). In fact, this may very well be the cause of the overruns you're seeing. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, A few questions arise here to me: * What serial port is used here? (DBGU, or something else) * No DMA was used, was flow-control enabled? (cannot with DBGU) * If some other UART, why not using DMA? DBGU, so no flow control Notice that the DBGU has no flow control, and just a 1 byte FIFO (thus no fifo at all). At high speeds (e.g. >=115200) it is _likely_ that you will miss characters, nothing can prevent that. DBGU should only be used at lower speeds, or just as text console. 115200 is running fine here as text-console. Overrun are admitted using DBGU and UART1..n without flow control, but with the old version of the driver I can send a file using lrz and with the new and full preemption is impossible. I would not expect that the behaviour is worse than without the patchset, because without it it does not work at all on Preempt-RT, but also: there was done much more in interrupt context previously, so the chance of buffer overruns was much more likely in the old situation. The real interrupt handler (doing the reading from the fifo) must be as short as possible, to be able to keep up with the data flow. A simple calculation: 115200bps results in approx. 11520 bytes per second. This means that the interrupt handler must be capable of handling each byte on DBGU within 87us. With a worst case interrupt latency of about 85us, and average between 2us and 54us (on Preempt-RT and AT91RM9200), you can simply understand that this will not match, how good/fast the interrupt handling will ever be. So, I suggest to either use flow-control, or DMA for bulkdata... (thus not DBGU) 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, On Wed, 30 Jan 2008 11:29:59 +0100 michael <[EMAIL PROTECTED]> wrote: 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. I explain it bad: - with spin_lock the system seems, there is no problem with Valuntary Preeption and Preemptible Kernel - with full preemption it runs but the serial line can't be used for receiving at high bit rate (using lrz) ...but if you drop the spinlock across the call to tty_flip_buffer_push, you get an Oops? Could you post the Oops? So this code /* * Drop the lock here since it might end up calling * uart_start(), which takes the lock. */ spin_unlock(>lock); tty_flip_buffer_push(port->info->tty); spin_lock(>lock); Works with: CONFIG_PREEMPT_RT=y CONFIG_PREEMPT=y CONFIG_PREEMPT_SOFTIRQS=y CONFIG_PREEMPT_HARDIRQS=y CONFIG_PREEMPT_BKL=y but crash with: # CONFIG_PREEMPT_NONE is not set CONFIG_PREEMPT_VOLUNTARY=y # CONFIG_PREEMPT_DESKTOP is not set # CONFIG_PREEMPT_RT is not set CONFIG_PREEMPT_SOFTIRQS=y # CONFIG_PREEMPT_HARDIRQS is not set # CONFIG_PREEMPT_BKL is not set CONFIG_CLASSIC_RCU=y Seems to work in the last config if I comment the spin_lock & spin_unlock call. /* * Drop the lock here since it might end up calling * uart_start(), which takes the lock. spin_unlock(>lock); */ tty_flip_buffer_push(port->info->tty); /* spin_lock(>lock); */ It is not readable because I can't compile it with debugging information (poor memory system) 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? In the complete preemption yes. Which question did you answer "yes" to? That it's worse than before or that the interrupt handler runs in hardirq context (i.e. IRQF_NODELAY)? The interrupt handler run in IRQF_NODELAY context. I think you're right. Can you change it and see if it helps? I just change it because I have corruption on receiving buffer. All my test are done with this fix Ok. 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. I just test it I don't have buffer overflow. No, I'd expect your allocation fix to take care of that. Or did you by any chance test without the fix and with slub_debug enabled? I just meant that the buffer never fills up to its size. I protect with a spinlock the access to the register when we sending from the tasklet. It is correct? I have no idea. Could you post some more specifics about what you modified, for example a diff? ... /* The interrupt handler does not take the lock */ spin_lock_irqsave(>lock, flags); atmel_tx_chars(port); spin_unlock_irqrestore(>lock, flags); spin_lock(>lock); ... The atmel_tx_chars using the serial device registers like the interrupt routine and so I think that it is possible to have interference during send operation. Most of the tasklet is already protected by the spinlock, so you must be careful to avoid any lock recursion. Haavard Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 30 Jan 2008 12:05:40 +0100 "Remy Bohmer" <[EMAIL PROTECTED]> wrote: > > > 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... ;-) > > They are in -mm. You were Cc'ed I think... > > Yes, I saw them, but I did not know if there were any updates in the > mean time, because I had seen some discussions, which confused me a > bit about the current status of the complete set. There's been one update, atmel_serial-add-dma-support-fix.patch, which you were also Cc'ed on. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 30 Jan 2008 11:29:59 +0100 michael <[EMAIL PROTECTED]> wrote: > > 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. > > > > > I explain it bad: > - with spin_lock the system seems, there is no problem with Valuntary > Preeption and > Preemptible Kernel > - with full preemption it runs but the serial line can't be used for > receiving at > high bit rate (using lrz) ...but if you drop the spinlock across the call to tty_flip_buffer_push, you get an Oops? Could you post the Oops? > >> 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? > > > > > In the complete preemption yes. Which question did you answer "yes" to? That it's worse than before or that the interrupt handler runs in hardirq context (i.e. IRQF_NODELAY)? > > I think you're right. Can you change it and see if it helps? > > > > > I just change it because I have corruption on receiving buffer. All > my test are done with this fix Ok. > > 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. > > > > > I just test it I don't have > buffer overflow. No, I'd expect your allocation fix to take care of that. Or did you by any chance test without the fix and with slub_debug enabled? > I protect with a spinlock the access to the register when we sending > from the tasklet. It is correct? I have no idea. Could you post some more specifics about what you modified, for example a diff? Most of the tasklet is already protected by the spinlock, so you must be careful to avoid any lock recursion. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello Haavard, > The code above is from the tasklet, so I don't think that's the problem. > The interrupt handler doesn't take any locks. > Or are spinlocks not allowed in softirq context either? They are allowed there... So, it has to be something different. > > 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... ;-) > They are in -mm. You were Cc'ed I think... Yes, I saw them, but I did not know if there were any updates in the mean time, because I had seen some discussions, which confused me a bit about the current status of the complete set. > - with full preemption it runs but the serial line can't be used for > receiving at high bit rate (using lrz) A few questions arise here to me: * What serial port is used here? (DBGU, or something else) * No DMA was used, was flow-control enabled? (cannot with DBGU) * If some other UART, why not using DMA? Notice that the DBGU has no flow control, and just a 1 byte FIFO (thus no fifo at all). At high speeds (e.g. >=115200) it is _likely_ that you will miss characters, nothing can prevent that. DBGU should only be used at lower speeds, or just as text console. 115200 is running fine here as text-console. I would not expect that the behaviour is worse than without the patchset, because without it it does not work at all on Preempt-RT, but also: there was done much more in interrupt context previously, so the chance of buffer overruns was much more likely in the old situation. The real interrupt handler (doing the reading from the fifo) must be as short as possible, to be able to keep up with the data flow. A simple calculation: 115200bps results in approx. 11520 bytes per second. This means that the interrupt handler must be capable of handling each byte on DBGU within 87us. With a worst case interrupt latency of about 85us, and average between 2us and 54us (on Preempt-RT and AT91RM9200), you can simply understand that this will not match, how good/fast the interrupt handling will ever be. So, I suggest to either use flow-control, or DMA for bulkdata... (thus not DBGU) 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 30 Jan 2008 11:21:49 +0100 "Remy Bohmer" <[EMAIL PROTECTED]> wrote: > > > * Drop the lock here since it might end up calling > > > * uart_start(), which takes the lock. > > >spin_unlock(>lock); > > > */ > > > tty_flip_buffer_push(port->info->tty); > > > /* > > > spin_lock(>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... The code above is from the tasklet, so I don't think that's the problem. The interrupt handler doesn't take any locks. Or are spinlocks not allowed in softirq context either? > 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... ;-) They are in -mm. You were Cc'ed I think... Haavard 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi On Wed, 30 Jan 2008 00:12:23 +0100 michael <[EMAIL PROTECTED]> wrote: - Voluntary Kernel Preemption the system (crash) - Preemptible Kernel (crash) Ouch. I'm assuming this is with DMA disabled? Yes, is with DMA disabled /* * Drop the lock here since it might end up calling * uart_start(), which takes the lock. spin_unlock(>lock); */ tty_flip_buffer_push(port->info->tty); /* spin_lock(>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. I explain it bad: - with spin_lock the system seems, there is no problem with Valuntary Preeption and Preemptible Kernel - with full preemption it runs but the serial line can't be used for receiving at high bit rate (using lrz) 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? In the complete preemption yes. The system is stable and doesn't crash reverting this patch. I don't test with the thread hardirqs active. Ok. 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 just change it because I have corruption on receiving buffer. All my test are done with this fix 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. I just test it I don't have buffer overflow. I protect with a spinlock the access to the register when we sending from the tasklet. It is correct? 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. Ok Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
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(>lock); > > */ > > tty_flip_buffer_push(port->info->tty); > > /* > > spin_lock(>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(>lock); > > */ > > tty_flip_buffer_push(port->info->tty); > > /* > > spin_lock(>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(_uart, >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(_uart, port); > > >> > > >> + tasklet_kill(_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]>
Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler
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(>lock); > */ > tty_flip_buffer_push(port->info->tty); > /* > spin_lock(>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(_uart, >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(_uart, port); > >> > >> + tasklet_kill(_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 -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
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 -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 30 Jan 2008 11:21:49 +0100 Remy Bohmer [EMAIL PROTECTED] wrote: * 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... The code above is from the tasklet, so I don't think that's the problem. The interrupt handler doesn't take any locks. Or are spinlocks not allowed in softirq context either? 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... ;-) They are in -mm. You were Cc'ed I think... Haavard 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hello Haavard, The code above is from the tasklet, so I don't think that's the problem. The interrupt handler doesn't take any locks. Or are spinlocks not allowed in softirq context either? They are allowed there... So, it has to be something different. 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... ;-) They are in -mm. You were Cc'ed I think... Yes, I saw them, but I did not know if there were any updates in the mean time, because I had seen some discussions, which confused me a bit about the current status of the complete set. - with full preemption it runs but the serial line can't be used for receiving at high bit rate (using lrz) A few questions arise here to me: * What serial port is used here? (DBGU, or something else) * No DMA was used, was flow-control enabled? (cannot with DBGU) * If some other UART, why not using DMA? Notice that the DBGU has no flow control, and just a 1 byte FIFO (thus no fifo at all). At high speeds (e.g. =115200) it is _likely_ that you will miss characters, nothing can prevent that. DBGU should only be used at lower speeds, or just as text console. 115200 is running fine here as text-console. I would not expect that the behaviour is worse than without the patchset, because without it it does not work at all on Preempt-RT, but also: there was done much more in interrupt context previously, so the chance of buffer overruns was much more likely in the old situation. The real interrupt handler (doing the reading from the fifo) must be as short as possible, to be able to keep up with the data flow. A simple calculation: 115200bps results in approx. 11520 bytes per second. This means that the interrupt handler must be capable of handling each byte on DBGU within 87us. With a worst case interrupt latency of about 85us, and average between 2us and 54us (on Preempt-RT and AT91RM9200), you can simply understand that this will not match, how good/fast the interrupt handling will ever be. So, I suggest to either use flow-control, or DMA for bulkdata... (thus not DBGU) 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
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
Re: [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi On Wed, 30 Jan 2008 00:12:23 +0100 michael [EMAIL PROTECTED] wrote: - Voluntary Kernel Preemption the system (crash) - Preemptible Kernel (crash) Ouch. I'm assuming this is with DMA disabled? Yes, 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. I explain it bad: - with spin_lock the system seems, there is no problem with Valuntary Preeption and Preemptible Kernel - with full preemption it runs but the serial line can't be used for receiving at high bit rate (using lrz) 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? In the complete preemption yes. The system is stable and doesn't crash reverting this patch. I don't test with the thread hardirqs active. Ok. 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 just change it because I have corruption on receiving buffer. All my test are done with this fix 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. I just test it I don't have buffer overflow. I protect with a spinlock the access to the register when we sending from the tasklet. It is correct? 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. Ok Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 30 Jan 2008 11:29:59 +0100 michael [EMAIL PROTECTED] wrote: 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. I explain it bad: - with spin_lock the system seems, there is no problem with Valuntary Preeption and Preemptible Kernel - with full preemption it runs but the serial line can't be used for receiving at high bit rate (using lrz) ...but if you drop the spinlock across the call to tty_flip_buffer_push, you get an Oops? Could you post the Oops? 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? In the complete preemption yes. Which question did you answer yes to? That it's worse than before or that the interrupt handler runs in hardirq context (i.e. IRQF_NODELAY)? I think you're right. Can you change it and see if it helps? I just change it because I have corruption on receiving buffer. All my test are done with this fix Ok. 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. I just test it I don't have buffer overflow. No, I'd expect your allocation fix to take care of that. Or did you by any chance test without the fix and with slub_debug enabled? I protect with a spinlock the access to the register when we sending from the tasklet. It is correct? I have no idea. Could you post some more specifics about what you modified, for example a diff? Most of the tasklet is already protected by the spinlock, so you must be careful to avoid any lock recursion. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 30 Jan 2008 12:05:40 +0100 Remy Bohmer [EMAIL PROTECTED] wrote: 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... ;-) They are in -mm. You were Cc'ed I think... Yes, I saw them, but I did not know if there were any updates in the mean time, because I had seen some discussions, which confused me a bit about the current status of the complete set. There's been one update, atmel_serial-add-dma-support-fix.patch, which you were also Cc'ed on. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, A few questions arise here to me: * What serial port is used here? (DBGU, or something else) * No DMA was used, was flow-control enabled? (cannot with DBGU) * If some other UART, why not using DMA? DBGU, so no flow control Notice that the DBGU has no flow control, and just a 1 byte FIFO (thus no fifo at all). At high speeds (e.g. =115200) it is _likely_ that you will miss characters, nothing can prevent that. DBGU should only be used at lower speeds, or just as text console. 115200 is running fine here as text-console. Overrun are admitted using DBGU and UART1..n without flow control, but with the old version of the driver I can send a file using lrz and with the new and full preemption is impossible. I would not expect that the behaviour is worse than without the patchset, because without it it does not work at all on Preempt-RT, but also: there was done much more in interrupt context previously, so the chance of buffer overruns was much more likely in the old situation. The real interrupt handler (doing the reading from the fifo) must be as short as possible, to be able to keep up with the data flow. A simple calculation: 115200bps results in approx. 11520 bytes per second. This means that the interrupt handler must be capable of handling each byte on DBGU within 87us. With a worst case interrupt latency of about 85us, and average between 2us and 54us (on Preempt-RT and AT91RM9200), you can simply understand that this will not match, how good/fast the interrupt handling will ever be. So, I suggest to either use flow-control, or DMA for bulkdata... (thus not DBGU) 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, On Wed, 30 Jan 2008 11:29:59 +0100 michael [EMAIL PROTECTED] wrote: 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. I explain it bad: - with spin_lock the system seems, there is no problem with Valuntary Preeption and Preemptible Kernel - with full preemption it runs but the serial line can't be used for receiving at high bit rate (using lrz) ...but if you drop the spinlock across the call to tty_flip_buffer_push, you get an Oops? Could you post the Oops? So this code /* * 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); Works with: CONFIG_PREEMPT_RT=y CONFIG_PREEMPT=y CONFIG_PREEMPT_SOFTIRQS=y CONFIG_PREEMPT_HARDIRQS=y CONFIG_PREEMPT_BKL=y but crash with: # CONFIG_PREEMPT_NONE is not set CONFIG_PREEMPT_VOLUNTARY=y # CONFIG_PREEMPT_DESKTOP is not set # CONFIG_PREEMPT_RT is not set CONFIG_PREEMPT_SOFTIRQS=y # CONFIG_PREEMPT_HARDIRQS is not set # CONFIG_PREEMPT_BKL is not set CONFIG_CLASSIC_RCU=y Seems to work in the last config if I comment the spin_lock spin_unlock call. /* * 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); */ It is not readable because I can't compile it with debugging information (poor memory system) 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? In the complete preemption yes. Which question did you answer yes to? That it's worse than before or that the interrupt handler runs in hardirq context (i.e. IRQF_NODELAY)? The interrupt handler run in IRQF_NODELAY context. I think you're right. Can you change it and see if it helps? I just change it because I have corruption on receiving buffer. All my test are done with this fix Ok. 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. I just test it I don't have buffer overflow. No, I'd expect your allocation fix to take care of that. Or did you by any chance test without the fix and with slub_debug enabled? I just meant that the buffer never fills up to its size. I protect with a spinlock the access to the register when we sending from the tasklet. It is correct? I have no idea. Could you post some more specifics about what you modified, for example a diff? ... /* The interrupt handler does not take the lock */ spin_lock_irqsave(port-lock, flags); atmel_tx_chars(port); spin_unlock_irqrestore(port-lock, flags); spin_lock(port-lock); ... The atmel_tx_chars using the serial device registers like the interrupt routine and so I think that it is possible to have interference during send operation. Most of the tasklet is already protected by the spinlock, so you must be careful to avoid any lock recursion. Haavard Regards Michael -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
On Wed, 30 Jan 2008 16:26:27 +0100 michael [EMAIL PROTECTED] wrote: I have no idea. Could you post some more specifics about what you modified, for example a diff? ... /* The interrupt handler does not take the lock */ spin_lock_irqsave(port-lock, flags); atmel_tx_chars(port); spin_unlock_irqrestore(port-lock, flags); Sorry, this isn't going to work. Please post a diff with the changes you did to the driver, and whatever output you got when it crashed. It's really difficult to help you when I don't know (a) what code you're actually running, or (b) anything about the crash. The atmel_tx_chars using the serial device registers like the interrupt routine and so I think that it is possible to have interference during send operation. No, it's only called from the tasklet, and the interrupt handler doesn't touch the TX data register. There shouldn't be any need to disable interrupts around the call to atmel_tx_chars(). In fact, this may very well be the cause of the overruns you're seeing. 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi, Haavard Skinnemoen wrote: On Wed, 30 Jan 2008 16:26:27 +0100 michael [EMAIL PROTECTED] wrote: I have no idea. Could you post some more specifics about what you modified, for example a diff? ... /* The interrupt handler does not take the lock */ spin_lock_irqsave(port-lock, flags); atmel_tx_chars(port); spin_unlock_irqrestore(port-lock, flags); Sorry, this isn't going to work. Please post a diff with the changes you did to the driver, and whatever output you got when it crashed. It's really difficult to help you when I don't know (a) what code you're actually running, or (b) anything about the crash. Ok, but the problem is that I have some added code for using the uart with smart card in iso mode, (is never called) and the patch is not so clean. Now I return to the original patch without the spin_lock_irqsave and with the fix of buffer allocation,and I don't see the crash anymore. In full preemptive all works with threading hardirqs and sofirqs. I will do other testing before posting again. The atmel_tx_chars using the serial device registers like the interrupt routine and so I think that it is possible to have interference during send operation. No, it's only called from the tasklet, and the interrupt handler doesn't touch the TX data register. There shouldn't be any need to disable interrupts around the call to atmel_tx_chars(). In fact, this may very well be the cause of the overruns you're seeing. Haavard The overrun still remain. An lrz receive session is impossible using full preemption. I will try the dma patch too and test in iso mode for smart card. Thanks -- 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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi,
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.
+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 = _port->rx_ring;
+ struct atmel_uart_char *c;
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)
/*
* Drop the lock here since it might end up calling
* uart_start(), which takes the lock.
spin_unlock(>lock);
*/
tty_flip_buffer_push(port->info->tty);
/*
spin_lock(>lock);
*/
The same code with this comments out runs
Complete Preemption (Real-Time) ok but the serials is just unusable due
to too many overruns (just using lrz)
The system is stable and doesn't crash reverting this patch.
I don't test with the thread hardirqs active.
+
+ 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);
+}
+
...
+
port = _ports[pdev->id];
atmel_init_port(port, pdev);
+ 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(_uart, >uart);
Is the kmalloc correct?
maybe:
data = kmalloc(ATMEL_SERIAL_RINGSIZE * sizeof(struct atmel_uart_char),
GFP_KERNEL);
if (ret)
goto err_add_port;
@@ -1013,6 +1142,9 @@ static int __devinit atmel_serial_probe(struct
platform_device *pdev)
return 0;
err_add_port:
+ kfree(port->rx_ring.buf);
+ port->rx_ring.buf = NULL;
+err_alloc_ring:
if (!atmel_is_console_port(>uart)) {
clk_disable(port->clk);
clk_put(port->clk);
@@ -1033,6 +1165,9 @@ static int __devexit atmel_serial_remove(struct
platform_device *pdev)
ret = uart_remove_one_port(_uart, port);
+ tasklet_kill(_port->tasklet);
+ kfree(atmel_port->rx_ring.buf);
+
Why the tasklet_kill is not done in atmel_shutdown?
Regards
Michael
--
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 -mm v4 6/9] atmel_serial: Split the interrupt handler
Hi,
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.
+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;
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)
/*
* 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
Complete Preemption (Real-Time) ok but the serials is just unusable due
to too many overruns (just using lrz)
The system is stable and doesn't crash reverting this patch.
I don't test with the thread hardirqs active.
+
+ 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);
+}
+
...
+
port = atmel_ports[pdev-id];
atmel_init_port(port, pdev);
+ 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);
if (ret)
goto err_add_port;
@@ -1013,6 +1142,9 @@ static int __devinit atmel_serial_probe(struct
platform_device *pdev)
return 0;
err_add_port:
+ kfree(port-rx_ring.buf);
+ port-rx_ring.buf = NULL;
+err_alloc_ring:
if (!atmel_is_console_port(port-uart)) {
clk_disable(port-clk);
clk_put(port-clk);
@@ -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?
Regards
Michael
--
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/
