diff mbox

[03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO

Message ID 1410377411-26656-4-git-send-email-bigeasy@linutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Sebastian Andrzej Siewior Sept. 10, 2014, 7:29 p.m. UTC
The serial8250_do_startup() function unconditionally clears the
interrupts and for that it reads from the RX-FIFO without checking if
there is a byte in the FIFO or not. This works fine on OMAP4+ HW like
AM335x or DRA7.
OMAP3630 ES1.1 (which means probably all OMAP3 and earlier) does not like
this:

|Unhandled fault: external abort on non-linefetch (0x1028) at 0xfb020000
|Internal error: : 1028 [#1] ARM
|Modules linked in:
|CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0-00022-g7edcb57-dirty #1213
|task: de0572c0 ti: de058000 task.ti: de058000
|PC is at mem32_serial_in+0xc/0x1c
|LR is at serial8250_do_startup+0x220/0x85c
|Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
|Control: 10c5387d  Table: 80004019  DAC: 00000015
|[<c03051d4>] (mem32_serial_in) from [<c0307fe8>] (serial8250_do_startup+0x220/0x85c)
|[<c0307fe8>] (serial8250_do_startup) from [<c0309e00>] (omap_8250_startup+0x5c/0xe0)
|[<c0309e00>] (omap_8250_startup) from [<c030863c>] (serial8250_startup+0x18/0x2c)
|[<c030863c>] (serial8250_startup) from [<c030394c>] (uart_startup+0x78/0x1d8)
|[<c030394c>] (uart_startup) from [<c0304678>] (uart_open+0xe8/0x114)
|[<c0304678>] (uart_open) from [<c02e9e10>] (tty_open+0x1a8/0x5a4)

Reviewed-by: Tony Lindgren <tony@atomide.com>
Tested-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Nicolas Schichan Feb. 9, 2015, 1:34 p.m. UTC | #1
On 09/10/2014 09:29 PM, Sebastian Andrzej Siewior wrote:
> The serial8250_do_startup() function unconditionally clears the
> interrupts and for that it reads from the RX-FIFO without checking if
> there is a byte in the FIFO or not. This works fine on OMAP4+ HW like
> AM335x or DRA7.
> OMAP3630 ES1.1 (which means probably all OMAP3 and earlier) does not like
> this:

Hello,

Sorry to wake up an old thread, but I'm affraid that this patch causes
problems on Marvell 88f6282 (Kirkwood).

When a caracter is received on the UART while the kernel is printing
the boot messages, as soon as the kernel configures the UART for
receiving (after root filesystem mount), it gets stuck printing the
following message repeatedly:

serial8250: too much work for irq29

Once stuck, the reception of another character allows the boot process
to finish.

From what I can gather, when we hit that, the UART_IIR_NO_INT is 0 (so the
interrupt is raised), but the UART_LSR_DR bit is 0 as well so the UART_RX
register is never read to clear the interrupt.

We are using the second UART multiplexed on mpps 15 and 16.

Reverting this particular patch fixes the issue.

We are seing the problem on a 3.18 kernel.

Regards,
Peter Hurley Feb. 9, 2015, 11:34 p.m. UTC | #2
Hi Nicolas,

Thanks for the report.

On 02/09/2015 08:34 AM, Nicolas Schichan wrote:
> On 09/10/2014 09:29 PM, Sebastian Andrzej Siewior wrote:
>> The serial8250_do_startup() function unconditionally clears the
>> interrupts and for that it reads from the RX-FIFO without checking if
>> there is a byte in the FIFO or not. This works fine on OMAP4+ HW like
>> AM335x or DRA7.
>> OMAP3630 ES1.1 (which means probably all OMAP3 and earlier) does not like
>> this:
> 
> Hello,
> 
> Sorry to wake up an old thread, but I'm affraid that this patch causes
> problems on Marvell 88f6282 (Kirkwood).
> 
> When a caracter is received on the UART while the kernel is printing
> the boot messages, as soon as the kernel configures the UART for
> receiving (after root filesystem mount), it gets stuck printing the
> following message repeatedly:
> 
> serial8250: too much work for irq29
> 
> Once stuck, the reception of another character allows the boot process
> to finish.
> 
> From what I can gather, when we hit that, the UART_IIR_NO_INT is 0 (so the
> interrupt is raised), but the UART_LSR_DR bit is 0 as well so the UART_RX
> register is never read to clear the interrupt.

The "too much work" message means serial8250_handle_irq() is returning 0,
ie., not handled. Which in turn means IIR indicates no interrupt is pending
(UART_IIR_NO_INT == 1).

Can you log the register values for LSR and IIR at both patch locations
in serial8250_do_startup()?

(I can get you a debug patch, if necessary. Let me know)

Regards,
Peter Hurley

> We are using the second UART multiplexed on mpps 15 and 16.
> 
> Reverting this particular patch fixes the issue.
> 
> We are seing the problem on a 3.18 kernel.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Andrzej Siewior Feb. 10, 2015, 9:32 a.m. UTC | #3
On 02/10/2015 12:34 AM, Peter Hurley wrote:
> The "too much work" message means serial8250_handle_irq() is returning 0,
> ie., not handled. Which in turn means IIR indicates no interrupt is pending
> (UART_IIR_NO_INT == 1).

The OMAP UART for instance have two possible TX-IRQ handling. The
default is to fire the interrupt once there is room for at least one
byte in the FIFO. I set the OMAP_UART_SCR_TX_EMPTY bit to get the same
behavior as the 8250 driver expects (interrupt while the FIFO is empty).
Without this bit set I've seen the message from time to time.

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

Patch

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 3cf5c98013e4..547afde9fdda 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2088,8 +2088,8 @@  int serial8250_do_startup(struct uart_port *port)
 	/*
 	 * Clear the interrupt registers.
 	 */
-	serial_port_in(port, UART_LSR);
-	serial_port_in(port, UART_RX);
+	if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
 	serial_port_in(port, UART_IIR);
 	serial_port_in(port, UART_MSR);
 
@@ -2250,8 +2250,8 @@  int serial8250_do_startup(struct uart_port *port)
 	 * saved flags to avoid getting false values from polling
 	 * routines or the previous session.
 	 */
-	serial_port_in(port, UART_LSR);
-	serial_port_in(port, UART_RX);
+	if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
 	serial_port_in(port, UART_IIR);
 	serial_port_in(port, UART_MSR);
 	up->lsr_saved_flags = 0;
@@ -2344,7 +2344,8 @@  void serial8250_do_shutdown(struct uart_port *port)
 	 * Read data port to reset things, and then unlink from
 	 * the IRQ chain.
 	 */
-	serial_port_in(port, UART_RX);
+	if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
 	serial8250_rpm_put(up);
 
 	del_timer_sync(&up->timer);