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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] serial: amba-pl011: stale port->rs485.flags used in pl011_rs485_config()
- [Medium] serial: amba-pl011: active TX REG_CR not updated in pl011_rs485_config()
- [Medium] serial: amba-pl011: pl011_tx_pending() mistakes queued data for active TX
--

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

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

This commit avoids calling pl011_rs485_tx_stop() in pl011_rs485_config()
when a transmission is already in progress, preventing the transmitter
from being disabled while TX interrupts are still pushing data into the
FIFO. It also preserves uap->rs485_break unless RS485 mode is being
disabled.

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 6adfe7a1e0487..14f0e8aa56fd2 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c

[ ... ]

> @@ -2550,7 +2550,9 @@ static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
>  		   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) {
> +	} 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: High]
Does this branch use stale flags from port->rs485 when called via
uart_set_rs485_config()?

In drivers/tty/serial/serial_core.c, uart_set_rs485_config() invokes
port->rs485_config() before updating port->rs485:

	scoped_guard(uart_port_lock_irqsave, port) {
		ret = port->rs485_config(port, &tty->termios, &rs485);
		if (!ret) {
			port->rs485 = rs485;

Because port->rs485 still holds the old settings inside
pl011_rs485_config(), two regressions can happen when uap->rs485_tx_state
is OFF:

1. When enabling RS485 from RS232 mode via TIOCSRS485 while no
transmission is pending (!pl011_tx_pending(uap, tx_stopped)),
port->rs485.flags does not have SER_RS485_ENABLED set yet. This branch is
skipped, leaving UART011_CR_TXE enabled from RS232 mode (which can also
strand paused DMA data if tx_stopped is true) and leaving UART011_CR_RTS
at its RS232 state rather than the RS485 idle level.

2. When RS485 is already enabled and TIOCSRS485 changes the RTS polarity
(SER_RS485_RTS_AFTER_SEND) while idle, pl011_rs485_tx_stop() calls
pl011_rs485_tx_stop_now(), which reads the old port->rs485.flags instead
of rs485->flags:

	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
		cr &= ~UART011_CR_RTS;
	else
		cr |= UART011_CR_RTS;

Should this condition check rs485->flags & SER_RS485_ENABLED, and pass the
new rs485 configuration down when setting the idle state?

[Severity: Medium]
What happens to REG_CR if SER_RS485_RTS_ON_SEND or SER_RS485_RX_DURING_TX
is reconfigured via TIOCSRS485 while a transmission is active
(uap->rs485_tx_state != OFF)?

When RS485 remains enabled and uap->rs485_tx_state is WAIT_AFTER_RTS,
SEND, WAIT_AFTER_SEND, or WAIT_AFTER_SEND_DELAY, pl011_rs485_config() now
skips all branches in this if/else-if chain.

However, pl011_rs485_tx_start() has already programmed UART011_CR_RTS and
UART011_CR_RXE in REG_CR using the old flags:
- In WAIT_AFTER_RTS, when trigger_start_tx expires,
  pl011_trigger_start_tx() sets uap->rs485_tx_state = SEND and calls
  pl011_start_tx() without re-running pl011_rs485_tx_start().
- In WAIT_AFTER_SEND or WAIT_AFTER_SEND_DELAY, if new TX data arrives,
  pl011_rs485_tx_start() cancels trigger_stop_tx, sets
  uap->rs485_tx_state = SEND, and returns early without updating REG_CR.

Won't this leave UART011_CR_RTS and UART011_CR_RXE at the old settings in
REG_CR for the rest of the active or pending transmission?

[Severity: Medium]
Can pl011_tx_pending() mistake queued data for an already active
transmission when called from pl011_rs485_config()?

In drivers/tty/serial/amba-pl011.c, pl011_tx_pending() checks:

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

If output was stopped (tty->flow.stopped is true) while uart_write()
queued data into tport->xmit_fifo (or pl011_stop_tx() paused a queued DMA
transfer), uap->rs485_tx_state is OFF and neither PIO (UART011_TXIM) nor
DMA (UART011_TXDMAE) is active in hardware.

When output is restarted in drivers/tty/tty_io.c:__start_tty():

	if (!tty->flow.stopped || tty->flow.tco_stopped)
		return;
	tty->flow.stopped = false;
	if (tty->ops->start)
		tty->ops->start(tty);

tty->flow.stopped is cleared under tty->flow.lock before uart_start()
acquires port->lock. If TIOCSRS485 runs pl011_rs485_config() under
port->lock during this window (or while __uart_start() has deferred
start_tx() to runtime PM resume, or between uart_put_char() and
uart_flush_chars()), tx_stopped is false and pl011_tx_pending() returns
true even though no transmission has started.

pl011_rs485_config() then calls pl011_rs485_tx_resume(), which asserts
RTS, enables UART011_CR_TXE, and sets uap->rs485_tx_state = SEND directly.
When uart_start() subsequently acquires port->lock and calls
pl011_start_tx(), won't it see uap->rs485_tx_state == SEND and transmit
immediately without waiting for delay_rts_before_send?

Should pl011_tx_pending() check whether TX is actually active in the
driver (such as uap->im & UART011_TXIM or uap->dmacr & UART011_TXDMAE)
rather than checking whether xmit_fifo or DMA has queued data while
!stopped?

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