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

[Nicolai Buchwitz <nb@tipi-net.de>]

On 17.10.2026 08:45, sashiko-bot@kernel.org wrote:

> [...]

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

Agree. I verified on my CM4 that BUSY stays set during a break and the 
drain timer
keeps polling until the break ends. Will fix this in v3.


> [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?

IMHO this is a pre-existing issue. I even have a fix for this in my 
local RS485
rework series, I plan to send for next cycle. So if there aren't any 
objections,
I will skip this for v3.

Thanks,
Nicolai