[PATCH] gpu: host1x: fix runtime PM reference leak on remove

[PATCH] gpu: host1x: fix runtime PM reference leak on remove

[Guangshuo Li]

host1x_probe() calls pm_runtime_resume_and_get() and intentionally keeps
the runtime PM usage reference for the lifetime of the driver because
host1x is not yet ready for dynamic runtime PM.

The probe error path drops this reference with
pm_runtime_put_sync_suspend(), but the remove path only calls
pm_runtime_force_suspend().

pm_runtime_force_suspend() disables runtime PM and invokes the runtime
suspend callback when necessary, but it does not decrement the runtime
PM usage counter. As a result, the reference acquired by
pm_runtime_resume_and_get() remains held after the driver is unbound.
Repeated bind and unbind cycles can therefore leave the runtime PM
usage counter increasingly unbalanced.

Drop the usage reference with pm_runtime_put_sync_suspend() before
forcing the device into suspend during removal.

This issue was found by manual code inspection.

Fixes: 6b6776e2ab8a ("gpu: host1x: Add initial runtime PM and OPP support")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/gpu/host1x/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index d2c64728f804..bb6a1647e4db 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -729,6 +729,7 @@ static void host1x_remove(struct platform_device *pdev)
 	host1x_unregister(host);
 	host1x_debug_deinit(host);
 
+	pm_runtime_put_sync_suspend(&pdev->dev);
 	pm_runtime_force_suspend(&pdev->dev);
 
 	host1x_intr_deinit(host);
-- 
2.43.0

Re: [PATCH] gpu: host1x: fix runtime PM reference leak on remove

[sashiko-bot]

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

Pre-existing issues:
- [High] Shared IRQ handler accesses hardware registers without PM checks, and runtime suspend powers down hardware without `synchronize_irq()`, risking system crashes.
- [High] `host1x_remove()` leaves the device in an invalid PM state (RPM_ACTIVE with hardware off) if children exist, causing bus hangs on driver rebind.
- [High] Resource leak (clocks left permanently on) in `host1x_probe` error path if child devices exist.
- [High] Use-After-Free risk due to allocating `struct device` objects as an array and freeing them synchronously in `host1x_memory_context_list_free`.
- [High] Use-After-Free risk due to missing `cancel_work_sync()` for `cdma->update_work` during CDMA de-initialization.
--

commit ee98077bd8be81a3568fe9ef94599ff2844511f0
Author: Guangshuo Li <lgs201920130244@gmail.com>

gpu: host1x: fix runtime PM reference leak on remove

The commit fixes a runtime PM reference leak in host1x_remove() by calling
pm_runtime_put_sync_suspend() before forcing the device into suspend. This
balances the reference acquired during host1x_probe().

> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> index d2c64728f8043..bb6a1647e4db9 100644
> --- a/drivers/gpu/host1x/dev.c
> +++ b/drivers/gpu/host1x/dev.c
> @@ -729,6 +729,7 @@ static void host1x_remove(struct platform_device *pdev)
>  	host1x_unregister(host);
>  	host1x_debug_deinit(host);
>  
> +	pm_runtime_put_sync_suspend(&pdev->dev);
>  	pm_runtime_force_suspend(&pdev->dev);

[Severity: High]
This isn't a bug introduced by this patch, but does this leave the device in
an invalid PM state (RPM_ACTIVE with hardware off) if children exist?

In host1x_remove(), pm_runtime_put_sync_suspend() drops the usage counter
but fails to suspend because devm-managed children might exist
(child_count > 0). pm_runtime_force_suspend() is then called, which powers
down the hardware. 

However, because child_count > 0, pm_runtime_need_not_resume() returns false,
causing pm_runtime_force_suspend() to leave the runtime state as RPM_ACTIVE.

When the driver is unbound and rebound, host1x_probe() calls
pm_runtime_resume_and_get(), which sees RPM_ACTIVE, skips the resume
callback, and accesses the powered-off hardware. Can this cause bus hangs on
driver rebind?

While reviewing the PM changes, I noticed a few other pre-existing issues in
the driver:

[Severity: High]
In drivers/gpu/host1x/dev.c:host1x_probe(), is there a resource leak in the
error path if child devices exist?

pm_put:
	pm_runtime_put_sync_suspend(&pdev->dev);
pm_disable:
	pm_runtime_disable(&pdev->dev);

This is a pre-existing issue, but if an error occurs in host1x_probe() after
devm_of_platform_populate() adds children, pm_runtime_put_sync_suspend()
fails to suspend with -EBUSY due to child_count > 0. 

The driver exits probe, and devm cleans up the children, but the device is
left in RPM_ACTIVE with PM disabled, leaving hardware clocks permanently
enabled.

[Severity: High]
In drivers/gpu/host1x/intr.c:host1x_intr_init(), the interrupt handler is
registered as a shared interrupt:

	err = devm_request_irq(host->dev, host->syncpt_irqs[i],
			       host->intr_op->isr, IRQF_SHARED,
			       "host1x_syncpt", &irq_data[i]);

This isn't a bug introduced by this patch, but doesn't this risk system
crashes?

If the device is runtime suspended and another device sharing the same IRQ
line triggers an interrupt, syncpt_thresh_isr() will execute and access
host1x registers without checking PM status:

	reg = host1x_sync_readl(host,
		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(i));

Additionally, host1x_runtime_suspend() powers down the hardware without
waiting for mid-flight IRQs via synchronize_irq().

[Severity: High]
This is a pre-existing issue, but does host1x_memory_context_list_free() in
drivers/gpu/host1x/context.c have a use-after-free risk?

	for (i = 0; i < cdl->len; i++)
		device_unregister(&cdl->devs[i].dev);

	kfree(cdl->devs);

The device array is synchronously freed immediately after device_unregister().

If userspace holds a reference (e.g., via an open sysfs file), the memory is
freed while in use, and when the final reference is dropped, the driver core
will operate on freed memory.

[Severity: High]
This is a pre-existing issue, but does host1x_cdma_deinit() in
drivers/gpu/host1x/cdma.c risk a use-after-free by missing a work cleanup?

	host1x_pushbuffer_destroy(pb);
	host1x_hw_cdma_timeout_destroy(host1x, cdma);

	return 0;

The function destroys CDMA resources and the channel memory is freed, but it
omits calling cancel_work_sync(&cdma->update_work). 

If the work executes after the resources are freed, cdma_update_work() will
dereference freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913120227.1559129-1-lgs201920130244@gmail.com?part=1