[PATCH] serial: xilinx_uartps: Fix race condition causing stuck TX

[PATCH] serial: xilinx_uartps: Fix race condition causing stuck TX

[Anssi Hannula]

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 <anssi.hannula@bitwise.fi>
---
 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)
 	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);
 }
-- 
2.31.1

Re: [PATCH] serial: xilinx_uartps: Fix race condition causing stuck TX

[Michal Simek]

On 10/26/21 12:27, Anssi Hannula wrote:
> 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 <anssi.hannula@bitwise.fi>
> ---
>   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)
>   	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);
>   }
> 

Shubhrajyoti: Can you please take a look at this one?

Thanks,
Michal

RE: [PATCH] serial: xilinx_uartps: Fix race condition causing stuck TX

[Shubhrajyoti Datta]

> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Monday, November 1, 2021 5:58 PM
> To: Anssi Hannula <anssi.hannula@bitwise.fi>; Michal Simek
> <michals@xilinx.com>; Shubhrajyoti Datta <shubhraj@xilinx.com>
> Cc: linux-serial@vger.kernel.org; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; 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:
> > 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 <anssi.hannula@bitwise.fi>
> > ---
> >   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)
> >   	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 <Shubhrajyoti.datta@xilinx.com>