Re: [PATCH v4] serial: max310x: drive RTS in software when hardware delays are too short

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] serial: max310x: race conditions from dropping port->lock in start_tx and rs485_config
- [High] serial: max310x: tx_work bypasses RS485 RTS before_send delay
- [Medium] serial: max310x: torn read of 64-bit one_character_duration on 32-bit platforms
--

commit 5b4790196e82e4dace1f26d242e90569b9edc027
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: drive RTS in software when hardware delays are too short
    
This commit adds a software-driven RTS path for the max310x UART driver to
handle RS485 setup and hold delays that exceed the hardware's capabilities.
It uses an hrtimer and worker threads to manually toggle RTS and drive data out.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 022502986c5fc..c217857f5d32a 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c

[ ... ]

> @@ -787,8 +907,13 @@ static void max310x_handle_tx(struct uart_port *port)
>  		return;
>  	}
>  
> -	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port))
> +	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port)) {
> +		struct max310x_one *one = to_max310x_port(port);
> +
> +		if (READ_ONCE(one->sw_rts_during_tx))
> +			max310x_delayed_stop_tx(port);
>  		return;
> +	}
>  
>  	/*
>  	 * It's a circ buffer -- wrap around.
>  	 * We could do that in one SPI transaction, but meh.
>  	 */
>  	while (!kfifo_is_empty(&tport->xmit_fifo)) {
>  		unsigned int txlen, to_send;

[Severity: High]
Does this bypass the before_send delay if an interrupt triggers tx_work early?

If any interrupt, such as an RX interrupt, occurs during the
MAX310X_TX_WAIT_BEFORE_SEND window, max310x_port_irq() reads IRQSTS and
observes the TXEMPTY bit is set. It then independently schedules tx_work.

Since max310x_handle_tx() doesn't verify if tx_state is MAX310X_TX_SEND,
could it execute prematurely and pump data into the hardware FIFO, shifting
it out before the RTS setup delay completes?

[ ... ]

> @@ -813,11 +938,48 @@ static void max310x_handle_tx(struct uart_port *port)
>  		uart_write_wakeup(port);
>  }
>  
> +/* called with port.lock taken and irqs off */
>  static void max310x_start_tx(struct uart_port *port)
>  {
>  	struct max310x_one *one = to_max310x_port(port);
>  
> -	schedule_work(&one->tx_work);
> +	if (READ_ONCE(one->sw_rts_during_tx)) {
> +		/*
> +		 * The before- and after-send phases share one delay timer. If an
> +		 * after-send release is pending, cancel it before starting a new
> +		 * TX so the just-asserted RTS is not yanked; re-arming the timer
> +		 * for the before-send phase then supersedes the release.
> +		 */
> +		int res = 0;
> +
> +		if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_AFTER_SEND)
> +			res = hrtimer_try_to_cancel(&one->tx_delay_tmr);
> +		if (unlikely(res == -1)) {
> +			one->cancel_tx_delay_tmr = true;
> +			uart_port_unlock(port);
> +			hrtimer_cancel(&one->tx_delay_tmr);
> +			uart_port_lock(port);
> +		}
> +
> +		max310x_delayed_start_tx(port);

[Severity: High]
Can device shutdown complete while the port lock is dropped here?

If max310x_shutdown() runs concurrently and powers off the device while
the lock is dropped, max310x_delayed_start_tx() will blindly queue
rts_work when the lock is reacquired.

Could this result in the worker later accessing hardware registers via
SPI/I2C on a powered-off device?

[ ... ]

> @@ -1042,38 +1306,55 @@ static void max310x_set_termios(struct uart_port *port,
>  
>  	/* Update timeout according to new baud rate */
>  	uart_update_timeout(port, termios->c_cflag, baud);
> +
> +	/*
> +	 * Cache the new baud rate and the time it takes to clock out one
> +	 * character so the RTS-timing decision in max310x_set_rts_ctl_params()
> +	 * and the post-TX delay in max310x_delayed_stop_tx() can use them.
> +	 */
> +	to_max310x_port(port)->baud = baud;
> +	to_max310x_port(port)->one_character_duration =
> +		us_to_ktime(DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud));

[Severity: Medium]
Could this result in a torn read on 32-bit platforms?

max310x_set_termios() writes the 64-bit one_character_duration without
holding the port lock (the serial core only holds termios_rwsem).

Concurrently, workers like max310x_delayed_stop_tx() and max310x_shutdown()
read this variable. Since 64-bit reads and writes are not atomic on 32-bit
platforms, could a torn read yield a massively inflated or negative value,
causing tx_delay_tmr to misfire or max310x_shutdown() to spin indefinitely?

[ ... ]

> @@ -1084,6 +1365,7 @@ static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios
>  				struct serial_rs485 *rs485)
>  {
>  	struct max310x_one *one = to_max310x_port(port);
>  
> -	if ((rs485->delay_rts_before_send > 0x0f) ||
> -	    (rs485->delay_rts_after_send > 0x0f))
> -		return -ERANGE;
> +	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
> +	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send,  100U);
> +
> +	/*
> +	 * Make sure no SW-timed RTS toggle survives an RS485 disable, even
> +	 * if the delay timer happens to be running right now.
> +	 */
> +	if (!(rs485->flags & SER_RS485_ENABLED)) {
> +		one->cancel_tx_delay_tmr = true;
> +		if (hrtimer_try_to_cancel(&one->tx_delay_tmr) == -1) {
> +			uart_port_unlock(port);
> +			hrtimer_cancel(&one->tx_delay_tmr);
> +			uart_port_lock(port);
> +		}
> +		WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
> +	}

[Severity: High]
Does dropping the lock here introduce a race condition with start_tx()?

If max310x_rs485_config() drops the lock to wait for the hrtimer, and
start_tx() concurrently queues rts_work, max310x_rs485_config() will
overwrite tx_state to MAX310X_TX_OFF when it reacquires the lock.

Could this sabotage the queued worker and permanently stall transmission
for pending data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-max310x-rs485-sw-delay-v4-1-d37a694342cf@vaisala.com?part=1