Re: [PATCH 4/4] tty: serial: qcom_geni_serial: Add GPI DMA support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] tty: serial: qcom_geni_serial: RX DMA is never safely stopped or cancelled, leading to UAF
- [High] tty: serial: qcom_geni_serial: Broken GPI TX DMA cancellation and TOCTOU races
- [High] tty: serial: qcom_geni_serial: Lockless TX kfifo access and missing flush validation cause data corruption
- [High] tty: serial: qcom_geni_serial: Workqueues destroyed before port unregistration cause UAF
- [High] tty: serial: qcom_geni_serial: Invalid DMA buffer ownership transfer corrupts TX data
- [High] tty: serial: qcom_geni_serial: TX path permanently stalls after a single transfer
- [Medium] tty: serial: qcom_geni_serial: Multiple resource and DMA mapping leaks on initialization error paths
- [Medium] tty: serial: qcom_geni_serial: Squashed dma_request_chan error breaks probe deferral
- [Medium] tty: serial: qcom_geni_serial: Incorrect tx_empty reporting breaks tcdrain
- [Medium] tty: serial: qcom_geni_serial: Callbacks access port state without locks on error
--

commit 795e95674391634c4084de89abe8cbcd0788b18d
Author: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
Subject: tty: serial: qcom_geni_serial: Add GPI DMA support

This commit adds support for the GPI DMA interface in the QCOM Geni serial
driver. It configures hardware flow control, DMA channels, and RX buffers
when the UART engine operates in GPI mode.

> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 4a6f526340b8d..eb66194a7f645 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c

[ ... ]

