| Message ID | 20240624133135.7445-4-johan+linaro@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | serial: qcom-geni: fix lockups | expand |
Hi, On Mon, Jun 24, 2024 at 6:31 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > The Qualcomm GENI serial driver does not handle buffer flushing and > outputs garbage (or NUL) characters for the remainder of any active TX > command after the write buffer has been cleared. > > Implement the flush_buffer() callback and use it to cancel any active TX > command when the write buffer has been emptied. I could be reading it wrong, but in the kernel-doc of `struct tty_ldisc_ops` it seems to indicate that flush_buffer() is for the other direction. Specifically, it says: This function instructs the line discipline to clear its buffers of any input characters it may have queued to be delivered to the user mode process. It's hard to figure out which direction that matches to, but looking at the descriptions of "read" and "write" makes me believe that it's supposed to flush characters that have been read by the UART, not characters that are being written out to the UART. Maybe I'm misunderstanding or the kernel doc is wrong/incomplete? I guess the underlying worry I have is that there's no guarantee that the flush function will be called when the kfifo loses bytes. If it ever happens we'll fall back to writing NUL bytes out and that doesn't seem amazing to me. To me it feels like qcom_geni_serial_send_chunk_fifo() should detect this situation and then it should be responsible for canceling, though better (in my mind) is if we never initiate any big transfers if we can get away with that and still be performant. -Doug
On Mon, Jun 24, 2024 at 03:19:33PM -0700, Doug Anderson wrote: > On Mon, Jun 24, 2024 at 6:31 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > The Qualcomm GENI serial driver does not handle buffer flushing and > > outputs garbage (or NUL) characters for the remainder of any active TX > > command after the write buffer has been cleared. > > > > Implement the flush_buffer() callback and use it to cancel any active TX > > command when the write buffer has been emptied. > > I could be reading it wrong, but in the kernel-doc of `struct > tty_ldisc_ops` it seems to indicate that flush_buffer() is for the > other direction. Specifically, it says: > > This function instructs the line discipline to clear its buffers of > any input characters it may have queued to be delivered to the user > mode process. Yes, but this a uart op (i.e. not tty_ldisc_ops), for which the doc states: Flush any write buffers, reset any DMA state and stop any ongoing DMA transfers. > I guess the underlying worry I have is that there's no guarantee that > the flush function will be called when the kfifo loses bytes. If it > ever happens we'll fall back to writing NUL bytes out and that doesn't > seem amazing to me. To me it feels like > qcom_geni_serial_send_chunk_fifo() should detect this situation and > then it should be responsible for canceling, though better (in my > mind) is if we never initiate any big transfers if we can get away > with that and still be performant. The flush buffer callback is called from the uart_flush_buffer() tty operation (again, not tty_ldisc_ops) when the FIFO is reset. Johan
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 72addeb9f461..5fbb72f1c0c7 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -1084,6 +1084,11 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport) qcom_geni_serial_clear_tx_fifo(uport); } +static void qcom_geni_serial_flush_buffer(struct uart_port *uport) +{ + qcom_geni_serial_clear_tx_fifo(uport); +} + static int qcom_geni_serial_port_setup(struct uart_port *uport) { struct qcom_geni_serial_port *port = to_dev_port(uport); @@ -1540,6 +1545,7 @@ static const struct uart_ops qcom_geni_console_pops = { .request_port = qcom_geni_serial_request_port, .config_port = qcom_geni_serial_config_port, .shutdown = qcom_geni_serial_shutdown, + .flush_buffer = qcom_geni_serial_flush_buffer, .type = qcom_geni_serial_get_type, .set_mctrl = qcom_geni_serial_set_mctrl, .get_mctrl = qcom_geni_serial_get_mctrl,
The Qualcomm GENI serial driver does not handle buffer flushing and outputs garbage (or NUL) characters for the remainder of any active TX command after the write buffer has been cleared. Implement the flush_buffer() callback and use it to cancel any active TX command when the write buffer has been emptied. Fixes: a1fee899e5be ("tty: serial: qcom_geni_serial: Fix softlock") Cc: stable@vger.kernel.org # 5.0 Reported-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/tty/serial/qcom_geni_serial.c | 6 ++++++ 1 file changed, 6 insertions(+)