[PATCH] tty: serial: amba-pl011: cancel RS485 trigger hrtimers before removal

[PATCH] tty: serial: amba-pl011: cancel RS485 trigger hrtimers before removal

[Fan Wu]

pl011_shutdown() calls pl011_rs485_tx_stop() when RS485 is enabled and
the transmit state machine is still active.  That function may re-arm
the trigger_stop_tx hrtimer via hrtimer_start() when the TX FIFO is not
yet empty, or when delay_rts_after_send is configured, and the trigger
timers can equally be left armed from a prior pl011_rs485_tx_start().

On removal, uart_remove_one_port() can finish without draining the RS485
hrtimers, and devm frees the uart_amba_port once pl011_remove() returns.
The existing teardown syncs do not help here: pl011_disable_interrupts()
only masks the UART IMSC register, pl011_dma_shutdown() cancels a
different (DMA rx) timer_list, and free_irq() plus serial_core's
synchronize_irq() only drain the UART interrupt handler, not the hrtimer
softirq callbacks.  Either trigger callback can therefore recover uap
from its embedded hrtimer and access uap->port after the free.

Cancel both RS485 trigger hrtimers in pl011_shutdown(), after the
re-arming pl011_rs485_tx_stop() call and before free_irq(), and again
in pl011_remove() after uart_remove_one_port().  The remove-side
calls cover paths such as suspend followed by unbind, where serial
core need not invoke the driver's shutdown callback.  The
callbacks pl011_trigger_start_tx and pl011_trigger_stop_tx return
HRTIMER_NORESTART, so hrtimer_cancel() fully drains any pending and
concurrently running callback without a self-restart loop.  As
pl011_trigger_start_tx() may re-enable the TX interrupt via
pl011_start_tx(), call pl011_disable_interrupts() again after the
cancels to re-mask IMSC before free_irq().  The cancel site does not
hold uart_port_lock across the cancel
(pl011_disable_interrupts() already released it) and the callbacks take
that lock internally, so there is no self-deadlock.  This mirrors the
RS485 teardown pattern in 8250_port.c (serial8250_em485_destroy),
which cancels both hrtimers before the embedding object is freed.

This issue was found by an in-house static analysis tool.

Fixes: 2c1fd53af21b ("serial: amba-pl011: Fix RTS handling in RS485 mode")
Cc: stable@vger.kernel.org   # v6.14+
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/tty/serial/amba-pl011.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8ed91e1da22b..021b542b3d99 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2083,6 +2083,16 @@ static void pl011_shutdown(struct uart_port *port)
 	if ((port->rs485.flags & SER_RS485_ENABLED && uap->rs485_tx_state != OFF))
 		pl011_rs485_tx_stop(uap);
 
+	/*
+	 * pl011_rs485_tx_stop() above may have (re-)armed the RS485 trigger
+	 * hrtimers.  A callback may also re-enable the TX interrupt, so
+	 * cancel them and mask interrupts again before freeing the IRQ.
+	 * pl011_remove() cancels them again before freeing the owner.
+	 */
+	hrtimer_cancel(&uap->trigger_start_tx);
+	hrtimer_cancel(&uap->trigger_stop_tx);
+	pl011_disable_interrupts(uap);
+
 	free_irq(uap->port.irq, uap);
 
 	pl011_disable_uart(uap);
@@ -3063,6 +3073,8 @@ static void pl011_remove(struct amba_device *dev)
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
 
 	uart_remove_one_port(&amba_reg, &uap->port);
+	hrtimer_cancel(&uap->trigger_start_tx);
+	hrtimer_cancel(&uap->trigger_stop_tx);
 	pl011_unregister_port(uap);
 }
 
-- 
2.34.1

Re: [PATCH] tty: serial: amba-pl011: cancel RS485 trigger hrtimers before removal

[Greg KH]

