Re: [PATCH] serial: qcom-geni: Fix unbalanced runtime PM resume for no_console_suspend

[sashiko-bot@kernel.org]

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