[PATCH 0/2] serial: amba-pl011: fix RS485 state on enable and reconfiguration

[PATCH 0/2] serial: amba-pl011: fix RS485 state on enable and reconfiguration

[Nicolai Buchwitz]

More RS485 fixes for pl011, on top of the ones already in tty-next.
Together they are the groundwork for a series I plan to send next
cycle, which refactors the RS485 code for better performance and
reliability.

Both patches fix changing the RS485 settings while data is going out.
Patch 1 keeps the transceiver from driving the bus after the data is
out when RS485 is enabled mid-transmission. Patch 2 keeps a
reconfiguration from stalling the port.

---
Nicolai Buchwitz (2):
      serial: amba-pl011: finish a transmission still running when RS485 is enabled
      serial: amba-pl011: keep RS485 transmission running on reconfiguration

 drivers/tty/serial/amba-pl011.c | 54 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)
---
base-commit: 1184f87e2790e8af9be3116f608d2e86b4792d7d
change-id: 20261016-devel-nbuchwitz-pl011-rs485-reconfig-stall-f4fe437cccae

Best regards,
--  
Nicolai Buchwitz <nb@tipi-net.de>

[PATCH 1/2] serial: amba-pl011: finish a transmission still running when RS485 is enabled

[Nicolai Buchwitz]

Disabling RS485 keeps a transmission in progress running. If RS485 is
enabled again before it ends, the state is OFF while data is still
going out. The TX path does not stop a transmission in state OFF, so
nothing sets the idle RTS level once the data is out and the
transceiver keeps driving the bus.

Hand such a transmission back to RS485 when it is enabled. Assert RTS
and mark it as sending, so that the TX path ends it like any other.
With only the FIFO left the TX path is already done, so drain it from
trigger_stop_tx instead.

Fixes: 97f85204275a ("serial: amba-pl011: keep transmitter enabled when disabling RS485")
Cc: stable@vger.kernel.org
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/tty/serial/amba-pl011.c | 47 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 79776a882ae3..057bb74f3d85 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;
 }
 
+/*
+ * Data still queued or on its way out. After RS485 was turned off and on
+ * again during a transmission, the state is OFF while this is true.
+ */
+static bool pl011_tx_pending(struct uart_amba_port *uap)
+{
+	struct tty_port *tport = &uap->port.state->port;
+
+	return !kfifo_is_empty(&tport->xmit_fifo) || pl011_dma_tx_queued(uap) ||
+	       !pl011_tx_empty(&uap->port);
+}
+
 static void pl011_maybe_set_bit(bool cond, unsigned int *ptr, unsigned int mask)
 {
 	if (cond)
@@ -2454,6 +2466,38 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
 	return ret;
 }
 
+/*
+ * RS485 is enabled while data sent before 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)
+{
+	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);
+
+	/*
+	 * With only the FIFO left, the TX path does not stop the transmission.
+	 * Drain it from the timer, which runs once the new settings are stored.
+	 */
+	if (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;
+	}
+}
+
 static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
 			      struct serial_rs485 *rs485)
 {
@@ -2483,6 +2527,9 @@ 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 && pl011_tx_pending(uap)) {
+		pl011_rs485_tx_resume(uap, rs485);
 	} else if (port->rs485.flags & SER_RS485_ENABLED) {
 		pl011_rs485_tx_stop(uap);
 	}

-- 
2.53.0

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

[Nicolai Buchwitz]

Changing the RS485 settings while RS485 stays enabled calls
pl011_rs485_tx_stop(), also while a transmission is in progress. Unlike
pl011_stop_tx() it leaves the TX interrupt enabled, so the remaining
data is still pushed into the FIFO after the drain turned off the
transmitter. The FIFO never empties, the port stalls, and closing it
hangs in pl011_dma_shutdown() waiting for the transmitter to go idle.
A start still waiting for delay_rts_before_send is dropped as well.

Only stop the transmitter when no transmission is in progress. The
call is still needed there, it sets the idle RTS level when the port
is configured or resumed. A transmission in progress sets that level
when it ends.

