[PATCH] Clear previous interrupts after fifo is disabled

[PATCH] Clear previous interrupts after fifo is disabled

[Chanho Min]

This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
with additional analysis.Bootloader can transfer control to kernel and
there are some pending interrupts. In this case, RXFE of the flag
register is set by clearing FEN(LCRH) even if rx data remains in the
fifo. It seems that the fifo's status is initiailized. Interrupt
handler can not get any data from data register because of the below
break condtion.

pl011_fifo_to_tty
 while (max_count--) {
   if (status & UART01x_FR_RXFE)
	break;

Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
hang. If we don't guarantee that no interrupt is pended until fifo is
disabled by calling 'writew(0, uap->port.membase + uap->lcrh_rx)',
this misbehave of the interrupt handelr can be occurred. So, All
pending interrupts should be cleared just after fifo is disabled under
the protection from interrupt. Also,'clear error interrupts' routine
can be removed becuase all interrupts are cleared before.

Signed-off-by: Chanho Min <chanho.min@lge.com>
---
 drivers/tty/serial/amba-pl011.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..8b5a824 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1397,7 +1397,11 @@ static int pl011_startup(struct uart_port *port)
 	writew(cr, uap->port.membase + UART011_CR);
 	writew(0, uap->port.membase + UART011_FBRD);
 	writew(1, uap->port.membase + UART011_IBRD);
+	spin_lock_irq(&uap->port.lock);
 	writew(0, uap->port.membase + uap->lcrh_rx);
+	/* Clear all pending interrupts. */
+	writew(0xffff, uap->port.membase + UART011_ICR);
+	spin_unlock_irq(&uap->port.lock);
 	if (uap->lcrh_tx != uap->lcrh_rx) {
 		int i;
 		/*
@@ -1417,10 +1421,6 @@ static int pl011_startup(struct uart_port *port)
 	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);

-	/* Clear pending error interrupts */
-	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
-	       uap->port.membase + UART011_ICR);
-
 	/*
 	 * initialise the old status of the modem signals
 	 */
-- 
1.7.0.4

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Linus Walleij]

On Mon, Feb 27, 2012 at 10:30 AM, Chanho Min <chanho0207@gmail.com> wrote:

> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
> with additional analysis.Bootloader can transfer control to kernel and
> there are some pending interrupts. In this case, RXFE of the flag
> register is set by clearing FEN(LCRH) even if rx data remains in the
> fifo. It seems that the fifo's status is initiailized. Interrupt
> handler can not get any data from data register because of the below
> break condtion.
>
> pl011_fifo_to_tty
>  while (max_count--) {
>   if (status & UART01x_FR_RXFE)
>        break;
>
> Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
> hang. If we don't guarantee that no interrupt is pended until fifo is
> disabled by calling 'writew(0, uap->port.membase + uap->lcrh_rx)',
> this misbehave of the interrupt handelr can be occurred. So, All
> pending interrupts should be cleared just after fifo is disabled under
> the protection from interrupt. Also,'clear error interrupts' routine
> can be removed becuase all interrupts are cleared before.
>
> Signed-off-by: Chanho Min <chanho.min@lge.com>

Looks correct to me.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Linus Walleij]

On Mon, Feb 27, 2012 at 10:30 AM, Chanho Min <chanho0207@gmail.com> wrote:

> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
> with additional analysis.Bootloader can transfer control to kernel and
> there are some pending interrupts. In this case, RXFE of the flag
> register is set by clearing FEN(LCRH) even if rx data remains in the
> fifo. It seems that the fifo's status is initiailized. Interrupt
> handler can not get any data from data register because of the below
> break condtion.
>
> pl011_fifo_to_tty
>  while (max_count--) {
>   if (status & UART01x_FR_RXFE)
>        break;
>
> Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
> hang. If we don't guarantee that no interrupt is pended until fifo is
> disabled by calling 'writew(0, uap->port.membase + uap->lcrh_rx)',
> this misbehave of the interrupt handelr can be occurred. So, All
> pending interrupts should be cleared just after fifo is disabled under
> the protection from interrupt. Also,'clear error interrupts' routine
> can be removed becuase all interrupts are cleared before.
>
> Signed-off-by: Chanho Min <chanho.min@lge.com>

Looks correct to me.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Russell King - ARM Linux]

