Re: [PATCH v4 2/2] serial: amba-pl011: keep RS485 transmission running on reconfiguration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] serial: amba-pl011: stale port->rs485 used in pl011_rs485_config()
--

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

serial: amba-pl011: keep RS485 transmission running on reconfiguration

This commit updates pl011_rs485_config() so that reconfiguring RS485 while it
remains enabled only calls pl011_rs485_tx_stop() when no transmission is in
progress (uap->rs485_tx_state == OFF), and only clears uap->rs485_break when
leaving RS485 mode.

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 1edbe75611e63..e4ac9225cdcba 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c

[ ... ]

> @@ -2540,7 +2541,9 @@ static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
>  		   tty_port_initialized(&port->state->port) &&
>  		   (pl011_tx_active(uap) || !pl011_tx_empty(port))) {
>  		pl011_rs485_tx_resume(uap, rs485);
> -	} else if (port->rs485.flags & SER_RS485_ENABLED) {
> +	} else if ((port->rs485.flags & SER_RS485_ENABLED) &&
> +		   uap->rs485_tx_state == OFF) {
> +		/* Set the idle level now, a transmission sets it when done */
>  		pl011_rs485_tx_stop(uap);
>  	}

[Severity: Medium]
During a TIOCSRS485 ioctl, uart_set_rs485_config() invokes
port->rs485_config() before assigning the new configuration to port->rs485:

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

In drivers/tty/serial/amba-pl011.c, pl011_rs485_config() calls
pl011_rs485_tx_stop() -> pl011_rs485_tx_stop_now(), which reads
port->rs485.flags rather than the new rs485->flags:

drivers/tty/serial/amba-pl011.c:pl011_rs485_tx_stop_now() {
	...
	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
		cr &= ~UART011_CR_RTS;
	else
		cr |= UART011_CR_RTS;
	...
}

Doesn't this cause three regressions when configuring RS485 via TIOCSRS485?

First, when RS485 is already enabled and idle (uap->rs485_tx_state == OFF)
and userspace changes SER_RS485_RTS_AFTER_SEND, pl011_rs485_tx_stop_now()
programs UART011_CR_RTS using the old port->rs485.flags instead of the new
rs485->flags.

Second, because the else-if condition in pl011_rs485_config() checks
port->rs485.flags & SER_RS485_ENABLED instead of
rs485->flags & SER_RS485_ENABLED, newly enabling RS485 on an open, idle port
(where pl011_tx_active(uap) is false and pl011_tx_empty(port) is true) skips
both pl011_rs485_tx_resume() and pl011_rs485_tx_stop(), leaving
UART011_CR_TXE enabled and UART011_CR_RTS at its previous non-RS485 level.

Third, when RS485 is reconfigured while a transmission or break is active
(uap->rs485_tx_state != OFF), all branches in pl011_rs485_config() are
skipped, so changes to SER_RS485_RTS_ON_SEND or SER_RS485_RX_DURING_TX in
rs485->flags are not applied to REG_CR for the ongoing transmission.

Should pl011_rs485_config() check rs485->flags & SER_RS485_ENABLED and apply
the new rs485->flags when setting the idle or active REG_CR state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-devel-nbuchwitz-pl011-rs485-reconfig-stall-v4-0-ed10e219159a@tipi-net.de?part=2