An active break counts as a transmission too. Forget it only when
leaving RS485 mode, so that pl011_break_ctl() still ends the
transmission when the break ends.

Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support")
Cc: stable@vger.kernel.org
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/tty/serial/amba-pl011.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 057bb74f3d85..66130a6d41fc 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2504,12 +2504,13 @@ static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
 	struct uart_amba_port *uap =
 		container_of(port, struct uart_amba_port, port);
 
-	uap->rs485_break = false;
 	if ((port->rs485.flags & SER_RS485_ENABLED) &&
 	    !(rs485->flags & SER_RS485_ENABLED)) {
 		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
@@ -2530,7 +2531,9 @@ static int pl011_rs485_config(struct uart_port *port, struct ktermios *termios,
 	} else if ((rs485->flags & SER_RS485_ENABLED) &&
 		   uap->rs485_tx_state == OFF && pl011_tx_pending(uap)) {
 		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);
 	}
 

-- 
2.53.0

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

[sashiko-bot]

> Changing the RS485 settings while RS485 stays enabled calls
> pl011_rs485_tx_stop(), also while a transmission is in progress. Unlike
> pl011_stop_tx() it leaves the TX interrupt enabled, so the remaining
> data is still pushed into the FIFO after the drain turned off the
> transmitter. The FIFO never empties, the port stalls, and closing it
> hangs in pl011_dma_shutdown() waiting for the transmitter to go idle.
> A start still waiting for delay_rts_before_send is dropped as well.
> 
> Only stop the transmitter when no transmission is in progress. The
> call is still needed there, it sets the idle RTS level when the port
> is configured or resumed. A transmission in progress sets that level
> when it ends.
> 
> An active break counts as a transmission too. Forget it only when
> leaving RS485 mode, so that pl011_break_ctl() still ends the
> transmission when the break ends.
> 
> Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH 1/2] serial: amba-pl011: finish a transmission still running when RS485 is enabled

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: amba-pl011: zero-interval hrtimer storm causing lockup
--

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

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

This commit addresses an issue in the amba-pl011 serial driver where
disabling and re-enabling RS485 during an ongoing transmission could leave
the state OFF while data is still going out. It introduces a mechanism to
resume the RS485 transmission and drain the FIFO properly.

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 79776a882ae35..057bb74f3d85d 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c

[ ... ]

> @@ -2454,6 +2466,38 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
>  	return ret;
>  }
>  
> +/*
> + * RS485 is enabled while data sent before 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)
> +{
> +	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);
> +
> +	/*
> +	 * With only the FIFO left, the TX path does not stop the transmission.
> +	 * Drain it from the timer, which runs once the new settings are stored.
> +	 */
> +	if (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);

[Severity: High]
Could uap->rs485_tx_drain_interval be zero here if this path is reached
before pl011_set_termios() has executed?

If RS485 is enabled at boot via device tree, uart_add_one_port() calls
pl011_rs485_config() before set_termios() runs. If the bootloader or
earlycon left characters in the hardware TX FIFO, pl011_tx_pending() will
evaluate to true and trigger this new pl011_rs485_tx_resume() path.

Since set_termios() hasn't run yet, rs485_tx_drain_interval is 0.
The timer will expire immediately in hardirq context and call
pl011_rs485_tx_stop().

Because the hardware FIFO is still not empty, pl011_rs485_tx_stop() will
observe the condition and endlessly reschedule itself with the same 0 
interval:

drivers/tty/serial/amba-pl011.c:pl011_rs485_tx_stop() {
    ...
    if (uap->rs485_tx_state == WAIT_AFTER_SEND) {
        if (!pl011_tx_empty(port)) {
            hrtimer_start(&uap->trigger_stop_tx,
                          uap->rs485_tx_drain_interval,
                          HRTIMER_MODE_REL);
            return;
        }
    ...
}

Would this create an interrupt storm that stalls the CPU until the hardware
FIFO drains (or permanently, if hardware flow control prevents draining)?

Note that the follow-up patch (Patch 2 of 2) in this series modifies
pl011_rs485_config(), but appears to leave this vulnerable condition
exactly the same.

> +	} else {
> +		uap->rs485_tx_state = SEND;
> +	}
> +}
> +

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