* [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line
@ 2026-09-04 7:09 Neil Armstrong
2026-09-04 7:27 ` sashiko-bot
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Neil Armstrong @ 2026-09-04 7:09 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Viken Dadhaniya
Cc: linux-arm-msm, linux-kernel, linux-serial, 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>
---
drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3633723acef8..bdbdfd7dfaac 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 (!uart_console(uport))
+ ida_free(&port_ida, uport->line);
goto error;
}
}
@@ -2024,7 +2025,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 (!uart_console(uport))
+ 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>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line
2026-09-04 7:09 [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line Neil Armstrong
@ 2026-09-04 7:27 ` sashiko-bot
2026-09-04 8:02 ` Konrad Dybcio
2026-09-07 4:37 ` Aniket RANDIVE
2 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-09-04 7:27 UTC (permalink / raw)
To: Neil Armstrong; +Cc: linux-serial
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Double free and double detach of PM domains due to mixing devm_ allocation with manual cleanup.
- [High] The allocated IDA for non-console ports is permanently leaked on most probe error paths.
- [High] Wake IRQ and device wakeup capabilities are leaked and not reverted on late probe failures, permanently breaking deferred probe.
- [High] `dma_unmap_single()` is executed against an invalid DMA address if `dma_map_single()` fails.
- [Medium] Unsafe ordering of `ida_free()` and `uart_remove_one_port()` in the driver's remove function.
--
commit b824bc9910a93e019533a2fc7bb8b725efd22ca7
Author: Neil Armstrong <neil.armstrong@linaro.org>
tty: serial: qcom_geni: don't ida_free() the console port line
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.
This commit updates the probe error path and remove function to only call
ida_free() for non-console ports.
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3633723acef8d..bdbdfd7dfaace 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 (!uart_console(uport))
> + ida_free(&port_ida, uport->line);
> goto error;
> }
> }
[Severity: High]
This is a pre-existing issue, but does the common error cleanup block fail
to release the allocated IDA for non-console ports on other probe error paths?
If get_port_from_line() successfully allocates an IDA, but an error occurs
later in qcom_geni_serial_probe() such as a dma_map_single() failure, control
jumps to the error block which completely bypasses ida_free():
error:
if (port->rx_dma_addr) {
dma_unmap_single(pdev->dev.parent, port->rx_dma_addr,
DMA_RX_BUF_SIZE, DMA_FROM_DEVICE);
port->rx_dma_addr = 0;
}
dev_pm_domain_detach_list(port->se.pd_list);
return ret;
Could this exhaust the finite pool of IDAs upon repeated deferred probe
failures?
[Severity: High]
This is also a pre-existing issue, but does the error label handle general
cleanup while missing the reversion of the wake IRQ and wakeup initialization?
If probe sets up the wake IRQ via dev_pm_set_dedicated_wake_irq() but fails
later, the error block completely bypasses dev_pm_clear_wake_irq() and
device_init_wakeup(). Will dev_pm_set_dedicated_wake_irq() return -EEXIST
upon a deferred probe retry because dev->power.wakeirq is already initialized,
permanently breaking the device?
[Severity: High]
Another pre-existing issue in the same error block: is dma_unmap_single()
executed against an invalid DMA address if dma_map_single() fails?
If dma_map_single() fails during probe, port->rx_dma_addr retains the mapping
error code. Does the error path conditionally unmap the buffer using
if (port->rx_dma_addr), mistakenly evaluating the non-zero error code as a
valid address?
[Severity: Critical]
This is a pre-existing issue, but does mixing devm_ allocation with manual
cleanup cause a double-free of the PM domains?
geni_se_domain_attach() uses devm_pm_domain_attach_list() to attach the PM
domains and registers an automatic devres cleanup handler. However, the error
block in qcom_geni_serial_probe() manually calls:
dev_pm_domain_detach_list(port->se.pd_list);
When devres executes its cleanup routine, won't it call
dev_pm_domain_detach_list() again on the same freed pointer?
[ ... ]
> @@ -2024,7 +2025,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 (!uart_console(uport))
> + ida_free(&port_ida, uport->line);
> uart_remove_one_port(drv, &port->uport);
[Severity: Medium]
This is a pre-existing issue, but is there an unsafe ordering of ida_free()
and uart_remove_one_port() in qcom_geni_serial_remove()?
The line identifier is returned to the global port_ida pool via ida_free()
before uart_remove_one_port() unregisters the subsystem structure that relies
on it. Could a concurrent probe of another qcom_geni_serial device claim
this newly freed ID, hit the state->uart_port check in
serial_core_add_one_port(), and fail with -EINVAL?
[Severity: Critical]
This is a pre-existing issue, but does qcom_geni_serial_remove() also
manually clean up the devm-managed PM domains?
Similar to the probe error path, the remove function includes:
dev_pm_domain_detach_list(port->se.pd_list);
Will the subsequent automatic devres cleanup trigger a double-free of the
dev_pm_domain_list structure?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-topic-sm8x50-upstream-tty-serial-geni-fix-ida-free-v1-1-02e18c31aeba@linaro.org?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line
2026-09-04 7:09 [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line Neil Armstrong
2026-09-04 7:27 ` sashiko-bot
@ 2026-09-04 8:02 ` Konrad Dybcio
2026-09-04 8:03 ` Konrad Dybcio
2026-09-07 4:37 ` Aniket RANDIVE
2 siblings, 1 reply; 6+ messages in thread
From: Konrad Dybcio @ 2026-09-04 8:02 UTC (permalink / raw)
To: Neil Armstrong, Greg Kroah-Hartman, Jiri Slaby, Viken Dadhaniya
Cc: linux-arm-msm, linux-kernel, linux-serial
On 9/4/26 9:09 AM, Neil Armstrong wrote:
> 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>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3633723acef8..bdbdfd7dfaac 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 (!uart_console(uport))
> + ida_free(&port_ida, uport->line);
GPT points out this will always return false before uart_add_one_port()
is called
Konrad
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line
2026-09-04 8:02 ` Konrad Dybcio
@ 2026-09-04 8:03 ` Konrad Dybcio
2026-09-04 8:04 ` Neil Armstrong
0 siblings, 1 reply; 6+ messages in thread
From: Konrad Dybcio @ 2026-09-04 8:03 UTC (permalink / raw)
To: Neil Armstrong, Greg Kroah-Hartman, Jiri Slaby, Viken Dadhaniya
Cc: linux-arm-msm, linux-kernel, linux-serial
On 9/4/26 10:02 AM, Konrad Dybcio wrote:
> On 9/4/26 9:09 AM, Neil Armstrong wrote:
>> 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>
>> ---
>
>
>> drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index 3633723acef8..bdbdfd7dfaac 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 (!uart_console(uport))
>> + ida_free(&port_ida, uport->line);
>
> GPT points out this will always return false before uart_add_one_port()
> is called
You can use if (data->console) instead
Konrad
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line
2026-09-04 8:03 ` Konrad Dybcio
@ 2026-09-04 8:04 ` Neil Armstrong
0 siblings, 0 replies; 6+ messages in thread
From: Neil Armstrong @ 2026-09-04 8:04 UTC (permalink / raw)
To: Konrad Dybcio, Greg Kroah-Hartman, Jiri Slaby, Viken Dadhaniya
Cc: linux-arm-msm, linux-kernel, linux-serial
On 9/4/26 10:03, Konrad Dybcio wrote:
> On 9/4/26 10:02 AM, Konrad Dybcio wrote:
>> On 9/4/26 9:09 AM, Neil Armstrong wrote:
>>> 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>
>>> ---
>>
>>
>>> drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>> index 3633723acef8..bdbdfd7dfaac 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 (!uart_console(uport))
>>> + ida_free(&port_ida, uport->line);
>>
>> GPT points out this will always return false before uart_add_one_port()
>> is called
>
> You can use if (data->console) instead
Yep will do
Thanks
>
> Konrad
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line
2026-09-04 7:09 [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line Neil Armstrong
2026-09-04 7:27 ` sashiko-bot
2026-09-04 8:02 ` Konrad Dybcio
@ 2026-09-07 4:37 ` Aniket RANDIVE
2 siblings, 0 replies; 6+ messages in thread
From: Aniket RANDIVE @ 2026-09-07 4:37 UTC (permalink / raw)
To: Neil Armstrong, Greg Kroah-Hartman, Jiri Slaby, Viken Dadhaniya
Cc: linux-arm-msm, linux-kernel, linux-serial
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 12:39 PM, Neil Armstrong wrote:
> 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>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3633723acef8..bdbdfd7dfaac 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 (!uart_console(uport))
> + ida_free(&port_ida, uport->line);
> goto error;
> }
> }
> @@ -2024,7 +2025,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 (!uart_console(uport))
> + 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>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-07 4:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-04 7:09 [PATCH] tty: serial: qcom_geni: don't ida_free() the console port line Neil Armstrong
2026-09-04 7:27 ` sashiko-bot
2026-09-04 8:02 ` Konrad Dybcio
2026-09-04 8:03 ` Konrad Dybcio
2026-09-04 8:04 ` Neil Armstrong
2026-09-07 4:37 ` Aniket RANDIVE
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.