Two msm_serial break fixes

The handle_rx() path calls uart_handle_sysrq_char() with the port lock held. This causes a spinlock recursion. Release and reacquire the lock here to avoid this.

BUG: spinlock recursion on CPU#0, swapper/0
 lock: msm_uart_ports+0x1e0/0x2d0, .magic: dead4ead, .owner: swapper/0, .owner_cpu: 0
CPU: 0 PID: 0 Comm: swapper Not tainted 3.17.0-rc7-00012-gb38ee8265941 #69
[<c0013964>] (unwind_backtrace) from [<c0011f74>] (show_stack+0x10/0x14)
[<c0011f74>] (show_stack) from [<c004ed1c>] (do_raw_spin_lock+0x11c/0x13c)
[<c004ed1c>] (do_raw_spin_lock) from [<c02d44c0>] (msm_console_write+0x78/0x188)
[<c02d44c0>] (msm_console_write) from [<c0052880>] (call_console_drivers.constprop.22+0xb4/0x144)
[<c0052880>] (call_console_drivers.constprop.22) from [<c0053570>] (console_unlock+0x27c/0x4ac)
[<c0053570>] (console_unlock) from [<c0053bb4>] (vprintk_emit+0x1f4/0x5a8)
[<c0053bb4>] (vprintk_emit) from [<c04ad0ac>] (printk+0x30/0x40)
[<c04ad0ac>] (printk) from [<c02c2990>] (__handle_sysrq+0x58/0x1b8)
[<c02c2990>] (__handle_sysrq) from [<c02d41b0>] (msm_irq+0x694/0x6f8)
[<c02d41b0>] (msm_irq) from [<c0055740>] (handle_irq_event_percpu+0x58/0x270)
[<c0055740>] (handle_irq_event_percpu) from [<c0055994>] (handle_irq_event+0x3c/0x5c)
[<c0055994>] (handle_irq_event) from [<c0057e84>] (handle_level_irq+0x9c/0x138)
[<c0057e84>] (handle_level_irq) from [<c005509c>] (generic_handle_irq+0x24/0x38)
[<c005509c>] (generic_handle_irq) from [<c000f730>] (handle_IRQ+0x44/0xb0)
[<c000f730>] (handle_IRQ) from [<c0008518>] (msm_vic_handle_irq+0x44/0x64)
[<c0008518>] (msm_vic_handle_irq) from [<c04b5ac4>] (__irq_svc+0x44/0x7c)
Exception stack(0xc0719f68 to 0xc0719fb0)

9f60: 00000001 00000001 00000000 c0722938 c0718000 c0769acc
9f80: 00000000 c0720098 c0769305 4117b362 c0769acc 00000000 01000000 c0719fb0 9fa0: c004cab0 c000f880 20000013 ffffffff
[<c04b5ac4>] (__irq_svc) from [<c000f880>] (arch_cpu_idle+0x20/0x30) [<c000f880>] (arch_cpu_idle) from [<c004691c>] (cpu_startup_entry+0xf4/0x23c) [<c004691c>] (cpu_startup_entry) from [<c06d8b70>] (start_kernel+0x32c/0x394)

Cc Frank Rowand <[email protected]>

Cc Daniel Thompson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

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

drivers/tty/serial/msm_serial.c +1 -0

