Re: [PATCH 03/24] serial: 8250: don't wait for the em485 timers locked

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7875 bytes --]

On Tue, 27 Oct 2026, Greg Kroah-Hartman wrote:

> From: Josef Bacik <josef@toxicpanda.com>
> 
> serial8250_em485_destroy() is called with the port lock held and
> interrupts off, from ->rs485_config() and from
> serial8250_unregister_port().  It calls hrtimer_cancel(), which waits
> for a handler which is running, and both handlers start by taking the
> port lock:
> 
>   CPU0                                  CPU1
>   ioctl(TIOCSRS485), rs485 off
>   uart_set_rs485_config()
>     uart_port_lock_irqsave()            serial8250_em485_handle_stop_tx()
>     serial8250_em485_config()             uart_port_lock_irqsave()
>       serial8250_em485_destroy()            spins
>         hrtimer_cancel()
>           spins
> 
> All it takes is to turn RS485 off right after the end of a transmission,
> when the timer for delay_rts_after_send expires.
> 
> The handlers do nothing unless em485->active_timer points to their
> timer, so it is enough to clear that pointer.  What cannot be done with
> the lock held is to free the structure: hrtimer_try_to_cancel() fails
> for a timer whose handler is running, even if it has been started again
> in the meantime.  So keep the structure in the port when RS485 is turned
> off, use it again when RS485 is turned on, and free it when the port is
> unregistered, where the timers can be waited for without the lock.
> 
> A port on which RS485 emulation has been used once keeps the structure,
> about 150 bytes, until it is unregistered.
> 
> Fixes: 6e0a5de2136b ("serial: 8250: Use hrtimers for rs485 delays")
> Cc: stable <stable@kernel.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@intel.com>
> Cc: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Cc: "Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>
> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
> Cc: Tate Whiteberg <whiteberg@arista.com>
> Cc: Christian Marangi <ansuelsmth@gmail.com>
> Cc: John Ogness <john.ogness@linutronix.de>
> Cc: Crescent Hsieh <crescentcy.hsieh@moxa.com>
> Cc: Fushuai Wang <wangfushuai@baidu.com>
> Cc: Rafael Gago Castano <rgc@hms.se>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/tty/serial/8250/8250.h      |  1 +
>  drivers/tty/serial/8250/8250_core.c |  5 +---
>  drivers/tty/serial/8250/8250_port.c | 60 ++++++++++++++++++++++++++++++-------
>  include/linux/serial_8250.h         |  1 +
>  4 files changed, 52 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 21ed9c41b751..82a3b8359643 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -235,6 +235,7 @@ int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
>  void serial8250_em485_start_tx(struct uart_8250_port *p, bool toggle_ier);
>  void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier);
>  void serial8250_em485_destroy(struct uart_8250_port *p);
> +void serial8250_em485_free(struct uart_8250_port *p);
>  extern struct serial_rs485 serial8250_em485_supported;
>  
>  /* MCR <-> TIOCM conversion */
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 42319843cb5b..0951d3e42e1f 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -947,10 +947,7 @@ void serial8250_unregister_port(struct uart_8250_port *uart)
>  {
>  	guard(mutex)(&serial_mutex);
>  
> -	if (uart->em485) {
> -		guard(uart_port_lock_irqsave)(&uart->port);
> -		serial8250_em485_destroy(uart);
> -	}
> +	serial8250_em485_free(uart);
>  
>  	uart_remove_one_port(&serial8250_reg, &uart->port);
>  	if (serial8250_isa_devs) {
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 36b2b0a2c05c..3d47f535d6f1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -574,15 +574,22 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>  	if (p->em485)
>  		goto deassert_rts;
>  
> -	p->em485 = kmalloc_obj(struct uart_8250_em485, GFP_ATOMIC);
> -	if (!p->em485)
> -		return -ENOMEM;
> +	if (p->em485_idle) {
> +		p->em485 = p->em485_idle;
> +		p->em485_idle = NULL;
> +	} else {
> +		p->em485 = kmalloc_obj(struct uart_8250_em485, GFP_ATOMIC);
> +		if (!p->em485)
> +			return -ENOMEM;
>  
> -	hrtimer_setup(&p->em485->stop_tx_timer, &serial8250_em485_handle_stop_tx, CLOCK_MONOTONIC,
> -		      HRTIMER_MODE_REL);
> -	hrtimer_setup(&p->em485->start_tx_timer, &serial8250_em485_handle_start_tx, CLOCK_MONOTONIC,
> -		      HRTIMER_MODE_REL);
> -	p->em485->port = p;
> +		hrtimer_setup(&p->em485->stop_tx_timer,
> +			      &serial8250_em485_handle_stop_tx, CLOCK_MONOTONIC,
> +			      HRTIMER_MODE_REL);
> +		hrtimer_setup(&p->em485->start_tx_timer,
> +			      &serial8250_em485_handle_start_tx, CLOCK_MONOTONIC,
> +			      HRTIMER_MODE_REL);
> +		p->em485->port = p;
> +	}
>  	p->em485->active_timer = NULL;
>  	p->em485->tx_stopped = true;
>  
> @@ -604,6 +611,11 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>   *	The function is supposed to be called from .rs485_config callback
>   *	or from any other callback protected with p->port.lock spinlock.
>   *
> + *	The handlers of the timers take that lock, so they cannot be waited for
> + *	here. One which is running does nothing, as no timer is active any more.
> + *	The memory is kept for the next serial8250_em485_init() and freed by
> + *	serial8250_em485_free().
> + *
>   *	See also serial8250_em485_init()
>   */
>  void serial8250_em485_destroy(struct uart_8250_port *p)
> @@ -611,14 +623,40 @@ void serial8250_em485_destroy(struct uart_8250_port *p)
>  	if (!p->em485)
>  		return;
>  
> -	hrtimer_cancel(&p->em485->start_tx_timer);
> -	hrtimer_cancel(&p->em485->stop_tx_timer);
> +	hrtimer_try_to_cancel(&p->em485->start_tx_timer);
> +	hrtimer_try_to_cancel(&p->em485->stop_tx_timer);
> +	p->em485->active_timer = NULL;
>  
> -	kfree(p->em485);
> +	p->em485_idle = p->em485;
>  	p->em485 = NULL;
>  }
>  EXPORT_SYMBOL_GPL(serial8250_em485_destroy);

The name is pretty misleading after this change when all it now does is 
stop timers.

--
 i.

>  
> +/**
> + *	serial8250_em485_free() - release the rs485 emulation of a port
> + *	@p:	uart_8250_port port instance
> + *
> + *	Must be called without p->port.lock, as it waits for the timers.
> + */
> +void serial8250_em485_free(struct uart_8250_port *p)
> +{
> +	struct uart_8250_em485 *em485;
> +
> +	scoped_guard(uart_port_lock_irqsave, &p->port) {
> +		serial8250_em485_destroy(p);
> +		em485 = p->em485_idle;
> +		p->em485_idle = NULL;
> +	}
> +
> +	if (!em485)
> +		return;
> +
> +	hrtimer_cancel(&em485->start_tx_timer);
> +	hrtimer_cancel(&em485->stop_tx_timer);
> +	kfree(em485);
> +}
> +EXPORT_SYMBOL_GPL(serial8250_em485_free);
> +
>  struct serial_rs485 serial8250_em485_supported = {
>  	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND |
>  		 SER_RS485_TERMINATE_BUS | SER_RS485_RX_DURING_TX,
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index d91c0477be9d..0556bb722db4 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -173,6 +173,7 @@ struct uart_8250_port {
>  	void			(*dl_write)(struct uart_8250_port *up, u32 value);
>  
>  	struct uart_8250_em485 *em485;
> +	struct uart_8250_em485 *em485_idle;	/* kept while rs485 is off */
>  	void			(*rs485_start_tx)(struct uart_8250_port *up, bool toggle_ier);
>  	void			(*rs485_stop_tx)(struct uart_8250_port *up, bool toggle_ier);
>  
> 
>