[PATCH v3 0/2] serial: 8250_mid: fix race condition between output flush and termios update

[PATCH v3 0/2] serial: 8250_mid: fix race condition between output flush and termios update

[Tate Whiteberg]

Fix a race condition in which setting termios while still transmitting
can corrupt transmission.

Changes in v3:
  1. Patch 2: use scoped_guard(), as suggested by jirislaby@kernel.org
  2. Patch 2: Update Fixes tag, formatting changes as suggested by
     andriy.shevchenko@intel.com

Changes in v2:
  Split original patch into two: a prerequisite to to expose
  wait_for_xmitr() and a successor to fix the bug. 

Tate Whiteberg (2):
  serial: 8250: export and rename wait_for_xmitr()
  serial: 8250_mid: wait for LSR tx empty before setting termios

 drivers/tty/serial/8250/8250.h      |  1 +
 drivers/tty/serial/8250/8250_mid.c  | 15 +++++++++++----
 drivers/tty/serial/8250/8250_port.c | 13 +++++++------
 3 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] serial: 8250: export and rename wait_for_xmitr()

[Tate Whiteberg]

Export wait_for_xmitr() and rename it to follow naming convention.

Signed-off-by: Tate Whiteberg <whiteberg@arista.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---

Changes in v3:
  -- None

Changes in v2:
  - Separate this patch from successor patch.

 drivers/tty/serial/8250/8250.h      |  1 +
 drivers/tty/serial/8250/8250_port.c | 13 +++++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 9337fec9394e..f99620b1cab7 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -180,6 +180,7 @@ void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p);
 void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up,
 				       struct nbcon_write_context *wctxt,
 				       unsigned int count);
+void serial8250_wait_for_xmitr(struct uart_8250_port *up, int bits);
 
 void serial8250_rpm_get(struct uart_8250_port *p);
 void serial8250_rpm_put(struct uart_8250_port *p);
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 38fa45e74a37..4eaf417f1fd7 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2024,7 +2024,7 @@ static bool wait_for_lsr(struct uart_8250_port *up, int bits)
 }
 
 /* Wait for transmitter and holding register to empty with timeout */
-static void wait_for_xmitr(struct uart_8250_port *up, int bits)
+void serial8250_wait_for_xmitr(struct uart_8250_port *up, int bits)
 {
 	unsigned int tmout;
 	bool tx_ready;
@@ -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");
 
 #ifdef CONFIG_CONSOLE_POLL
 /*
@@ -2098,7 +2099,7 @@ static void serial8250_put_poll_char(struct uart_port *port,
 	ier = serial_port_in(port, UART_IER);
 	__serial8250_clear_IER(up);
 
-	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
+	serial8250_wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
 	/*
 	 *	Send the character out.
 	 */
@@ -2108,7 +2109,7 @@ static void serial8250_put_poll_char(struct uart_port *port,
 	 *	Finally, wait for transmitter to become empty
 	 *	and restore the IER
 	 */
-	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
+	serial8250_wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
 	serial_port_out(port, UART_IER, ier);
 }
 
@@ -2223,7 +2224,7 @@ static void serial8250_THRE_test(struct uart_port *port)
 	 * Synchronize UART_IER access against the console.
 	 */
 	scoped_guard(uart_port_lock_irqsave, port) {
-		wait_for_xmitr(up, UART_LSR_THRE);
+		serial8250_wait_for_xmitr(up, UART_LSR_THRE);
 		serial_port_out_sync(port, UART_IER, UART_IER_THRI);
 		udelay(1); /* allow THRE to set */
 		iir_noint1 = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
@@ -3293,7 +3294,7 @@ static void serial8250_console_wait_putchar(struct uart_port *port, unsigned cha
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 
-	wait_for_xmitr(up, UART_LSR_THRE);
+	serial8250_wait_for_xmitr(up, UART_LSR_THRE);
 	serial8250_console_putchar(port, ch);
 }
 
@@ -3504,7 +3505,7 @@ void serial8250_console_write(struct uart_8250_port *up,
 	 *	Finally, wait for transmitter to become empty
 	 *	and restore the IER
 	 */
-	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
+	serial8250_wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
 
 	if (em485) {
 		mdelay(port->rs485.delay_rts_after_send);
-- 
2.43.0

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

[Tate Whiteberg]

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.

Fixes: f549e94effa1 ("serial: 8250_pci: add Intel Penwell ports")
Signed-off-by: Tate Whiteberg <whiteberg@arista.com>
---

Changes in v3:
  - Update Fixes tag
  - Apply formatting change from andriy.shevchenko@intel.com
  - Use scoped_guard() as suggested by jirislaby@kernel.org

Changes in v2:
  - Separate changes to 8250.h and 8250_port.c into prerequisite patch,
    as recommended by andriy.shevchenko@intel.com
  - Apply feedback from andriy.shevchenko@intel.com to 8250_mid.c

 drivers/tty/serial/8250/8250_mid.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_mid.c b/drivers/tty/serial/8250/8250_mid.c
index 82656645b8a6..9c5ef207bf8a 100644
--- a/drivers/tty/serial/8250/8250_mid.c
+++ b/drivers/tty/serial/8250/8250_mid.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/rational.h>
@@ -209,6 +210,7 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
 				const struct ktermios *old)
 {
 	unsigned int baud = tty_termios_baud_rate(termios);
+	struct uart_8250_port *up = up_to_u8250p(p);
 	struct mid8250 *mid = p->private_data;
 	unsigned short ps = 16;
 	unsigned long fuart = baud * ps;
@@ -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) {
+		p->uartclk = fuart * 16 / ps;	/* core uses ps = 16 always */
+
+		serial8250_wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
+
+		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);
 }
-- 
2.43.0

Re: [PATCH v3 1/2] serial: 8250: export and rename wait_for_xmitr()

[sashiko-bot]

> Export wait_for_xmitr() and rename it to follow naming convention.
> 
> Signed-off-by: Tate Whiteberg <whiteberg@arista.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

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

[sashiko-bot]

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

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

[Andy Shevchenko]

On Mon, Aug 31, 2026 at 04:59:08PM +0000, Tate Whiteberg wrote:
> 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko