Re: [PATCH v3 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 2 potential issue(s) to consider:
- [High] serial: amba-pl011: pl011_tx_pending() treats unstarted TX as active
- [High] serial: amba-pl011: old port->rs485 used in pl011_rs485_config() idle path
--

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

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

When RS485 is enabled on an open port while a transmission from before RS485
was enabled is still running, this commit hands the transmission back to the
RS485 state machine so that the idle RTS level is set when it finishes.

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 79776a882ae35..6adfe7a1e0487 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1734,6 +1734,18 @@ static unsigned int pl011_tx_empty(struct uart_port *port)
>  							0 : TIOCSER_TEMT;
>  }
>  
> +/* While output is stopped, only data already in the FIFO counts */
> +static bool pl011_tx_pending(struct uart_amba_port *uap, bool stopped)
> +{
> +	struct tty_port *tport = &uap->port.state->port;
> +
> +	if (!pl011_tx_empty(&uap->port))
> +		return true;
> +
> +	return !stopped &&
> +	       (!kfifo_is_empty(&tport->xmit_fifo) || pl011_dma_tx_queued(uap));
> +}
> +
>  static void pl011_maybe_set_bit(bool cond, unsigned int *ptr, unsigned int mask)
>  {
>  	if (cond)
> @@ -2454,13 +2466,63 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
>  	return ret;
>  }
>  
> +/*
> + * Data from before RS485 was enabled 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, bool stopped)
> +{
> +	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);
> +
> +	/* pl011_break_ctl() ends the transmission when the break ends */
> +	if (pl011_read(uap, REG_LCRH_TX) & UART01x_LCRH_BRK) {
> +		uap->rs485_break = true;
> +		uap->rs485_tx_state = SEND;
> +		return;
> +	}
> +
> +	/*
> +	 * With only the FIFO left or output stopped, the TX path won't end it.
> +	 * The timer does, once the new settings are stored.
> +	 */
> +	if (stopped ||
> +	    (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);
> +	} else {
> +		uap->rs485_tx_state = SEND;
> +	}
> +}

[Severity: High]
Can pl011_tx_pending() and pl011_rs485_tx_resume() treat an unstarted
transmission as already active in hardware when pl011_tx_empty(&uap->port) is
true?

When !stopped && (!kfifo_is_empty(&tport->xmit_fifo) ||
pl011_dma_tx_queued(uap)), pl011_tx_pending() returns true and
pl011_rs485_tx_resume() asserts RTS, enables UART011_CR_TXE, and sets
uap->rs485_tx_state = SEND without starting PIO/DMA or arming trigger_stop_tx.
This assumes that either PIO (uap->im & UART011_TXIM) or DMA (uap->dmacr &
UART011_TXDMAE) is already active and will later invoke pl011_stop_tx().

However, there appear to be two paths where xmit_fifo is non-empty (or
dmatx.queued is true) and tx_stopped is false while neither UART011_TXIM nor
UART011_TXDMAE is active and the hardware FIFO is empty:

1. Asynchronous runtime PM resume on port->port_dev->dev:
amba_probe() enables runtime PM on port->dev, and serial_port_probe() enables
runtime PM autosuspend on port->port_dev->dev. When port_dev->dev is
runtime-suspended, uart_write() queues data in tport->xmit_fifo and calls
__uart_start(). Because pm_runtime_get(&port_dev->dev) is asynchronous,
__uart_start() returns without calling port->ops->start_tx(port), deferring
pl011_start_tx() to serial_port_runtime_resume().

If ioctl(TIOCSRS485) runs before serial_port_runtime_resume() acquires
port->lock, pl011_rs485_tx_resume() sets uap->rs485_tx_state = SEND and
asserts RTS. When serial_port_runtime_resume() then calls pl011_start_tx(),
it sees uap->rs485_tx_state == SEND and transmits immediately, bypassing
delay_rts_before_send. Furthermore, if tcflush(fd, TCOFLUSH) (via
uart_flush_buffer()) clears xmit_fifo before serial_port_runtime_resume()
runs, serial_port_runtime_resume() sees __serial_port_busy(port) == false and
skips start_tx(), leaving uap->rs485_tx_state stuck in SEND with RTS asserted
and driving the RS485 bus indefinitely.

