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

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

[Marek Vasut]

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);
 
-- 
2.45.2

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

[Esben Haabendal]

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>

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3754 bytes --]

On Wed, Oct 02, 2024 at 06:11:16AM +0200, Marek Vasut wrote:
> 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.

Why is that? Just because these don't stop transmission?

> Fixes: ceca629e0b48 ("[ARM] 2971/1: i.MX uart handle rts irq")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  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);

I didn't grab the whole picture, but I think this deserves a code
comment. 

Would it make sense to replace the current code in __imx_uart_rtsint by
a call to imx_uart_mctrl_check()?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marek Vasut]

On 10/2/24 9:49 AM, Uwe Kleine-König wrote:
> On Wed, Oct 02, 2024 at 06:11:16AM +0200, Marek Vasut wrote:
>> 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.
> 
> Why is that? Just because these don't stop transmission?

If imx_uart_mctrl_check() does not detect the RTS asserted->deasserted 
transition, it will never call uart_handle_cts_change(), which will 
never clear port->hw_stopped and the port will remain stopped 
indefinitely, and never be able to transmit more data AFTER this event 
happens (port close/open will reset the flag too, but that is undesired 
workaround).

>> Fixes: ceca629e0b48 ("[ARM] 2971/1: i.MX uart handle rts irq")
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>>   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);
> 
> I didn't grab the whole picture, but I think this deserves a code
> comment.

Added in V2.

> Would it make sense to replace the current code in __imx_uart_rtsint by
> a call to imx_uart_mctrl_check()?
No, the __imx_uart_rtsint() only handles RTS state change interrupt and 
no other interrupts, so calling imx_uart_mctrl_check() would handle 
other unrelated signals for no reason and make the interrupt handler 
slower. I don't think this is an improvement.