On Mon, Feb 27, 2012 at 06:30:20PM +0900, Chanho Min wrote:
> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
> with additional analysis.Bootloader can transfer control to kernel and
> there are some pending interrupts. In this case, RXFE of the flag
> register is set by clearing FEN(LCRH) even if rx data remains in the
> fifo. It seems that the fifo's status is initiailized. Interrupt
> handler can not get any data from data register because of the below
> break condtion.
> 
> pl011_fifo_to_tty
>  while (max_count--) {
>    if (status & UART01x_FR_RXFE)
> 	break;
> 
> Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
> hang. If we don't guarantee that no interrupt is pended until fifo is
> disabled by calling 'writew(0, uap->port.membase + uap->lcrh_rx)',
> this misbehave of the interrupt handelr can be occurred. So, All
> pending interrupts should be cleared just after fifo is disabled under
> the protection from interrupt. Also,'clear error interrupts' routine
> can be removed becuase all interrupts are cleared before.

I'd much prefer to only clear those interrupts which actually need to be
cleared at this point.  So, I'd suggest this approach instead:

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..a13a825 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1381,6 +1381,10 @@ static int pl011_startup(struct uart_port *port)
 
 	uap->port.uartclk = clk_get_rate(uap->clk);
 
+	/* Clear pending error and receive interrupts */
+	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+
 	/*
 	 * Allocate the IRQ
 	 */
@@ -1417,10 +1421,6 @@ static int pl011_startup(struct uart_port *port)
 	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);
 
-	/* Clear pending error interrupts */
-	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
-	       uap->port.membase + UART011_ICR);
-
 	/*
 	 * initialise the old status of the modem signals
 	 */

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Russell King - ARM Linux]

On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
> I'd much prefer to only clear those interrupts which actually need to be
> cleared at this point.  So, I'd suggest this approach instead:

Thinking about this a little more, we definitely want to mask and clear
interrupts at probe time as well:

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..6b781bd 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1381,6 +1381,10 @@ static int pl011_startup(struct uart_port *port)
 
 	uap->port.uartclk = clk_get_rate(uap->clk);
 
+	/* Clear pending error and receive interrupts */
+	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+
 	/*
 	 * Allocate the IRQ
 	 */
@@ -1417,10 +1421,6 @@ static int pl011_startup(struct uart_port *port)
 	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);
 
-	/* Clear pending error interrupts */
-	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
-	       uap->port.membase + UART011_ICR);
-
 	/*
 	 * initialise the old status of the modem signals
 	 */
@@ -1927,6 +1927,10 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 		goto unmap;
 	}
 
+	/* Ensure interrupts from this UART are masked and cleared */
+	writew(0, uap->port.membase + UART011_IMSC);
+	writew(0xffff, uap->port.membase + UART011_ICR);
+
 	uap->vendor = vendor;
 	uap->lcrh_rx = vendor->lcrh_rx;
 	uap->lcrh_tx = vendor->lcrh_tx;

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Linus Walleij]

On Mon, Feb 27, 2012 at 12:02 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
>> I'd much prefer to only clear those interrupts which actually need to be
>> cleared at this point.  So, I'd suggest this approach instead:
>
> Thinking about this a little more, we definitely want to mask and clear
> interrupts at probe time as well:

Acked-by: Linus Walleij <linus.walleij@linaro.org>

On the RMK-improved patch.

Thanks,
Linus Walleij

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Chanho Min]

On Mon, Feb 27, 2012 at 8:02 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
>> I'd much prefer to only clear those interrupts which actually need to be
>> cleared at this point.  So, I'd suggest this approach instead:
>
> Thinking about this a little more, we definitely want to mask and clear
> interrupts at probe time as well:
>
I'm not satisfied with this completely. RIS has some pending
interrupts even if interrupts are masked/disabled in IMSC. If your
patch is applied, interrupt can be pended as bellows and RXFE of the
flag register is set as well.

writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
...
Interrupt is occured and pended in RIS
..
writew(uap->im, uap->port.membase + UART011_IMSC);

Root cause is that Rx interrupt set but Rx fifo is empty. If we just
remove the sentence for clearing LCRH, nothing happens and interrupt
handler don't this misbehave. Also I don't fully understand why we
need to clear interrupts at probe time. If we prefer to only clear
those interrupts which actually need to be cleared, this is the
improved patch.

Thanks,

Signed-off-by: Chanho Min <chanho.min@lge.com>
---
 drivers/tty/serial/amba-pl011.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..96d1828 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1397,7 +1397,12 @@ static int pl011_startup(struct uart_port *port)
 	writew(cr, uap->port.membase + UART011_CR);
 	writew(0, uap->port.membase + UART011_FBRD);
 	writew(1, uap->port.membase + UART011_IBRD);
