Re: [PATCH v3] tty: serial: qcom_geni: don't ida_free() the console port line

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] UART line IDA is permanently leaked on error paths in get_port_from_line() and early qcom_geni_serial_probe().
- [High] Dedicated wake IRQ and device wakeup capabilities are leaked if probe fails late.
- [High] Manual call to `dev_pm_domain_detach_list()` causes a double-free because the list is managed by devres.
- [High] The static console port permanently fails probing if a transient error occurs during `probe()`.
- [High] The probe error path blindly unmaps the RX DMA buffer, even if the mapping failed.
- [Medium] Premature `ida_free()` in `qcom_geni_serial_remove()` creates a race condition for the line ID.
--

commit 8e6b3b4c6a06ece45749a076a423f5ca442974fa
Author: Neil Armstrong <neil.armstrong@linaro.org>

tty: serial: qcom_geni: don't ida_free() the console port line

The console port is a static instance whose line is hardcoded to 0 and is
never allocated from port_ida. This patch fixes qcom_geni_serial_remove()
and the matching probe() error path so unbinding the console device doesn't
hit a warning.

> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3633723acef8d..1a36e9fad9768 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1979,7 +1979,8 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  						port->wakeup_irq);
>  		if (ret) {
>  			device_init_wakeup(&pdev->dev, false);
> -			ida_free(&port_ida, uport->line);
> +			if (!data->console)
> +				ida_free(&port_ida, uport->line);
>  			goto error;

[Severity: High]
This isn't a bug introduced by this patch, but does qcom_geni_serial_probe()
leak the UART line IDA on other error paths?

If deferred probing causes qcom_geni_serial_probe() to repeatedly fail, the
IDA leaks multiple times and can exhaust the IDA range, permanently breaking
device initialization.

In get_port_from_line(), an IDA is dynamically allocated via
ida_alloc_range(). If the subsequent devm_kzalloc() fails:

	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
	if (!port)
		return ERR_PTR(-ENOMEM);

the function returns directly without freeing the IDA.

Similarly, in qcom_geni_serial_probe(), early failures like resources_init()
or late failures jumping to the shared error: label fail to call ida_free()
for non-console ports. The patch fixes one specific path but leaves the rest
leaking.

[Severity: High]
This is a pre-existing issue, but is the shared error cleanup path missing a
rollback of the wake IRQ or wakeup capabilities?

If dev_pm_set_dedicated_wake_irq() succeeds, the wake IRQ is registered on
the device. If a subsequent initialization step fails (e.g.,
pm_runtime_resume_and_get()), the code jumps to the error: label:

error:
	if (port->rx_dma_addr) {
...

The error: block entirely fails to call dev_pm_clear_wake_irq() and
device_init_wakeup(&pdev->dev, false), leaving a dangling interrupt handler
registered to a failing device. This can cause spurious interrupts, crashes,
or panics when triggered after the device memory is freed.

[Severity: High]
This isn't a bug introduced by this patch, but will the error path blindly
unmap the RX DMA buffer, even if the mapping failed?

In qcom_geni_serial_probe(), dma_map_single() returns an error token when
mapping fails, which is non-zero (typically ~0ULL):

		if (dma_mapping_error(pdev->dev.parent, port->rx_dma_addr)) {
			ret = -EIO;
			dev_err(&pdev->dev, "Failed to map RX DMA buffer: %d\n", ret);
			goto error;
		}

The error: block checks if (port->rx_dma_addr), which evaluates to true for
the non-zero error token, and erroneously calls dma_unmap_single() on the
error token:

error:
	if (port->rx_dma_addr) {
		dma_unmap_single(pdev->dev.parent, port->rx_dma_addr,
				 DMA_RX_BUF_SIZE, DMA_FROM_DEVICE);

Calling dma_unmap_single() on an error token violates the DMA API and can
cause kernel panics or IOMMU faults.

[Severity: High]
This is a pre-existing issue, but can a transient error during probe()
permanently break the static console port?

qcom_geni_serial_probe() checks if (uport->private_data) to prevent two
drivers from claiming the same port, and subsequently assigns:

	port->private_data.drv = drv;
	uport->private_data = &port->private_data;

If a later step in probe() fails, the function returns an error but does not
clear uport->private_data. For the console, port points to the statically
allocated qcom_geni_console_port. Thus, the non-NULL state persists.

When deferred probe retries, it hits the uport->private_data check and
returns -ENODEV permanently, rendering the system without a working console.

>  		}
>  	}
> @@ -2015,6 +2016,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  static void qcom_geni_serial_remove(struct platform_device *pdev)
>  {
>  	struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> +	const struct qcom_geni_device_data *data = port->dev_data;
>  	struct uart_port *uport = &port->uport;
>  	struct uart_driver *drv = port->private_data.drv;
>  
> @@ -2024,7 +2026,8 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>  	irq_work_sync(&port->tx_kick);
>  	dev_pm_clear_wake_irq(&pdev->dev);
>  	device_init_wakeup(&pdev->dev, false);
> -	ida_free(&port_ida, uport->line);
> +	if (!data->console)
> +		ida_free(&port_ida, uport->line);
>  	uart_remove_one_port(drv, &port->uport);

[Severity: Medium]
This isn't a bug introduced by this patch, but does calling ida_free() before
uart_remove_one_port() create a race condition for the line ID?

ida_free(&port_ida, uport->line) marks the port's line ID as available
immediately. A concurrent probe (e.g., via hotplug or deferred probe
resolution) could allocate the freed line ID.

The concurrent probe would attempt to register a new port using the same ID
while uart_remove_one_port() is still running for the old port, leading to a
collision in the serial subsystem.

[Severity: High]
This is a pre-existing issue, but does the manual call to
dev_pm_domain_detach_list() here and in the probe error path cause a
double-free?

For the sa8255p compatible device, resources_init() points to
geni_se_domain_attach(), which calls devm_pm_domain_attach_list() to attach
power domains. This registers a devres action to automatically free the list.

Both the error: path in qcom_geni_serial_probe() and
qcom_geni_serial_remove() manually call:

	dev_pm_domain_detach_list(port->se.pd_list);

dev_pm_domain_detach_list() calls kfree(list). When devres subsequently
cleans up, it invokes devm_pm_domain_detach_list() on the exact same dangling
pointer. This double-free on a list pointer leads to a Use-After-Free kernel
panic, which can be triggered by probe failure or by unbinding the driver.

>  
>  	if (port->rx_dma_addr) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-topic-sm8x50-upstream-tty-serial-geni-fix-ida-free-v3-1-541757ec8a95@linaro.org?part=1