| Message ID | 20240902152451.862-9-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: qcom-geni: fix console corruption | expand |
Hi, On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > The polled UART operations are used by the kernel debugger (KDB, KGDB), > which can interrupt the kernel at any point in time. The current > Qualcomm GENI implementation does not really work when there is on-going > serial output as it inadvertently "hijacks" the current tx command, > which can result in both the initial debugger output being corrupted as > well as the corruption of any on-going serial output (up to 4k > characters) when execution resumes: > > 0190: abcdefghijklmnopqrstuvwxyz0123456789 0190: abcdefghijklmnopqrstuvwxyz0123456789 > 0191: abcdefghijklmnop[ 50.825552] sysrq: DEBUG > qrstuvwxyz0123456789 0191: abcdefghijklmnopqrstuvwxyz0123456789 > Entering kdb (current=0xffff53510b4cd280, pid 640) on processor 2 due to Keyboard Entry > [2]kdb> go > omlji3h3h2g2g1f1f0e0ezdzdycycxbxbwawav :t72r2rp > o9n976k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawavu:t7t8s8s8r2r2q0q0p > o9n9n8ml6k6k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawav v u:u:t9t0s4s4rq0p > o9n9n8m8m7l7l6k6k5j5j40q0p p o > o9n9n8m8m7l7l6k6k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawav :t8t9s4s4r4r4q0q0p > > Fix this by making sure that the polled output implementation waits for > the tx fifo to drain before cancelling any on-going longer transfers. As > the polled code cannot take any locks, leave the state variables as they > are and instead make sure that the interrupt handler always starts a new > tx command when there is data in the write buffer. > > Since the debugger can interrupt the interrupt handler when it is > writing data to the tx fifo, it is currently not possible to fully > prevent losing up to 64 bytes of tty output on resume. > > Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP") > Cc: stable@vger.kernel.org # 4.17 > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/tty/serial/qcom_geni_serial.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) Reviewed-by: Douglas Anderson <dianders@chromium.org>
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index fbed143c90a3..cf8bafd99a09 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -145,6 +145,7 @@ static const struct uart_ops qcom_geni_uart_pops; static struct uart_driver qcom_geni_console_driver; static struct uart_driver qcom_geni_uart_driver; +static void __qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport); static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport); static inline struct qcom_geni_serial_port *to_dev_port(struct uart_port *uport) @@ -403,13 +404,14 @@ static int qcom_geni_serial_get_char(struct uart_port *uport) static void qcom_geni_serial_poll_put_char(struct uart_port *uport, unsigned char c) { - writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG); + if (qcom_geni_serial_main_active(uport)) { + qcom_geni_serial_poll_tx_done(uport); + __qcom_geni_serial_cancel_tx_cmd(uport); + } + writel(M_CMD_DONE_EN, uport->membase + SE_GENI_M_IRQ_CLEAR); qcom_geni_serial_setup_tx(uport, 1); - WARN_ON(!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, - M_TX_FIFO_WATERMARK_EN, true)); writel(c, uport->membase + SE_GENI_TX_FIFOn); - writel(M_TX_FIFO_WATERMARK_EN, uport->membase + SE_GENI_M_IRQ_CLEAR); qcom_geni_serial_poll_tx_done(uport); } #endif @@ -688,13 +690,10 @@ static void qcom_geni_serial_stop_tx_fifo(struct uart_port *uport) writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN); } -static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport) +static void __qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport) { struct qcom_geni_serial_port *port = to_dev_port(uport); - if (!qcom_geni_serial_main_active(uport)) - return; - geni_se_cancel_m_cmd(&port->se); if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, M_CMD_CANCEL_EN, true)) { @@ -704,6 +703,16 @@ static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport) writel(M_CMD_ABORT_EN, uport->membase + SE_GENI_M_IRQ_CLEAR); } writel(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR); +} + +static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport) +{ + struct qcom_geni_serial_port *port = to_dev_port(uport); + + if (!qcom_geni_serial_main_active(uport)) + return; + + __qcom_geni_serial_cancel_tx_cmd(uport); port->tx_remaining = 0; port->tx_queued = 0; @@ -930,7 +939,7 @@ static void qcom_geni_serial_handle_tx_fifo(struct uart_port *uport, if (!chunk) goto out_write_wakeup; - if (!port->tx_remaining) { + if (!active) { qcom_geni_serial_setup_tx(uport, pending); port->tx_remaining = pending; port->tx_queued = 0;
The polled UART operations are used by the kernel debugger (KDB, KGDB), which can interrupt the kernel at any point in time. The current Qualcomm GENI implementation does not really work when there is on-going serial output as it inadvertently "hijacks" the current tx command, which can result in both the initial debugger output being corrupted as well as the corruption of any on-going serial output (up to 4k characters) when execution resumes: 0190: abcdefghijklmnopqrstuvwxyz0123456789 0190: abcdefghijklmnopqrstuvwxyz0123456789 0191: abcdefghijklmnop[ 50.825552] sysrq: DEBUG qrstuvwxyz0123456789 0191: abcdefghijklmnopqrstuvwxyz0123456789 Entering kdb (current=0xffff53510b4cd280, pid 640) on processor 2 due to Keyboard Entry [2]kdb> go omlji3h3h2g2g1f1f0e0ezdzdycycxbxbwawav :t72r2rp o9n976k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawavu:t7t8s8s8r2r2q0q0p o9n9n8ml6k6k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawav v u:u:t9t0s4s4rq0p o9n9n8m8m7l7l6k6k5j5j40q0p p o o9n9n8m8m7l7l6k6k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawav :t8t9s4s4r4r4q0q0p Fix this by making sure that the polled output implementation waits for the tx fifo to drain before cancelling any on-going longer transfers. As the polled code cannot take any locks, leave the state variables as they are and instead make sure that the interrupt handler always starts a new tx command when there is data in the write buffer. Since the debugger can interrupt the interrupt handler when it is writing data to the tx fifo, it is currently not possible to fully prevent losing up to 64 bytes of tty output on resume. Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP") Cc: stable@vger.kernel.org # 4.17 Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/tty/serial/qcom_geni_serial.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)