2. Concurrent __start_tty() (via TCXONC/TCOON or IXON START_CHAR):
n_tty_ioctl_helper() and n_tty_lookahead_flow_ctrl() call __start_tty()
without holding termios_rwsem or port->mutex. __start_tty() clears
tty->flow.stopped under tty->flow.lock before uart_start() acquires
port->lock to call pl011_start_tx(). If data was queued in xmit_fifo while
output was stopped (or DMA was paused with dmatx.queued == true and the
hardware FIFO drained) and pl011_rs485_config() runs in that window,
tx_stopped is false, so pl011_rs485_tx_resume() sets uap->rs485_tx_state =
SEND and uart_start() -> pl011_start_tx() skips delay_rts_before_send.

> +
>  static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
>  			      struct serial_rs485 *rs485)
>  {
>  	struct uart_amba_port *uap =
>  		container_of(port, struct uart_amba_port, port);
> +	bool tx_stopped = uart_tx_stopped(port);
>  
>  	uap->rs485_break = false;
> +
> +	/*
> +	 * Output is marked stopped before stop_tx pauses DMA. Pause it now, so
> +	 * that clearing TXE below cannot strand DMA data in the FIFO. Output
> +	 * stopped after this check is ended by stop_tx.
> +	 */
> +	if ((rs485->flags & SER_RS485_ENABLED) && tx_stopped)
> +		pl011_dma_tx_stop(uap);
> +
>  	if ((port->rs485.flags & SER_RS485_ENABLED) &&
>  	    !(rs485->flags & SER_RS485_ENABLED)) {
>  		bool tx_pending = uap->rs485_tx_state == WAIT_AFTER_RTS;
> @@ -2483,6 +2545,11 @@ static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
>  		if (tx_pending && !uart_tx_stopped(port) &&
>  		    !pl011_dma_tx_start(uap))
>  			pl011_start_tx_pio(uap);
> +	} else if ((rs485->flags & SER_RS485_ENABLED) &&
> +		   uap->rs485_tx_state == OFF &&
> +		   tty_port_initialized(&port->state->port) &&
> +		   pl011_tx_pending(uap, tx_stopped)) {
> +		pl011_rs485_tx_resume(uap, rs485, tx_stopped);
>  	} else if (port->rs485.flags & SER_RS485_ENABLED) {
>  		pl011_rs485_tx_stop(uap);
>  	}

[Severity: High]
When enabling RS485 via TIOCSRS485 while pl011_tx_pending(uap, tx_stopped) is
false, doesn't checking port->rs485.flags in the final else-if branch skip
pl011_rs485_tx_stop(uap) because port->rs485 still holds the old configuration?

In uart_set_rs485_config(), port->rs485_config(port, &tty->termios, &rs485) is
called before port->rs485 = rs485 is assigned:

drivers/tty/serial/serial_core.c:uart_set_rs485_config() {
    ...
	ret = port->rs485_config(port, &tty->termios, &rs485);
	if (!ret) {
		port->rs485 = rs485;
    ...
}

When RS485 was previously disabled, UART011_CR_TXE is set in REG_CR (either
from pl011_startup() or from disabling RS485 in pl011_rs485_config()) and
UART011_CR_RTS reflects the RS232 mctrl state. If RS485 is then enabled when
no transmission is pending (or when tx_stopped is true and the hardware FIFO
is empty, which is the case the new pl011_dma_tx_stop(uap) call above prepares
for so that clearing TXE below cannot strand DMA data), pl011_tx_pending()
returns false.

Execution then reaches:

	} else if (port->rs485.flags & SER_RS485_ENABLED) {
		pl011_rs485_tx_stop(uap);
	}

Because port->rs485.flags does not yet have SER_RS485_ENABLED set,
pl011_rs485_tx_stop() is not called (and if pl011_stop_tx() runs later in the
tx_stopped case, it also skips pl011_rs485_tx_stop() because
uap->rs485_tx_state == OFF). As a result, UART011_CR_TXE is not cleared and
UART011_CR_RTS is not set to the RS485 idle level.

Additionally, even when port->rs485.flags already had SER_RS485_ENABLED set
and TIOCSRS485 reconfigures SER_RS485_RTS_AFTER_SEND while idle,
pl011_rs485_tx_stop() -> pl011_rs485_tx_stop_now() reads
port->rs485.flags & SER_RS485_RTS_AFTER_SEND from the old port->rs485 rather
than the new rs485 struct.

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