+	spin_lock_irq(&uap->port.lock);
 	writew(0, uap->port.membase + uap->lcrh_rx);
+	/* Clear pending error and receive interrupts */
+	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+		UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+	spin_unlock_irq(&uap->port.lock);
 	if (uap->lcrh_tx != uap->lcrh_rx) {
 		int i;
 		/*
@@ -1417,10 +1422,6 @@ static int pl011_startup(struct uart_port *port)
 	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);

-	/* Clear pending error interrupts */
-	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
-	       uap->port.membase + UART011_ICR);
-
 	/*
 	 * initialise the old status of the modem signals
 	 */
-- 
1.7.0.4

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Russell King - ARM Linux]

On Tue, Feb 28, 2012 at 10:35:30AM +0900, Chanho Min wrote:
> On Mon, Feb 27, 2012 at 8:02 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
> >> I'd much prefer to only clear those interrupts which actually need to be
> >> cleared at this point.  So, I'd suggest this approach instead:
> >
> > Thinking about this a little more, we definitely want to mask and clear
> > interrupts at probe time as well:
> >
> I'm not satisfied with this completely. RIS has some pending
> interrupts even if interrupts are masked/disabled in IMSC. If your
> patch is applied, interrupt can be pended as bellows and RXFE of the
> flag register is set as well.

RXFE _will_ be set.  Think about it - RXFE means Receive Fifo Empty.
If the receive fifo is empty, it _will_ be set.

And RIS is the _Raw_ interrupt status.  That's the status _before_ the
mask is acted upon.

> writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
>        UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
> ...
> Interrupt is occured and pended in RIS

But it won't be delivered because the mask register is zero.

> ..
> writew(uap->im, uap->port.membase + UART011_IMSC);
> 
> Root cause is that Rx interrupt set but Rx fifo is empty. If we just
> remove the sentence for clearing LCRH, nothing happens and interrupt
> handler don't this misbehave.

No.

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Russell King - ARM Linux]

On Tue, Feb 28, 2012 at 10:35:30AM +0900, Chanho Min wrote:
> On Mon, Feb 27, 2012 at 8:02 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Feb 27, 2012 at 10:48:58AM +0000, Russell King - ARM Linux wrote:
> >> I'd much prefer to only clear those interrupts which actually need to be
> >> cleared at this point.  So, I'd suggest this approach instead:
> >
> > Thinking about this a little more, we definitely want to mask and clear
> > interrupts at probe time as well:
> >
> I'm not satisfied with this completely. RIS has some pending
> interrupts even if interrupts are masked/disabled in IMSC. If your
> patch is applied, interrupt can be pended as bellows and RXFE of the
> flag register is set as well.

RXFE _will_ be set.  Think about it - RXFE means Receive Fifo Empty.
If the receive fifo is empty, it _will_ be set.

And RIS is the _Raw_ interrupt status.  That's the status _before_ the
mask is acted upon.

> writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
>        UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
> ...
> Interrupt is occured and pended in RIS

But it won't be delivered because the mask register is zero.

> ..
> writew(uap->im, uap->port.membase + UART011_IMSC);
> 
> Root cause is that Rx interrupt set but Rx fifo is empty. If we just
> remove the sentence for clearing LCRH, nothing happens and interrupt
> handler don't this misbehave.

No.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Chanho Min]

> RXFE _will_ be set.  Think about it - RXFE means Receive Fifo Empty.
> If the receive fifo is empty, it _will_ be set.
I know meaning of the RXFE. I also don't understand why RXFE is set by
clearing FEN. We checked this by bellow debug codes.

 fr_before = readw(uap->port.membase + UART01x_FR);
 writew(0, uap->port.membase + uap->lcrh_rx);
 fr_after = readw(uap->port.membase + UART01x_FR);

If rx interrupt is ocurred before, fr_after becomes 0x90 but fr_before is 0x80.

> And RIS is the _Raw_ interrupt status.  That's the status _before_ the
> mask is acted upon.
>
> But it won't be delivered because the mask register is zero.
It can be delivered just after mask register is set to 1.

>> Root cause is that Rx interrupt set but Rx fifo is empty. If we just
>> remove the sentence for clearing LCRH, nothing happens and interrupt
>> handler don't this misbehave.
>
> No.
When we just removed the sentence for clearing LCRH, this hangup doesn't happen.

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Chanho Min]

