Re: [PATCH v4 2/4] serial: core: allow drivers to reserve flushed transmit storage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] serial: core: race and late unblock in uart_resume_port()
- [High] serial: core: stale xmit_blocked in uart_alloc_xmit_buf() when reusing xmit_buf
- [High] serial: core: permanent TX block if uart_xmit_unblock() runs during startup
- [Medium] serial: core: lost deferred write wakeup when restarting open port
--

commit f5e8cd060228c45857c337ffa1d0fd793601a7e0
Author: James Hilliard <james.hilliard1@gmail.com>

serial: core: allow drivers to reserve flushed transmit storage

This commit adds uart_xmit_block() and uart_xmit_unblock() so UART drivers can
reserve the transmit FIFO buffer after uart_flush_buffer() until any in-flight
asynchronous DMA transfer and its completion callbacks have retired. While
blocked, write_room() reports zero, write() and put_char() reject new data,
and write wakeups are deferred until the reservation is released.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c0deec0e6af41..e5756934b8f47 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -119,10 +119,56 @@ void uart_write_wakeup(struct uart_port *port)

[ ... ]

> +void uart_xmit_unblock(struct uart_port *port)
> +{
> +	lockdep_assert_held_once(&port->lock);
> +	if (!port->state->xmit_blocked || !tty_port_initialized(&port->state->port))
> +		return;
> +	port->state->xmit_blocked = false;
> +	uart_write_wakeup(port);
> +}
> +EXPORT_SYMBOL_GPL(uart_xmit_unblock);

[Severity: High]
Can checking !tty_port_initialized(&port->state->port) in uart_xmit_unblock()
leave state->xmit_blocked permanently stuck at true if a flush and DMA
retirement complete during port startup?

In tty_port_open(), uart_set_info() (TIOCSSERIAL), and uart_do_autoconfig()
(TIOCSERCONFIG), uart_startup() -> uart_port_startup() allocates
state->port.xmit_buf, invokes uport->ops->startup(uport) (enabling interrupts
and DMA), and configures line settings before tty_port_set_initialized(port,
true) is called:

drivers/tty/tty_port.c:tty_port_open() {
	...
		clear_bit(TTY_IO_ERROR, &tty->flags);
		if (port->ops->activate) {
			int retval = port->ops->activate(port, tty);
			if (retval) {
				set_bit(TTY_IO_ERROR, &tty->flags);
				return retval;
			}
		}
		tty_port_set_initialized(port, true);
	...
}

During that window, tty_port_initialized() is still false, while TTY_IO_ERROR
is already cleared and neither uart_write(), uart_put_char(), nor
uart_flush_buffer() checks tty_port_initialized() or acquires port->mutex.

If TX DMA is started and flushed in this window (for example via RX echo and
a VINTR/VQUIT/VSUSP signal character in n_tty_receive_signal_char() ->
isig() -> tty_driver_flush_buffer(), or via a concurrent write() and
ioctl(TCFLSH, TCOFLUSH) on an existing descriptor), uart_flush_buffer() calls
port->ops->flush_buffer(port) -> uart_xmit_block().

If that DMA transfer finishes retiring and calls uart_xmit_unblock() before
tty_port_set_initialized(port, true) runs, uart_xmit_unblock() sees
!tty_port_initialized() and returns early without clearing
state->xmit_blocked. When tty_port_set_initialized(port, true) subsequently
executes in tty_port_open(), uart_set_info(), or uart_do_autoconfig(),
nothing calls uart_xmit_unblock(), leaving TX permanently wedged with
uart_write_room() returning 0 and uart_write()/uart_put_char() dropping all
writes.

[ ... ]

