Re: [PATCH v2 2/4] 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: uart_flush_buffer() and uart_write_wakeup() ignore xmit_blocked
- [Medium] serial: core: uart_write() and __uart_start() start TX while xmit_blocked
--

commit 31d69e636df648aacf81276c435bae5c06cd9f96
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
with asynchronous TX DMA can reserve flushed transmit buffer storage until
in-flight transfers and completion callbacks retire. While blocked,
uart_write_room() returns zero and uart_write() and uart_put_char() reject
new data until uart_xmit_unblock() wakes writers.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c0deec0e6af41..f82c812f842f1 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -123,6 +123,46 @@ void uart_write_wakeup(struct uart_port *port)
>  }
>  EXPORT_SYMBOL(uart_write_wakeup);
>  
> +/**
> + * uart_xmit_block - reserve flushed transmit storage
> + * @port: port whose transmit storage is still in use
> + *
> + * A driver may call this from ->flush_buffer() if DMA still owns the old
> + * transmit data. The logical FIFO has already been emptied, but writers
> + * must not overwrite its storage until uart_xmit_unblock() is called.
> + * write_room() reports zero and write()/put_char() accept no data meanwhile.
> + *
> + * The driver must unblock after retiring the transfer, including on shutdown.
> + * Repeated calls are idempotent, not nested reservations.
> + *
> + * Context: @port->lock held.
> + */
> +void uart_xmit_block(struct uart_port *port)
> +{
> +	lockdep_assert_held_once(&port->lock);
> +	port->state->xmit_blocked = true;
> +}
> +EXPORT_SYMBOL_GPL(uart_xmit_block);
> +
> +/**
> + * uart_xmit_unblock - release reserved transmit storage and wake writers
> + * @port: port whose storage is available again
> + *
> + * The old transfer must no longer access the FIFO storage, and any required
> + * DMA synchronization must have completed before this call.
> + *
> + * Context: @port->lock held.
> + */
> +void uart_xmit_unblock(struct uart_port *port)
> +{
> +	lockdep_assert_held_once(&port->lock);
> +	if (!port->state->xmit_blocked)
> +		return;
> +	port->state->xmit_blocked = false;
> +	uart_write_wakeup(port);
> +}
> +EXPORT_SYMBOL_GPL(uart_xmit_unblock);

[Severity: High]
Should uart_flush_buffer() and uart_write_wakeup() suppress waking the TTY
layer while state->xmit_blocked is true?

In drivers/tty/serial/serial_core.c, uart_flush_buffer() resets the FIFO,
calls port->ops->flush_buffer(port) where the driver calls
uart_xmit_block(port), drops port->lock, and then unconditionally calls
tty_port_tty_wakeup():

drivers/tty/serial/serial_core.c:uart_flush_buffer() {
	...
	kfifo_reset(&state->port.xmit_fifo);
	if (port->ops->flush_buffer)
		port->ops->flush_buffer(port);
	uart_port_unlock_deref(port, flags);
	tty_port_tty_wakeup(&state->port);
}

Similarly, uart_write_wakeup() does not check state->xmit_blocked:

drivers/tty/serial/serial_core.c:uart_write_wakeup() {
	struct uart_state *state = port->state;
	...
	BUG_ON(!state);
	tty_port_tty_wakeup(&state->port);
}

If a caller invokes uart_write_wakeup() while state->xmit_blocked is true
(for example via uart_handle_cts_change(), __uart_port_tx(), or a driver TX
interrupt seeing kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS), or when
uart_flush_buffer() finishes, tty_port_tty_wakeup() -> tty_wakeup() runs
while uart_write_room() is still 0 and uart_write() / uart_put_char() still
reject all data.

Because uart_flush_buffer() also drops port->lock before calling
tty_port_tty_wakeup(), a concurrent writer that sees state->xmit_blocked,
gets 0 from uart_write(), and sets TTY_DO_WRITE_WAKEUP in that window will
have its wakeup bit consumed immediately by uart_flush_buffer() while
state->xmit_blocked is still true.

When tty_wakeup() fires while state->xmit_blocked is true, several line
disciplines and TTY clients break:

1. In drivers/tty/n_tty.c:n_tty_write_wakeup():

	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);