> RXFE _will_ be set.  Think about it - RXFE means Receive Fifo Empty.
> If the receive fifo is empty, it _will_ be set.
I know meaning of the RXFE. I also don't understand why RXFE is set by
clearing FEN. We checked this by bellow debug codes.

 fr_before = readw(uap->port.membase + UART01x_FR);
 writew(0, uap->port.membase + uap->lcrh_rx);
 fr_after = readw(uap->port.membase + UART01x_FR);

If rx interrupt is ocurred before, fr_after becomes 0x90 but fr_before is 0x80.

> And RIS is the _Raw_ interrupt status.  That's the status _before_ the
> mask is acted upon.
>
> But it won't be delivered because the mask register is zero.
It can be delivered just after mask register is set to 1.

>> Root cause is that Rx interrupt set but Rx fifo is empty. If we just
>> remove the sentence for clearing LCRH, nothing happens and interrupt
>> handler don't this misbehave.
>
> No.
When we just removed the sentence for clearing LCRH, this hangup doesn't happen.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Russell King - ARM Linux]

On Tue, Feb 28, 2012 at 06:16:10PM +0900, Chanho Min wrote:
> > RXFE _will_ be set.  Think about it - RXFE means Receive Fifo Empty.
> > If the receive fifo is empty, it _will_ be set.
> I know meaning of the RXFE. I also don't understand why RXFE is set by
> clearing FEN. We checked this by bellow debug codes.
> 
>  fr_before = readw(uap->port.membase + UART01x_FR);
>  writew(0, uap->port.membase + uap->lcrh_rx);
>  fr_after = readw(uap->port.membase + UART01x_FR);
> 
> If rx interrupt is ocurred before, fr_after becomes 0x90 but fr_before is 0x80.

Because the flags are manipulated to give the illusion of a one byte
FIFO, as stated in the TRM.

> > And RIS is the _Raw_ interrupt status.  That's the status _before_ the
> > mask is acted upon.
> >
> > But it won't be delivered because the mask register is zero.
> It can be delivered just after mask register is set to 1.

And we don't set the mask register to 1 until later.

> >> Root cause is that Rx interrupt set but Rx fifo is empty. If we just
> >> remove the sentence for clearing LCRH, nothing happens and interrupt
> >> handler don't this misbehave.
> >
> > No.
> When we just removed the sentence for clearing LCRH, this hangup doesn't
> happen.

But we want to do the transmit interrupt provocation with the FIFO disabled.

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Chanho Min]

> Because the flags are manipulated to give the illusion of a one byte
> FIFO, as stated in the TRM.
Yes. It is the problem that rx interrupt is pended with this status as
I mentioned.

> And we don't set the mask register to 1 until later.
In the last part of startup, set to 1. Interrupt can be occurred just after it.

uap->im = UART011_RTIM;
if (!pl011_dma_rx_running(uap))
 uap->im |= UART011_RXIM;
 writew(uap->im, uap->port.membase + UART011_IMSC);

> But we want to do the transmit interrupt provocation with the FIFO disabled.
I know. It's test only.

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Russell King - ARM Linux]

On Tue, Feb 28, 2012 at 06:46:12PM +0900, Chanho Min wrote:
> > Because the flags are manipulated to give the illusion of a one byte
> > FIFO, as stated in the TRM.
> Yes. It is the problem that rx interrupt is pended with this status as
> I mentioned.

Which is why my patch explicitly clears the receive interrupt status
before requesting the interrupt.  Have you read my patch?
 
> > And we don't set the mask register to 1 until later.
> In the last part of startup, set to 1. Interrupt can be occurred just
> after it.
> 
> uap->im = UART011_RTIM;
> if (!pl011_dma_rx_running(uap))
>  uap->im |= UART011_RXIM;
>  writew(uap->im, uap->port.membase + UART011_IMSC);
> 
> > But we want to do the transmit interrupt provocation with the FIFO disabled.
> I know. It's test only.

Wrong, it's fundamental to the UARTs operation.

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Chanho Min]

> Which is why my patch explicitly clears the receive interrupt status
> before requesting the interrupt.  Have you read my patch?
This is the hang-up scenario with your patch.

pl011_startup(struct uart_port *port)
 	uap->port.uartclk = clk_get_rate(uap->clk);

	/* Clear pending error and receive interrupts */
	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
