Re: [PATCH v8 3/6] serial: core: allow drivers to reserve flushed transmit storage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: core: lockdep assertion failure in uart_write_wakeup() via asc_startup()
- [High] serial: core: lost tty->write_wait wakeup on flush when xmit is not writable
--

commit 887d77aad417709eb7acabd98bee111266577f06
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 serial drivers
can reserve flushed transmit FIFO storage until asynchronous DMA transfers
have retired. It also gates writes and write-ready wakeups on port readiness
and storage reservation state under the UART port lock.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c0deec0e6af41..11bd1260cdc63 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -100,6 +100,11 @@ static inline struct uart_port *uart_port_check(struct uart_state *state)
>  	return state->uart_port;
>  }
>  
> +static bool uart_xmit_writable(const struct uart_state *state)
> +{
> +	return state->xmit_ready && !state->xmit_blocked;
> +}
> +
>  /**
>   * uart_write_wakeup - schedule write processing
>   * @port: port to be processed
> @@ -114,15 +119,79 @@ static inline struct uart_port *uart_port_check(struct uart_state *state)
>  void uart_write_wakeup(struct uart_port *port)
>  {
>  	struct uart_state *state = port->state;
> +
> +	lockdep_assert_held_once(&port->lock);

[Severity: High]
Can this trigger a lockdep warning when opening or resuming an ST ASC serial
port?

During port startup or resume, uart_port_startup() and uart_resume_port()
invoke uport->ops->startup(uport) with port->mutex held, but without
port->lock held.

In drivers/tty/serial/st-asc.c, asc_startup() calls asc_transmit_chars()
without holding port->lock:

drivers/tty/serial/st-asc.c:asc_startup() {
	...
	asc_transmit_chars(port);
	asc_enable_rx_interrupts(port);

	return 0;
}

asc_transmit_chars() calls uart_port_tx_limited(), which expands
__uart_port_tx() in include/linux/serial_core.h:

	pending = kfifo_len(&__tport->xmit_fifo);
	if (pending < WAKEUP_CHARS) {
		uart_write_wakeup(__port);

Because pending is below WAKEUP_CHARS on startup, __uart_port_tx() calls
uart_write_wakeup() without port->lock held, hitting the new
lockdep_assert_held_once(&port->lock) assertion and reading
state->xmit_ready and state->xmit_blocked unlocked.

[ ... ]

> @@ -682,8 +762,9 @@ static void uart_flush_buffer(struct tty_struct *tty)
>  	kfifo_reset(&state->port.xmit_fifo);
>  	if (port->ops->flush_buffer)
>  		port->ops->flush_buffer(port);
> +	/* Serialize the wakeup with reservation changes, including another flush. */
> +	uart_write_wakeup(port);
>  	uart_port_unlock_deref(port, flags);
> -	tty_port_tty_wakeup(&state->port);
>  }

[Severity: High]
Could replacing tty_port_tty_wakeup(&state->port) with uart_write_wakeup(port)
here cause threads waiting for the transmit buffer to drain in
tty_wait_until_sent() or set_termios() to miss their wakeup and hang?

Waiters in tty_wait_until_sent() and set_termios() sleep on tty->write_wait
until !tty_chars_in_buffer(tty), which checks kfifo_len(&state->port.xmit_fifo).
Previously, uart_flush_buffer() emptied xmit_fifo via kfifo_reset() and
unconditionally called tty_port_tty_wakeup(&state->port), which woke
tty->write_wait.

With uart_write_wakeup(port), the wakeup is skipped whenever
!uart_xmit_writable(state) (when state->xmit_ready is false or
state->xmit_blocked is true):

1. If uart_flush_buffer() runs while the port is suspended
   (state->xmit_ready is false), kfifo_reset() empties xmit_fifo so
   tty_chars_in_buffer(tty) becomes 0, but uart_write_wakeup() returns early
   without waking tty->write_wait.

2. If port->ops->flush_buffer(port) calls uart_xmit_block(port),
   state->xmit_blocked is set to true and uart_write_wakeup() suppresses the
   wakeup in uart_flush_buffer(). If the port is subsequently shut down or
   suspended before the async transfer retires (via uart_shutdown(),
   uart_tty_port_shutdown(), or uart_suspend_port()), state->xmit_ready is set
   to false before uport->ops->shutdown(uport) runs:

drivers/tty/serial/serial_core.c:uart_shutdown() {
	...
	if (uport) {
		scoped_guard(uart_port_lock_irq, uport)
			state->xmit_ready = false;
		serial_base_port_shutdown(uport);
	}
	...
	if (tty_port_initialized(port)) {
		...
		uart_port_shutdown(port);
	}
}

   When uport->ops->shutdown(uport) then retires the transfer and calls
   uart_xmit_unblock(uport), uart_write_wakeup(uport) sees state->xmit_ready
   is false and returns early again, permanently losing the deferred wakeup
   for any drain waiters on tty->write_wait.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-submit-serial-8250-dma-cancel-v1-v8-0-3fd56ba352ed@gmail.com?part=3