index 4b6c78331a64..cedcc36762a2 100644
@@ -174,6 +174,7 @@ static void handle_rx(struct uart_port *port)
 	while ((sr = msm_read(port, UART_SR)) & UART_SR_RX_READY) {
 		unsigned int c;
 		char flag = TTY_NORMAL;
+		int sysrq;
 
 		c = msm_read(port, UART_RF);
 

Quoted 54 lines · Stephen Boyd

The handle_rx() path calls uart_handle_sysrq_char() with the port lock held. This causes a spinlock recursion. Release and reacquire the lock here to avoid this.

BUG: spinlock recursion on CPU#0, swapper/0
 lock: msm_uart_ports+0x1e0/0x2d0, .magic: dead4ead, .owner: swapper/0, .owner_cpu: 0
CPU: 0 PID: 0 Comm: swapper Not tainted 3.17.0-rc7-00012-gb38ee8265941 #69
[<c0013964>] (unwind_backtrace) from [<c0011f74>] (show_stack+0x10/0x14)
[<c0011f74>] (show_stack) from [<c004ed1c>] (do_raw_spin_lock+0x11c/0x13c)
[<c004ed1c>] (do_raw_spin_lock) from [<c02d44c0>] (msm_console_write+0x78/0x188)
[<c02d44c0>] (msm_console_write) from [<c0052880>] (call_console_drivers.constprop.22+0xb4/0x144)
[<c0052880>] (call_console_drivers.constprop.22) from [<c0053570>] (console_unlock+0x27c/0x4ac)
[<c0053570>] (console_unlock) from [<c0053bb4>] (vprintk_emit+0x1f4/0x5a8)
[<c0053bb4>] (vprintk_emit) from [<c04ad0ac>] (printk+0x30/0x40)
[<c04ad0ac>] (printk) from [<c02c2990>] (__handle_sysrq+0x58/0x1b8)
[<c02c2990>] (__handle_sysrq) from [<c02d41b0>] (msm_irq+0x694/0x6f8)
[<c02d41b0>] (msm_irq) from [<c0055740>] (handle_irq_event_percpu+0x58/0x270)
[<c0055740>] (handle_irq_event_percpu) from [<c0055994>] (handle_irq_event+0x3c/0x5c)
[<c0055994>] (handle_irq_event) from [<c0057e84>] (handle_level_irq+0x9c/0x138)
[<c0057e84>] (handle_level_irq) from [<c005509c>] (generic_handle_irq+0x24/0x38)
[<c005509c>] (generic_handle_irq) from [<c000f730>] (handle_IRQ+0x44/0xb0)
[<c000f730>] (handle_IRQ) from [<c0008518>] (msm_vic_handle_irq+0x44/0x64)
[<c0008518>] (msm_vic_handle_irq) from [<c04b5ac4>] (__irq_svc+0x44/0x7c)
Exception stack(0xc0719f68 to 0xc0719fb0)

9f60: 00000001 00000001 00000000 c0722938 c0718000 c0769acc
9f80: 00000000 c0720098 c0769305 4117b362 c0769acc 00000000 01000000 c0719fb0 9fa0: c004cab0 c000f880 20000013 ffffffff
[<c04b5ac4>] (__irq_svc) from [<c000f880>] (arch_cpu_idle+0x20/0x30) [<c000f880>] (arch_cpu_idle) from [<c004691c>] (cpu_startup_entry+0xf4/0x23c) [<c004691c>] (cpu_startup_entry) from [<c06d8b70>] (start_kernel+0x32c/0x394)

Cc Frank Rowand <[email protected]>

Cc Daniel Thompson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

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

drivers/tty/serial/msm_serial.c +1 -0

index 4b6c78331a64..cedcc36762a2 100644
@@ -174,6 +174,7 @@ static void handle_rx(struct uart_port *port)
 	while ((sr = msm_read(port, UART_SR)) & UART_SR_RX_READY) {
 		unsigned int c;
 		char flag = TTY_NORMAL;
+		int sysrq;
 
 		c = msm_read(port, UART_RF);
 

@@ -195,7 +196,10 @@ static void handle_rx(struct uart_port *port)
 		else if (sr & UART_SR_PAR_FRAME_ERR)
 			flag = TTY_FRAME;
 

drivers/tty/serial/msm_serial.c static void handle_rx(struct uart_port *port) · line 203

Quoted 6 lines

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

-		if (!uart_handle_sysrq_char(port, c))
+		spin_unlock(&port->lock);
+		sysrq = uart_handle_sysrq_char(port, c);
+		spin_lock(&port->lock);
+		if (!sysrq)
 			tty_insert_flip_char(tport, c, flag);

Does tty_insert_flip_char() need the port to be locked?

To properly support sysrq on uartDM hardware we need to properly handle break characters. With the DM hardware the fifo can pack 4 characters at a time, where a break is indicated by an all zero byte. Unfortunately, we can't differentiate between an all zero byte for a break and an all zero byte of data, so try and do as best we can. First unmask the RX break start interrupt and record the interrupt when it arrives. Then while processing the fifo, detect the break by searching for an all zero character as long as we recently received an RX break start interrupt. This should make sysrq work fairly well.

Cc Frank Rowand <[email protected]>

Cc Daniel Thompson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/tty/serial/msm_serial.c | 41 +++++++++++++++++++++++++++++++----------
 drivers/tty/serial/msm_serial.h |  2 ++
 2 files changed, 33 insertions(+), 10 deletions(-)

drivers/tty/serial/msm_serial.c +7 -9

index cedcc36762a2..d44c04976f7a 100644
@@ -54,6 +54,7 @@ struct msm_port {
 	unsigned int		imr;
 	int			is_uartdm;
 	unsigned int		old_snap_state;
+	bool			break_detected;
 };
 
 static inline void wait_for_xmitr(struct uart_port *port)

@@ -126,23 +127,38 @@ static void handle_rx_dm(struct uart_port *port, unsigned int misr)
 
 	while (count > 0) {
 		unsigned char buf[4];
+		int sysrq, r_count, i;
 
 		sr = msm_read(port, UART_SR);
 		if ((sr & UART_SR_RX_READY) == 0) {
 			msm_port->old_snap_state -= count;
 			break;
 		}
+
 		ioread32_rep(port->membase + UARTDM_RF, buf, 1);
-		if (sr & UART_SR_RX_BREAK) {
-			port->icount.brk++;
-			if (uart_handle_break(port))
-				continue;
-		} else if (sr & UART_SR_PAR_FRAME_ERR)
-			port->icount.frame++;
+		r_count = min_t(int, count, sizeof(buf));
+
+		for (i = 0; i < r_count; i++) {
+			char flag = TTY_NORMAL;
 
-		/* TODO: handle sysrq */
-		tty_insert_flip_string(tport, buf, min(count, 4));
-		count -= 4;

drivers/tty/serial/msm_serial.c +8 -1

+		}
+		count -= r_count;
 	}
 
 	spin_unlock(&port->lock);

@@ -291,6 +307,11 @@ static irqreturn_t msm_irq(int irq, void *dev_id)
 	misr = msm_read(port, UART_MISR);
 	msm_write(port, 0, UART_IMR); /* disable interrupt */
 
+	if (misr & UART_IMR_RXBREAK_START) {
+		msm_port->break_detected = true;
+		msm_write(port, UART_CR_CMD_RESET_RXBREAK_START, UART_CR);
+	}
+
 	if (misr & (UART_IMR_RXLEV | UART_IMR_RXSTALE)) {
 		if (msm_port->is_uartdm)
 			handle_rx_dm(port, misr);

@@ -496,7 +517,7 @@ static int msm_startup(struct uart_port *port)
 
 	/* turn on RX and CTS interrupts */
 	msm_port->imr = UART_IMR_RXLEV | UART_IMR_RXSTALE |
-			UART_IMR_CURRENT_CTS;
+			UART_IMR_CURRENT_CTS | UART_IMR_RXBREAK_START;
 
 	if (msm_port->is_uartdm) {
 		msm_write(port, 0xFFFFFF, UARTDM_DMRX);

drivers/tty/serial/msm_serial.h +2 -0

index 73d3abe71e79..3e1c7138d8cd 100644
@@ -65,6 +65,7 @@
 #define UART_CR_TX_ENABLE		(1 << 2)
 #define UART_CR_RX_DISABLE		(1 << 1)
 #define UART_CR_RX_ENABLE		(1 << 0)
+#define UART_CR_CMD_RESET_RXBREAK_START	((1 << 11) | (2 << 4))
 
 #define UART_IMR		0x0014
 #define UART_IMR_TXLEV		(1 << 0)

@@ -72,6 +73,7 @@
 #define UART_IMR_RXLEV		(1 << 4)
 #define UART_IMR_DELTA_CTS	(1 << 5)
 #define UART_IMR_CURRENT_CTS	(1 << 6)
+#define UART_IMR_RXBREAK_START	(1 << 10)
 
 #define UART_IPR_RXSTALE_LAST		0x20
 #define UART_IPR_STALE_LSB		0x1F

Quoted 5 lines · Stephen Boyd

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

+		r_count = min_t(int, count, sizeof(buf));
+
+		for (i = 0; i < r_count; i++) {
+			char flag = TTY_NORMAL;

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_port *port, unsigned int misr) · line 153

Quoted 13 lines

drivers/tty/serial/msm_serial.c +10 -3

-		/* TODO: handle sysrq */
-		tty_insert_flip_string(tport, buf, min(count, 4));
-		count -= 4;
+			if (msm_port->break_detected && buf[i] == 0) {
+				port->icount.brk++;
+				flag = TTY_BREAK;
+				msm_port->break_detected = false;
+				if (uart_handle_break(port))
+					continue;
+			}
+
+			if (!(port->read_status_mask & UART_SR_RX_BREAK))
+				flag = TTY_NORMAL;

flag is already known to be TTY_NORMAL.

Review

Quoted 4 lines

(quoted diff) +2 -0

+
+			spin_unlock(&port->lock);

Is it safe to unlock at this point? count may no longer be valid when we return.

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_port *port, unsigned int misr) · line 159

Quoted 6 lines

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

+			sysrq = uart_handle_sysrq_char(port, buf[i]);
+			spin_lock(&port->lock);
+			if (!sysrq)
+				tty_insert_flip_char(tport, buf[i], flag);

flag has a constant value here.

Quoted 6 lines · Daniel Thompson

Quoted 5 lines · Stephen Boyd

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

+		r_count = min_t(int, count, sizeof(buf));
+
+		for (i = 0; i < r_count; i++) {
+			char flag = TTY_NORMAL;

Quoted 15 lines

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_port *port, unsigned int misr) · line 153

Quoted 13 lines

drivers/tty/serial/msm_serial.c +10 -3

-		/* TODO: handle sysrq */
-		tty_insert_flip_string(tport, buf, min(count, 4));
-		count -= 4;
+			if (msm_port->break_detected && buf[i] == 0) {
+				port->icount.brk++;
+				flag = TTY_BREAK;
+				msm_port->break_detected = false;
+				if (uart_handle_break(port))
+					continue;
+			}
+
+			if (!(port->read_status_mask & UART_SR_RX_BREAK))
+				flag = TTY_NORMAL;

flag is already known to be TTY_NORMAL.

Huh? If we detected a break we would set the flag to TTY_BREAK and if uart_handle_break() returned 0 (perhaps sysrq config is diasbled) then we would get down here, and then we want to reset the flag to TTY_NORMAL if the read_status_mask bits indicate that we want to skip checking for breaks. Otherwise we want to indicate to the tty layer that it's a break character.

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+			spin_unlock(&port->lock);

Is it safe to unlock at this point? count may no longer be valid when we return.

Can you explain further? If it actually isn't valid something needs to be done. I believe other serial drivers are doing this sort of thing though so it doesn't seem that uncommon (of course those drivers could also be broken I suppose).

Quoted 10 lines

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_port *port, unsigned int misr) · line 159

Quoted 7 lines

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

+			sysrq = uart_handle_sysrq_char(port, buf[i]);
+			spin_lock(&port->lock);
+			if (!sysrq)
+				tty_insert_flip_char(tport, buf[i], flag);

flag has a constant value here.

Quoted 23 lines · Stephen Boyd

Quoted 6 lines · Daniel Thompson

Quoted 5 lines · Stephen Boyd

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

+		r_count = min_t(int, count, sizeof(buf));
+
+		for (i = 0; i < r_count; i++) {
+			char flag = TTY_NORMAL;

Quoted 15 lines

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_port *port, unsigned int misr) · line 153

Quoted 13 lines

drivers/tty/serial/msm_serial.c +10 -3

-		/* TODO: handle sysrq */
-		tty_insert_flip_string(tport, buf, min(count, 4));
-		count -= 4;
+			if (msm_port->break_detected && buf[i] == 0) {
+				port->icount.brk++;
+				flag = TTY_BREAK;
+				msm_port->break_detected = false;
+				if (uart_handle_break(port))
+					continue;
+			}
+
+			if (!(port->read_status_mask & UART_SR_RX_BREAK))
+				flag = TTY_NORMAL;

flag is already known to be TTY_NORMAL.

Quoted 6 lines

Huh? If we detected a break we would set the flag to TTY_BREAK and if uart_handle_break() returned 0 (perhaps sysrq config is diasbled) then we would get down here, and then we want to reset the flag to TTY_NORMAL if the read_status_mask bits indicate that we want to skip checking for breaks. Otherwise we want to indicate to the tty layer that it's a break character.

Agreed. Sorry for noise.

It now reaches the level of silly quibble (meaning I won't bother to raise the issue again if there is a v2 patch) but perhaps updating the flag after the continue would be easier to read.

Quoted 10 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+			spin_unlock(&port->lock);

Is it safe to unlock at this point? count may no longer be valid when we return.

Can you explain further? If it actually isn't valid something needs to be done. I believe other serial drivers are doing this sort of thing though so it doesn't seem that uncommon (of course those drivers could also be broken I suppose).

Calling spin_unlock() means we are allow code to alter the state of the UART. In particular the subsequent call to uart_handle_sysrq_char() can make significant changes to the FIFO state (by calling the poll_char functions). Given count is shadowing the FIFO state, when we retake the lock I think it is possible for count to no longer be valid.

Quoted 4 lines

Quoted 4 lines

Quoted 8 lines

Quoted 7 lines

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_port *port, unsigned int misr) · line 159

Quoted 4 lines

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

+			sysrq = uart_handle_sysrq_char(port, buf[i]);
+			spin_lock(&port->lock);
+			if (!sysrq)
+				tty_insert_flip_char(tport, buf[i], flag);

flag has a constant value here.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 49 lines · Daniel Thompson

Quoted 23 lines · Stephen Boyd

Quoted 6 lines · Daniel Thompson

Quoted 5 lines · Stephen Boyd

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

+		r_count = min_t(int, count, sizeof(buf));
+
+		for (i = 0; i < r_count; i++) {
+			char flag = TTY_NORMAL;

Quoted 15 lines

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_port *port, unsigned int misr) · line 153

Quoted 13 lines

drivers/tty/serial/msm_serial.c +10 -3

-		/* TODO: handle sysrq */
-		tty_insert_flip_string(tport, buf, min(count, 4));
-		count -= 4;
+			if (msm_port->break_detected && buf[i] == 0) {
+				port->icount.brk++;
+				flag = TTY_BREAK;
+				msm_port->break_detected = false;
+				if (uart_handle_break(port))
+					continue;
+			}
+
+			if (!(port->read_status_mask & UART_SR_RX_BREAK))
+				flag = TTY_NORMAL;

flag is already known to be TTY_NORMAL.

Quoted 6 lines

Huh? If we detected a break we would set the flag to TTY_BREAK and if uart_handle_break() returned 0 (perhaps sysrq config is diasbled) then we would get down here, and then we want to reset the flag to TTY_NORMAL if the read_status_mask bits indicate that we want to skip checking for breaks. Otherwise we want to indicate to the tty layer that it's a break character.

Agreed. Sorry for noise.

It now reaches the level of silly quibble (meaning I won't bother to raise the issue again if there is a v2 patch) but perhaps updating the flag after the continue would be easier to read.

Quoted 10 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+			spin_unlock(&port->lock);

Is it safe to unlock at this point? count may no longer be valid when we return.

Can you explain further? If it actually isn't valid something needs to be done. I believe other serial drivers are doing this sort of thing though so it doesn't seem that uncommon (of course those drivers could also be broken I suppose).

Quoted 5 lines

Calling spin_unlock() means we are allow code to alter the state of the UART. In particular the subsequent call to uart_handle_sysrq_char() can make significant changes to the FIFO state (by calling the poll_char functions). Given count is shadowing the FIFO state, when we retake the lock I think it is possible for count to no longer be valid.

uart_handle_sysrq_char() will not _read_ from the serial port. So it will not directly modify the FIFO state.

Quoted 56 lines · Frank Rowand

Quoted 54 lines · Daniel Thompson

Quoted 23 lines · Stephen Boyd

Quoted 6 lines · Daniel Thompson

Quoted 5 lines · Stephen Boyd

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

+		r_count = min_t(int, count, sizeof(buf));
+
+		for (i = 0; i < r_count; i++) {
+			char flag = TTY_NORMAL;

Quoted 15 lines

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_port *port, unsigned int misr) · line 153

Quoted 13 lines

drivers/tty/serial/msm_serial.c +10 -3

-		/* TODO: handle sysrq */
-		tty_insert_flip_string(tport, buf, min(count, 4));
-		count -= 4;
+			if (msm_port->break_detected && buf[i] == 0) {
+				port->icount.brk++;
+				flag = TTY_BREAK;
+				msm_port->break_detected = false;
+				if (uart_handle_break(port))
+					continue;
+			}
+
+			if (!(port->read_status_mask & UART_SR_RX_BREAK))
+				flag = TTY_NORMAL;

flag is already known to be TTY_NORMAL.

Quoted 6 lines

Huh? If we detected a break we would set the flag to TTY_BREAK and if uart_handle_break() returned 0 (perhaps sysrq config is diasbled) then we would get down here, and then we want to reset the flag to TTY_NORMAL if the read_status_mask bits indicate that we want to skip checking for breaks. Otherwise we want to indicate to the tty layer that it's a break character.

Agreed. Sorry for noise.

It now reaches the level of silly quibble (meaning I won't bother to raise the issue again if there is a v2 patch) but perhaps updating the flag after the continue would be easier to read.

Quoted 10 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+			spin_unlock(&port->lock);

Is it safe to unlock at this point? count may no longer be valid when we return.

Can you explain further? If it actually isn't valid something needs to be done. I believe other serial drivers are doing this sort of thing though so it doesn't seem that uncommon (of course those drivers could also be broken I suppose).

Calling spin_unlock() means we are allow code to alter the state of the UART. In particular the subsequent call to uart_handle_sysrq_char() can make significant changes to the FIFO state (by calling the poll_char functions). Given count is shadowing the FIFO state, when we retake the lock I think it is possible for count to no longer be valid.

Quoted 2 lines

uart_handle_sysrq_char() will not _read_ from the serial port. So it will not directly modify the FIFO state.

poll_char does not read from the FIFO? Since when?

SysRq-g will enter cause the system to enter kdb/kgdb from within uart_handle_sysrq_char().