* [PATCH 0/2] Two msm_serial break fixes @ 2014-10-29 18:14 Stephen Boyd 2014-10-29 18:14 ` [PATCH 1/2] tty: serial: msm: Fix sysrq spinlock recursion on non-DM Stephen Boyd 2014-10-29 18:14 ` [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices Stephen Boyd 0 siblings, 2 replies; 11+ messages in thread From: Stephen Boyd @ 2014-10-29 18:14 UTC (permalink / raw) To: linux-arm-kernel The first patch fixes a spinlock recursion. I suppose we can ship it back to stable but I doubt anyone has noticed or cares. The second one adds break support to the DM path, and allows sysrq to work. Stephen Boyd (2): tty: serial: msm: Fix sysrq spinlock recursion on non-DM tty: serial: msm: Support sysrq on uartDM devices drivers/tty/serial/msm_serial.c | 47 +++++++++++++++++++++++++++++++---------- drivers/tty/serial/msm_serial.h | 2 ++ 2 files changed, 38 insertions(+), 11 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] tty: serial: msm: Fix sysrq spinlock recursion on non-DM 2014-10-29 18:14 [PATCH 0/2] Two msm_serial break fixes Stephen Boyd @ 2014-10-29 18:14 ` Stephen Boyd 2014-10-30 11:26 ` Daniel Thompson 2014-10-29 18:14 ` [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices Stephen Boyd 1 sibling, 1 reply; 11+ messages in thread From: Stephen Boyd @ 2014-10-29 18:14 UTC (permalink / raw) To: linux-arm-kernel 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 <frank.rowand@sonymobile.com> Cc: Daniel Thompson <daniel.thompson@linaro.org> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/tty/serial/msm_serial.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c index 4b6c78331a64..cedcc36762a2 100644 --- a/drivers/tty/serial/msm_serial.c +++ b/drivers/tty/serial/msm_serial.c @@ -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; - 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); } -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 1/2] tty: serial: msm: Fix sysrq spinlock recursion on non-DM 2014-10-29 18:14 ` [PATCH 1/2] tty: serial: msm: Fix sysrq spinlock recursion on non-DM Stephen Boyd @ 2014-10-30 11:26 ` Daniel Thompson 2014-10-30 13:29 ` Peter Hurley 0 siblings, 1 reply; 11+ messages in thread From: Daniel Thompson @ 2014-10-30 11:26 UTC (permalink / raw) To: linux-arm-kernel On 29/10/14 18:14, Stephen Boyd wrote: > 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 <frank.rowand@sonymobile.com> > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/tty/serial/msm_serial.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c > index 4b6c78331a64..cedcc36762a2 100644 > --- a/drivers/tty/serial/msm_serial.c > +++ b/drivers/tty/serial/msm_serial.c > @@ -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; > > - 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] tty: serial: msm: Fix sysrq spinlock recursion on non-DM 2014-10-30 11:26 ` Daniel Thompson @ 2014-10-30 13:29 ` Peter Hurley 0 siblings, 0 replies; 11+ messages in thread From: Peter Hurley @ 2014-10-30 13:29 UTC (permalink / raw) To: linux-arm-kernel On 10/30/2014 07:26 AM, Daniel Thompson wrote: > On 29/10/14 18:14, Stephen Boyd wrote: >> 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 <frank.rowand@sonymobile.com> >> Cc: Daniel Thompson <daniel.thompson@linaro.org> >> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> >> --- >> drivers/tty/serial/msm_serial.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c >> index 4b6c78331a64..cedcc36762a2 100644 >> --- a/drivers/tty/serial/msm_serial.c >> +++ b/drivers/tty/serial/msm_serial.c >> @@ -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; >> >> - 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? No. It does not serialize internally, so concurrent use will blow up, but that doesn't look possible here. Can bad things happen if a well-timed set_termios() happens while the lock is dropped? Regards, Peter Hurley ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices 2014-10-29 18:14 [PATCH 0/2] Two msm_serial break fixes Stephen Boyd 2014-10-29 18:14 ` [PATCH 1/2] tty: serial: msm: Fix sysrq spinlock recursion on non-DM Stephen Boyd @ 2014-10-29 18:14 ` Stephen Boyd 2014-10-30 11:30 ` Daniel Thompson 1 sibling, 1 reply; 11+ messages in thread From: Stephen Boyd @ 2014-10-29 18:14 UTC (permalink / raw) To: linux-arm-kernel 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 <frank.rowand@sonymobile.com> Cc: Daniel Thompson <daniel.thompson@linaro.org> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/tty/serial/msm_serial.c | 41 +++++++++++++++++++++++++++++++---------- drivers/tty/serial/msm_serial.h | 2 ++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c index cedcc36762a2..d44c04976f7a 100644 --- a/drivers/tty/serial/msm_serial.c +++ b/drivers/tty/serial/msm_serial.c @@ -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; + 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; + + spin_unlock(&port->lock); + sysrq = uart_handle_sysrq_char(port, buf[i]); + spin_lock(&port->lock); + if (!sysrq) + tty_insert_flip_char(tport, buf[i], flag); + } + 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); diff --git a/drivers/tty/serial/msm_serial.h b/drivers/tty/serial/msm_serial.h index 73d3abe71e79..3e1c7138d8cd 100644 --- a/drivers/tty/serial/msm_serial.h +++ b/drivers/tty/serial/msm_serial.h @@ -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 -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices 2014-10-29 18:14 ` [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices Stephen Boyd @ 2014-10-30 11:30 ` Daniel Thompson 2014-10-31 6:41 ` Stephen Boyd 0 siblings, 1 reply; 11+ messages in thread From: Daniel Thompson @ 2014-10-30 11:30 UTC (permalink / raw) To: linux-arm-kernel On 29/10/14 18:14, Stephen Boyd wrote: > 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 <frank.rowand@sonymobile.com> > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/tty/serial/msm_serial.c | 41 +++++++++++++++++++++++++++++++---------- > drivers/tty/serial/msm_serial.h | 2 ++ > 2 files changed, 33 insertions(+), 10 deletions(-) > > diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c > index cedcc36762a2..d44c04976f7a 100644 > --- a/drivers/tty/serial/msm_serial.c > +++ b/drivers/tty/serial/msm_serial.c > @@ -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; > + 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. > + > + spin_unlock(&port->lock); Is it safe to unlock at this point? count may no longer be valid when we return. > + 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. > + } > + 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); > diff --git a/drivers/tty/serial/msm_serial.h b/drivers/tty/serial/msm_serial.h > index 73d3abe71e79..3e1c7138d8cd 100644 > --- a/drivers/tty/serial/msm_serial.h > +++ b/drivers/tty/serial/msm_serial.h > @@ -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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices 2014-10-30 11:30 ` Daniel Thompson @ 2014-10-31 6:41 ` Stephen Boyd 2014-10-31 9:43 ` Daniel Thompson 0 siblings, 1 reply; 11+ messages in thread From: Stephen Boyd @ 2014-10-31 6:41 UTC (permalink / raw) To: linux-arm-kernel On 10/30, Daniel Thompson wrote: > On 29/10/14 18:14, Stephen Boyd wrote: > > + 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; > > + 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. > > > > + > > + 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). > > > > + 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. > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices 2014-10-31 6:41 ` Stephen Boyd @ 2014-10-31 9:43 ` Daniel Thompson 2014-10-31 18:08 ` Frank Rowand 0 siblings, 1 reply; 11+ messages in thread From: Daniel Thompson @ 2014-10-31 9:43 UTC (permalink / raw) To: linux-arm-kernel On 31/10/14 06:41, Stephen Boyd wrote: > On 10/30, Daniel Thompson wrote: >> On 29/10/14 18:14, Stephen Boyd wrote: >>> + 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; >>> + 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. 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. >>> + >>> + 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. > >> >> >>> + 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. >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices 2014-10-31 9:43 ` Daniel Thompson @ 2014-10-31 18:08 ` Frank Rowand 2014-11-03 10:05 ` Daniel Thompson 0 siblings, 1 reply; 11+ messages in thread From: Frank Rowand @ 2014-10-31 18:08 UTC (permalink / raw) To: linux-arm-kernel On 10/31/2014 2:43 AM, Daniel Thompson wrote: > On 31/10/14 06:41, Stephen Boyd wrote: >> On 10/30, Daniel Thompson wrote: >>> On 29/10/14 18:14, Stephen Boyd wrote: >>>> + 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; >>>> + 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. > > 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. > > >>>> + >>>> + 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. uart_handle_sysrq_char() will not _read_ from the serial port. So it will not directly modify the FIFO state. > >> >>> >>> >>>> + 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices 2014-10-31 18:08 ` Frank Rowand @ 2014-11-03 10:05 ` Daniel Thompson 2014-11-04 3:00 ` Frank Rowand 0 siblings, 1 reply; 11+ messages in thread From: Daniel Thompson @ 2014-11-03 10:05 UTC (permalink / raw) To: linux-arm-kernel On 31/10/14 18:08, Frank Rowand wrote: > On 10/31/2014 2:43 AM, Daniel Thompson wrote: >> On 31/10/14 06:41, Stephen Boyd wrote: >>> On 10/30, Daniel Thompson wrote: >>>> On 29/10/14 18:14, Stephen Boyd wrote: >>>>> + 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; >>>>> + 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. >> >> 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. >> >> >>>>> + >>>>> + 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. > > 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(). ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices 2014-11-03 10:05 ` Daniel Thompson @ 2014-11-04 3:00 ` Frank Rowand 0 siblings, 0 replies; 11+ messages in thread From: Frank Rowand @ 2014-11-04 3:00 UTC (permalink / raw) To: linux-arm-kernel On 11/3/2014 2:05 AM, Daniel Thompson wrote: > On 31/10/14 18:08, Frank Rowand wrote: >> On 10/31/2014 2:43 AM, Daniel Thompson wrote: >>> On 31/10/14 06:41, Stephen Boyd wrote: >>>> On 10/30, Daniel Thompson wrote: >>>>> On 29/10/14 18:14, Stephen Boyd wrote: >>>>>> + 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; >>>>>> + 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. >>> >>> 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. >>> >>> >>>>>> + >>>>>> + 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. >> >> 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(). Aarrgh. You are correct. I overlooked the obvious SysRq-g. /me searches for paper bag. -Frank ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-11-04 3:00 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-29 18:14 [PATCH 0/2] Two msm_serial break fixes Stephen Boyd 2014-10-29 18:14 ` [PATCH 1/2] tty: serial: msm: Fix sysrq spinlock recursion on non-DM Stephen Boyd 2014-10-30 11:26 ` Daniel Thompson 2014-10-30 13:29 ` Peter Hurley 2014-10-29 18:14 ` [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices Stephen Boyd 2014-10-30 11:30 ` Daniel Thompson 2014-10-31 6:41 ` Stephen Boyd 2014-10-31 9:43 ` Daniel Thompson 2014-10-31 18:08 ` Frank Rowand 2014-11-03 10:05 ` Daniel Thompson 2014-11-04 3:00 ` Frank Rowand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).