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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] serial: core: uart_xmit_unblock() unblocks TX and wakes writers on shutdown
--

commit 6406836c9f0fb8695332478ed1176afc95924b74
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() to allow UART
drivers to reserve the transmit FIFO buffer across asynchronous DMA flush
completion while deferring TTY write wakeups and blocking new writes.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c0deec0e6af41..cb4e98886cee9 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -119,10 +119,53 @@ void uart_write_wakeup(struct uart_port *port)
>  	 * closed.  No cookie for you.
>  	 */
>  	BUG_ON(!state);
> +	if (state->xmit_blocked)
> +		return;
>  	tty_port_tty_wakeup(&state->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.
> + * Write wakeups are deferred until the reservation is released.
> + *
> + * 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);
> +}

[Severity: Medium]
When a driver calls uart_xmit_unblock() from its uport->ops->shutdown()
callback as required by the documented contract in include/linux/serial_core.h
(for example via serial8250_release_dma() in the follow-up 8250 change), can
this unblock transmit and trigger uart_write_wakeup() while the port is
shutting down or suspending and state->port.xmit_buf is still allocated?

If a transmit flush calls uart_xmit_block() while TX DMA is active and the
port then enters shutdown or suspend before the asynchronous flush completes,
uport->ops->shutdown() runs before state->port.xmit_buf is freed in
drivers/tty/serial/serial_core.c:

uart_tty_port_shutdown()
    serial_base_port_shutdown(uport);
    uart_port_shutdown(port);
        uport->ops->shutdown(uport);
    ...
    uart_free_xmit_buf(port);

and in uart_suspend_port(), ops->shutdown(uport) is called without freeing
state->port.xmit_buf at all:

uart_suspend_port()
    ops->shutdown(uport);

In tty_port_close(), tty_port_shutdown() also runs before tty->flags has
TTY_IO_ERROR set, before tty_port_tty_set(port, NULL), and before
tty_ldisc_release(), while drivers drop port->lock across sleeping calls such
as dma_release_channel() and free_irq() after calling uart_xmit_unblock().

If a woken or concurrent writer (such as a line discipline write_work
handler, a serdev client, or a thread in n_tty_write() during uart_set_info()
or uart_do_autoconfig() -> uart_shutdown()) calls uart_write() in that window,
it sees state->port.xmit_buf != NULL and !state->xmit_blocked, queues bytes
into state->port.xmit_fifo, and calls __uart_start(state) ->
port->ops->start_tx(port) on a shut-down or suspending port (which in 8250
drivers can acquire an unbalanced runtime PM reference via
serial8250_rpm_get_tx() or re-enable transmit interrupts after DMA teardown,
and in uart_tty_port_shutdown() any newly queued bytes are then discarded by
uart_free_xmit_buf()).

Should uart_xmit_unblock(), uart_write(), and __uart_start() avoid waking
writers or starting transmission when the port is no longer initialized or is
being shut down?

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