Re: [PATCH] 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] `serial8250_wait_for_xmitr` will timeout prematurely if the TX FIFO contains more than two characters, defeating the patch's attempt to prevent data corruption.
- [High] Locking gap between updating MID registers and 8250 core registers creates a deterministic race condition that corrupts transmission.
- [High] `uart_port_unlock_irq` unconditionally enables local interrupts, violating locking constraints for contexts that require interrupts to be disabled.
- [High] Busy-waiting with interrupts disabled for the UART transmitter to drain causes severe latency spikes and potential soft lockups.
- [Medium] Missing MODULE_IMPORT_NS("SERIAL_8250") in 8250_mid.c breaks module namespace dependencies and causes modpost failures.
--

commit 77c9d48edc835f42331c18bc6abbd15412b526d2
Author: Tate Whiteberg <whiteberg@arista.com>

    serial: 8250_mid: wait for LSR tx empty before setting termios
    
    If mid8250_set_termios is called while data is still in transmission,
    the corresponding register updates will corrupt the transmission.
    
    Fix this by locking the port and and waiting for the transmitter to
    empty before performing updates. It is necessary to wait for both
    UART_LSR_THRE and UART_LSR_TEMT to ensure the final character is sent.

> diff --git a/drivers/tty/serial/8250/8250_mid.c b/drivers/tty/serial/8250/8250_mid.c
> index 82656645b8a64..c77e117b7822c 100644
> --- a/drivers/tty/serial/8250/8250_mid.c
> +++ b/drivers/tty/serial/8250/8250_mid.c
[ ... ]
> @@ -218,6 +219,7 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  	/* Gracefully handle the B0 case: fall back to B9600 */
>  	fuart = fuart ? fuart : 9600 * 16;
>  
> +	uart_port_lock_irq(p);
>  	if (mid->board->freq < fuart) {
>  		/* Find prescaler value that satisfies Fuart < Fref */
>  		if (mid->board->freq > baud)
> @@ -231,11 +233,14 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  	}
>  
>  	rational_best_approximation(fuart, mid->board->freq, w, w, &mul, &div);
> +
> +	serial8250_wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);

[Severity: High]
Will this wait function timeout prematurely?

Because wait_for_lsr() caps its internal timeout at 2 frame times, it seems
it will inevitably time out before a full FIFO (up to 64 bytes) can drain.

If serial8250_wait_for_xmitr() returns silently on timeout when hardware flow
control is inactive, won't mid8250_set_termios() proceed to update the clock
registers while the FIFO is still actively transmitting data?

[Severity: High]
Is it safe to invoke this wait function while interrupts are disabled?

Since uart_port_lock_irq() was just called, local interrupts are disabled. If
hardware flow control is active and CTS is deasserted, the innermost loop in
serial8250_wait_for_xmitr() can spin in a udelay(1) loop up to 1,000,000
times:

drivers/tty/serial/8250/8250_port.c:serial8250_wait_for_xmitr() {
    ...
    for (tmout = 1000000; tmout; tmout--) {
        ...
        udelay(1);
        touch_nmi_watchdog();
    }
}

Could spinning for 1 second with local CPU interrupts disabled cause severe
latency spikes or soft lockups?

>  	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);
> +	uart_port_unlock_irq(p);

[Severity: High]
Since set_termios() callbacks can be executed from contexts where interrupts
are strictly disabled (such as early console initialization), is it safe to
use uart_port_unlock_irq() here?

uart_port_unlock_irq() expands to spin_unlock_irq(), which unconditionally
enables local CPU interrupts. Shouldn't this save and restore the IRQ state
using uart_port_lock_irqsave() and uart_port_unlock_irqrestore() instead?

>  
>  	serial8250_do_set_termios(p, termios, old);

[Severity: High]
Does releasing the lock before calling serial8250_do_set_termios() create a
race condition?

If the hardware FIFO was just explicitly emptied, could the UART_IIR_THRI
(TX empty) interrupt fire the exact moment local interrupts are re-enabled by
uart_port_unlock_irq()?

If the interrupt handler executes and pushes new characters into the FIFO
before serial8250_do_set_termios() updates the 8250 divisors, might those
characters be transmitted with a mismatched clock configuration?

>  }
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 38fa45e74a37a..4eaf417f1fd79 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
[ ... ]
> @@ -2052,6 +2052,7 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>  		}
>  	}
>  }
> +EXPORT_SYMBOL_NS_GPL(serial8250_wait_for_xmitr, "SERIAL_8250");

[Severity: Medium]
Will this cause a module load failure under strict namespace policies?

Because this exports the symbol into the "SERIAL_8250" namespace, but
8250_mid.c (compiled into the separate 8250_mid.ko module) consumes it without
a corresponding MODULE_IMPORT_NS("SERIAL_8250") statement, won't this trigger
modpost warnings and break module dependencies?

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