Re: [PATCH 1/2] serial: amba-pl011: finish a transmission still running when RS485 is enabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: amba-pl011: zero-interval hrtimer storm causing lockup
--

commit 7358b5653ca0d3eb43821ade63d833e853201457
Author: Nicolai Buchwitz <nb@tipi-net.de>

serial: amba-pl011: finish a transmission still running when RS485 is enabled

This commit addresses an issue in the amba-pl011 serial driver where
disabling and re-enabling RS485 during an ongoing transmission could leave
the state OFF while data is still going out. It introduces a mechanism to
resume the RS485 transmission and drain the FIFO properly.

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 79776a882ae35..057bb74f3d85d 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c

[ ... ]

> @@ -2454,6 +2466,38 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
>  	return ret;
>  }
>  
> +/*
> + * RS485 is enabled while data sent before is still going out. Finish it as
> + * an RS485 transmission, so that its end sets the idle level.
> + */
> +static void pl011_rs485_tx_resume(struct uart_amba_port *uap,
> +				  const struct serial_rs485 *rs485)
> +{
> +	struct tty_port *tport = &uap->port.state->port;
> +	u32 cr = pl011_read(uap, REG_CR);
> +
> +	cr |= UART011_CR_TXE;
> +	if (!(rs485->flags & SER_RS485_RX_DURING_TX))
> +		cr &= ~UART011_CR_RXE;
> +	if (rs485->flags & SER_RS485_RTS_ON_SEND)
> +		cr &= ~UART011_CR_RTS;
> +	else
> +		cr |= UART011_CR_RTS;
> +	pl011_write(cr, uap, REG_CR);
> +
> +	/*
> +	 * With only the FIFO left, the TX path does not stop the transmission.
> +	 * Drain it from the timer, which runs once the new settings are stored.
> +	 */
> +	if (kfifo_is_empty(&tport->xmit_fifo) && !pl011_dma_tx_queued(uap)) {
> +		uap->rs485_tx_state = WAIT_AFTER_SEND;
> +		hrtimer_start(&uap->trigger_stop_tx, uap->rs485_tx_drain_interval,
> +			      HRTIMER_MODE_REL);

[Severity: High]
Could uap->rs485_tx_drain_interval be zero here if this path is reached
before pl011_set_termios() has executed?

If RS485 is enabled at boot via device tree, uart_add_one_port() calls
pl011_rs485_config() before set_termios() runs. If the bootloader or
earlycon left characters in the hardware TX FIFO, pl011_tx_pending() will
evaluate to true and trigger this new pl011_rs485_tx_resume() path.

Since set_termios() hasn't run yet, rs485_tx_drain_interval is 0.
The timer will expire immediately in hardirq context and call
pl011_rs485_tx_stop().

Because the hardware FIFO is still not empty, pl011_rs485_tx_stop() will
observe the condition and endlessly reschedule itself with the same 0 
interval:

drivers/tty/serial/amba-pl011.c:pl011_rs485_tx_stop() {
    ...
    if (uap->rs485_tx_state == WAIT_AFTER_SEND) {
        if (!pl011_tx_empty(port)) {
            hrtimer_start(&uap->trigger_stop_tx,
                          uap->rs485_tx_drain_interval,
                          HRTIMER_MODE_REL);
            return;
        }
    ...
}

Would this create an interrupt storm that stalls the CPU until the hardware
FIFO drains (or permanently, if hardware flow control prevents draining)?

Note that the follow-up patch (Patch 2 of 2) in this series modifies
pl011_rs485_config(), but appears to leave this vulnerable condition
exactly the same.

> +	} else {
> +		uap->rs485_tx_state = SEND;
> +	}
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-devel-nbuchwitz-pl011-rs485-reconfig-stall-v1-0-f07d4da714be@tipi-net.de?part=1