Re: [PATCH] serial: imx: Update mctrl old_status on RTSD interrupt

[Esben Haabendal <esben@geanix.com>]

Marek Vasut <marex@denx.de> writes:

> When sending data using DMA at high baudrate (4 Mbdps in local test case) to
> a device with small RX buffer which keeps asserting RTS after every received
> byte, it is possible that the iMX UART driver would not recognize the falling
> edge of RTS input signal and get stuck, unable to transmit any more data.
>
> This condition happens when the following sequence of events occur:
> - imx_uart_mctrl_check() is called at some point and takes a snapshot of UART
>   control signal status into sport->old_status using imx_uart_get_hwmctrl().
>   The RTSS/TIOCM_CTS bit is of interest here (*).
> - DMA transfer occurs, the remote device asserts RTS signal after each byte.
>   The i.MX UART driver recognizes each such RTS signal change, raises an
>   interrupt with USR1 register RTSD bit set, which leads to invocation of
>   __imx_uart_rtsint(), which calls uart_handle_cts_change().
>   - If the RTS signal is deasserted, uart_handle_cts_change() clears
>     port->hw_stopped and unblocks the port for further data transfers.
>   - If the RTS is asserted, uart_handle_cts_change() sets port->hw_stopped
>     and blocks the port for further data transfers. This may occur as the
>     last interrupt of a transfer, which means port->hw_stopped remains set
>     and the port remains blocked (**).
> - Any further data transfer attempts will trigger imx_uart_mctrl_check(),
>   which will read current status of UART control signals by calling
>   imx_uart_get_hwmctrl() (***) and compare it with sport->old_status .
>   - If current status differs from sport->old_status for RTS signal,
>     uart_handle_cts_change() is called and possibly unblocks the port
>     by clearing port->hw_stopped .
>   - If current status does not differ from sport->old_status for RTS
>     signal, no action occurs. This may occur in case prior snapshot (*)
>     was taken before any transfer so the RTS is deasserted, current
>     snapshot (***) was taken after a transfer and therefore RTS is
>     deasserted again, which means current status and sport->old_status
>     are identical. In case (**) triggered when RTS got asserted, and
>     made port->hw_stopped set, the port->hw_stopped will remain set
>     because no change on RTS line is recognized by this driver and
>     uart_handle_cts_change() is not called from here to unblock the
>     port->hw_stopped.
>
> Update sport->old_status in __imx_uart_rtsint() accordingly to make
> imx_uart_mctrl_check() detect such RTS change. Note that TIOCM_CAR
> and TIOCM_RI bits in sport->old_status do not suffer from this problem.
>
> Fixes: ceca629e0b48 ("[ARM] 2971/1: i.MX uart handle rts irq")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Cc: Esben Haabendal <esben@geanix.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Rickard x Andersson <rickaran@axis.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-serial@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
>  drivers/tty/serial/imx.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 67d4a72eda770..3ad7f42790ef9 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -762,6 +762,10 @@ static irqreturn_t __imx_uart_rtsint(int irq, void *dev_id)
>  
>  	imx_uart_writel(sport, USR1_RTSD, USR1);
>  	usr1 = imx_uart_readl(sport, USR1) & USR1_RTSS;
> +	if (usr1 & USR1_RTSS)
> +		sport->old_status |= TIOCM_CTS;
> +	else
> +		sport->old_status &= ~TIOCM_CTS;
>  	uart_handle_cts_change(&sport->port, usr1);
>  	wake_up_interruptible(&sport->port.state->port.delta_msr_wait);

Reviewed-by: Esben Haabendal <esben@geanix.com>