> @@ -260,6 +309,7 @@ static int uart_alloc_xmit_buf(struct tty_port *port)
>  	uport = uart_port_ref_lock(state, &flags);
>  	if (!state->port.xmit_buf) {
>  		state->port.xmit_buf = buf;
> +		state->xmit_blocked = false;
>  		kfifo_init(&state->port.xmit_fifo, state->port.xmit_buf,
>  				PAGE_SIZE);
>  		uart_port_unlock_deref(uport, flags);

[Severity: High]
What happens in uart_alloc_xmit_buf() when state->port.xmit_buf is already
allocated and the else branch is taken?

In uart_alloc_xmit_buf(), state->xmit_blocked is only cleared when
state->port.xmit_buf is NULL:

drivers/tty/serial/serial_core.c:uart_alloc_xmit_buf() {
	...
	uport = uart_port_ref_lock(state, &flags);
	if (!state->port.xmit_buf) {
		state->port.xmit_buf = buf;
		state->xmit_blocked = false;
		kfifo_init(&state->port.xmit_fifo, state->port.xmit_buf,
				PAGE_SIZE);
		uart_port_unlock_deref(uport, flags);
	} else {
		uart_port_unlock_deref(uport, flags);
		kfree(buf);
	}
}

If uart_flush_buffer() sets state->xmit_blocked = true on an open port and
uart_suspend_port() then runs, uart_suspend_port() sets
tty_port_set_initialized(port, false) and calls ops->shutdown(uport) to
retire DMA without freeing state->port.xmit_buf, leaving state->xmit_blocked
set to true.

If the port is then closed while tty_port_initialized(port) is false,
tty_port_shutdown_locked() returns early without calling
uart_tty_port_shutdown() or uart_free_xmit_buf():

drivers/tty/tty_port.c:tty_port_shutdown_locked() {
	if (!tty_port_initialized(port))
		return;
	...
}

This can be reached without uart_resume_port() having run its unblock path,
for example when:
- a driver's suspend or freeze callback fails after calling
  uart_suspend_port() or serial8250_suspend_port() (such as
  omap8250_suspend(), pci1xxxx_suspend(), or imx_uart_suspend()), so the PM
  core skips the driver's resume callback;
- a driver's resume callback fails before calling uart_resume_port() or
  serial8250_resume_port() (such as omap8250_resume(), pci1xxxx_resume(),
  bcm2835aux_resume(), brcmuart_resume(), loongson_uart_resume(), or
  uniphier_uart_resume());
- a PCMCIA serial port (serial_cs.c) is suspended at runtime via sysfs
  pm_state and then closed and reopened; or
- a console port is suspended with console_suspend_enabled == true and
  resumed after console_suspend_enabled is set to false, skipping the
  (console_suspend_enabled || !uart_console(uport)) block in
  uart_resume_port().

On a subsequent open (uart_open() -> tty_port_open() -> uart_port_activate()
-> uart_startup() -> uart_port_startup() -> uart_alloc_xmit_buf()) or
reconfiguration (uart_set_info() -> uart_startup() -> uart_port_startup()),
uart_alloc_xmit_buf() sees state->port.xmit_buf != NULL, takes the else
branch, and leaves state->xmit_blocked set to true with no active DMA
transfer to ever call uart_xmit_unblock(). Could state->xmit_blocked remain
stuck true and permanently block all transmit operations on the port?

[Severity: Medium]
Also in uart_alloc_xmit_buf(), if state->xmit_blocked was true and a new
xmit_buf is allocated while the TTY remains open, could clearing
state->xmit_blocked = false directly without calling uart_write_wakeup() lose
a deferred write wakeup?

When uart_flush_buffer() runs on a port whose driver calls uart_xmit_block(),
uart_write_wakeup() suppresses tty_port_tty_wakeup(&state->port) and defers
waking writers on tty->write_wait and one-shot TTY_DO_WRITE_WAKEUP line
discipline callbacks until the reservation is released.

If the open port is then shut down and restarted via TIOCSERCONFIG
(uart_do_autoconfig()) or TIOCSSERIAL (uart_set_info() when change_port or
change_irq is true), or if uart_resume_port() fails ops->startup(uport) and
calls uart_shutdown(tty, state) followed by a later open() on the still-open
TTY, uart_shutdown() clears TTY_PORT_INITIALIZED before ops->shutdown(uport)
(making any driver uart_xmit_unblock() call a no-op) and frees xmit_buf via
uart_free_xmit_buf().

When uart_startup() -> uart_port_startup() -> uart_alloc_xmit_buf() then
allocates a new xmit_buf, it sets state->xmit_blocked = false without calling
uart_write_wakeup(), and nothing in the startup path triggers a TX wakeup
because the newly allocated FIFO is empty. Wouldn't writers sleeping on
tty->write_wait or line disciplines waiting for write_wakeup() stall?

[ ... ]

> @@ -2500,6 +2551,8 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>  					ops->start_tx(uport);
>  				}
>  				tty_port_set_initialized(port, true);
> +				scoped_guard(uart_port_lock_irq, uport)
> +					uart_xmit_unblock(uport);
>  			} else {
>  				/*
>  				 * Failed to resume - maybe hardware went away?

[Severity: High]
In uart_resume_port(), is calling uart_xmit_unblock(uport) after
ops->startup(uport), uart_change_line_settings(), and ops->start_tx(uport)
have already run and dropped uport->lock safe against a concurrent
uart_flush_buffer()?

Looking at the resume sequence in uart_resume_port():

drivers/tty/serial/serial_core.c:uart_resume_port() {
	...
			ret = ops->startup(uport);
			if (ret == 0) {
				if (tty)
					uart_change_line_settings(tty, state, NULL);
				uart_rs485_config(uport);
				scoped_guard(uart_port_lock_irq, uport) {
					if (!(uport->rs485.flags & SER_RS485_ENABLED))
						ops->set_mctrl(uport, uport->mctrl);
					ops->start_tx(uport);
				}
				tty_port_set_initialized(port, true);
				scoped_guard(uart_port_lock_irq, uport)
					uart_xmit_unblock(uport);
			}
}

This placement appears to create two issues:

1. If state->xmit_blocked was false prior to suspend, ops->start_tx(uport) can
   start a new TX DMA transfer from state->port.xmit_buf (using data queued
   before suspend or written after ops->startup()). When the first
   scoped_guard(uart_port_lock_irq, uport) exits, uport->lock is released
   before tty_port_set_initialized(port, true) and the second scoped_guard
   calling uart_xmit_unblock(uport).

   Because uart_flush_buffer() only serializes on uport->lock and does not
   take port->mutex, a concurrent uart_flush_buffer() in that unlocked window
   (for example via tcflush(TCOFLUSH), serdev_device_write_flush(), or an RX
   signal character in n_tty_receive_signal_char() ->
   tty_driver_flush_buffer()) will reset xmit_fifo and call
   port->ops->flush_buffer(port) -> uart_xmit_block(uport) to reserve
   state->port.xmit_buf for the newly started post-resume DMA transfer.

   Immediately afterward, uart_resume_port() sets
   tty_port_set_initialized(port, true), re-acquires uport->lock, and calls
   uart_xmit_unblock(uport), which cannot distinguish a stale pre-suspend
   reservation from the new post-resume reservation. Doesn't this clear
   state->xmit_blocked = false and wake writers to overwrite
   state->port.xmit_buf while the post-resume TX DMA transfer is still in
   flight?

2. Conversely, if state->xmit_blocked was true before suspend (and the old
   transfer was already retired by ops->shutdown(uport) in
   uart_suspend_port()), state->xmit_blocked remains true while
   uart_change_line_settings() runs during resume. If uport->hw_stopped
   transitions from true to false in uart_change_line_settings(),
   __uart_start(state) sees state->xmit_blocked && !port->x_char and returns
   early without starting TX.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-submit-serial-8250-dma-cancel-v1-v4-0-e759de1839de@gmail.com?part=2