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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: max310x: tx_work bypasses rs485_pending gate allowing transmission during reconfigure
- [High] tty: max310x: TOCTOU race on chip_tx_empty causes incorrect state rewind
--

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

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

This commit defers transmissions while an RS485 configuration is pending,
leaving data in the kfifo until rs_work finishes reprogramming the registers.
It drops the direct tx_work kick in the disable path in favor of restarting
the transmission once the configuration is fully applied.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 5fdb9dfca6027..833ab4d461235 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c

[ ... ]

> @@ -979,6 +980,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]
Does this check sufficiently gate the transmission?

While this defers synchronous writes, a TXEMPTY interrupt can asynchronously
queue tx_work during the pending RS485 reconfiguration:

max310x_port_irq()
  if (ists & MAX310X_IRQ_TXEMPTY_BIT)
      schedule_work(&s->p[portno].tx_work);

If tx_work executes concurrently with max310x_rs_proc(), it calls
max310x_handle_tx() which pumps data from xmit_fifo to the hardware FIFO:

max310x_handle_tx()
  while (!kfifo_is_empty(&tport->xmit_fifo)) {
      /* Limit to space available in TX FIFO */
      txlen = max310x_port_read(port, MAX310X_TXFIFOLVL_REG);
      ...
  }

Since max310x_handle_tx() runs without holding a synchronizing lock or
checking the rs485_pending flag, can this regression allow background workers
to bypass the deferral mechanism and transmit data with a half-programmed
transceiver?

[ ... ]

> @@ -1465,6 +1479,31 @@ 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. start_tx() can drop and retake the lock through the
> +	 * uart_port API - a raw spinlock guard here would unbalance it.
> +	 */
> +	chip_tx_empty = !max310x_port_read(&one->port,
> +					   MAX310X_TXFIFOLVL_REG);
> +
> +	uart_port_lock_irqsave(&one->port, &flags);
> +	one->rs485_pending = false;
> +	if (one->port.x_char ||
> +	    !kfifo_is_empty(&one->port.state->port.xmit_fifo)) {
> +		/*
> +		 * A send state adopted without chip data is the deferred
> +		 * write itself: restart it as a fresh envelope so the
> +		 * before-send delay is honoured.
> +		 */
> +		if (chip_tx_empty &&
> +		    READ_ONCE(one->tx_state) == MAX310X_TX_SEND)
> +			WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);

[Severity: High]
Is it possible for chip_tx_empty to be stale here due to a race with tx_work?

The TX FIFO level is read outside the lock. Concurrently, tx_work can execute
max310x_handle_tx() and write new data to the chip FIFO without acquiring
port->lock.

When max310x_rs_proc() then acquires port->lock, it uses the stale
chip_tx_empty value to rewind the transmission state to MAX310X_TX_OFF, even
though data is already being transmitted.

Can this incorrect rewind introduce a regression by breaking the timing
envelope, causing data to hit the RS485 bus without the configured
before-send delay?

> +		max310x_start_tx(&one->port);
> +	}
> +	uart_port_unlock_irqrestore(&one->port, flags);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-max310x-rs485-sw-delay-v7-0-6ae392eca08c@vaisala.com?part=9