Re: [PATCH v5 7/8] 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:
- [Critical] serial: max310x: sleeping while atomic in hrtimer on PREEMPT_RT
- [High] serial: max310x: start_tx breaks active software RTS envelope
- [High] serial: max310x: TOCTOU race in RTS idle level settling
--

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

serial: max310x: drive RTS in software when hardware delays are too short

This commit introduces a software-driven RTS path for the MAX310X serial driver
to handle RS485 RTS setup and hold delays that exceed the hardware's
capabilities. It uses a high-resolution timer and work queues to manage the
RTS envelope phases.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 8f8817b127c89..6ad27eb8a3258 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c

[ ... ]

> @@ -792,6 +837,68 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
>  	tty_flip_buffer_push(&port->state->port);
>  }
>  
> +static enum hrtimer_restart max310x_tmr_tx(struct hrtimer *timer)
> +{
> +	struct max310x_one *one = container_of(timer, struct max310x_one,
> +					       tx_delay_tmr);
> +
> +	guard(spinlock_irqsave)(&one->port.lock);

[Severity: Critical]
Does acquiring this lock cause a panic on PREEMPT_RT kernels?

The timer is set up with HRTIMER_MODE_REL, meaning this callback runs in
hardirq context. However, port->lock is a sleepable spinlock on PREEMPT_RT,
which will lead to a sleeping while atomic BUG.

[ ... ]

> +/* 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);
> +	/* A teardown is in progress; nothing may start an envelope or TX. */
> +	if (one->tx_teardown)
> +		return;
> +
> +	if (READ_ONCE(one->sw_rts_during_tx)) {
> +		/*
> +		 * The before- and after-send phases share one delay timer. If an
[ ... ]
> +			if (one->tx_teardown)
> +				return;
> +		}
> +
> +		max310x_delayed_start_tx(port);

[Severity: High]
Does this unconditional call to max310x_delayed_start_tx() overwrite the
active tx_state to MAX310X_TX_WAIT_BEFORE_SEND?

If userspace performs fragmented writes to the UART, uart_write() could
call max310x_start_tx() while the driver is actively transmitting (in the
MAX310X_TX_SEND state).

If the state is overwritten here, the after-send delay timer will not be
armed, potentially causing the RTS line to be held indefinitely or dropped
incorrectly, resulting in bus contention and packet loss.

> +	} else {
> +		schedule_work(&one->tx_work);
> +	}
>  }

[ ... ]

> @@ -983,12 +1156,48 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
[ ... ]
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		if (sw_rts) {
> +			/*
> +			 * Only settle RTS at idle when no transmission owns it.
> +			 * A reconfigure while one is in flight - rs_work runs
> +			 * on every TIOCSRS485 - would otherwise release the
> +			 * transceiver mid-character.
> +			 */
> +			if (READ_ONCE(one->tx_state) == MAX310X_TX_OFF) {
> +				max310x_rts_ctl(port,
> +						max310x_rts_level(port, false));
> +				/*
> +				 * serial_core calls set_termios() without
> +				 * port->lock, so an envelope may have started
> +				 * while the idle level was written and the
> +				 * settle can land after its RTS assert.
> +				 * rts_work re-derives the level from
> +				 * tx_state; requeue it to converge.
> +				 */
> +				if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF)
> +					schedule_work(&one->rts_work);
> +			}

[Severity: High]
Can this lockless check and write sequence overwrite the RTS level to idle
during an active transmission envelope?

There appears to be a time-of-check to time-of-use race here between settling
the idle level and max310x_start_tx() starting a new RTS envelope.

If a userspace program changes terminal parameters or triggers an RS485
reconfiguration while data is being written, this could drive the RTS pin to
the idle state while data is actively transmitting, disabling the transceiver
and dropping packets on the wire.

[ ... ]

> @@ -1557,6 +1846,11 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
>  		INIT_WORK(&s->p[i].md_work, max310x_md_proc);
>  		/* Initialize queue for changing RS485 mode */
>  		INIT_WORK(&s->p[i].rs_work, max310x_rs_proc);
> +		/* Initialize queue for software-driven RTS toggling */
> +		INIT_WORK(&s->p[i].rts_work, max310x_rts_work_proc);
> +		hrtimer_setup(&s->p[i].tx_delay_tmr, max310x_tmr_tx,
> +			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);

[Severity: Critical]
Should this timer be configured to run in softirq context (e.g., using
HRTIMER_MODE_REL_SOFT) since its callback acquires port->lock?

As currently set up, it will run in hardirq context, leading to the PREEMPT_RT
sleep-while-atomic regression noted above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-max310x-rs485-sw-delay-v5-0-ae46afa583f2@vaisala.com?part=7