...
1. RX Interrupt is occurred after interrupt is cleared. Even if
interrupt is masked/disabled before(or probe time), RIS's Rx interrupt
is set to 1. Of course, masked status is zero.
...
2. RXFE of flag register is zero and fifo is not empty before LCRH is cleared.
	writew(0, uap->port.membase + uap->lcrh_rx);
3. RXFE of flag register is changed to '1'  after LCRH is cleared. but
the fifo is not actually empty.
...
4. Finally, We enable interrupts.
	spin_lock_irq(&uap->port.lock);
	uap->im = UART011_RTIM;
	if (!pl011_dma_rx_running(uap))
		uap->im |= UART011_RXIM;
	writew(uap->im, uap->port.membase + UART011_IMSC);
	spin_unlock_irq(&uap->port.lock);
5. The RIS's field which is enabled by IMSC is reflected to MIS as
soon as the interrupt enable. (We checked this on our ARM platform )
6. IRQ context is started. pl011_fifo_to_tty is called by pl011_int.
static int pl011_fifo_to_tty(struct uart_amba_port *uap)
...
	while (max_count--) {
		status = readw(uap->port.membase + UART01x_FR);
		if (status & UART01x_FR_RXFE)
			break;
...
7. pl011_fifo_to_tty can't read any data from DR because of the break
condition for RXFE. Rx interrupt can't be cleared. cpu is looping in
irq context.

This is why we need to be cleared just after LCRH is cleared not
before irq_request. Let's get back to my patch. Even if data is
received before or after interrupt is cleared, flag register will show
actual fifo status. Interrupt handler runs normally after the uart
operation is started up by enabling interrupt.

+       spin_lock_irq(&uap->port.lock);
        writew(0, uap->port.membase + uap->lcrh_rx);
+       /* Clear pending error and receive interrupts */
+       writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+               UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+       spin_unlock_irq(&uap->port.lock);

Thanks,
Chanho Min

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Chanho Min]

> Which is why my patch explicitly clears the receive interrupt status
> before requesting the interrupt.  Have you read my patch?
This is the hang-up scenario with your patch.

pl011_startup(struct uart_port *port)
 	uap->port.uartclk = clk_get_rate(uap->clk);

	/* Clear pending error and receive interrupts */
	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
...
1. RX Interrupt is occurred after interrupt is cleared. Even if
interrupt is masked/disabled before(or probe time), RIS's Rx interrupt
is set to 1. Of course, masked status is zero.
...
2. RXFE of flag register is zero and fifo is not empty before LCRH is cleared.
	writew(0, uap->port.membase + uap->lcrh_rx);
3. RXFE of flag register is changed to '1'  after LCRH is cleared. but
the fifo is not actually empty.
...
4. Finally, We enable interrupts.
	spin_lock_irq(&uap->port.lock);
	uap->im = UART011_RTIM;
	if (!pl011_dma_rx_running(uap))
		uap->im |= UART011_RXIM;
	writew(uap->im, uap->port.membase + UART011_IMSC);
	spin_unlock_irq(&uap->port.lock);