> @@ -828,6 +828,11 @@ static void handle_rx_uart(struct uart_port *uport, u32 bytes)
>  
>  static unsigned int qcom_geni_serial_tx_empty(struct uart_port *uport)
>  {
> +	struct qcom_geni_serial_port *port = to_dev_port(uport);
> +
> +	if (port->gpi_mode)
> +		return !port->tx_dma_addr ? TIOCSER_TEMT : 0;
> +

[Severity: Medium]
Does this reporting logic correctly account for pending asynchronous TX work? 

If qcom_geni_serial_start_tx_dma() queues the tx_xfer_work on the workqueue,
the tx_dma_addr remains 0 until the work actually maps the buffer. Could this
cause polling functions like uart_wait_until_sent() to prematurely return
TIOCSER_TEMT and assume the shift registers are idle while data is still
waiting to be transmitted?

>  	return !readl(uport->membase + SE_GENI_TX_FIFO_STATUS);
>  }
>  
> @@ -848,6 +849,17 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport)
>  {
>  	struct qcom_geni_serial_port *port = to_dev_port(uport);
>  
> +	if (port->gpi_mode && port->tx_dma_addr) {
> +		/*
> +		 * Allow tx_xfer_work to finish. Buffer cleanup is deferred until
> +		 * the DMA channel has been terminated.
> +		 */
> +		port->gpi->tx_cancel_pending = true;
> +		complete(&port->gpi->tx_xfer_completion);
> +		queue_work(port->gpi->tx_wq, &port->gpi->tx_cancel_work);
> +		return;
> +	}
> +
>  	if (qcom_geni_serial_main_active(uport))
>  		__qcom_geni_serial_cancel_tx_cmd(uport);

[Severity: High]
Is it safe to bypass cancellation if tx_dma_addr is zero?

If start_tx_dma() has queued tx_xfer_work but hasn't yet executed and mapped
the buffer, tx_dma_addr will still be 0. This seems to allow the asynchronous
work to start up later even after the port believes it has stopped.

Furthermore, if tx_dma_addr is 0 in GPI mode, this drops through to call
__qcom_geni_serial_cancel_tx_cmd(), which writes directly to the SE hardware
registers. Can this collide with the GPI DMA controller's operation?

[ ... ]

> @@ -1056,6 +1056,11 @@ static void qcom_geni_serial_stop_rx_dma(struct uart_port *uport)
>  	if (!qcom_geni_serial_secondary_active(uport))
>  		return;
>  
> +	if (port->gpi_mode) {
> +		queue_work(port->gpi->rx_wq, &port->gpi->rx_cancel_work);
> +		return;
> +	}
> +

[Severity: Critical]
Will this GPI mode branch ever execute?

In GPI mode, S_IRQ_EN is disabled during initialization, so the call to
qcom_geni_serial_secondary_active() above it will always evaluate to false.
Could this prevent the rx_cancel_work from ever being queued, leaving the
RX DMA permanently active?

[ ... ]

> @@ -1381,6 +1381,16 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
>  	/* Atomic console output queues tx_kick without taking the port lock. */
>  	WRITE_ONCE(port->tx_kick_enabled, false);
>  	irq_work_sync(&port->tx_kick);
> +
> +	if (port->gpi_mode) {
> +		qcom_geni_serial_stop_tx_dma(uport);
> +		flush_workqueue(port->gpi->tx_wq);
> +		flush_workqueue(port->gpi->rx_wq);
> +
> +		pm_runtime_put_sync(uport->dev);
> +		return;
> +	}
> +

[Severity: Critical]
Should qcom_geni_serial_stop_rx_dma() be called in this branch before we shut
down the device? 

Without stopping RX, the DMA channel seems to be left running while
pm_runtime_put_sync() powers off the device, which might lead to severe
memory corruption or IOMMU faults.

[Severity: High]
Can these flush_workqueue() calls trigger a use-after-free panic during device
removal? 

Looking at qcom_geni_serial_remove(), it calls destroy_workqueue() on both of
these workqueues before it invokes uart_remove_one_port(), which synchronously
executes this shutdown callback. Passing destroyed workqueues into the flush
API can cause crashes.

[ ... ]

> +static void
> +qcom_geni_serial_gpi_rx_callback_result(void *cb, const struct dmaengine_result *result)
> +{
> +	struct qcom_geni_serial_port *port = cb;
> +	struct uart_port *uport = &port->uport;
> +	struct tty_port *tport = &uport->state->port;
> +	size_t bytes = UART_XMIT_SIZE - result->residue;
> +	u8 idx = port->gpi->rx_done_index;
> +	int ret;
> +
> +	if (result->result != DMA_TRANS_NOERROR) {
> +		dev_err(uport->dev, "rx dma xfer failed: %d\n", result->result);
> +		qcom_geni_serial_stop_rx_dma(uport);
> +		qcom_geni_serial_start_rx_dma(uport);
> +		return;
> +	}

[Severity: Medium]
Is it intended to call qcom_geni_serial_stop_rx_dma() here without holding
the uport->lock? The stop routines appear to assume they are serialized by
the port lock.

[Severity: Critical]
Does this logic unconditionally restart the DMA pipeline even on an intentional
cancellation?

If the DMA channel is intentionally halted (which might return an error code
like DMA_TRANS_ABORTED), qcom_geni_serial_start_rx_dma() will immediately
re-arm it, completely nullifying any attempt to cleanly halt the RX channel.

[ ... ]

> +static void
> +qcom_geni_serial_gpi_tx_callback_result(void *cb, const struct dmaengine_result *result)
> +{
> +	struct qcom_geni_serial_port *port = cb;
> +	struct uart_port *uport = &port->uport;
> +	struct tty_port *tport = &uport->state->port;
> +	unsigned long flags;
> +
> +	if (result->result != DMA_TRANS_NOERROR) {
> +		dev_err(uport->dev, "tx dma xfer failed: %d\n", result->result);
> +		qcom_geni_serial_stop_tx_dma(uport);
> +		return;
> +	}

[Severity: Medium]
Similar to the RX callback, does this call to qcom_geni_serial_stop_tx_dma()
need to be protected by uart_port_lock_irqsave() to prevent concurrent
modifications to the hardware and driver state?

> +
> +	uart_port_lock_irqsave(uport, &flags);
> +	uart_xmit_advance(uport, port->gpi->xmit_size - result->residue);
> +	complete(&port->gpi->tx_xfer_completion);
> +
> +	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
> +		uart_write_wakeup(uport);
> +	uart_port_unlock_irqrestore(uport, flags);
> +}

[Severity: High]
Does this blindly advance the kfifo pointers after they could have been reset?

Because the tx_xfer_work reads the kfifo without holding the spinlock, another
TTY operation (like uart_flush_buffer) could zero out the FIFO pointers while
the DMA transfer is mid-flight. When the transfer finishes, calling
uart_xmit_advance() here will push the out pointer past the in pointer (0),
corrupting the kfifo state and causing out-of-bounds accesses.

[Severity: High]
Are we missing logic here to queue another TX work element if there is still
unsent data remaining in the FIFO? 

Because hardware TX interrupts are disabled in GPI mode, if we don't manually
re-arm the next DMA transfer, the transmission pipeline could permanently stall
until a new userspace write explicitly triggers it.

[ ... ]

> +static void qcom_geni_serial_gpi_tx_xfer_work(struct work_struct *work)
> +{
> +	struct qcom_geni_serial_gpi *gpi = container_of(work, struct qcom_geni_serial_gpi,
> +							tx_xfer_work);
> +	struct qcom_geni_serial_port *port = gpi->port;
> +	struct uart_port *uport = &port->uport;
> +	struct tty_port *tport = &uport->state->port;
> +	struct dma_slave_config config = {};
> +	struct dma_async_tx_descriptor *desc;
> +	unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
> +	unsigned long time_left;
> +	unsigned long timeout;
> +	dma_cookie_t tx_cookie;
> +	u8 *tail = NULL;
> +	int ret;
> +
> +	gpi->xmit_size = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail,
> +					      UART_XMIT_SIZE);

[Severity: High]
Can this race with the TTY layer modifying the transmit buffer?

The shared transmit buffer is being accessed here without the necessary
spinlock serialization (uport->lock). This lockless access makes it possible
for the kfifo state to be modified out from underneath the running DMA worker.

> +	if (!gpi->xmit_size)
> +		return;
> +
> +	config.peripheral_config = &gpi->peripheral;
> +	config.peripheral_size = sizeof(gpi->peripheral);
> +
> +	ret = dmaengine_slave_config(gpi->tx_chan, &config);
> +	if (ret) {
> +		dev_err(uport->dev, "tx dma config error: %d\n", ret);
> +		return;
> +	}
> +
> +	port->tx_dma_addr = dma_map_single(port->se.dev->parent, tail,
> +					   gpi->xmit_size, DMA_TO_DEVICE);
> +	ret = dma_mapping_error(port->se.dev->parent, port->tx_dma_addr);
> +	if (ret) {
> +		dev_err(uport->dev, "DMA mapping failure: %d\n", ret);
> +		port->tx_dma_addr = (dma_addr_t)NULL;
> +		return;
> +	}
> +
> +	dma_sync_single_for_cpu(port->se.dev->parent, port->tx_dma_addr,
> +				gpi->xmit_size, DMA_TO_DEVICE);

[Severity: High]
Does calling dma_sync_single_for_cpu() immediately after dma_map_single()
invalidate the device's ownership of the memory?

By forcefully revoking the device's ownership before transmission begins, the
DMA controller might be instructed to read stale or uninitialized CPU cache
lines on non-coherent platforms, which corrupts the transmitted data.

> +	desc = dmaengine_prep_slave_single(gpi->tx_chan, port->tx_dma_addr,
> +					   gpi->xmit_size, DMA_MEM_TO_DEV, flags);
> +	if (!desc) {
> +		dev_err(uport->dev, "Error setting up tx desc\n");
> +		goto err;
> +	}
> +
> +	desc->callback_result = qcom_geni_serial_gpi_tx_callback_result;
> +	desc->callback_param = port;
> +	tx_cookie = dmaengine_submit(desc);
> +	if (dma_submit_error(tx_cookie)) {
> +		dev_err(uport->dev, "%s: dmaengine_submit failed: %d\n", __func__, tx_cookie);
> +		dmaengine_terminate_sync(gpi->tx_chan);
> +		goto err;
> +	}
> +	reinit_completion(&port->gpi->tx_xfer_completion);
> +	dma_async_issue_pending(gpi->tx_chan);

[Severity: High]
Could the reinit_completion() call overwrite the cancellation signal?

If qcom_geni_serial_stop_tx_dma() concurrently executes and issues a
complete() signal, the reinit_completion() here would wipe out that signal.
This causes the worker thread to hang on the subsequent completion timeout.

[ ... ]

> +static int qcom_geni_serial_prepare_gpi_rx_dma(struct qcom_geni_serial_port *port)
> +{
> +	struct qcom_geni_serial_gpi *gpi = port->gpi;
> +	struct uart_port *uport = &port->uport;
> +	int i, ret;
> +
> +	for (i = 0; i < NUM_GPI_RX_BUFFERS; i++) {
> +		gpi->rx_buf[i] = devm_kzalloc(uport->dev, UART_XMIT_SIZE, GFP_KERNEL);
> +		if (!gpi->rx_buf[i])
> +			return dev_err_probe(uport->dev, -ENOMEM, "RX buffer %d\n", i);
> +
> +		gpi->rx_dma_addr[i] = dma_map_single(port->se.dev->parent, gpi->rx_buf[i],
> +						     UART_XMIT_SIZE, DMA_FROM_DEVICE);
> +		ret = dma_mapping_error(port->se.dev->parent, gpi->rx_dma_addr[i]);
> +		if (ret) {
> +			dev_err(uport->dev, "Failed to map gpi rx buffer[%d]\n", i);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}

[Severity: Medium]
Are the earlier DMA mappings leaked if this loop fails?

If dma_map_single() fails on an iteration i > 0, the function returns the
error directly without unwinding and unmapping the successful mappings from
the previous iterations.

[ ... ]

> +static int qcom_geni_serial_setup_gpi_dma(struct qcom_geni_serial_port *port)
> +{
> +	struct qcom_geni_serial_gpi *gpi = port->gpi;
> +	int ret = -EINVAL;
> +
> +	gpi->tx_chan = dma_request_chan(port->se.dev, "tx");
> +	if (IS_ERR(gpi->tx_chan)) {
> +		dev_err(port->se.dev, "Failed to get tx DMA ch\n");
> +		return ret;
> +	}

[Severity: Medium]
Does this squash important probe deferral error codes?

The original error from dma_request_chan() (such as -EPROBE_DEFER) is lost
and replaced by the hardcoded -EINVAL. This converts a normal transient probe
deferral into a permanent initialization failure for the device.

[ ... ]

> @@ -1352,13 +1805,26 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>  		return -ENXIO;
>  	}
>  
> +	port->gpi_mode = readl(uport->membase + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> +	if (port->gpi_mode) {
> +		geni_se_select_mode(&port->se, GENI_GPI_DMA);
> +		ret = qcom_geni_serial_gpi_init(port);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	qcom_geni_serial_stop_rx(uport);
>  
>  	ret = setup_fifos(port);
>  	if (ret)
>  		return ret;

[Severity: Medium]
Does this return early without cleaning up the initialized GPI state?

If setup_fifos() fails, the error code is returned without unwinding the
resources acquired in qcom_geni_serial_gpi_init(). Since port->setup has not
yet been set to true, the teardown logic in qcom_geni_serial_remove() will
also skip freeing these leaked workqueues and DMA mappings.

[ ... ]

> @@ -2035,6 +2565,14 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>  
>  	WRITE_ONCE(port->tx_kick_enabled, false);
>  	irq_work_sync(&port->tx_kick);
> +
> +	if (port->gpi_mode && port->setup) {
> +		destroy_workqueue(port->gpi->tx_wq);
> +		destroy_workqueue(port->gpi->rx_wq);
> +		qcom_geni_serial_release_gpi_dma(port);
> +		qcom_geni_serial_unprepare_gpi_rx_dma(port);
> +	}
> +
>  	dev_pm_clear_wake_irq(&pdev->dev);
>  	device_init_wakeup(&pdev->dev, false);
>  	if (!port->dev_data->console)

[Severity: High]
Are the workqueues being destroyed too early in the port lifecycle?

This code destroys the workqueues right before calling uart_remove_one_port(),
which synchronously runs qcom_geni_serial_shutdown() if the port is open. As
seen earlier, that shutdown callback invokes flush_workqueue() on these
newly-freed workqueues.

[Severity: Medium]
Will this leak resources if initialization failed partway through?

Because cleanup is gated behind the port->setup boolean, early failures in
qcom_geni_serial_port_setup() mean these resources will never be freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-add-gpi-dma-support-for-uart-v1-0-c69e728df686@oss.qualcomm.com?part=4