* Re: [PATCH] serial: qcom-geni: Fix unbalanced runtime PM resume for no_console_suspend
2026-08-28 14:32 [PATCH] serial: qcom-geni: Fix unbalanced runtime PM resume for no_console_suspend Abel Vesa
@ 2026-08-28 14:40 ` Praveen Talari
2026-08-28 14:43 ` sashiko-bot
1 sibling, 0 replies; 3+ messages in thread
From: Praveen Talari @ 2026-08-28 14:40 UTC (permalink / raw)
To: Abel Vesa, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-arm-msm, linux-kernel, linux-serial
Hi
On 28-08-2026 20:02, Abel Vesa wrote:
> When no_console_suspend is set for the serial console, the system
> suspend callback skips pm_runtime_force_suspend() to keep the console
> active. The resume callback still unconditionally calls
> pm_runtime_force_resume(), which triggers an "Unbalanced
> pm_runtime_enable!" warning.
>
> Only force resume when the suspend callback force suspended the device.
>
> Fixes: d0cd9c8d0fd5 ("serial: qcom-geni: add force suspend/resume to system sleep callbacks")
> Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3633723acef8..a180a00c9e23 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -2099,9 +2099,11 @@ static int qcom_geni_serial_resume(struct device *dev)
> struct uart_port *uport = &port->uport;
> struct qcom_geni_private_data *private_data = uport->private_data;
>
> - ret = pm_runtime_force_resume(dev);
> - if (ret)
> - return ret;
> + if (console_suspend_enabled || !uart_console(uport)) {
> + ret = pm_runtime_force_resume(dev);
> + if (ret)
> + return ret;
> + }
Reviewed-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
Thanks,
Praveen Talari
>
> ret = uart_resume_port(private_data->drv, uport);
> if (uart_console(uport)) {
>
> ---
> base-commit: 3d83758432b5e6ed9507500a57efb0f3af41ee7d
> change-id: 20260828-serial-qcom-geni-fix-unbalanced-rpm-resume-814e630326b9
>
> Best regards,
> --
> Abel Vesa <abel.vesa@oss.qualcomm.com>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] serial: qcom-geni: Fix unbalanced runtime PM resume for no_console_suspend
2026-08-28 14:32 [PATCH] serial: qcom-geni: Fix unbalanced runtime PM resume for no_console_suspend Abel Vesa
2026-08-28 14:40 ` Praveen Talari
@ 2026-08-28 14:43 ` sashiko-bot
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-28 14:43 UTC (permalink / raw)
To: Abel Vesa; +Cc: linux-serial
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Reallocating the DMA receive buffer in setup_fifos() frees the active DMA-mapped memory without unmapping it, leading to DMA use-after-free and massive out-of-bounds reads/writes.
- [High] Asynchronous pm_runtime_put() is used on the probe error path before devres pm_runtime_disable(), leaving hardware powered on indefinitely.
- [High] Missing rollback in qcom_geni_serial_suspend() if pm_runtime_force_suspend() fails, leaving the UART port permanently disabled.
- [High] IDA line ID is freed before the UART port is unregistered, allowing a race condition with a concurrent probe.
--
commit 7fc2859a848a4f1ec1c0bc7d7815a5c518730a33
Author: Abel Vesa <abel.vesa@oss.qualcomm.com>
serial: qcom-geni: Fix unbalanced runtime PM resume for no_console_suspend
When no_console_suspend is set for the serial console, the system
suspend callback skips pm_runtime_force_suspend() to keep the console
active. The resume callback still unconditionally calls
pm_runtime_force_resume(), which triggers an "Unbalanced
pm_runtime_enable!" warning.
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3633723acef8d..a180a00c9e23d 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1074,10 +1074,10 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
[ ... ]
> if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) {
> /*
> * Use krealloc rather than krealloc_array because rx_buf is
> * accessed as 1 byte entries as well as 4 byte entries so it's
> * not necessarily an array.
> */
> port->rx_buf = devm_krealloc(uport->dev, port->rx_buf,
> port->rx_fifo_depth * sizeof(u32),
> GFP_KERNEL);
[Severity: Critical]
This is a pre-existing issue, but it looks like devm_krealloc() frees the
original active DMA-mapped memory buffer here without updating
port->rx_dma_addr or unmapping the original DMA mapping.
Could this allow the hardware DMA to write to the freed memory block, and
cause handle_rx_uart() to later read out-of-bounds from the newly allocated,
shrunk port->rx_buf, leaking kernel heap memory to the TTY layer?
> @@ -1991,7 +1991,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
[ ... ]
> ret = pm_runtime_resume_and_get(uport->dev);
> if (ret < 0) {
> dev_err(uport->dev, "Failed to resume and get %d\n", ret);
> goto error;
> }
>
> ret = uart_add_one_port(drv, uport);
> pm_runtime_put(uport->dev);
> if (ret)
> goto error;
[Severity: High]
This isn't a bug introduced by this patch, but doesn't using an asynchronous
pm_runtime_put() here on the probe error path leave the hardware powered on
indefinitely?
The devres pm_runtime_disable() that follows will cancel any pending
asynchronous suspend work.
> @@ -2022,6 +2022,6 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
[ ... ]
> WRITE_ONCE(port->tx_kick_enabled, false);
> 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);
> uart_remove_one_port(drv, &port->uport);
[Severity: High]
This is a pre-existing issue, but is the IDA line ID prematurely freed here
before the UART port is completely unregistered on the next line?
If a concurrent device probe allocates the just-freed IDA line ID while
uart_remove_one_port() is still running for the old device, it seems this
could cause a collision in the serial core due to duplicate line IDs being
registered simultaneously.
> @@ -2086,10 +2086,10 @@ static int qcom_geni_serial_suspend(struct device *dev)
[ ... ]
> /*
> * When no_console_suspend is set the console must remain active
> * across system sleep, so skip the force suspend path.
> */
> if (!console_suspend_enabled && uart_console(uport))
> return 0;
>
> return pm_runtime_force_suspend(dev);
[Severity: High]
This isn't a bug introduced by this patch, but if pm_runtime_force_suspend()
fails, does this return the error without reverting the effects of
uart_suspend_port() and geni_icc_set_tag() executed earlier in the function?
It seems this could leave the UART port permanently disabled.
> }
>
> static int qcom_geni_serial_resume(struct device *dev)
> {
> int ret;
> struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> struct uart_port *uport = &port->uport;
> struct qcom_geni_private_data *private_data = uport->private_data;
>
> - ret = pm_runtime_force_resume(dev);
> - if (ret)
> - return ret;
> + if (console_suspend_enabled || !uart_console(uport)) {
> + ret = pm_runtime_force_resume(dev);
> + if (ret)
> + return ret;
> + }
>
> ret = uart_resume_port(private_data->drv, uport);
> if (uart_console(uport)) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-serial-qcom-geni-fix-unbalanced-rpm-resume-v1-1-af3e5aef17a6@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread