Re: [PATCH v5 8/8] serial: max310x: don't transmit while an RS485 reconfigure is pending

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: max310x: missing port->x_char check when restarting deferred transmission
- [High] tty: max310x: tx_work bypasses rs485_pending check and transmits prematurely
- [Medium] tty: max310x: missing rs485_pending re-validation in max310x_start_tx()
--

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

    serial: max310x: don't transmit while an RS485 reconfigure is pending

This commit defers transmission in the max310x serial driver while an RS485
reconfiguration is pending. It aims to prevent transmitting data onto the wire
while the transceiver is in an inconsistent state by keeping data in the kfifo
until the new configuration is fully applied.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 6ad27eb8a3258..7036710942a42 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
[ ... ]
> @@ -965,6 +966,16 @@ static void max310x_start_tx(struct uart_port *port)
>  	if (one->tx_teardown)
>  		return;
>  
> +	/*
> +	 * An RS485 reconfigure is scheduled but not applied yet: transmitting
> +	 * now would use the old path against half-programmed registers - a
> +	 * TIOCSRS485 switching paths followed immediately by a write puts the
> +	 * data on the wire with the transceiver released. Leave the data in
> +	 * the kfifo; rs_work restarts TX once the configuration is applied.
> +	 */
> +	if (one->rs485_pending)
> +		return;
> +

[Severity: High]
Is premature transmission adequately prevented here if an interrupt occurs or
if tx_work is already queued?

If rs485_pending is true, max310x_start_tx() defers transmission, but
max310x_port_irq() can still queue tx_work on a hardware TX empty interrupt:

static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
{
	...
		if (ists & MAX310X_IRQ_TXEMPTY_BIT)
			schedule_work(&s->p[portno].tx_work);
	...
}

When max310x_handle_tx() runs, it unconditionally pumps data without checking
if an RS485 reconfigure is currently pending:

static void max310x_handle_tx(struct uart_port *port)
{
	...
	while (!kfifo_is_empty(&tport->xmit_fifo)) {
		...
		to_send = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail, txlen);
		max310x_batch_write(port, tail, to_send);
		...
	}
}

Will this cause data to be transmitted onto the wire while the RS485 transceiver
is in an inconsistent, partially-programmed state, defeating the purpose of
the patch?


[Severity: Medium]
Could a race condition cause transmission against a pending RS485
reconfiguration when the lock is dropped later in max310x_start_tx()?

max310x_start_tx() drops the port->lock to wait for timer cancellation:

		if (unlikely(res == -1)) {
			one->cancel_tx_delay_tmr = true;
			uart_port_unlock(port);
			hrtimer_cancel(&one->tx_delay_tmr);
			uart_port_lock(port);
			/*
			 * The lock was dropped: a teardown may have run to
			 * completion meanwhile. Re-check before starting.
			 */
			if (one->tx_teardown)
				return;
		}

		max310x_delayed_start_tx(port);

If ioctl(TIOCSRS485) runs concurrently while the lock is dropped and sets
rs485_pending = true, we re-acquire the lock and check tx_teardown, but we do
not check rs485_pending again. Does this allow transmission to begin while the
transceiver state is mid-switch?

>  	if (READ_ONCE(one->sw_rts_during_tx)) {
>  		/*
>  		 * The before- and after-send phases share one delay timer. If an
[ ... ]
> @@ -1395,6 +1406,16 @@ static void max310x_rs_proc(struct work_struct *ws)
>  
>  	max310x_port_update(&one->port, MAX310X_MODE2_REG,
>  			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
> +
> +	/*
> +	 * The configuration is applied: release any TX that start_tx()
> +	 * deferred while the reconfigure was pending, now on the right path.
> +	 */
> +	scoped_guard(spinlock_irqsave, &one->port.lock) {
> +		one->rs485_pending = false;
> +		if (!kfifo_is_empty(&one->port.state->port.xmit_fifo))
> +			max310x_start_tx(&one->port);
> +	}
>  }

[Severity: High]
Are deferred transmissions properly restarted if there is a pending XON/XOFF
character but the FIFO is empty?

If uart_send_xchar() queues an XON/XOFF character in port->x_char while an
RS485 reconfiguration is pending, transmission is deferred because
max310x_start_tx() returns immediately.

Later, this logic attempts to restart deferred transmissions but only checks
!kfifo_is_empty(&one->port.state->port.xmit_fifo). If the standard FIFO is
empty, max310x_start_tx() is not re-invoked.

Could this leave port->x_char permanently unsent and cause flow control to stall
indefinitely?

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