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

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

[Neil Armstrong]

The console port (qcom_geni_console_port) is a static instance whose
uport.line is hardcoded to 0 and is never allocated from port_ida.
Only the non-console path in get_port_from_line() calls ida_alloc_range().

Fix the qcom_geni_serial_remove() and the matching probe() error path
so unbinding the console device doesn't hit:

  WARNING: ida_free called for id=0 which is not allocated
  <snip>
  qcom_geni_serial_remove+0x58/0x80

Fixes: a53be6945f51 ("serial: qcom-geni: Remove alias dependency from qcom serial driver")
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v3:
- Also use data->console in remove() because it oly return true if it's the main system console (Konrad)
- Link to v2: https://patch.msgid.link/20260904-topic-sm8x50-upstream-tty-serial-geni-fix-ida-free-v2-1-0e83d8df410c@linaro.org

Changes in v2:
- use data->console in probe() because uart_console() return false before uart_add_one_port() (Konrad)
- Link to v1: https://patch.msgid.link/20260904-topic-sm8x50-upstream-tty-serial-geni-fix-ida-free-v1-1-02e18c31aeba@linaro.org
---
 drivers/tty/serial/qcom_geni_serial.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3633723acef8..1a36e9fad976 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;
 		}
 	}
@@ -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);
 
 	if (port->rx_dma_addr) {

---
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
change-id: 20260904-topic-sm8x50-upstream-tty-serial-geni-fix-ida-free-4a199c25fd84

Best regards,
--  
Neil Armstrong <neil.armstrong@linaro.org>

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

[sashiko-bot]

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

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

[Aniket RANDIVE]

Hi Neil,
This appears to be already fixed by:

commit ("serial: qcom-geni: Fix port_ida handling for console and probe 
errors")

which also fixes additional port_ida leak paths.

On 9/4/2026 9:14 PM, sashiko-bot@kernel.org wrote:
> 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) {
>