-----Original Message-----
From: Michal Simek <redacted>
Sent: Monday, November 1, 2021 5:58 PM
To: Anssi Hannula <redacted>; Michal Simek
[off-list ref]; Shubhrajyoti Datta [off-list ref]
Cc: linux-serial@vger.kernel.org; Greg Kroah-Hartman
[off-list ref]; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: Re: [PATCH] serial: xilinx_uartps: Fix race condition causing stuck TX



On 10/26/21 12:27, Anssi Hannula wrote:

quoted

xilinx_uartps .start_tx() clears TXEMPTY when enabling TXEMPTY to
avoid any previous TXEVENT event asserting the UART interrupt. This
clear operation is done immediately after filling the TX FIFO.

However, if the bytes inserted by cdns_uart_handle_tx() are consumed
by the UART before the TXEMPTY is cleared, the clear operation eats
the new TXEMPTY event as well, causing cdns_uart_isr() to never
receive the TXEMPTY event. If there are bytes still queued in circbuf,
TX will get stuck as they will never get transferred to FIFO (unless
new bytes are queued to circbuf in which case .start_tx() is called again).

While the racy missed TXEMPTY occurs fairly often with short data
sequences (e.g. write 1 byte), in those cases circbuf is usually empty
so no action on TXEMPTY would have been needed anyway. On the other
hand, longer data sequences make the race much more unlikely as UART
takes longer to consume the TX FIFO. Therefore it is rare for this
race to cause visible issues in general.

Fix the race by clearing the TXEMPTY bit in ISR *before* filling the
FIFO.

The TXEMPTY bit in ISR will only get asserted at the exact moment the
TX FIFO *becomes* empty, so clearing the bit before filling FIFO does
not cause an extra immediate assertion even if the FIFO is initially
empty.

This is hard to reproduce directly on a normal system, but inserting
e.g. udelay(200) after cdns_uart_handle_tx(port), setting 4000000
baud, and then running "dd if=/dev/zero bs=128 of=/dev/ttyPS0 count=50"
reliably reproduces the issue on my ZynqMP test system unless this fix
is applied.

Fixes: 85baf542d54e ("tty: xuartps: support 64 byte FIFO size")
Signed-off-by: Anssi Hannula <redacted>
---
  drivers/tty/serial/xilinx_uartps.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c

b/drivers/tty/serial/xilinx_uartps.c
index 962e522ccc45..d5e243908d9f 100644

--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c

@@ -601,9 +601,10 @@ static void cdns_uart_start_tx(struct uart_port

*port)

quoted

  	if (uart_circ_empty(&port->state->xmit))
  		return;

+	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
+
  	cdns_uart_handle_tx(port);

-	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
  	/* Enable the TX Empty interrupt */
  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
  }

Reviewed-by: Shubhrajyoti Datta <redacted>