Re: [PATCH v3 2/2] serial: 8250_mid: wait for LSR tx empty before setting termios

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Spin-waiting for up to 1 second with interrupts disabled causes severe scheduling latency and hard lockups.
- [High] The baud rate hardware state update is split across two critical sections, leaving a race window for corrupted transmissions.
- [High] Bypassing the `uart_port_lock` wrappers fails to acquire the nbcon console lock.
- [High] `scoped_guard(spinlock_irq)` unconditionally enables interrupts on exit, violating caller context guarantees.
- [Low] Missing `MODULE_IMPORT_NS("SERIAL_8250")` causes modpost errors for modular builds.
--

commit 7b0bf61b54075c1cffc2c3cc1ed56d43ba829821
Author: Tate Whiteberg <whiteberg@arista.com>
Subject: serial: 8250_mid: wait for LSR tx empty before setting termios

This commit modifies mid8250_set_termios() to lock the port and wait for the
transmitter to empty before updating baud rate registers. This is intended to
prevent transmission corruption when updates occur while data is still in
flight.

> diff --git a/drivers/tty/serial/8250/8250_mid.c b/drivers/tty/serial/8250/8250_mid.c
> index 82656645b8a64..c02ecb96e8b6f 100644
> --- a/drivers/tty/serial/8250/8250_mid.c
> +++ b/drivers/tty/serial/8250/8250_mid.c

[ ... ]

> @@ -231,11 +233,16 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  	}
>  
>  	rational_best_approximation(fuart, mid->board->freq, w, w, &mul, &div);
> -	p->uartclk = fuart * 16 / ps;		/* core uses ps = 16 always */
>  
> -	writel(ps, p->membase + INTEL_MID_UART_PS);		/* set PS */
> -	writel(mul, p->membase + INTEL_MID_UART_MUL);		/* set MUL */
> -	writel(div, p->membase + INTEL_MID_UART_DIV);
> +	scoped_guard(spinlock_irq, &p->lock) {

[Severity: High]
Does using scoped_guard(spinlock_irq, ...) here unconditionally enable
interrupts on exit, violating caller context guarantees?

If set_termios is called from a context where interrupts are already disabled
(such as early console initialization or system resume), this will unmask
interrupts prematurely.

Also, does bypassing the uart_port_lock wrappers fail to acquire the nbcon
console lock? This could result in concurrent nbcon atomic console printers
mutating UART state simultaneously.

> +		p->uartclk = fuart * 16 / ps;	/* core uses ps = 16 always */
> +
> +		serial8250_wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);

[Severity: High]
Can this call cause a hard lockup?

Because scoped_guard(spinlock_irq, ...) is active, interrupts are strictly
disabled here. Looking at the called function:

void serial8250_wait_for_xmitr(struct uart_8250_port *up, int bits)
{
    ...
    if (uart_console_hwflow_active(&up->port) && !up->canary) {
        for (tmout = 1000000; tmout; tmout--) {
            ...
            udelay(1);
...

This loop busy-waits for up to 1 second. When called under a spinlock with
interrupts disabled, this can block the CPU and trigger the NMI watchdog.

[Severity: Low]
Does this missing namespace import cause modpost errors for modular builds?

The patch uses the serial8250_wait_for_xmitr symbol which is exported in the
SERIAL_8250 namespace, but fails to import the namespace via
MODULE_IMPORT_NS("SERIAL_8250").

> +
> +		writel(ps, p->membase + INTEL_MID_UART_PS);	/* set PS */
> +		writel(mul, p->membase + INTEL_MID_UART_MUL);	/* set MUL */
> +		writel(div, p->membase + INTEL_MID_UART_DIV);
> +	}
>  
>  	serial8250_do_set_termios(p, termios, old);

[Severity: High]
Is there a race condition introduced by splitting the hardware state update
across two critical sections?

The lock is released at the closing brace of the scoped_guard, and then
reacquired inside serial8250_do_set_termios():

void serial8250_do_set_termios(...)
{
    ...
    scoped_guard(serial8250_rpm, up) {
        guard(uart_port_lock_irqsave)(port);
        ...

This creates a window where a concurrent uart_write or TX interrupt handler
could acquire the lock between these two critical sections and transmit data
using a mismatched clock and divisor configuration.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831165911.2029839-1-whiteberg@arista.com?part=2