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 <[email protected]> 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/[email protected]?part=1