TTY_DO_WRITE_WAKEUP is cleared and SIGIO is sent while uart_write_room() is
0 (so n_tty_poll() reports no EPOLLOUT). If the process checks poll() or
select() rather than calling write() during the premature signal,
TTY_DO_WRITE_WAKEUP remains cleared and the later uart_xmit_unblock() ->
uart_write_wakeup() will not send SIGIO when the buffer is actually
unblocked.

2. In drivers/tty/serdev/serdev-ttyport.c:ttyport_write_wakeup() and
drivers/tty/n_gsm.c:gsm_data_kick(), TTY_DO_WRITE_WAKEUP is cleared and the
write wakeup callback is invoked while uart_write() still returns 0, losing
the one-shot wakeup needed when uart_xmit_unblock() is later called.

3. In drivers/net/mctp/mctp-serial.c:mctp_serial_tty_write_wakeup() ->
mctp_serial_tx_work():

	txlen = write_chunk(dev, buf + dev->txpos, 3 - dev->txpos);
	if (txlen <= 0) {
		dev->txstate = STATE_ERR;

Waking TX while state->xmit_blocked is true causes tty->ops->write() to
return 0, which puts the device into STATE_ERR and leaves its netdev TX
queue permanently stopped.

4. In drivers/input/joystick/iforce/iforce-serio.c:iforce_serio_write_wakeup()
-> iforce_serio_xmit(), queued packets are drained into tty->ops->write()
while state->xmit_blocked is true and silently dropped. In serdev clients
such as btnxpuart_tx_work(), waking TX while serdev_device_write_buf()
returns 0 causes the worker to dequeue an skb, write 0 bytes, requeue it at
the head of nxpdev->txq, and loop. In slcan_transmit(), slip_transmit(), and
qcauart_transmit(), when xleft / tx_left is 0, the premature wakeup clears
TTY_DO_WRITE_WAKEUP and calls netif_wake_queue() while the UART still
rejects writes.

[ ... ]

> @@ -624,7 +665,7 @@ static ssize_t uart_write(struct tty_struct *tty, const u8 *buf, size_t count)
>  		return 0;
>  	}
>  
> -	if (port)
> +	if (port && !state->xmit_blocked)
>  		ret = kfifo_in(&state->port.xmit_fifo, buf, count);
>  
>  	__uart_start(state);

[Severity: Medium]
When state->xmit_blocked is true, uart_write() skips kfifo_in() and queues 0
bytes, but still calls __uart_start(state) (which can also be reached via
uart_flush_chars() -> uart_start() when n_tty_write() sees 0 write_room in
O_OPOST mode):

drivers/tty/serial/serial_core.c:__uart_start() {
	struct uart_port *port = state->uart_port;
	struct serial_port_device *port_dev;
	int err;

	if (!port || port->flags & UPF_DEAD || uart_tx_stopped(port))
		return;
	...
	if (!pm_runtime_enabled(port->dev) || pm_runtime_active(&port_dev->dev))
		port->ops->start_tx(port);
}

Should __uart_start() (or uart_write()) avoid calling port->ops->start_tx()
while state->xmit_blocked is true?

In drivers whose start_tx() touches hardware before checking whether
xmit_fifo is empty, calling start_tx() with an empty FIFO while TX storage
is blocked has unintended side effects:

1. In drivers/tty/serial/fsl_lpuart.c:lpuart_start_tx(),
drivers/tty/serial/sh-sci.c:sci_start_tx(), and
drivers/tty/serial/atmel_serial.c:atmel_start_tx(), start_tx() enables TX
interrupts before checking kfifo_is_empty(), triggering a spurious TX
interrupt or tasklet that sees kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS
and calls uart_write_wakeup(port) while state->xmit_blocked is still true.

2. In drivers/tty/serial/atmel_serial.c:atmel_start_tx(), half-duplex RX is
stopped via atmel_stop_rx(port) and ATMEL_US_TXEN is re-enabled even though
TX storage is blocked and no data was queued.

3. In drivers/tty/serial/amba-pl011.c:pl011_start_tx(), when RS485 is
enabled (SER_RS485_ENABLED), pl011_rs485_tx_start() is called before
checking whether xmit_fifo has data, asserting RTS and disabling RX
(~UART011_CR_RXE) on the RS485 bus despite 0 bytes being accepted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-submit-serial-8250-dma-cancel-v1-v2-0-088ea51e3891@gmail.com?part=2