5. The RIS's field which is enabled by IMSC is reflected to MIS as
soon as the interrupt enable. (We checked this on our ARM platform )
6. IRQ context is started. pl011_fifo_to_tty is called by pl011_int.
static int pl011_fifo_to_tty(struct uart_amba_port *uap)
...
	while (max_count--) {
		status = readw(uap->port.membase + UART01x_FR);
		if (status & UART01x_FR_RXFE)
			break;
...
7. pl011_fifo_to_tty can't read any data from DR because of the break
condition for RXFE. Rx interrupt can't be cleared. cpu is looping in
irq context.

This is why we need to be cleared just after LCRH is cleared not
before irq_request. Let's get back to my patch. Even if data is
received before or after interrupt is cleared, flag register will show
actual fifo status. Interrupt handler runs normally after the uart
operation is started up by enabling interrupt.

+       spin_lock_irq(&uap->port.lock);
        writew(0, uap->port.membase + uap->lcrh_rx);
+       /* Clear pending error and receive interrupts */
+       writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+               UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+       spin_unlock_irq(&uap->port.lock);

Thanks,
Chanho Min
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Clear previous interrupts after fifo is disabled

[Kim, Jong-Sung]

> -----Original Message-----
> From: Chanho Min [mailto:chanho0207@gmail.com]
> Sent: Monday, February 27, 2012 6:30 PM
> To: Russell King; Alan Cox; Greg Kroah-Hartman; Linus Walleij; Shreshtha
> Kumar Sahu; Kim, Jong-Sung
> Cc: linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org
> Subject: [PATCH] Clear previous interrupts after fifo is disabled
> 
> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
> with additional analysis.Bootloader can transfer control to kernel and
there
> are some pending interrupts. In this case, RXFE of the flag register is
set
> by clearing FEN(LCRH) even if rx data remains in the fifo. It seems that
the
> fifo's status is initiailized. Interrupt handler can not get any data from
> data register because of the below break condtion.
> 
> pl011_fifo_to_tty
>  while (max_count--) {
>    if (status & UART01x_FR_RXFE)
> 	break;
> 
> Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
hang.
> If we don't guarantee that no interrupt is pended until fifo is disabled
by
> calling 'writew(0, uap->port.membase + uap->lcrh_rx)', this misbehave of
the
> interrupt handelr can be occurred. So, All pending interrupts should be
> cleared just after fifo is disabled under the protection from interrupt.
> Also,'clear error interrupts' routine can be removed becuase all
interrupts
> are cleared before.
> 
> Signed-off-by: Chanho Min <chanho.min@lge.com>

May I suggest another approach at this point? The problematic condition you
reported could be considered as an exceptional Rx interrupt status. So, we
can handle it in the Rx ISR. Simply:

diff --git a/drivers/tty/serial/amba-pl011.c
b/drivers/tty/serial/amba-pl011.c
index 6800f5f..5b5358705 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -224,6 +224,10 @@ static int pl011_fifo_to_tty(struct uart_amba_port
*uap)
                uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
        }
 
+       /* RXIS but RXFE? Just clear the interrupt */
+       if(unlikely(fifotaken == 0))
+               writew(UART011_RXIS, uap->port.membase + UART01x_ICR);
+
        return fifotaken;
 }

RE: [PATCH] Clear previous interrupts after fifo is disabled

[Kim, Jong-Sung]

> -----Original Message-----
> From: Chanho Min [mailto:chanho0207@gmail.com]
> Sent: Monday, February 27, 2012 6:30 PM
> To: Russell King; Alan Cox; Greg Kroah-Hartman; Linus Walleij; Shreshtha
> Kumar Sahu; Kim, Jong-Sung
> Cc: linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org
> Subject: [PATCH] Clear previous interrupts after fifo is disabled
> 
> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
> with additional analysis.Bootloader can transfer control to kernel and
there
> are some pending interrupts. In this case, RXFE of the flag register is
set
> by clearing FEN(LCRH) even if rx data remains in the fifo. It seems that
the
> fifo's status is initiailized. Interrupt handler can not get any data from
> data register because of the below break condtion.
> 
> pl011_fifo_to_tty
>  while (max_count--) {
>    if (status & UART01x_FR_RXFE)
> 	break;
> 
> Then, Rx interrupt is never cleared. cpu is looping in ISR. System is
hang.
> If we don't guarantee that no interrupt is pended until fifo is disabled
by
> calling 'writew(0, uap->port.membase + uap->lcrh_rx)', this misbehave of
the
> interrupt handelr can be occurred. So, All pending interrupts should be
> cleared just after fifo is disabled under the protection from interrupt.
> Also,'clear error interrupts' routine can be removed becuase all
interrupts
> are cleared before.
> 
> Signed-off-by: Chanho Min <chanho.min@lge.com>

May I suggest another approach at this point? The problematic condition you
reported could be considered as an exceptional Rx interrupt status. So, we
can handle it in the Rx ISR. Simply:

diff --git a/drivers/tty/serial/amba-pl011.c
b/drivers/tty/serial/amba-pl011.c
index 6800f5f..5b5358705 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -224,6 +224,10 @@ static int pl011_fifo_to_tty(struct uart_amba_port
*uap)
                uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
        }
 
+       /* RXIS but RXFE? Just clear the interrupt */
+       if(unlikely(fifotaken == 0))
+               writew(UART011_RXIS, uap->port.membase + UART01x_ICR);
+
        return fifotaken;
 }

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Greg Kroah-Hartman]

On Mon, Feb 27, 2012 at 06:30:20PM +0900, Chanho Min wrote:
> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
> with additional analysis.Bootloader can transfer control to kernel and
> there are some pending interrupts. In this case, RXFE of the flag
> register is set by clearing FEN(LCRH) even if rx data remains in the
> fifo. It seems that the fifo's status is initiailized. Interrupt
> handler can not get any data from data register because of the below
> break condtion.
> 
> pl011_fifo_to_tty
>  while (max_count--) {
>    if (status & UART01x_FR_RXFE)
> 	break;

This patch never seemed to be agreed on, so I'm not taking it.

Can someone, if this is still needed, and everyone agrees on how to
solve it, please send me the needed fix?

thanks,

greg k-h

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Linus Walleij]

