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

[PATCH v2 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 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  |  7 +++++++
 drivers/tty/serial/8250/8250_port.c | 13 +++++++------
 3 files changed, 15 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH v2 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>
---
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 v2 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: d9eda9bab237 ("serial: 8250_pci: Intel MID UART support to its own driver")
Signed-off-by: Tate Whiteberg <whiteberg@arista.com>
---
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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_mid.c b/drivers/tty/serial/8250/8250_mid.c
index 82656645b8a6..ce792888acfe 100644
--- a/drivers/tty/serial/8250/8250_mid.c
+++ b/drivers/tty/serial/8250/8250_mid.c
@@ -209,6 +209,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,12 +232,18 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
 	}
 
 	rational_best_approximation(fuart, mid->board->freq, w, w, &mul, &div);
+
+	uart_port_lock_irq(p);
+
 	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);
 
+	uart_port_unlock_irq(p);
+
 	serial8250_do_set_termios(p, termios, old);
 }
 
-- 
2.43.0

Re: [PATCH v2 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/20260828175733.1650267-1-whiteberg@arista.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `serial8250_wait_for_xmitr()` timeout is too short to drain the FIFO, failing to prevent data corruption.
- [High] `uart_port_unlock_irq()` unconditionally enables local interrupts, violating caller context expectations.
- [High] Releasing the port lock between Intel-specific register updates and core 8250 register updates introduces a TOCTOU race condition that corrupts transmission.
- [Low] Missing MODULE_IMPORT_NS("SERIAL_8250") causes a modpost namespace warning/error.
--

commit f345869677f080c82ebb31b7ea7f98c00bfefd55
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. This patch
locks the port and waits for the transmitter to empty before performing
the register updates.

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

[ ... ]

> @@ -231,12 +232,18 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  	}
>  
>  	rational_best_approximation(fuart, mid->board->freq, w, w, &mul, &div);
> +
> +	uart_port_lock_irq(p);

[Severity: High]
Does acquiring the port lock here using uart_port_lock_irq() break caller
context expectations?

Since mid8250_set_termios() serves as the set_termios callback, it is
frequently called from contexts where local interrupts are already disabled
(such as early boot console setup). 

This will eventually pair with uart_port_unlock_irq() below, which
unconditionally re-enables hardware interrupts via local_irq_enable(),
rather than restoring the previous interrupt state.

Should this use uart_port_lock_irqsave(p, &flags) instead?

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

[Severity: High]
Can this wait silently time out and fail to prevent data corruption?

The 8250_mid UART (PORT_16750) has a 64-byte FIFO. However, 
serial8250_wait_for_xmitr() internally calls wait_for_lsr(), which uses a
timeout of up->port.frame_time * 2 (approximately 2 characters).

If there is a large burst in the FIFO at a low baud rate, won't this wait
time out long before the 64-byte FIFO drains, proceeding to modify the
PS/MUL/DIV registers while data is still actively shifting out?


[Severity: Low]
Does this module need to import the SERIAL_8250 namespace?

The function serial8250_wait_for_xmitr() is exported to the "SERIAL_8250"
namespace in drivers/tty/serial/8250/8250_port.c. Since 8250_mid.c is
compiled as a separate module (8250_mid.ko), failing to include
MODULE_IMPORT_NS("SERIAL_8250") at the module definition level will trigger
a modpost namespace warning or error when building the driver as a module.

>  
>  	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);
> +
>  	serial8250_do_set_termios(p, termios, old);

[Severity: High]
Does releasing the port lock here introduce a race condition?

The lock is dropped before calling serial8250_do_set_termios(), which then
re-acquires the lock to update DLL/DLM and LCR.

During this unlocked window, the port is left in an inconsistent hardware
state with new clock multipliers but the old divisor. 

Because interrupts are enabled, could a TX interrupt or concurrent console
write acquire the lock, fill the hardware FIFO with new data, and transmit
it at a garbled frequency before the core 8250 function completes the update?

>  }

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

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

[Jiri Slaby]

On 28. 08. 26, 19:57, 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.
> 
> Fixes: d9eda9bab237 ("serial: 8250_pci: Intel MID UART support to its own driver")
> Signed-off-by: Tate Whiteberg <whiteberg@arista.com>
> ---
> 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 | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_mid.c b/drivers/tty/serial/8250/8250_mid.c
> index 82656645b8a6..ce792888acfe 100644
> --- a/drivers/tty/serial/8250/8250_mid.c
> +++ b/drivers/tty/serial/8250/8250_mid.c
> @@ -209,6 +209,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,12 +232,18 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
>   	}
>   
>   	rational_best_approximation(fuart, mid->board->freq, w, w, &mul, &div);
> +
> +	uart_port_lock_irq(p);

Use scoped_guard(), please.

-- 
js
suse labs

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

[Andy Shevchenko]

On Fri, Aug 28, 2026 at 05:57:28PM +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.

> Fixes: d9eda9bab237 ("serial: 8250_pci: Intel MID UART support to its own driver")

Obviously this is not the commit when the problem appeared.
Check the commit f549e94effa1 ("serial: 8250_pci: add Intel Penwell ports").

...

> +	uart_port_lock_irq(p);
> +
>  	p->uartclk = fuart * 16 / ps;		/* core uses ps = 16 always */

+ blank line

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Aug 28, 2026 at 05:57:27PM +0000, Tate Whiteberg wrote:
> Export wait_for_xmitr() and rename it to follow naming convention.

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

-- 
With Best Regards,
Andy Shevchenko