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
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(-)
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
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(-)
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
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 <[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(-)
>
> 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
>
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 <[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(-)
>
> 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?
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 <[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(-)
>>
>> 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
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
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.
>>
>
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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/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().
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