On Thu, Mar 8, 2012 at 7:49 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Feb 27, 2012 at 06:30:20PM +0900, Chanho Min wrote:
>> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
>> with additional analysis.Bootloader can transfer control to kernel and
>> there are some pending interrupts. In this case, RXFE of the flag
>> register is set by clearing FEN(LCRH) even if rx data remains in the
>> fifo. It seems that the fifo's status is initiailized. Interrupt
>> handler can not get any data from data register because of the below
>> break condtion.
>>
>> pl011_fifo_to_tty
>>  while (max_count--) {
>>    if (status & UART01x_FR_RXFE)
>>       break;
>
> This patch never seemed to be agreed on, so I'm not taking it.
>
> Can someone, if this is still needed, and everyone agrees on how to
> solve it, please send me the needed fix?

To me it seems Russell's patch solves part of the problem,
and Jong-Sung Kim's patch on top of that solves the entire
problem, but Chanho need to come back and tell whether
this is the case in practice.

Yours,
Linus Walleij

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Linus Walleij]

On Thu, Mar 8, 2012 at 7:49 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Feb 27, 2012 at 06:30:20PM +0900, Chanho Min wrote:
>> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
>> with additional analysis.Bootloader can transfer control to kernel and
>> there are some pending interrupts. In this case, RXFE of the flag
>> register is set by clearing FEN(LCRH) even if rx data remains in the
>> fifo. It seems that the fifo's status is initiailized. Interrupt
>> handler can not get any data from data register because of the below
>> break condtion.
>>
>> pl011_fifo_to_tty
>>  while (max_count--) {
>>    if (status & UART01x_FR_RXFE)
>>       break;
>
> This patch never seemed to be agreed on, so I'm not taking it.
>
> Can someone, if this is still needed, and everyone agrees on how to
> solve it, please send me the needed fix?

To me it seems Russell's patch solves part of the problem,
and Jong-Sung Kim's patch on top of that solves the entire
problem, but Chanho need to come back and tell whether
this is the case in practice.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Greg Kroah-Hartman]

On Fri, Mar 09, 2012 at 05:34:03PM +0100, Linus Walleij wrote:
> On Thu, Mar 8, 2012 at 7:49 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Feb 27, 2012 at 06:30:20PM +0900, Chanho Min wrote:
> >> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
> >> with additional analysis.Bootloader can transfer control to kernel and
> >> there are some pending interrupts. In this case, RXFE of the flag
> >> register is set by clearing FEN(LCRH) even if rx data remains in the
> >> fifo. It seems that the fifo's status is initiailized. Interrupt
> >> handler can not get any data from data register because of the below
> >> break condtion.
> >>
> >> pl011_fifo_to_tty
> >>  while (max_count--) {
> >>    if (status & UART01x_FR_RXFE)
> >>       break;
> >
> > This patch never seemed to be agreed on, so I'm not taking it.
> >
> > Can someone, if this is still needed, and everyone agrees on how to
> > solve it, please send me the needed fix?
> 
> To me it seems Russell's patch solves part of the problem,
> and Jong-Sung Kim's patch on top of that solves the entire
> problem, but Chanho need to come back and tell whether
> this is the case in practice.

Ok, then, once it is tested, can someone resend them to me?

thanks,

greg k-h

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Greg Kroah-Hartman]

On Fri, Mar 09, 2012 at 05:34:03PM +0100, Linus Walleij wrote:
> On Thu, Mar 8, 2012 at 7:49 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Feb 27, 2012 at 06:30:20PM +0900, Chanho Min wrote:
> >> This is another workaroud of  'https://lkml.org/lkml/2012/1/17/104'
> >> with additional analysis.Bootloader can transfer control to kernel and
> >> there are some pending interrupts. In this case, RXFE of the flag
> >> register is set by clearing FEN(LCRH) even if rx data remains in the
> >> fifo. It seems that the fifo's status is initiailized. Interrupt
> >> handler can not get any data from data register because of the below
> >> break condtion.
> >>
> >> pl011_fifo_to_tty
> >>  while (max_count--) {
> >>    if (status & UART01x_FR_RXFE)
> >>       break;
> >
> > This patch never seemed to be agreed on, so I'm not taking it.
> >
> > Can someone, if this is still needed, and everyone agrees on how to
> > solve it, please send me the needed fix?
> 
> To me it seems Russell's patch solves part of the problem,
> and Jong-Sung Kim's patch on top of that solves the entire
> problem, but Chanho need to come back and tell whether
> this is the case in practice.

Ok, then, once it is tested, can someone resend them to me?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Chanho Min]

