tty/serial: at91: fixes dealing with port shutdown

Cc <[email protected]> # v3.12

---
 drivers/tty/serial/atmel_serial.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/tty/serial/atmel_serial.c +13 -7

index c7d99af46a96..48ea47a32d5f 100644
@@ -1650,12 +1650,24 @@ static int atmel_startup(struct uart_port *port)
 static void atmel_shutdown(struct uart_port *port)
 {
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+
 	/*
-	 * Ensure everything is stopped.
+	 * Clear out any scheduled tasklets before
+	 * we destroy the buffers
+	 */
+	tasklet_kill(&atmel_port->tasklet);
+
+	/*
+	 * Ensure everything is stopped and
+	 * disable all interrupts, port and break condition.
 	 */
 	atmel_stop_rx(port);
 	atmel_stop_tx(port);
 
+	UART_PUT_CR(port, ATMEL_US_RSTSTA);
+	UART_PUT_IDR(port, -1);
+
+
 	/*
 	 * Shut-down the DMA.
 	 */

@@ -1665,12 +1677,6 @@ static void atmel_shutdown(struct uart_port *port)
 		atmel_port->release_tx(port);
 
 	/*
-	 * Disable all interrupts, port and break condition.
-	 */
-	UART_PUT_CR(port, ATMEL_US_RSTSTA);
-	UART_PUT_IDR(port, -1);
-
-	/*
 	 * Free the interrupt
 	 */
 	free_irq(port->irq, port);

Signature · 1 line

1.8.2.2

Nicolas Ferre

author

2014-01-07 10:45 UTC #2 lore raw

PATCH 2/4 tty/serial: at91: fix race condition in atmel_serial_remove

From: Marek Roszko <[email protected]>

The _remove callback could be called when a tasklet is scheduled. tasklet_kill was called inside the function in order to free up any scheduled tasklets. However it was called after uart_remove_one_port which destroys tty references needed in the port for atmel_tasklet_func.
Simply putting the tasklet_kill at the start of the function will prevent this conflict.

Signed-off-by Marek Roszko <[email protected]>

Cc <[email protected]> # v3.12

---
 drivers/tty/serial/atmel_serial.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/tty/serial/atmel_serial.c +2 -1

index 48ea47a32d5f..c421d11b3d4c 100644
@@ -2447,11 +2447,12 @@ static int atmel_serial_remove(struct platform_device *pdev)
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	int ret = 0;
 
+	tasklet_kill(&atmel_port->tasklet);
+
 	device_init_wakeup(&pdev->dev, 0);
 
 	ret = uart_remove_one_port(&atmel_uart, port);
 
-	tasklet_kill(&atmel_port->tasklet);
 	kfree(atmel_port->rx_ring.buf);
 
 	/* "port" is allocated statically, so we shouldn't free it */

Signature · 1 line

1.8.2.2

Nicolas Ferre

author

2014-01-07 10:45 UTC #3 lore raw

PATCH 3/4 tty/serial: at91: prevent null dereference in tasklet function

From: Marek Roszko <[email protected]>

Something asks a tasklet to be scheduled when the uart port is closed. Need to supress the kernel panic for now by checking if the port is NULL or not.

Signed-off-by Marek Roszko <[email protected]>

Cc <[email protected]> # v3.12

http://daringfireball.net/2007/07/on_top

---
 drivers/tty/serial/atmel_serial.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/tty/serial/atmel_serial.c +4 -0

index c421d11b3d4c..6e68486c83cb 100644
@@ -1360,6 +1360,10 @@ static void atmel_tasklet_func(unsigned long data)
 	unsigned int status;
 	unsigned int status_change;
 
+	if(!port->state || !port->state->port.tty)
+		/* uart has been closed */
+		return;
+
 	/* The interrupt handler does not take the lock */
 	spin_lock(&port->lock);

Signature · 1 line

1.8.2.2

Greg KH

maintainer

2014-01-08 01:11 UTC #4 lore raw

Re: [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function

Quoted 3 lines · Nicolas Ferre

From: Marek Roszko <[email protected]>
Something asks a tasklet to be scheduled when the uart port is closed.

What is that something? Shouldn't you track that down and find the real problem here?

Quoted 16 lines

Need to supress the kernel panic for now by checking if the port is NULL or not.
Signed-off-by Marek Roszko <[email protected]>
Acked-by Leilei Zhao <[email protected]>
Cc <[email protected]> # v3.12
Signed-off-by Nicolas Ferre <[email protected]>
---
 drivers/tty/serial/atmel_serial.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/tty/serial/atmel_serial.c +0 -0
index c421d11b3d4c..6e68486c83cb 100644

drivers/tty/serial/atmel_serial.c static void atmel_tasklet_func(unsigned long data) · line 1365

Quoted 6 lines

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

@@ -1360,6 +1360,10 @@ static void atmel_tasklet_func(unsigned long data)
 	unsigned int status;
 	unsigned int status_change;
 
+	if(!port->state || !port->state->port.tty)
+		/* uart has been closed */
+		return;

Did you really test this?

How about running it through checkpatch?

thanks,

greg k-h

A message that is not in the archive (sent off-list or not yet received): CAJjB1qLDd6JFb4LwD5iokg5=O8Bwp5MkKsrr45QDodZkZrx75g@mail.gmail.com

Greg KH

maintainer

2014-01-08 03:44 UTC #6 lore raw

Re: [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function

A: No.
Q: Should I include quotations after my reply?

Quoted 2 lines · Mark Roszko

This patch I was somewhat hesitant to pushing to Atmel when I did the other two patches.

Then why did you? :)

Quoted 5 lines

The main issue is the use of systemd causes the serial driver to somehow get out of sync on startups randomly. i.e. on one bootup it's fine, and on other it'll kernel panic.
It occurs because systemd causes the startup and shutdown driver ops to be fired in rapid succession.

How does systemd cause this? Is this when the serial port is being used as a console or something else?

Quoted 2 lines

Every single service start message causes the _startup callback first, then the message prints and _shutdown callbacks fires.

So something is opening and closing the port quickly? That should be easy to test without systemd.

Any console involved?

Quoted 11 lines

And a kernel panic always occurs somewhere during the service status output, never before when it's just the kernel startup and never after once systemd finishes and getty for example takes over.
The stacktrace looked like this:
Unable to handle kernel NULL pointer dereference at virtual address 00000088
pgd = c0004000
[00000088] *pgd=00000000
Internal error: Oops: 17 [#1] ARM
Modules linked in: autofs4
CPU: 0    Not tainted  (3.6.9-yocto-standard #1)

3.6.9 is _very_ old, loads of things have happened in the tty layer since then, can you duplicate this on 3.12 or 3.13-rc?

Quoted 28 lines

PC is at tty_wakeup+0x8/0x58
LR is at atmel_tasklet_func+0x238/0x80c
pc : [<c01efd84>]    lr : [<c0208fc0>]    psr: a00f0013
sp : df84ff28 ip : 00000000 fp : 00000100
r10: c05d1ec0 r9 : 04208040 r8 : c05d1e80
r7 : 0000000a r6 : 00000000 r5 : dedf7c00 r4 : 00000000 r3 : dedf7c00 r2 : 00000000 r1 : 600f0013 r0 : 00000000
Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 10c53c7d Table: 3fb0c059 DAC: 00000015 Process ksoftirqd/0 (pid: 3, stack limit = 0xdf84e2f0)
Stack: (0xdf84ff28 to 0xdf850000)
ff20: c05e4378 dedf7c00 00000000 c0208fc0 c05aa458 df8496b0
ff40: df849680 c05aa458 df8496b0 c00394a8 c0039420 c05a8d04 00000000 00000000
ff60: 0000000a c05d1e80 04208040 c05d1ec0 00000100 c001fd34 00000009 00000018
ff80: df84e000 c001f60c df84ffbc c03f8e60 00000013 00000006 00000000 c05d1e80
ffa0: df84e000 00000000 00000013 00000000 00000000 00000000 00000000 c001f728
ffc0: df84bf3c 00000000 c001f6c0 c0030870 00000000 00000000 00000000 00000000
ffe0: df84ffe0 df84ffe0 df84bf3c c00307f0 c000e348 c000e348 fff73fbf 3fbeffff [<c01efd84>] (tty_wakeup+0x8/0x58) from [<c0208fc0>] (atmel_tasklet_func+0x238/ 0x80c)
[<c0208fc0>] (atmel_tasklet_func+0x238/0x80c) from [<c001fd34>] (tasklet_action+0x70/0xa8)
[<c001fd34>] (tasklet_action+0x70/0xa8) from [<c001f60c>] (__do_softirq+0x90/ 0x144)
[<c001f60c>] (__do_softirq+0x90/0x144) from [<c001f728>] (run_ksoftirqd+0x68/ 0x104)
[<c001f728>] (run_ksoftirqd+0x68/0x104) from [<c0030870>] (kthread+0x80/0x90) [<c0030870>] (kthread+0x80/0x90) from [<c000e348>] (kernel_thread_exit+0x0/0x8)

Quoted 7 lines

Code: e8bd8070 c05ac0b8 e92d4070 e1a04000 (e5903088)
---[ end trace 15b8a9aeaf7b457f ]---
Testing wise I originally used a BUG_ON statement in atmel_tasklet_func to panic before the null deference hit. BUG_ON confirmed that tty was NULL at the very start of the tasklet being called.

And did you test that this patch actually fixed it?

Quoted 4 lines

The atmel_shutdown function should be killing the tasklet (after patch "Handle shutdown more safely") and does disable interrupts so I've been at a loss at where the race condition was occurring that a tasklet could escape beyond shutdown.

Are you sure you aren't just racing with shutdown? Need a lock somewhere?

greg k-h

Nicolas Ferre

author

2014-01-07 10:45 UTC #7 lore raw

PATCH 4/4 tty/serial: at91: reset rx_ring when port is shutdown

From: Mark Deneen <[email protected]>

When using RX DMA, the driver won't pass any data to the uart layer until the buffer is flipped. When the port is shutdown, the dma buffers are unmapped, but the head and tail of the ring buffer are not reseted. Since the serial console will keep the port open, this will only present itself when the uart is not shared.

To reproduce the issue, with an unpatched driver, run a getty on /dev/ttyS0 with no serial console and exit. Getty will exit, and when the new one returns you will be unable to log in. If you hold down a key long enough to fill the DMA buffer and flip it, you can then log in.

Signed-off-by Mark Deneen <[email protected]>

[nicolas.ferre at atmel.com: adapt to mainline kernel, handle !DMA case]

Cc <[email protected]> # v3.12

---
 drivers/tty/serial/atmel_serial.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/tty/serial/atmel_serial.c +6 -0

index 6e68486c83cb..2d925455c1ec 100644
@@ -1681,6 +1681,12 @@ static void atmel_shutdown(struct uart_port *port)
 		atmel_port->release_tx(port);
 
 	/*
+	 * Reset ring buffer pointers
+	 */
+	atmel_port->rx_ring.head = 0;
+	atmel_port->rx_ring.tail = 0;
+
+	/*
 	 * Free the interrupt
 	 */
 	free_irq(port->irq, port);

Signature · 1 line

1.8.2.2

Nicolas Ferre

author

2014-01-07 10:48 UTC #8 lore raw