| Message ID | 1414606478-13709-3-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. >
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. >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)