>> To me it seems Russell's patch solves part of the problem,
>> and Jong-Sung Kim's patch on top of that solves the entire
>> problem, but Chanho need to come back and tell whether
>> this is the case in practice.
>
> Ok, then, once it is tested, can someone resend them to me?
>
> thanks,
>
> greg k-h

I checked that Jong-Sung Kim's patch solved this hang-up issue
and agree on it. But, RTIS seems to be cleared as well.

RE: [PATCH] Clear previous interrupts after fifo is disabled

[Kim, Jong-Sung]

> -----Original Message-----
> From: Chanho Min [mailto:chanho0207@gmail.com]
> Sent: Saturday, March 10, 2012 11:15 AM
> To: Greg Kroah-Hartman
> Cc: Linus Walleij; Russell King; Alan Cox; Shreshtha Kumar Sahu; Kim,
Jong-
> Sung; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org
> Subject: Re: [PATCH] Clear previous interrupts after fifo is disabled
> 
> >> To me it seems Russell's patch solves part of the problem, and
> >> Jong-Sung Kim's patch on top of that solves the entire problem, but
> >> Chanho need to come back and tell whether this is the case in
> >> practice.
> >
> > Ok, then, once it is tested, can someone resend them to me?
> >
> > thanks,
> >
> > greg k-h
> 
> I checked that Jong-Sung Kim's patch solved this hang-up issue and agree
on
> it. But, RTIS seems to be cleared as well.

You're right. RTIS should be cleared as well as RXIS. Revised patch:

diff --git a/drivers/tty/serial/amba-pl011.c
b/drivers/tty/serial/amba-pl011.c
index 6800f5f..39520db 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -224,6 +224,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port
*uap)
 		uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
 	}
 
+	/* RTIS and/or RXIS, but RXFE? Just clear the interrupt(s) */
+	if(unlikely(fifotaken == 0))
+		writew(UART011_RTIS | UART011_RXIS, uap->port.membase +
+		       UART011_ICR);
+
 	return fifotaken;
 }

RE: [PATCH] Clear previous interrupts after fifo is disabled

[Kim, Jong-Sung]

> -----Original Message-----
> From: Chanho Min [mailto:chanho0207@gmail.com]
> Sent: Saturday, March 10, 2012 11:15 AM
> To: Greg Kroah-Hartman
> Cc: Linus Walleij; Russell King; Alan Cox; Shreshtha Kumar Sahu; Kim,
Jong-
> Sung; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org
> Subject: Re: [PATCH] Clear previous interrupts after fifo is disabled
> 
> >> To me it seems Russell's patch solves part of the problem, and
> >> Jong-Sung Kim's patch on top of that solves the entire problem, but
> >> Chanho need to come back and tell whether this is the case in
> >> practice.
> >
> > Ok, then, once it is tested, can someone resend them to me?
> >
> > thanks,
> >
> > greg k-h
> 
> I checked that Jong-Sung Kim's patch solved this hang-up issue and agree
on
> it. But, RTIS seems to be cleared as well.

You're right. RTIS should be cleared as well as RXIS. Revised patch:

diff --git a/drivers/tty/serial/amba-pl011.c
b/drivers/tty/serial/amba-pl011.c
index 6800f5f..39520db 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -224,6 +224,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port
*uap)
 		uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
 	}
 
+	/* RTIS and/or RXIS, but RXFE? Just clear the interrupt(s) */
+	if(unlikely(fifotaken == 0))
+		writew(UART011_RTIS | UART011_RXIS, uap->port.membase +
+		       UART011_ICR);
+
 	return fifotaken;
 }

Re: [PATCH] Clear previous interrupts after fifo is disabled

[Linus Walleij]

On Thu, Mar 8, 2012 at 7:49 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:

> Can someone, if this is still needed, and everyone agrees on how to
> solve it, please send me the needed fix?

I've send a patch combining the probe() and startup() fixes from Russell
with the latest patch from Jong-Sung Kim into what may very well
be the silver bullet.
Subject "serial: PL011: clear pending interrupts"

Yours,
Linus Walleij