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

[PATCH] 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.

Signed-off-by: Tate Whiteberg <whiteberg@arista.com>
---
 drivers/tty/serial/8250/8250.h      |  1 +
 drivers/tty/serial/8250/8250_mid.c  |  5 +++++
 drivers/tty/serial/8250/8250_port.c | 13 +++++++------
 3 files changed, 13 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_mid.c b/drivers/tty/serial/8250/8250_mid.c
index 82656645b8a6..c77e117b7822 100644
--- a/drivers/tty/serial/8250/8250_mid.c
+++ b/drivers/tty/serial/8250/8250_mid.c
@@ -210,6 +210,7 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
 {
 	unsigned int baud = tty_termios_baud_rate(termios);
 	struct mid8250 *mid = p->private_data;
+	struct uart_8250_port *up = up_to_u8250p(p);
 	unsigned short ps = 16;
 	unsigned long fuart = baud * ps;
 	unsigned long w = BIT(24) - 1;
@@ -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);
 	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);
 
 	serial8250_do_set_termios(p, termios, old);
 }
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

Re: [PATCH] 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] `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

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

[Andy Shevchenko]

On Fri, Aug 28, 2026 at 02:45:17PM +0000, Tate Whiteberg wrote:
> If mid8250_set_termios is called while data is still in transmission,

mid8250_set_termios()

> 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 tag?

...

> static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,

>  {
>  	unsigned int baud = tty_termios_baud_rate(termios);
>  	struct mid8250 *mid = p->private_data;
> +	struct uart_8250_port *up = up_to_u8250p(p);

Move it one line up.

>  	unsigned short ps = 16;
>  	unsigned long fuart = baud * ps;
>  	unsigned long w = BIT(24) - 1;

>  	/* Gracefully handle the B0 case: fall back to B9600 */
>  	fuart = fuart ? fuart : 9600 * 16;
>  
> +	uart_port_lock_irq(p);

+ blank line.

Do we need to protect the preliminary calculations?

>  	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);

Can't it be done one line after? Somewhere...

>  	p->uartclk = fuart * 16 / ps;		/* core uses ps = 16 always */

...here?

>  	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);

+ blank line.

> +	uart_port_unlock_irq(p);
>  
>  	serial8250_do_set_termios(p, termios, old);

...

> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 38fa45e74a37..4eaf417f1fd7 100644

All below better to split to another prerequisite patch.

-- 
With Best Regards,
Andy Shevchenko

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

[kernel test robot]

Hi Tate,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master next-20260828]
[cannot apply to v7.2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tate-Whiteberg/serial-8250_mid-wait-for-LSR-tx-empty-before-setting-termios/20260828-144517
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20260828144617.1210599-2-whiteberg%40arista.com
patch subject: [PATCH] serial: 8250_mid: wait for LSR tx empty before setting termios
config: powerpc64-randconfig-001-20260829 (https://download.01.org/0day-ci/archive/20260831/202608310138.0uIPCjfP-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260831/202608310138.0uIPCjfP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202608310138.0uIPCjfP-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: drivers/tty/serial/8250/8250_mid.ko: module uses symbol 'serial8250_wait_for_xmitr' from namespace 'SERIAL_8250', but does not import it.

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Tate,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master next-20260828]
[cannot apply to v7.2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tate-Whiteberg/serial-8250_mid-wait-for-LSR-tx-empty-before-setting-termios/20260828-144517
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20260828144617.1210599-2-whiteberg%40arista.com
patch subject: [PATCH] serial: 8250_mid: wait for LSR tx empty before setting termios
config: alpha-randconfig-r073-20260829 (https://download.01.org/0day-ci/archive/20260831/202608311414.nybi87OD-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 8.5.0
smatch: v0.5.0-9187-g5189e3fb
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260831/202608311414.nybi87OD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202608311414.nybi87OD-lkp@intel.com/

All warnings (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: vmlinux: 'saved_config' is COMMON symbol
>> WARNING: modpost: drivers/tty/serial/8250/8250_mid.ko: module uses symbol 'serial8250_wait_for_xmitr' from namespace 'SERIAL_8250', but does not import it.

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki