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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] serial: amba-pl011: use stale port->rs485 in pl011_rs485_config() when idle
- [Medium] serial: amba-pl011: missing break state adoption in pl011_rs485_tx_resume()
--

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

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

This commit prevents pl011_rs485_config() from stopping an ongoing RS485
transmission when RS485 settings are updated, and 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 3b9cce435c650..036a7245f1f29 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2506,8 +2506,6 @@ static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
>  		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
> @@ -2521,6 +2519,8 @@ static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
>  		bool tx_pending = uap->rs485_tx_state == WAIT_AFTER_RTS;
>  		u32 cr = pl011_read(uap, REG_CR);
>  
> +		uap->rs485_break = false;
> +
>  		/*
>  		 * Leaving RS485 mode, which only enables the transmitter while
>  		 * sending. Don't stop it, as that stalls a DMA transfer still
> @@ -2543,7 +2543,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);

[Severity: Medium]
What happens if RS485 is enabled via TIOCSRS485 while a break is active on
the port (or if RS485 is disabled during a break, clearing uap->rs485_break
above while UART01x_LCRH_BRK remains set in REG_LCRH_TX, and then re-enabled)?

While UART01x_LCRH_BRK is set, the PL011 keeps UART01x_FR_BUSY asserted in
REG_FR, so pl011_tx_pending() returns true via !pl011_tx_empty() and enters
pl011_rs485_tx_resume():

drivers/tty/serial/amba-pl011.c:pl011_rs485_tx_resume() {
    ...
	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;
	}
}

Because pl011_rs485_tx_resume() does not check REG_LCRH_TX for
UART01x_LCRH_BRK and set uap->rs485_break = true, uap->rs485_break stays
false. Could this cause pl011_trigger_stop_tx() -> pl011_rs485_tx_stop() to
bypass the uap->rs485_break check and continuously re-arm trigger_stop_tx
every character interval in hardirq context for the entire duration of the
break?

> -	} 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 TIOCSRS485, uart_ioctl() -> uart_set_rs485_config() invokes
port->rs485_config() before storing the new configuration into port->rs485:

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

And pl011_rs485_tx_stop() -> pl011_rs485_tx_stop_now() also reads
port->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;
    ...
}

Because port->rs485 still holds the old settings here, can this cause two
issues when configuring an idle port (uap->rs485_tx_state == OFF and
!pl011_tx_pending()) via TIOCSRS485:

1. When enabling RS485 for the first time, port->rs485.flags & SER_RS485_ENABLED
   is still false, so pl011_rs485_tx_stop() is skipped, leaving UART011_CR_TXE
   enabled while uap->rs485_tx_state == OFF and leaving UART011_CR_RTS at its
   RS232 level?

2. When RS485 is already enabled and TIOCSRS485 changes the RTS polarity
   (SER_RS485_RTS_ON_SEND vs 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, driving the wrong RTS
   idle level?

Should this branch check rs485->flags & SER_RS485_ENABLED and apply the new
rs485->flags when setting the idle state?

>

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