serial: omap: fix the overrun case

PATCH New Shubhrajyoti D 5 participants 10 messages Sep 21, 2012 TTY layer and serial drivers

Share & tools

Shubhrajyoti D

author

2012-09-21 10:22 UTC #0 lore raw

PATCH 1/1 serial: omap: fix the overrun case

Overrun also causes an internal flag to be set, which disables further reception. Before the next frame can
be received, the MPU must:
? Reset the RX FIFO.
? clear the internal flag.

In the uart mode a dummy read is needed. Add the same.

Signed-off-by Shubhrajyoti D <[email protected]>

---
- functional testing on omap4sdp
- Verified idle and suspend path hits off on beagle.

 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

drivers/tty/serial/omap-serial.c +7 -0

index a0d4460..bc22a2b 100644
@@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
 static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
 {
 	unsigned int flag;
+	unsigned char ch = 0;
+
+	if (!(lsr & UART_LSR_BRK_ERROR_BITS))
+		return;
+
+	if (likely(lsr & UART_LSR_DR))
+		ch = serial_in(up, UART_RX);
 
 	up->port.icount.rx++;
 	flag = TTY_NORMAL;

Signature · 1 line

1.7.5.4

Felipe Balbi

2012-09-21 11:00 UTC #1 lore raw

PATCH 1/1 serial: omap: fix the overrun case

Quoted 7 lines · Shubhrajyoti D

Overrun also causes an internal flag to be set, which disables further reception. Before the next frame can
be received, the MPU must:
? Reset the RX FIFO.
? clear the internal flag.
In the uart mode a dummy read is needed. Add the same.

Very nice patch but I think commit log can be a bit more verbose.

Please make the problem a little clearer. Why do we even get that interrupt fired if BRK_ERROR_BITS aren't set ?

Quoted 13 lines

Signed-off-by Shubhrajyoti D <[email protected]>
---
- functional testing on omap4sdp
- Verified idle and suspend path hits off on beagle.
 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
drivers/tty/serial/omap-serial.c +0 -0
index a0d4460..bc22a2b 100644

drivers/tty/serial/omap-serial.c static unsigned int check_modem_status(struct uart_omap_port *up) · line 343

Quoted 10 lines

drivers/tty/serial/omap-serial.c +7 -0

@@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
 static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
 {
 	unsigned int flag;
+	unsigned char ch = 0;
+
+	if (!(lsr & UART_LSR_BRK_ERROR_BITS))
+		return;
+
+	if (likely(lsr & UART_LSR_DR))
+		ch = serial_in(up, UART_RX);

Maybe add a comment before this condition stating why this character read is necessary ?

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120921/0dfb7196/attachment.sig>

Shubhrajyoti

author

2012-09-21 11:16 UTC #2 lore raw

PATCH 1/1 serial: omap: fix the overrun case

Quoted 9 lines · Felipe Balbi

Quoted 7 lines · Shubhrajyoti D
Overrun also causes an internal flag to be set, which disables further reception. Before the next frame can
be received, the MPU must:
? Reset the RX FIFO.
? clear the internal flag.
In the uart mode a dummy read is needed. Add the same.
Very nice patch but I think commit log can be a bit more verbose.

Quoted 2 lines

Please make the problem a little clearer. Why do we even get that interrupt fired if BRK_ERROR_BITS aren't set ?

I did not get this point.

it it is ! BRK_ERROR_BITS I return.

If it is and there is data in the fifo then a read is done.

Quoted 17 lines

Quoted 17 lines
Signed-off-by Shubhrajyoti D <[email protected]>
---
- functional testing on omap4sdp
- Verified idle and suspend path hits off on beagle.
 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
drivers/tty/serial/omap-serial.c +1 -0
index a0d4460..bc22a2b 100644
@@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
 static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
 {
 	unsigned int flag;
+	unsigned char ch = 0;

Quoted 8 lines

drivers/tty/serial/omap-serial.c static unsigned int check_modem_status(struct uart_omap_port *up) · line 343
Quoted 6 lines
drivers/tty/serial/omap-serial.c +6 -0
+
+	if (!(lsr & UART_LSR_BRK_ERROR_BITS))
+		return;
+
+	if (likely(lsr & UART_LSR_DR))
+		ch = serial_in(up, UART_RX);
Maybe add a comment before this condition stating why this character read is necessary ?

OK.

Quoted 1 line

Felipe Balbi

2012-09-21 11:35 UTC #3 lore raw

PATCH 1/1 serial: omap: fix the overrun case

Quoted 8 lines · Shubhrajyoti

Quoted 7 lines · Felipe Balbi
Quoted 6 lines · Shubhrajyoti D
Overrun also causes an internal flag to be set, which disables further reception. Before the next frame can
be received, the MPU must:
? Reset the RX FIFO.
? clear the internal flag.

Quoted 8 lines

Quoted 2 lines
Quoted 1 line
In the uart mode a dummy read is needed. Add the same.
Very nice patch but I think commit log can be a bit more verbose.
ok
Quoted 2 lines
Please make the problem a little clearer. Why do we even get that interrupt fired if BRK_ERROR_BITS aren't set ?
I did not get this point.
it it is ! BRK_ERROR_BITS I return.

That's what I mean. rlsi handler is basically taking care of those bits... So how come we get RLSI IRQ when those bits aren't set ?

Meaning, you shouldn't ever need that check, right ? Ideally, whenever that handler is called, it's because BRK_ERROR_BITS are set.

Maybe add a WARN_ONCE() kinda thing just to see if that will ever really happen ??

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120921/2690f140/attachment.sig>

Shubhrajyoti Datta

2012-09-21 11:48 UTC #4 lore raw

PATCH 1/1 serial: omap: fix the overrun case

Quoted 3 lines · Felipe Balbi

Quoted 2 lines · Shubhrajyoti
Quoted 1 line · Felipe Balbi
On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:

[...]

Quoted 10 lines

Quoted 1 line
it it is ! BRK_ERROR_BITS I return.
That's what I mean. rlsi handler is basically taking care of those bits... So how come we get RLSI IRQ when those bits aren't set ?
Meaning, you shouldn't ever need that check, right ? Ideally, whenever that handler is called, it's because BRK_ERROR_BITS are set.
Maybe add a WARN_ONCE() kinda thing just to see if that will ever really happen ??

hmm yes. will get back.

Quoted 3 lines

--
balbi

Poddar, Sourav

2012-09-21 11:39 UTC #5 lore raw

PATCH 1/1 serial: omap: fix the overrun case

In reply to Felipe Balbi

Hi Felipe,

Quoted 7 lines · Felipe Balbi

Quoted 6 lines · Shubhrajyoti D
Overrun also causes an internal flag to be set, which disables further reception. Before the next frame can
be received, the MPU must:
? Reset the RX FIFO.
? clear the internal flag.

Quoted 7 lines

Quoted 1 line
In the uart mode a dummy read is needed. Add the same.
Very nice patch but I think commit log can be a bit more verbose.
Please make the problem a little clearer. Why do we even get that interrupt fired if BRK_ERROR_BITS aren't set ?

According to LSR registers, there are few other bits like RX_FIFO_E( atleast 1 character in RX_FIFO or TX FIFO Empty), which might be the cause of an interrupt. ?

Quoted 29 lines

Quoted 13 lines
Signed-off-by Shubhrajyoti D <[email protected]>
---
- functional testing on omap4sdp
- Verified idle and suspend path hits off on beagle.
 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
drivers/tty/serial/omap-serial.c +0 -0
index a0d4460..bc22a2b 100644
drivers/tty/serial/omap-serial.c static unsigned int check_modem_status(struct uart_omap_port *up) · line 343
Quoted 10 lines
drivers/tty/serial/omap-serial.c +7 -0
@@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
 static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
 {
      unsigned int flag;
+     unsigned char ch = 0;
+
+     if (!(lsr & UART_LSR_BRK_ERROR_BITS))
+             return;
+
+     if (likely(lsr & UART_LSR_DR))
+             ch = serial_in(up, UART_RX);
Maybe add a comment before this condition stating why this character read is necessary ?
--
balbi

Felipe Balbi

2012-09-21 12:07 UTC #6 lore raw

PATCH 1/1 serial: omap: fix the overrun case

Quoted 12 lines · Sourav

Hi Felipe,
Quoted 9 lines · Felipe Balbi
Quoted 7 lines · Shubhrajyoti D
Overrun also causes an internal flag to be set, which disables further reception. Before the next frame can
be received, the MPU must:
? Reset the RX FIFO.
? clear the internal flag.
In the uart mode a dummy read is needed. Add the same.

Quoted 8 lines

Quoted 5 lines
Very nice patch but I think commit log can be a bit more verbose.
Please make the problem a little clearer. Why do we even get that interrupt fired if BRK_ERROR_BITS aren't set ?
According to LSR registers, there are few other bits like RX_FIFO_E( atleast 1 character in RX_FIFO or TX FIFO Empty), which might be the cause of an interrupt. ?

right. In that case, is it really correct to just return if BRK_ERROR_BITS aren't set ? IRQ line will not toggle and we just might end up with an IRQ storm, right ?

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120921/902c7fce/attachment.sig>

Poddar, Sourav

2012-09-21 11:18 UTC #7 lore raw

PATCH 1/1 serial: omap: fix the overrun case

In reply to Shubhrajyoti D

Hi,

Quoted 21 lines · Shubhrajyoti D

Overrun also causes an internal flag to be set, which disables further reception. Before the next frame can
be received, the MPU must:
? Reset the RX FIFO.
? clear the internal flag.
In the uart mode a dummy read is needed. Add the same.
Signed-off-by Shubhrajyoti D <[email protected]>
---
- functional testing on omap4sdp
- Verified idle and suspend path hits off on beagle.
 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
drivers/tty/serial/omap-serial.c +0 -0
index a0d4460..bc22a2b 100644

drivers/tty/serial/omap-serial.c static unsigned int check_modem_status(struct uart_omap_port *up) · line 339

Quoted 6 lines

drivers/tty/serial/omap-serial.c +3 -0

@@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
 static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
 {
        unsigned int flag;
+       unsigned char ch = 0;
+
+       if (!(lsr & UART_LSR_BRK_ERROR_BITS))

By using this flag, you are trying to take into account not just the overrun case but also
frame, parity and break condition case as the flag is the OR of all these.

I suppose the commit log should reflect this.

Quoted 10 lines

drivers/tty/serial/omap-serial.c +4 -1

+               return;
+
+       if (likely(lsr & UART_LSR_DR))
+               ch = serial_in(up, UART_RX);
 
        up->port.icount.rx++;
        flag = TTY_NORMAL;
--

1.7.5.4

Kevin Hilman

2012-09-21 14:18 UTC #8 lore raw

PATCH 1/1 serial: omap: fix the overrun case

In reply to Shubhrajyoti D

Quoted 6 lines · Shubhrajyoti D

Overrun also causes an internal flag to be set, which disables further reception. Before the next frame can
be received, the MPU must:
? Reset the RX FIFO.
? clear the internal flag.

Quoted 6 lines

In the uart mode a dummy read is needed. Add the same.
Signed-off-by Shubhrajyoti D <[email protected]>
---
- functional testing on omap4sdp
- Verified idle and suspend path hits off on beagle.

Tested-by Kevin Hilman <[email protected]>

This fixes the console hang I was seeing on 3530/Overo.

Thanks,

Kevin

Quoted 21 lines

 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

drivers/tty/serial/omap-serial.c +7 -0

index a0d4460..bc22a2b 100644
@@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
 static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
 {
 	unsigned int flag;
+	unsigned char ch = 0;
+
+	if (!(lsr & UART_LSR_BRK_ERROR_BITS))
+		return;
+
+	if (likely(lsr & UART_LSR_DR))
+		ch = serial_in(up, UART_RX);
 
 	up->port.icount.rx++;
 	flag = TTY_NORMAL;

Shubhrajyoti Datta

2012-09-21 15:08 UTC #9 lore raw

PATCH 1/1 serial: omap: fix the overrun case

Quoted 2 lines · Kevin Hilman

Shubhrajyoti D <[email protected]> writes:

[...]

Quoted 9 lines

Quoted 5 lines
Signed-off-by Shubhrajyoti D <[email protected]>
---
- functional testing on omap4sdp
- Verified idle and suspend path hits off on beagle.
Tested-by Kevin Hilman <[email protected]>
This fixes the console hang I was seeing on 3530/Overo.

Thanks for the test.

Could you test the v2
http://www.spinics.net/lists/arm-kernel/msg197050.html

I have removed the redundant check.
Thanks,

Quoted 30 lines

Thanks,
Kevin
Quoted 11 lines
 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
drivers/tty/serial/omap-serial.c +0 -0
index a0d4460..bc22a2b 100644
@@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
 static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
 {
      unsigned int flag;
drivers/tty/serial/omap-serial.c static unsigned int check_modem_status(struct uart_omap_port *up) · line 346
Quoted 10 lines
drivers/tty/serial/omap-serial.c +7 -0
+     unsigned char ch = 0;
+
+     if (!(lsr & UART_LSR_BRK_ERROR_BITS))
+             return;
+
+     if (likely(lsr & UART_LSR_DR))
+             ch = serial_in(up, UART_RX);
 
      up->port.icount.rx++;
      flag = TTY_NORMAL;
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html