On Tue, Jul 21, 2026 at 04:28:58AM +0000, Fan Wu wrote:
> pl011_shutdown() calls pl011_rs485_tx_stop() when RS485 is enabled and
> the transmit state machine is still active.  That function may re-arm
> the trigger_stop_tx hrtimer via hrtimer_start() when the TX FIFO is not
> yet empty, or when delay_rts_after_send is configured, and the trigger
> timers can equally be left armed from a prior pl011_rs485_tx_start().
> 
> On removal, uart_remove_one_port() can finish without draining the RS485
> hrtimers, and devm frees the uart_amba_port once pl011_remove() returns.
> The existing teardown syncs do not help here: pl011_disable_interrupts()
> only masks the UART IMSC register, pl011_dma_shutdown() cancels a
> different (DMA rx) timer_list, and free_irq() plus serial_core's
> synchronize_irq() only drain the UART interrupt handler, not the hrtimer
> softirq callbacks.  Either trigger callback can therefore recover uap
> from its embedded hrtimer and access uap->port after the free.
> 
> Cancel both RS485 trigger hrtimers in pl011_shutdown(), after the
> re-arming pl011_rs485_tx_stop() call and before free_irq(), and again
> in pl011_remove() after uart_remove_one_port().  The remove-side
> calls cover paths such as suspend followed by unbind, where serial
> core need not invoke the driver's shutdown callback.  The
> callbacks pl011_trigger_start_tx and pl011_trigger_stop_tx return
> HRTIMER_NORESTART, so hrtimer_cancel() fully drains any pending and
> concurrently running callback without a self-restart loop.  As
> pl011_trigger_start_tx() may re-enable the TX interrupt via
> pl011_start_tx(), call pl011_disable_interrupts() again after the
> cancels to re-mask IMSC before free_irq().  The cancel site does not
> hold uart_port_lock across the cancel
> (pl011_disable_interrupts() already released it) and the callbacks take
> that lock internally, so there is no self-deadlock.  This mirrors the
> RS485 teardown pattern in 8250_port.c (serial8250_em485_destroy),
> which cancels both hrtimers before the embedding object is freed.
> 
> This issue was found by an in-house static analysis tool.

Yes, but a LLM obviously wrote the text here.  Please shorten it up a
LOT and make it more concise, as-is, it's impossible to read this
wall-of-text...

thanks,

greg k-h

Re: [PATCH] tty: serial: amba-pl011: cancel RS485 trigger hrtimers before removal

[Greg KH]

On Tue, Jul 21, 2026 at 04:28:58AM +0000, Fan Wu wrote:
> pl011_shutdown() calls pl011_rs485_tx_stop() when RS485 is enabled and
> the transmit state machine is still active.  That function may re-arm
> the trigger_stop_tx hrtimer via hrtimer_start() when the TX FIFO is not
> yet empty, or when delay_rts_after_send is configured, and the trigger
> timers can equally be left armed from a prior pl011_rs485_tx_start().
> 
> On removal, uart_remove_one_port() can finish without draining the RS485
> hrtimers, and devm frees the uart_amba_port once pl011_remove() returns.
> The existing teardown syncs do not help here: pl011_disable_interrupts()
> only masks the UART IMSC register, pl011_dma_shutdown() cancels a
> different (DMA rx) timer_list, and free_irq() plus serial_core's
> synchronize_irq() only drain the UART interrupt handler, not the hrtimer
> softirq callbacks.  Either trigger callback can therefore recover uap
> from its embedded hrtimer and access uap->port after the free.
> 
> Cancel both RS485 trigger hrtimers in pl011_shutdown(), after the
> re-arming pl011_rs485_tx_stop() call and before free_irq(), and again
> in pl011_remove() after uart_remove_one_port().  The remove-side
> calls cover paths such as suspend followed by unbind, where serial
> core need not invoke the driver's shutdown callback.  The
> callbacks pl011_trigger_start_tx and pl011_trigger_stop_tx return
> HRTIMER_NORESTART, so hrtimer_cancel() fully drains any pending and
> concurrently running callback without a self-restart loop.  As
> pl011_trigger_start_tx() may re-enable the TX interrupt via
> pl011_start_tx(), call pl011_disable_interrupts() again after the
> cancels to re-mask IMSC before free_irq().  The cancel site does not
> hold uart_port_lock across the cancel
> (pl011_disable_interrupts() already released it) and the callbacks take
> that lock internally, so there is no self-deadlock.  This mirrors the
> RS485 teardown pattern in 8250_port.c (serial8250_em485_destroy),
> which cancels both hrtimers before the embedding object is freed.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 2c1fd53af21b ("serial: amba-pl011: Fix RTS handling in RS485 mode")
> Cc: stable@vger.kernel.org   # v6.14+
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/tty/serial/amba-pl011.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 8ed91e1da22b..021b542b3d99 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2083,6 +2083,16 @@ static void pl011_shutdown(struct uart_port *port)
>  	if ((port->rs485.flags & SER_RS485_ENABLED && uap->rs485_tx_state != OFF))
>  		pl011_rs485_tx_stop(uap);
>  
> +	/*
> +	 * pl011_rs485_tx_stop() above may have (re-)armed the RS485 trigger
> +	 * hrtimers.  A callback may also re-enable the TX interrupt, so
> +	 * cancel them and mask interrupts again before freeing the IRQ.
> +	 * pl011_remove() cancels them again before freeing the owner.
> +	 */
> +	hrtimer_cancel(&uap->trigger_start_tx);
> +	hrtimer_cancel(&uap->trigger_stop_tx);
> +	pl011_disable_interrupts(uap);

Please read the review:
	https://sashiko.dev/#/patchset/20260721042858.3187311-1-fanwu01@zju.edu.cn
and determine if what it reports about this change is correct or not
when you submit your second version.

thanks,

greg k-h