Re: [PATCH v6 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 1 potential issue(s) to consider:
- [High] serial: nbcon lock imbalance due to raw spinlock guard bypassing uart_port_lock API
--

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

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

This commit defers RS485 transmissions while a reconfigure is pending to
prevent transmitting with a half-switched state. It adds a pending flag
evaluated under the port lock and restarts transmission once applied.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index eb1d2e3bc870e..f8247cf0358ba 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
[ ... ]
> @@ -997,9 +1008,10 @@ static void max310x_start_tx(struct uart_port *port)
>  			uart_port_lock(port);
>  			/*
>  			 * The lock was dropped: a teardown may have run to
> -			 * completion meanwhile. Re-check before starting.
> +			 * completion or a reconfigure may have been posted
> +			 * meanwhile. Re-check before starting.
>  			 */
> -			if (one->tx_teardown)
> +			if (one->tx_teardown || one->rs485_pending)
>  				return;
>  		}
>  
[ ... ]
> @@ -1453,6 +1465,17 @@ 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 (one->port.x_char ||
> +		    !kfifo_is_empty(&one->port.state->port.xmit_fifo))
> +			max310x_start_tx(&one->port);
> +	}
>  }

[Severity: High]
Will this scoped guard usage cause an unbalanced nbcon lock release and
subsequent leak when max310x_start_tx() is called?

max310x_rs_proc() acquires port.lock using a raw spinlock_irqsave guard,
bypassing the mandatory nbcon acquisition performed by the uart_port_lock()
API.

Inside max310x_start_tx(), if the timer cancellation fails, it temporarily
drops and reacquires the lock using the standard API:

max310x_start_tx()
    ...
    uart_port_unlock(port);
    hrtimer_cancel(&one->tx_delay_tmr);
    uart_port_lock(port);
    ...

When this sequence is reached via max310x_rs_proc(), uart_port_unlock() will
attempt to release an nbcon lock that was never acquired, and
uart_port_lock() will acquire it.

When max310x_start_tx() returns, the scoped guard in max310x_rs_proc() will
only release the raw spinlock, leaving the nbcon lock leaked. This can
permanently freeze the console or trigger lockdep crashes when a user space
process reconfigures RS-485 while a transmission delay timer is active.

Should this use the serial core uart_port_lock APIs instead of a raw
spinlock guard to ensure balanced locking?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004-max310x-rs485-sw-delay-v6-0-3a0ef13ed9e3@vaisala.com?part=9